Patchwork D1201: dirstate: clean up when restoring identical backups

login
register
mail settings
Submitter phabricator
Date Oct. 20, 2017, 12:57 p.m.
Message ID <differential-rev-PHID-DREV-xqz643mkcd2ds55enuth-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/25260/
State Superseded
Headers show

Comments

phabricator - Oct. 20, 2017, 12:57 p.m.
mbthomas created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  When a dirstate backup is restored, it is possible that no actual changes to
  the dirstate have been made.  In this case, the backup is still a hardlink
  to the original dirstate.
  
  Unfortunately, `os.rename` silently fails (nothing happens, and no error
  occurs) when `src` and `dst` are hardlinks to the same file.  As a result,
  the backup is left lying around.  Over time, these files accumulate.
  
  When restoring dirstate backups, check if the backup and the dirstate are
  the same file, and if so, just delete the backup.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/dirstate.py
  tests/test-dirstate-backup.t

CHANGE DETAILS




To: mbthomas, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 20, 2017, 3:20 p.m.
krbullock added subscribers: durham, krbullock.
krbullock accepted this revision as: krbullock.
krbullock added a comment.


  LGTM but I'd like someone with more familiarity with dirstate to have a look too. Maybe @durham ?

REPOSITORY
  rHG Mercurial

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

To: mbthomas, #hg-reviewers, krbullock
Cc: krbullock, durham, mercurial-devel
phabricator - Oct. 27, 2017, 3:58 p.m.
durham accepted this revision.
durham added a comment.


  lgtm.  Is this a recent regression?  Seems like this would be polluting lots of peoples repositories.

REPOSITORY
  rHG Mercurial

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

To: mbthomas, #hg-reviewers, krbullock, durham
Cc: krbullock, durham, mercurial-devel
phabricator - Oct. 27, 2017, 4:21 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D1201#20829, @durham wrote:
  
  > lgtm.  Is this a recent regression?  Seems like this would be polluting lots of peoples repositories.
  
  
  I never noticed, but now that you ask, here's in my hg repo:
  
  $ ls -1 .hg/dirstate.backup.import.*
  .hg/dirstate.backup.import.139672779498704
  .hg/dirstate.backup.import.139940118873296
  .hg/dirstate.backup.import.140117795275984
  .hg/dirstate.backup.import.140181671269584
  .hg/dirstate.backup.import.140261652248144
  .hg/dirstate.backup.import.140345443734736
  .hg/dirstate.backup.import.140542706046160
  .hg/dirstate.backup.import.140585297004112

REPOSITORY
  rHG Mercurial

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

To: mbthomas, #hg-reviewers, krbullock, durham
Cc: martinvonz, krbullock, durham, mercurial-devel
phabricator - Oct. 30, 2017, 9:05 p.m.
durin42 added a comment.


  based on behavior I've seen, this is a recent regression. Landing this series for stable.

REPOSITORY
  rHG Mercurial

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

To: mbthomas, #hg-reviewers, krbullock, durham
Cc: durin42, martinvonz, krbullock, durham, mercurial-devel

Patch

diff --git a/tests/test-dirstate-backup.t b/tests/test-dirstate-backup.t
--- a/tests/test-dirstate-backup.t
+++ b/tests/test-dirstate-backup.t
@@ -11,9 +11,8 @@ 
   abort: stdin: no diffs found
   [255]
 
-A dirstate backup is left behind
+No dirstate backups are left behind
 
   $ ls .hg/dirstate* | sort
   .hg/dirstate
-  .hg/dirstate.backup.import.* (glob)
 
diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -1189,7 +1189,11 @@ 
         # changes of dirstate out after restoring from backup file
         self.invalidate()
         filename = self._actualfilename(tr)
-        self._opener.rename(backupname, filename, checkambig=True)
+        o = self._opener
+        if util.samefile(o.join(backupname), o.join(filename)):
+            o.unlink(backupname)
+        else:
+            o.rename(backupname, filename, checkambig=True)
 
     def clearbackup(self, tr, backupname):
         '''Clear backup file'''