This is an archive of the discontinued Mercurial Phabricator instance.

Differential D680

scmutil: handle conflicting files and dirs in origbackuppath
ClosedPublic

Authored by mbthomas on Sep 11 2017, 2:03 PM.

Details

Reviewers
ryanmce
durham
yuja
ikostia
Group Reviewers
hg-reviewers
Commits
rHG6fad8059a970: scmutil: handle conflicting files and dirs in origbackuppath
rHGe68deea20e9a: scmutil: handle conflicting files and dirs in origbackuppath
Summary

When ui.origbackuppath is set, .orig files are stored outside of the working
copy. However conflicts can occur when files or directories end up having the
same name. These conflicts cause Mercurial to abort, even if they've been
created as a result of different backups.

Make sure we always replace files or directories in the origbackuppath if
they conflict with another file or directory.

Test Plan

Add new unit test for conflicting paths.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mbthomas created this revision.Sep 11 2017, 2:03 PM
ryanmce accepted this revision.Sep 11 2017, 2:21 PM

looks good

durham accepted this revision.Sep 12 2017, 12:01 PM
durham added a subscriber: durham.
durham added inline comments.
mercurial/scmutil.py
599

I tried to think of where this could go wrong or be abused. I think it's fine though.

Like if someone set ui.origbackuppath to empty string I think the wjoin above would result in paths inside the working copy, and rmtree would then delete parts of the working copy. So maybe the if origbackuppath is None: in line 559 should be if not origbackuppath:

yuja requested changes to this revision.Sep 13 2017, 10:54 AM
yuja added a subscriber: yuja.
yuja added inline comments.
mercurial/scmutil.py
591

Perhaps fullorigpath may be a Windows path, but util.finddirs()
can only handle internal path.

595

It shouldn't delete files out of the origbackuppath directory
even if the configured path points to (or under) a file.

Maybe we could use a new vfs dedicated to backup directory to
audit operations.

This revision now requires changes to proceed.Sep 13 2017, 10:54 AM
mbthomas updated this revision to Diff 1892.Sep 19 2017, 5:46 AM
mbthomas planned changes to this revision.Sep 19 2017, 5:47 AM
mbthomas updated this revision to Diff 1893.Sep 19 2017, 6:17 AM
mbthomas updated this revision to Diff 1894.
mbthomas marked 2 inline comments as done.Sep 19 2017, 6:21 AM
mbthomas added inline comments.
mercurial/scmutil.py
591

I've changed it to use util.normpath, which I think converts back to / as separator on windows (windows.normpath calls windows.pconvert), and is probably a good idea anyway. Is this sufficient?

yuja added inline comments.Sep 19 2017, 11:12 AM
mercurial/scmutil.py
591

That's probably okay now.

I'm not sure if it conflicts with the long paths plan. IIRC, forward
slash isn't allowed in \\?\ path.

ryanmce added a subscriber: ikostia.Sep 19 2017, 11:21 AM

Calling in windows path expert @ikostia

ikostia requested changes to this revision.Sep 19 2017, 11:36 AM

Please wait until the util.finddirs() is tweaked and remove normpath call.

mercurial/scmutil.py
591

I think we should adjust finddirs to split on os.path.sep, not hardcoded /, then we can get rid of util.normpath a couple of lines above.

I am long overdue with re-sending my series, I will add this change there as well.

This revision now requires changes to proceed.Sep 19 2017, 11:36 AM
mbthomas added inline comments.Sep 20 2017, 9:12 AM
mercurial/scmutil.py
591

At this point the path is a vfs-path within the repo, so it should be using / as a separator.

In the case of UNC paths, os.path.relpath seems to do the right thing, so os.path.relpath(r'\\?\c:\repo\dir\file', r'\\?\c:\repo') gives dir\file, which util.normpath converts to dir/file.

It's a bit of a shame that scmutil.origpath takes an absolute path rather than a vfs path within the repo, as it means we have to jump through these hoops. It doesn't look like we can easily change it, though.

quark added a subscriber: quark.Sep 27 2017, 6:31 PM

If it's just file and directory conflict, maybe we can encode the path so it becomes impossible to have such conflict. See _encodedir in store.py for an example. It requires a suffix so partially reverts D679 though.

