This is an archive of the discontinued Mercurial Phabricator instance.

Differential D784

merge: check for path conflicts when merging (issue5628)
ClosedPublic

Authored by mbthomas on Sep 22 2017, 5:28 AM.

Details

Reviewers
ryanmce
Group Reviewers
hg-reviewers
Commits
rHG989e884d1be9: merge: check for path conflicts when merging (issue5628)
rHGfd912d81b319: merge: check for path conflicts when merging (issue5628)
Summary

When merging, check for any path conflicts introduced by the manifest
merge and rename the conflicting file to a safe name.

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 22 2017, 5:28 AM
mbthomas added inline comments.Sep 22 2017, 5:31 AM
mercurial/merge.py
905–923

This is potentially slow for large repos, as it has to iterate over the whole manifest.

What I actually want to do is find the files in the directories named by remoteconflicts, which should be a small set, however I can't see a way to do that with the manifest API. Suggestions for improvement welcome.

mbthomas updated this revision to Diff 2220.Oct 1 2017, 5:33 AM
mbthomas retitled this revision from merge: check for path conflicts when merging to merge: check for path conflicts when merging (issue5628).Oct 2 2017, 5:15 PM
mbthomas updated this revision to Diff 2361.
ryanmce accepted this revision.Oct 5 2017, 12:46 PM
ryanmce added a subscriber: ryanmce.

I'm sooooo excited by these improvements. one nit inline but this is such an improvement!

mercurial/merge.py
905–923

Correct is more important. In a future with treemanifest, I could see some potential optimization opportunities. We may consider experimenting with them inside of treemanifest and upstream the results if this turns out to be a performance concern in very large repos in the wild.

952

nit: we can be more precise here for clarity:

repo.ui.warn(_("%s: is both a file and a directory at destination\n") % p)
tests/test-audit-path.t
106–108

so much better!

ryanmce added inline comments.Oct 5 2017, 12:52 PM
mercurial/merge.py
927

nit: worth a comment as to why we strip the +

Closed by commit rHGfd912d81b319: merge: check for path conflicts when merging (issue5628) (authored by mbthomas). · Explain WhyOct 9 2017, 9:30 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 2555

mercurial/merge.py

