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
Lint Skipped
Unit
Unit Tests Skipped

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
902–920

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
902–920

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.

949

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–107

so much better!

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

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 2220

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 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 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, check if it
# conflicts with a local path.
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:
# The file is in a directory which aliases a local file.
# We will need to rename the local file.
localconflicts.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)
# Rename all local conflicting files that have not been deleted.
for p in localconflicts:
if p not in deletedfiles:
pnew = util.safename(p, str(wctx), 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.
for f in mf:
if f not in deletedfiles:
for p in util.finddirs(f):
if p in remoteconflicts:
m, args, msg = actions[p]
pnew = util.safename(p, str(mctx), 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
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…
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):
""" """
ryanmceUnsubmitted
Not Done

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

ryanmce: nit: worth a comment as to why we strip the +
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
""" """
if matcher is not None and matcher.always(): if matcher is not None and matcher.always():
matcher = None matcher = None
copy, movewithdir, diverge, renamedelete, dirmove = {}, {}, {}, {}, {} copy, movewithdir, diverge, renamedelete, dirmove = {}, {}, {}, {}, {}
# manifests fetched in order are going to be faster, so prime the caches # manifests fetched in order are going to be faster, so prime the caches
[x.manifest() for x in [x.manifest() for x in
sorted(wctx.parents() + [p2, pa], key=scmutil.intrev)] sorted(wctx.parents() + [p2, pa], key=scmutil.intrev)]
if followcopies: if followcopies:
ret = copies.mergecopies(repo, wctx, p2, pa) ret = copies.mergecopies(repo, wctx, p2, pa)
copy, movewithdir, diverge, renamedelete, dirmove = ret copy, movewithdir, diverge, renamedelete, dirmove = ret
boolbm = pycompat.bytestr(bool(branchmerge)) boolbm = pycompat.bytestr(bool(branchmerge))
boolf = pycompat.bytestr(bool(force)) boolf = pycompat.bytestr(bool(force))
boolm = pycompat.bytestr(bool(matcher)) boolm = pycompat.bytestr(bool(matcher))
repo.ui.note(_("resolving manifests\n")) repo.ui.note(_("resolving manifests\n"))
repo.ui.debug(" branchmerge: %s, force: %s, partial: %s\n" repo.ui.debug(" branchmerge: %s, force: %s, partial: %s\n"
% (boolbm, boolf, boolm)) % (boolbm, boolf, boolm))
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…
repo.ui.debug(" ancestor: %s, local: %s, remote: %s\n" % (pa, wctx, p2)) repo.ui.debug(" ancestor: %s, local: %s, remote: %s\n" % (pa, wctx, p2))
m1, m2, ma = wctx.manifest(), p2.manifest(), pa.manifest() m1, m2, ma = wctx.manifest(), p2.manifest(), pa.manifest()
copied = set(copy.values()) copied = set(copy.values())
copied.update(movewithdir.values()) copied.update(movewithdir.values())
if '.hgsubstate' in m1: if '.hgsubstate' in m1:
# check whether sub state is modified # check whether sub state is modified
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.
if branchmerge or (not force and wctx.dirty(missing=True, branch=False)):
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' abort: path 'back/test' traverses symbolic link 'back'
[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

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