mbthomas updated this revision to Diff 2208.Oct 1 2017, 5:32 AM
ikostia added a comment.Oct 1 2017, 5:39 AM

Just as an FYI: I revoke my previous comments, as stuff started working for non-prefixed paths and I no longer intend to land anything related to this.

• kiilerix added a subscriber: • kiilerix.Oct 1 2017, 6:42 AM

I am concerned that this seems to rely even more on not having conflicts inside origbackuppath from files/dirs with same name but in different directories. If we have a backup file handling, it must be 100% reliable. That seems to be what this series is trying to fix - then it seems unfortunate it just moves the problem elsewhere.

mercurial/scmutil.py
578

Now, this seems like a separate trivial change, fixing the problem that origbackuppath couldn't be overridden.

mbthomas added a comment.Oct 2 2017, 5:04 PM

With ui.origbackuppath set to some value, backup should be completely reliable. The file at $origbackuppath/path/to/file will be the most recent version of that file that was backed up. It will be overwritten/deleted if a backup of any file named $origbackuppath/path/to/file, $origbackuppath/path/to or $origbackuppath/path is subsequently made. This seems reasonable to me.

If ui.origbackuppath not set, then behaviour is still unreliable in the presence of files also named something.orig, but that is not changed behaviour. We discussed this at the sprint and decided that we didn't want to change the default behaviour of creating .orig files.

mercurial/scmutil.py
578

Happy to split this out, but this stack is already pretty tall.

mbthomas updated this revision to Diff 2350.Oct 2 2017, 5:14 PM
ryanmce accepted this revision.Oct 5 2017, 9:46 AM

It looks like all concerns have been addressed. This looks good to me.

mercurial/scmutil.py
584–587

In the future, we may want to move this to the repo class like other vfs instances. That will mean we don't have to re-instantiate this each time. However, these instantiations look cheap so I'm not worried and it's outside the scope of this change.

591

I agree that it seems out of scope to fix this in this patch series. Might be worth opening a task in bugzilla to track improving this, but I think this series is already fairly long and too important to hold up on a small thing like this. Since it works, I think we should move towards shipping it.

595

@mbthomas and I had talked about adding a new vfs; thanks for pushing us in that direction. Getting a dedicated vfs feels like a good win here.

Closed by commit rHGe68deea20e9a: scmutil: handle conflicting files and dirs in origbackuppath (authored by mbthomas). · Explain WhyOct 9 2017, 9:25 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/scmutil.py (33 lines)
AMtests/test-origbackup-conflict.t (118 lines)

Diff 2543

mercurial/scmutil.py

obsutil, obsutil,
pathutil, pathutil,
phases, phases,
pycompat, pycompat,
revsetlang, revsetlang,
similar, similar,
url, url,
util, util,
vfs,
) )
if pycompat.osname == 'nt': if pycompat.osname == 'nt':
from . import scmwindows as scmplatform from . import scmwindows as scmplatform
else: else:
from . import scmposix as scmplatform from . import scmposix as scmplatform
termsize = scmplatform.termsize termsize = scmplatform.termsize
def origpath(ui, repo, filepath): def origpath(ui, repo, filepath):
'''customize where .orig files are created '''customize where .orig files are created
Fetch user defined path from config file: [ui] origbackuppath = <path> Fetch user defined path from config file: [ui] origbackuppath = <path>
Fall back to default (filepath with .orig suffix) if not specified Fall back to default (filepath with .orig suffix) if not specified
''' '''
origbackuppath = ui.config('ui', 'origbackuppath') origbackuppath = ui.config('ui', 'origbackuppath')
if origbackuppath is None: if not origbackuppath:
return filepath + ".orig" return filepath + ".orig"
kiilerixUnsubmitted
Not Done

Now, this seems like a separate trivial change, fixing the problem that origbackuppath couldn't be overridden.

kiilerix: Now, this seems like a separate trivial change, fixing the problem that origbackuppath couldn't…
mbthomasAuthorUnsubmitted
Not Done

Happy to split this out, but this stack is already pretty tall.

