This is an archive of the discontinued Mercurial Phabricator instance.

Differential D779

merge: add _checkunknowndirs function for detecting path conflicts
ClosedPublic

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

Details

Reviewers
ryanmce
Group Reviewers
hg-reviewers
Commits
rHG53e4bcab346b: merge: add _checkunknowndirs function for detecting path conflicts
rHG48d83ba649e1: merge: add _checkunknowndirs function for detecting path conflicts
Summary

Add a new function which, given a file name, finds the shortest path for which
there is a conflicting file or directory in the working directory.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

mbthomas created this revision.Sep 22 2017, 5:28 AM
ryanmce added a subscriber: ryanmce.Sep 25 2017, 3:58 PM
ryanmce added inline comments.
mercurial/merge.py
654

Why do we need to audit this path first? If we fail the audit, what happens? This looks weird to me.

mbthomas added inline comments.Sep 26 2017, 12:22 PM
mercurial/merge.py
654

As in _checkunknownfile, this is to avoid symlink traversal while checking these files.

Consider:

$ ln -s bar foo
$ hg add foo
$ hg up <some revision where foo is a directory containing directories>

When checking foo/dir1/file1, we will go through foo then foo/dir1 in this loop. The first case doesn't trigger the rule, as although foo is a link, it's in the dirstate, so should have been resolved by the merge. The second loop will dereference foo and look at bar/dir1, which we don't want it to do.

Doing nothing is the right thing here - we will resolve the pathconflict via the merge resolution (there will be a 'pr' action for this link).

mbthomas updated this revision to Diff 2215.Oct 1 2017, 5:32 AM
mbthomas updated this revision to Diff 2356.Oct 2 2017, 5:15 PM
ryanmce accepted this revision.Oct 5 2017, 12:28 PM

Would love to see the comment added, but the code looks good to me.

mercurial/merge.py
654

This should be turned into a comment in the code. It's super useful to have this concrete example so future readers don't have the same question I did here.

Closed by commit rHG48d83ba649e1: merge: add _checkunknowndirs function for detecting path conflicts (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/merge.py (28 lines)

Diff 2215

mercurial/merge.py

def _checkunknownfile(repo, wctx, mctx, f, f2=None): def _checkunknownfile(repo, wctx, mctx, f, f2=None):
if f2 is None: if f2 is None:
f2 = f f2 = f
return (repo.wvfs.audit.check(f) return (repo.wvfs.audit.check(f)
and repo.wvfs.isfileorlink(f) and repo.wvfs.isfileorlink(f)
and repo.dirstate.normalize(f) not in repo.dirstate and repo.dirstate.normalize(f) not in repo.dirstate
and mctx[f2].cmp(wctx[f])) and mctx[f2].cmp(wctx[f]))
def _checkunknowndirs(repo, f):
"""
Look for any unknown files or directories that may have a path conflict
with a file. If any path prefix of the file exists as a file or link,
then it conflicts. If the file itself is a directory that contains any
file that is not tracked, then it conflicts.
Returns the shortest path at which a conflict occurs, or None if there is
no conflict.
"""
# Check for path prefixes that exist as unknown files.
for p in reversed(list(util.finddirs(f))):
if (repo.wvfs.audit.check(p)
ryanmceUnsubmitted
Not Done

Why do we need to audit this path first? If we fail the audit, what happens? This looks weird to me.

ryanmce: Why do we need to audit this path first? If we fail the audit, what happens? This looks weird…
mbthomasAuthorUnsubmitted
Not Done

As in _checkunknownfile, this is to avoid symlink traversal while checking these files.

Consider:

$ ln -s bar foo
$ hg add foo
$ hg up <some revision where foo is a directory containing directories>

When checking foo/dir1/file1, we will go through foo then foo/dir1 in this loop. The first case doesn't trigger the rule, as although foo is a link, it's in the dirstate, so should have been resolved by the merge. The second loop will dereference foo and look at bar/dir1, which we don't want it to do.

Doing nothing is the right thing here - we will resolve the pathconflict via the merge resolution (there will be a 'pr' action for this link).

mbthomas: As in `_checkunknownfile`, this is to avoid symlink traversal while checking these files.
ryanmceUnsubmitted
Not Done

This should be turned into a comment in the code. It's super useful to have this concrete example so future readers don't have the same question I did here.

ryanmce: This should be turned into a comment in the code. It's super useful to have this concrete…
and repo.wvfs.isfileorlink(p)
and repo.dirstate.normalize(p) not in repo.dirstate):
return p
# Check if the file conflicts with a directory containing unknown files.
if repo.wvfs.audit.check(f) and repo.wvfs.isdir(f):
# Does the directory contain any files that are not in the dirstate?
for p, dirs, files in repo.wvfs.walk(f):
for fn in files:
relf = repo.dirstate.normalize(repo.wvfs.reljoin(p, fn))
if relf not in repo.dirstate:
return f
return None
def _checkunknownfiles(repo, wctx, mctx, force, actions, mergeforce): def _checkunknownfiles(repo, wctx, mctx, force, actions, mergeforce):
""" """
Considers any actions that care about the presence of conflicting unknown Considers any actions that care about the presence of conflicting unknown
files. For some actions, the result is to abort; for others, it is to files. For some actions, the result is to abort; for others, it is to
choose a different action. choose a different action.
""" """
conflicts = set() conflicts = set()
warnconflicts = set() warnconflicts = set()