return True return True
def driverconclude(repo, ms, wctx, labels=None): def driverconclude(repo, ms, wctx, labels=None):
"""run the conclude step of the merge driver, if any """run the conclude step of the merge driver, if any
This is currently not implemented -- it's an extension point.""" This is currently not implemented -- it's an extension point."""
return True return True
def _filesindirs(repo, manifest, dirs):
"""
Generator that yields pairs of all the files in the manifest that are found
inside the directories listed in dirs, and which directory they are found
in.
"""
for f in manifest:
for p in util.finddirs(f):
if p in dirs:
yield f, p
break
def checkpathconflicts(repo, wctx, mctx, actions):
"""
Check if any actions introduce path conflicts in the repository, updating
actions to record or handle the path conflict accordingly.
"""
mf = wctx.manifest()
# The set of local files that conflict with a remote directory.
localconflicts = set()
# The set of directories that conflict with a remote file, and so may cause
# conflicts if they still contain any files after the merge.
remoteconflicts = set()
# The set of directories that appear as both a file and a directory in the
# remote manifest. These indicate an invalid remote manifest, which
# can't be updated to cleanly.
invalidconflicts = set()
# The set of files deleted by all the actions.
deletedfiles = set()
for f, (m, args, msg) in actions.items():
if m in ('c', 'dc', 'm', 'cm'):
# This action may create a new local file.
if mf.hasdir(f):
# The file aliases a local directory. This might be ok if all
# the files in the local directory are being deleted. This
# will be checked once we know what all the deleted files are.
remoteconflicts.add(f)
for p in util.finddirs(f):
if p in mf:
if p in mctx:
# The file is in a directory which aliases both a local
# and a remote file. This is an internal inconsistency
# within the remote manifest.
invalidconflicts.add(p)
else:
# The file is in a directory which aliases a local file.
# We will need to rename the local file.
localconflicts.add(p)
if p in actions and actions[p][0] in ('c', 'dc', 'm', 'cm'):
# The file is in a directory which aliases a remote file.
# This is an internal inconsistency within the remote
# manifest.
invalidconflicts.add(p)
# Track the names of all deleted files.
if m == 'r':
deletedfiles.add(f)
if m == 'm':
f1, f2, fa, move, anc = args
if move:
deletedfiles.add(f1)
if m == 'dm':
f2, flags = args
deletedfiles.add(f2)
mbthomasAuthorUnsubmitted
Not Done

This is potentially slow for large repos, as it has to iterate over the whole manifest.

What I actually want to do is find the files in the directories named by remoteconflicts, which should be a small set, however I can't see a way to do that with the manifest API. Suggestions for improvement welcome.

mbthomas: This is potentially slow for large repos, as it has to iterate over the whole manifest. What I…
ryanmceUnsubmitted
Not Done

Correct is more important. In a future with treemanifest, I could see some potential optimization opportunities. We may consider experimenting with them inside of treemanifest and upstream the results if this turns out to be a performance concern in very large repos in the wild.

ryanmce: Correct is more important. In a future with treemanifest, I could see some potential…
# Rename all local conflicting files that have not been deleted.
for p in localconflicts:
if p not in deletedfiles:
ctxname = str(wctx).rstrip('+')
ryanmceUnsubmitted
Not Done

nit: worth a comment as to why we strip the +

ryanmce: nit: worth a comment as to why we strip the +
pnew = util.safename(p, ctxname, wctx, set(actions.keys()))
actions[pnew] = ('pr', (p,), "local path conflict")
actions[p] = ('p', (pnew, 'l'), "path conflict")
if remoteconflicts:
# Check if all files in the conflicting directories have been removed.
ctxname = str(mctx).rstrip('+')
for f, p in _filesindirs(repo, mf, remoteconflicts):
if f not in deletedfiles:
m, args, msg = actions[p]
pnew = util.safename(p, ctxname, wctx, set(actions.keys()))
if m in ('dc', 'm'):
# Action was merge, just update target.
actions[pnew] = (m, args, msg)
else:
# Action was create, change to renamed get action.
fl = args[0]
actions[pnew] = ('dg', (p, fl), "remote path conflict")
actions[p] = ('p', (pnew, 'r'), "path conflict")
remoteconflicts.remove(p)
break
if invalidconflicts:
for p in invalidconflicts:
repo.ui.warn(_("%s: is both a file and a directory\n") % p)
ryanmceUnsubmitted
Not Done

nit: we can be more precise here for clarity:

repo.ui.warn(_("%s: is both a file and a directory at destination\n") % p)
ryanmce: nit: we can be more precise here for clarity: ``` repo.ui.warn(_("%s: is both a file and a…
raise error.Abort(_("destination manifest contains path conflicts"))
def manifestmerge(repo, wctx, p2, pa, branchmerge, force, matcher, def manifestmerge(repo, wctx, p2, pa, branchmerge, force, matcher,
acceptremote, followcopies, forcefulldiff=False): acceptremote, followcopies, forcefulldiff=False):
""" """
Merge wctx and p2 with ancestor pa and generate merge action list Merge wctx and p2 with ancestor pa and generate merge action list
branchmerge and force are as passed in to update branchmerge and force are as passed in to update
matcher = matcher to filter file lists matcher = matcher to filter file lists
acceptremote = accept the incoming changes without prompting acceptremote = accept the incoming changes without prompting
actions[df] = ('m', (df, f, f, False, pa.node()), actions[df] = ('m', (df, f, f, False, pa.node()),
"local directory rename - respect move from " + f) "local directory rename - respect move from " + f)
elif acceptremote: elif acceptremote:
actions[f] = ('c', (fl2,), "remote recreating") actions[f] = ('c', (fl2,), "remote recreating")
else: else:
actions[f] = ('dc', (None, f, f, False, pa.node()), actions[f] = ('dc', (None, f, f, False, pa.node()),
"prompt deleted/changed") "prompt deleted/changed")
# If we are merging, look for path conflicts.
checkpathconflicts(repo, wctx, p2, actions)
return actions, diverge, renamedelete return actions, diverge, renamedelete
def _resolvetrivial(repo, wctx, mctx, ancestor, actions): def _resolvetrivial(repo, wctx, mctx, ancestor, actions):
"""Resolves false conflicts where the nodeid changed but the content """Resolves false conflicts where the nodeid changed but the content
remained the same.""" remained the same."""
for f, (m, args, msg) in actions.items(): for f, (m, args, msg) in actions.items():
if m == 'cd' and f in ancestor and not wctx[f].cmp(ancestor[f]): if m == 'cd' and f in ancestor and not wctx[f].cmp(ancestor[f]):

tests/test-audit-path.t

attack back/test where back symlinks to .. attack back/test where back symlinks to ..
$ hg manifest -r2 $ hg manifest -r2
back back
back/test back/test
#if symlink #if symlink
$ hg update -Cr2 $ hg update -Cr2
abort: path 'back/test' traverses symbolic link 'back' back: is both a file and a directory
abort: destination manifest contains path conflicts
[255] [255]
ryanmceUnsubmitted
Not Done

so much better!

ryanmce: so much better!
#else #else
('back' will be a file and cause some other system specific error) ('back' will be a file and cause some other system specific error)
$ hg update -Cr2 $ hg update -Cr2
abort: * (glob) abort: * (glob)
[255] [255]
#endif #endif
attack ../test attack ../test
|/ |/
o 0: base o 0: base
try trivial merge try trivial merge
$ hg up -qC 1 $ hg up -qC 1
$ hg merge 2 $ hg merge 2
abort: path 'a/poisoned' traverses symbolic link 'a' a: path conflict - a file or link has the same name as a directory
[255] the local file has been renamed to a~aa04623eb0c3
resolve manually then use 'hg resolve --mark a'
1 files updated, 0 files merged, 0 files removed, 1 files unresolved
use 'hg resolve' to retry unresolved file merges or 'hg update -C .' to abandon
[1]
try rebase onto other revision: cache of audited paths should be discarded, try rebase onto other revision: cache of audited paths should be discarded,
and the rebase should fail (issue5628) and the rebase should fail (issue5628)
$ hg up -qC 2 $ hg up -qC 2
$ hg rebase -s 2 -d 1 --config extensions.rebase= $ hg rebase -s 2 -d 1 --config extensions.rebase=
rebasing 2:e73c21d6b244 "file a/poisoned" (tip) rebasing 2:e73c21d6b244 "file a/poisoned" (tip)
abort: path 'a/poisoned' traverses symbolic link 'a' a: path conflict - a file or link has the same name as a directory
[255] the local file has been renamed to a~aa04623eb0c3
resolve manually then use 'hg resolve --mark a'
unresolved conflicts (see hg resolve, then hg rebase --continue)
[1]
$ ls ../merge-symlink-out $ ls ../merge-symlink-out
$ cd .. $ cd ..
Test symlink traversal on update: Test symlink traversal on update:
--------------------------------- ---------------------------------
$ mkdir update-symlink-out $ mkdir update-symlink-out
|/ |/
o 0: symlink a -> ../update-symlink-out o 0: symlink a -> ../update-symlink-out
try linear update where symlink already exists: try linear update where symlink already exists:
$ hg up -qC 0 $ hg up -qC 0
$ hg up 1 $ hg up 1
abort: path 'a/b' traverses symbolic link 'a' a: is both a file and a directory
abort: destination manifest contains path conflicts
[255] [255]
try linear update including symlinked directory and its content: paths are try linear update including symlinked directory and its content: paths are
audited first by calculateupdates(), where no symlink is created so both audited first by calculateupdates(), where no symlink is created so both
'a' and 'a/b' are taken as good paths. still applyupdates() should fail. 'a' and 'a/b' are taken as good paths. still applyupdates() should fail.
$ hg up -qC null $ hg up -qC null
$ hg up 1 $ hg up 1
abort: path 'a/b' traverses symbolic link 'a' a: is both a file and a directory
abort: destination manifest contains path conflicts
[255] [255]
$ ls ../update-symlink-out $ ls ../update-symlink-out
try branch update replacing directory with symlink, and its content: the try branch update replacing directory with symlink, and its content: the
path 'a' is audited as a directory first, which should be audited again as path 'a' is audited as a directory first, which should be audited again as
a symlink. a symlink.
$ rm -f a $ rm -f a
$ hg up -qC 2 $ hg up -qC 2
$ hg up 1 $ hg up 1
abort: path 'a/b' traverses symbolic link 'a' a: is both a file and a directory
abort: destination manifest contains path conflicts
[255] [255]
$ ls ../update-symlink-out $ ls ../update-symlink-out
$ cd .. $ cd ..
#endif #endif

tests/test-commandserver.t

... # audit a/poisoned as a good path ... # audit a/poisoned as a good path
... runcommand(server, ['up', '-qC', '2']) ... runcommand(server, ['up', '-qC', '2'])
... runcommand(server, ['up', '-qC', '1']) ... runcommand(server, ['up', '-qC', '1'])
... # here a is a symlink, so a/poisoned is bad ... # here a is a symlink, so a/poisoned is bad
... runcommand(server, ['merge', '2']) ... runcommand(server, ['merge', '2'])
*** runcommand up -qC 2 *** runcommand up -qC 2
*** runcommand up -qC 1 *** runcommand up -qC 1
*** runcommand merge 2 *** runcommand merge 2
abort: path 'a/poisoned' traverses symbolic link 'a' a: path conflict - a file or link has the same name as a directory
[255] the local file has been renamed to a~aa04623eb0c3
resolve manually then use 'hg resolve --mark a'
1 files updated, 0 files merged, 0 files removed, 1 files unresolved
use 'hg resolve' to retry unresolved file merges or 'hg update -C .' to abandon
[1]
$ ls ../merge-symlink-out $ ls ../merge-symlink-out
cache of repo.auditor should be discarded, so matcher would never traverse cache of repo.auditor should be discarded, so matcher would never traverse
symlinks: symlinks:
$ hg up -qC 0 $ hg up -qC 0
$ touch ../merge-symlink-out/poisoned $ touch ../merge-symlink-out/poisoned
>>> from hgclient import check, readchannel, runcommand >>> from hgclient import check, readchannel, runcommand

tests/test-pathconflicts-basic.t

$ hg bookmark -i dir $ hg bookmark -i dir
Basic merge - local file conflicts with remote directory Basic merge - local file conflicts with remote directory
$ hg up -q file $ hg up -q file
$ hg bookmark -i $ hg bookmark -i
$ hg merge --verbose dir $ hg merge --verbose dir
resolving manifests resolving manifests
a: path conflict - a file or link has the same name as a directory
the local file has been renamed to a~853701544ac3
resolve manually then use 'hg resolve --mark a'
moving a to a~853701544ac3
getting a/b getting a/b
abort: *: '$TESTTMP/repo/a/b' (glob) 1 files updated, 0 files merged, 0 files removed, 1 files unresolved
[255] use 'hg resolve' to retry unresolved file merges or 'hg update -C .' to abandon
[1]
$ hg update --clean . $ hg update --clean .
0 files updated, 0 files merged, 0 files removed, 0 files unresolved 1 files updated, 0 files merged, 1 files removed, 0 files unresolved
Basic update - local directory conflicts with remote file Basic update - local directory conflicts with remote file
$ hg up -q 0 $ hg up -q 0
$ mkdir a $ mkdir a
$ echo 3 > a/b $ echo 3 > a/b
$ hg up file $ hg up file
a: untracked directory conflicts with file a: untracked directory conflicts with file