mbthomas: Happy to split this out, but this stack is already pretty tall.
filepathfromroot = os.path.relpath(filepath, start=repo.root) # Convert filepath from an absolute path into a path inside the repo.
fullorigpath = repo.wjoin(origbackuppath, filepathfromroot) filepathfromroot = util.normpath(os.path.relpath(filepath,
start=repo.root))
origvfs = vfs.vfs(repo.wjoin(origbackuppath))
origbackupdir = origvfs.dirname(filepathfromroot)
if not origvfs.isdir(origbackupdir) or origvfs.islink(origbackupdir):
ui.note(_('creating directory: %s\n') % origvfs.join(origbackupdir))
ryanmceUnsubmitted
Not Done

In the future, we may want to move this to the repo class like other vfs instances. That will mean we don't have to re-instantiate this each time. However, these instantiations look cheap so I'm not worried and it's outside the scope of this change.

ryanmce: In the future, we may want to move this to the repo class like other `vfs` instances. That will…
# Remove any files that conflict with the backup file's path
for f in reversed(list(util.finddirs(filepathfromroot))):
if origvfs.isfileorlink(f):
yujaUnsubmitted
Not Done

Perhaps fullorigpath may be a Windows path, but util.finddirs()
can only handle internal path.

yuja: Perhaps fullorigpath may be a Windows path, but util.finddirs() can only handle internal path.
mbthomasAuthorUnsubmitted
Not Done

I've changed it to use util.normpath, which I think converts back to / as separator on windows (windows.normpath calls windows.pconvert), and is probably a good idea anyway. Is this sufficient?

mbthomas: I've changed it to use `util.normpath`, which I think converts back to `/` as separator on…
yujaUnsubmitted
Not Done

That's probably okay now.

I'm not sure if it conflicts with the long paths plan. IIRC, forward
slash isn't allowed in \\?\ path.

yuja: That's probably okay now. I'm not sure if it conflicts with the long paths plan. IIRC, forward…
ikostiaUnsubmitted
Not Done

I think we should adjust finddirs to split on os.path.sep, not hardcoded /, then we can get rid of util.normpath a couple of lines above.

I am long overdue with re-sending my series, I will add this change there as well.

ikostia: I think we should adjust `finddirs` to split on `os.path.sep`, not hardcoded `/`, then we can…
mbthomasAuthorUnsubmitted
Not Done

At this point the path is a vfs-path within the repo, so it should be using / as a separator.

In the case of UNC paths, os.path.relpath seems to do the right thing, so os.path.relpath(r'\\?\c:\repo\dir\file', r'\\?\c:\repo') gives dir\file, which util.normpath converts to dir/file.

It's a bit of a shame that scmutil.origpath takes an absolute path rather than a vfs path within the repo, as it means we have to jump through these hoops. It doesn't look like we can easily change it, though.

mbthomas: At this point the path is a vfs-path within the repo, so it should be using `/` as a separator.
ryanmceUnsubmitted
Not Done

I agree that it seems out of scope to fix this in this patch series. Might be worth opening a task in bugzilla to track improving this, but I think this series is already fairly long and too important to hold up on a small thing like this. Since it works, I think we should move towards shipping it.

ryanmce: I agree that it seems out of scope to fix this in this patch series. Might be worth opening a…
ui.note(_('removing conflicting file: %s\n')
% origvfs.join(f))
origvfs.unlink(f)
break
yujaUnsubmitted
Done

It shouldn't delete files out of the origbackuppath directory
even if the configured path points to (or under) a file.

Maybe we could use a new vfs dedicated to backup directory to
audit operations.

yuja: It shouldn't delete files out of the origbackuppath directory even if the configured path…
ryanmceUnsubmitted
Not Done

@mbthomas and I had talked about adding a new vfs; thanks for pushing us in that direction. Getting a dedicated vfs feels like a good win here.

ryanmce: @mbthomas and I had talked about adding a new vfs; thanks for pushing us in that direction.
origvfs.makedirs(origbackupdir)
origbackupdir = repo.vfs.dirname(fullorigpath) if origvfs.isdir(filepathfromroot):
durhamUnsubmitted
Done

I tried to think of where this could go wrong or be abused. I think it's fine though.

