( )⚙ D1201 dirstate: clean up when restoring identical backups

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1201

dirstate: clean up when restoring identical backups
ClosedPublic

Authored by mbthomas on Oct 20 2017, 8:57 AM.

Details

Reviewers
krbullock
durham
durin42
Group Reviewers
hg-reviewers
Commits
rHGc2b30348930f: dirstate: clean up when restoring identical backups
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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

mbthomas created this revision.Oct 20 2017, 8:57 AM
krbullock added subscribers: durham, krbullock.Oct 20 2017, 11:20 AM
krbullock accepted this revision as: krbullock.

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

durham accepted this revision.Oct 27 2017, 11:58 AM

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

martinvonz added a subscriber: martinvonz.Oct 27 2017, 12:21 PM
In 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

durin42 added a subscriber: durin42.Oct 30 2017, 5:05 PM

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

durin42 accepted this revision.Oct 30 2017, 5:06 PM
This revision is now accepted and ready to land.Oct 30 2017, 5:06 PM
Closed by commit rHGc2b30348930f: dirstate: clean up when restoring identical backups (authored by mbthomas). · Explain WhyOct 30 2017, 5:30 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/dirstate.py (6 lines)
Mtests/test-dirstate-backup.t (3 lines)

Diff 3048

mercurial/dirstate.py

self._opener.join(backupname), hardlink=True) self._opener.join(backupname), hardlink=True)
def restorebackup(self, tr, backupname): def restorebackup(self, tr, backupname):
'''Restore dirstate by backup file''' '''Restore dirstate by backup file'''
# this "invalidate()" prevents "wlock.release()" from writing # this "invalidate()" prevents "wlock.release()" from writing
# changes of dirstate out after restoring from backup file # changes of dirstate out after restoring from backup file
self.invalidate() self.invalidate()
filename = self._actualfilename(tr) 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): def clearbackup(self, tr, backupname):
'''Clear backup file''' '''Clear backup file'''
self._opener.unlink(backupname) self._opener.unlink(backupname)
class dirstatemap(object): class dirstatemap(object):
def __init__(self, ui, opener, root): def __init__(self, ui, opener, root):
self._ui = ui self._ui = ui

tests/test-dirstate-backup.t

Set up Set up
$ hg init repo $ hg init repo
$ cd repo $ cd repo
Try to import an empty patch Try to import an empty patch
$ hg import --no-commit - <<EOF $ hg import --no-commit - <<EOF
> EOF > EOF
applying patch from stdin applying patch from stdin
abort: stdin: no diffs found abort: stdin: no diffs found
[255] [255]
A dirstate backup is left behind No dirstate backups are left behind
$ ls .hg/dirstate* | sort $ ls .hg/dirstate* | sort
.hg/dirstate .hg/dirstate
.hg/dirstate.backup.import.* (glob)