D3858: unlinkpath: make empty directory removal optional (issue5901) (issue5826)

spectral (Kyle Lippincott) phabricator at mercurial-scm.org
Fri Jun 29 01:10:34 UTC 2018


spectral created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  There are known cases where performing operations such as rebase from a
  directory that is newly created can fail or at least lead to being in a
  directory handle that no longer exists.
  
  This is even reproducible by just doing something as simple as:
  
    cd foo; hg rm *
  
  The behavior is different if you use `hg addremove`, the directory is not
  removed until we attempt to go back to the node after committing it:
  
    cd foo; rm *; hg addremove; hg ci -m'bye foo'; hg co .^; hg co tip

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3858

AFFECTED FILES
  mercurial/cmdutil.py
  mercurial/configitems.py
  mercurial/context.py
  mercurial/patch.py
  mercurial/util.py
  mercurial/vfs.py
  tests/test-removeemptydirs.t

CHANGE DETAILS

diff --git a/tests/test-removeemptydirs.t b/tests/test-removeemptydirs.t
new file mode 100644
--- /dev/null
+++ b/tests/test-removeemptydirs.t
@@ -0,0 +1,145 @@
+Tests for experimental.removeemptydirs
+
+  $ NO_RM=--config=experimental.removeemptydirs=0
+  $ isdir() { if [[ -d $1 ]]; then echo yes; else echo no; fi }
+  $ isfile() { if [[ -f $1 ]]; then echo yes; else echo no; fi }
+
+`hg rm` of the last file in a directory:
+  $ hg init hgrm
+  $ cd hgrm
+  $ mkdir somedir
+  $ echo hi > somedir/foo
+  $ hg ci -qAm foo
+  $ isdir somedir
+  yes
+  $ hg rm somedir/foo
+  $ isdir somedir
+  no
+  $ hg revert -qa
+  $ isdir somedir
+  yes
+  $ hg $NO_RM rm somedir/foo
+  $ isdir somedir
+  yes
+  $ ls somedir
+  $ cd $TESTTMP
+
+`hg mv` of the last file in a directory:
+  $ hg init hgmv
+  $ cd hgmv
+  $ mkdir somedir
+  $ mkdir destdir
+  $ echo hi > somedir/foo
+  $ hg ci -qAm foo
+  $ isdir somedir
+  yes
+  $ hg mv somedir/foo destdir/foo
+  $ isdir somedir
+  no
+  $ hg revert -qa
+(revert doesn't get rid of destdir/foo?)
+  $ rm destdir/foo
+  $ isdir somedir
+  yes
+  $ hg $NO_RM mv somedir/foo destdir/foo
+  $ isdir somedir
+  yes
+  $ ls somedir
+  $ cd $TESTTMP
+
+Updating to a commit that doesn't have the directory:
+  $ hg init hgupdate
+  $ cd hgupdate
+  $ echo hi > r0
+  $ hg ci -qAm r0
+  $ mkdir somedir
+  $ echo hi > somedir/foo
+  $ hg ci -qAm r1
+  $ isdir somedir
+  yes
+  $ hg co -q -r ".^"
+  $ isdir somedir
+  no
+  $ hg co -q tip
+  $ isdir somedir
+  yes
+  $ hg $NO_RM co -q -r ".^"
+  $ isdir somedir
+  yes
+  $ ls somedir
+  $ cd $TESTTMP
+
+Rebasing across a commit that doesn't have the directory, from inside the
+directory:
+  $ hg init hgrebase
+  $ cd hgrebase
+  $ echo hi > r0
+  $ hg ci -qAm r0
+  $ mkdir somedir
+  $ echo hi > somedir/foo
+  $ hg ci -qAm first_rebase_source
+  $ hg $NO_RM co -q -r ".^"
+  $ echo hi > somedir/bar
+  $ hg ci -qAm first_rebase_dest
+  $ hg $NO_RM co -q -r ".^"
+  $ echo hi > somedir/baz
+  $ hg ci -qAm second_rebase_dest
+  $ hg co -qr 'desc(first_rebase_source)'
+  $ cd $TESTTMP/hgrebase/somedir
+  $ hg --config extensions.rebase= rebase -qr . -d 'desc(first_rebase_dest)'
+  current directory was removed
+  (consider changing to repo root: $TESTTMP/hgrebase)
+  $ cd $TESTTMP/hgrebase/somedir
+(The current node is the rebased first_rebase_source on top of
+first_rebase_dest)
+This should not output anything about current directory being removed:
+  $ hg $NO_RM --config extensions.rebase= rebase -qr . -d 'desc(second_rebase_dest)'
+  $ cd $TESTTMP
+
+Histediting across a commit that doesn't have the directory, from inside the
+directory (reordering nodes):
+  $ hg init hghistedit
+  $ cd hghistedit
+  $ echo hi > r0
+  $ hg ci -qAm r0
+  $ echo hi > r1
+  $ hg ci -qAm r1
+  $ echo hi > r2
+  $ hg ci -qAm r2
+  $ mkdir somedir
+  $ echo hi > somedir/foo
+  $ hg ci -qAm migrating_revision
+  $ cat > histedit_commands <<EOF
+  > pick 89079fab8aee 0 r0
+  > pick e6d271df3142 1 r1
+  > pick 89e25aa83f0f 3 migrating_revision
+  > pick b550aa12d873 2 r2
+  > EOF
+  $ cd $TESTTMP/hghistedit/somedir
+  $ hg --config extensions.histedit= histedit -q --commands ../histedit_commands
+
+histedit doesn't output anything when the current diretory is removed. We rely
+on the tests being commonly run on machines where the current directory
+disappearing from underneath us actually has an observable effect, such as an
+error or no files listed
+#if linuxormacos
+  $ isfile foo
+  no
+#endif
+  $ cd $TESTTMP/hghistedit/somedir
+  $ isfile foo
+  yes
+
+  $ cd $TESTTMP/hghistedit
+  $ cat > histedit_commands <<EOF
+  > pick 89079fab8aee 0 r0
+  > pick 7c7a22c6009f 3 migrating_revision
+  > pick e6d271df3142 1 r1
+  > pick 40a53c2d4276 2 r2
+  > EOF
+  $ cd $TESTTMP/hghistedit/somedir
+  $ hg $NO_RM --config extensions.histedit= histedit -q --commands ../histedit_commands
+Regardless of system, we should always get a 'yes' here.
+  $ isfile foo
+  yes
+  $ cd $TESTTMP
diff --git a/mercurial/vfs.py b/mercurial/vfs.py
--- a/mercurial/vfs.py
+++ b/mercurial/vfs.py
@@ -246,8 +246,9 @@
         """Attempt to remove a file, ignoring missing file errors."""
         util.tryunlink(self.join(path))
 
-    def unlinkpath(self, path=None, ignoremissing=False):
-        return util.unlinkpath(self.join(path), ignoremissing=ignoremissing)
+    def unlinkpath(self, path=None, ignoremissing=False, rmdir=True):
+        return util.unlinkpath(self.join(path), ignoremissing=ignoremissing,
+                               rmdir=rmdir)
 
     def utime(self, path=None, t=None):
         return os.utime(self.join(path), t)
diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -2139,17 +2139,18 @@
         else:
             self.close()
 
-def unlinkpath(f, ignoremissing=False):
+def unlinkpath(f, ignoremissing=False, rmdir=True):
     """unlink and remove the directory if it is empty"""
     if ignoremissing:
         tryunlink(f)
     else:
         unlink(f)
-    # try removing directories that might now be empty
-    try:
-        removedirs(os.path.dirname(f))
-    except OSError:
-        pass
+    if rmdir:
+        # try removing directories that might now be empty
+        try:
+            removedirs(os.path.dirname(f))
+        except OSError:
+            pass
 
 def tryunlink(f):
     """Attempt to remove a file, ignoring ENOENT errors."""
diff --git a/mercurial/patch.py b/mercurial/patch.py
--- a/mercurial/patch.py
+++ b/mercurial/patch.py
@@ -497,7 +497,9 @@
                 self.opener.setflags(fname, False, True)
 
     def unlink(self, fname):
-        self.opener.unlinkpath(fname, ignoremissing=True)
+        rmdir = self.ui.configbool('experimental', 'removeemptydirs')
+        self.opener.unlinkpath(fname, ignoremissing=True,
+                               rmdir=rmdir)
 
     def writerej(self, fname, failed, total, lines):
         fname = fname + ".rej"
diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -1707,7 +1707,9 @@
 
     def remove(self, ignoremissing=False):
         """wraps unlink for a repo's working directory"""
-        self._repo.wvfs.unlinkpath(self._path, ignoremissing=ignoremissing)
+        rmdir = self._repo.ui.configbool('experimental', 'removeemptydirs')
+        self._repo.wvfs.unlinkpath(self._path, ignoremissing=ignoremissing,
+                                   rmdir=rmdir)
 
     def write(self, data, flags, backgroundclose=False, **kwargs):
         """wraps repo.wwrite"""
diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -566,6 +566,9 @@
 coreconfigitem('experimental', 'remotenames',
     default=False,
 )
+coreconfigitem('experimental', 'removeemptydirs',
+    default=True,
+)
 coreconfigitem('experimental', 'revlogv2',
     default=None,
 )
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -1242,7 +1242,8 @@
                              dryrun=dryrun, cwd=cwd)
         if rename and not dryrun:
             if not after and srcexists and not samefile:
-                repo.wvfs.unlinkpath(abssrc)
+                rmdir = repo.ui.configbool('experimental', 'removeemptydirs')
+                repo.wvfs.unlinkpath(abssrc, rmdir=rmdir)
             wctx.forget([abssrc])
 
     # pat: ossep
@@ -2269,7 +2270,9 @@
                 for f in list:
                     if f in added:
                         continue # we never unlink added files on remove
-                    repo.wvfs.unlinkpath(f, ignoremissing=True)
+                    rmdir = repo.ui.configbool('experimental',
+                                               'removeemptydirs')
+                    repo.wvfs.unlinkpath(f, ignoremissing=True, rmdir=rmdir)
             repo[None].forget(list)
 
     if warn:
@@ -3029,7 +3032,8 @@
 
     def doremove(f):
         try:
-            repo.wvfs.unlinkpath(f)
+            rmdir = repo.ui.configbool('experimental', 'removeemptydirs')
+            repo.wvfs.unlinkpath(f, rmdir=rmdir)
         except OSError:
             pass
         repo.dirstate.remove(f)



To: spectral, #hg-reviewers
Cc: mercurial-devel


More information about the Mercurial-devel mailing list