Like if someone set ui.origbackuppath to empty string I think the wjoin above would result in paths inside the working copy, and rmtree would then delete parts of the working copy. So maybe the if origbackuppath is None: in line 559 should be if not origbackuppath:

durham: I tried to think of where this could go wrong or be abused. I //think// it's fine though. Like…
if not repo.vfs.exists(origbackupdir): ui.note(_('removing conflicting directory: %s\n')
ui.note(_('creating directory: %s\n') % origbackupdir) % origvfs.join(filepathfromroot))
util.makedirs(origbackupdir) origvfs.rmtree(filepathfromroot, forcibly=True)
return fullorigpath return origvfs.join(filepathfromroot)
class _containsnode(object): class _containsnode(object):
"""proxy __contains__(node) to container.__contains__ which accepts revs""" """proxy __contains__(node) to container.__contains__ which accepts revs"""
def __init__(self, repo, revcontainer): def __init__(self, repo, revcontainer):
self._torev = repo.changelog.rev self._torev = repo.changelog.rev
self._revcontains = revcontainer.__contains__ self._revcontains = revcontainer.__contains__

tests/test-origbackup-conflict.t

  • This file was added.
Set up repo
$ cat << EOF >> $HGRCPATH
> [ui]
> origbackuppath=.hg/origbackups
> [merge]
> checkunknown=warn
> EOF
$ hg init repo
$ cd repo
$ echo base > base
$ hg add base
$ hg commit -m "base"
Make a dir named b that contains a file
$ mkdir -p b
$ echo c1 > b/c
$ hg add b/c
$ hg commit -m "c1"
$ hg bookmark c1
Peform an update that causes b/c to be backed up
$ hg up -q 0
$ mkdir -p b
$ echo c2 > b/c
$ hg up --verbose c1
resolving manifests
b/c: replacing untracked file
getting b/c
creating directory: $TESTTMP/repo/.hg/origbackups/b
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
(activating bookmark c1)
$ test -f .hg/origbackups/b/c
Make a file named b
$ hg up -q 0
$ echo b1 > b
$ hg add b
$ hg commit -m b1
created new head
$ hg bookmark b1
Perform an update that causes b to be backed up - it should replace the backup b dir
$ hg up -q 0
$ echo b2 > b
$ hg up --verbose b1
resolving manifests
b: replacing untracked file
getting b
removing conflicting directory: $TESTTMP/repo/.hg/origbackups/b
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
(activating bookmark b1)
$ test -f .hg/origbackups/b
Perform an update the causes b/c to be backed up again - it should replace the backup b file
$ hg up -q 0
$ mkdir b
$ echo c3 > b/c
$ hg up --verbose c1
resolving manifests
b/c: replacing untracked file
getting b/c
creating directory: $TESTTMP/repo/.hg/origbackups/b
removing conflicting file: $TESTTMP/repo/.hg/origbackups/b
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
(activating bookmark c1)
$ test -d .hg/origbackups/b
Cause a symlink to be backed up that points to a valid location from the backup dir
$ hg up -q 0
$ mkdir ../sym-link-target
$ ln -s ../../../sym-link-target b
$ hg up b1
b: replacing untracked file
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
(activating bookmark b1)
$ readlink .hg/origbackups/b
../../../sym-link-target
Perform an update that causes b/c to be backed up again - it should not go into the target dir
$ hg up -q 0
$ mkdir b
$ echo c4 > b/c
$ hg up --verbose c1
resolving manifests
b/c: replacing untracked file
getting b/c
creating directory: $TESTTMP/repo/.hg/origbackups/b
removing conflicting file: $TESTTMP/repo/.hg/origbackups/b
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
(activating bookmark c1)
$ cat .hg/origbackups/b/c
c4
$ ls ../sym-link-target
Incorrectly configure origbackuppath to be under a file
$ echo data > .hg/badorigbackups
$ hg up -q 0
$ mkdir b
$ echo c5 > b/c
$ hg up --verbose c1 --config ui.origbackuppath=.hg/badorigbackups
resolving manifests
b/c: replacing untracked file
getting b/c
creating directory: $TESTTMP/repo/.hg/badorigbackups/b
abort: Not a directory: '$TESTTMP/repo/.hg/badorigbackups/b'
[255]
$ cat .hg/badorigbackups
data