( )⚙ D1122 arbitraryfilecontext: skip the cmp fast path if any side is a symlink

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1122

arbitraryfilecontext: skip the cmp fast path if any side is a symlink
ClosedPublic

Authored by phillco on Oct 16 2017, 3:01 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHG1d7d2a3559b4: arbitraryfilecontext: skip the cmp fast path if any side is a symlink
rHG85cee4c0c329: arbitraryfilecontext: skip the cmp fast path if any side is a symlink
Summary

filecmp follows symlinks by default, which a filectx.cmp() call should not
be doing as it should only compare the requested entry. After this patch, only
the contexts' data are compared, which is the correct contract.

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

phillco created this revision.Oct 16 2017, 3:01 PM
phillco edited the summary of this revision. (Show Details)Oct 16 2017, 3:36 PM
phillco updated this revision to Diff 2838.
phillco updated this revision to Diff 2839.Oct 16 2017, 3:37 PM
phillco updated this revision to Diff 2840.
phillco edited the summary of this revision. (Show Details)Oct 16 2017, 3:41 PM
phillco updated this revision to Diff 2841.
durin42 accepted this revision.Oct 16 2017, 9:42 PM
This revision is now accepted and ready to land.Oct 16 2017, 9:42 PM
Closed by commit rHG85cee4c0c329: arbitraryfilecontext: skip the cmp fast path if any side is a symlink (authored by phillco). · Explain WhyOct 16 2017, 9:47 PM
This revision was automatically updated to reflect the committed changes.
ryanmce added a subscriber: ryanmce.Oct 17 2017, 6:29 AM

Hm, check-code is failing here.

ryanmce added a comment.Oct 17 2017, 7:47 AM

I'm dropping this from hg-committed for now. Please re-submit with the check-code issues fixed.

@hg-reviewers: If I'm doing this wrong by dropping the patch, please let me know so I can correct my behavior in the future.

tests/test-arbitraryfilectx.t
53

why is there a space after the e here?

durin42 added a comment.Oct 17 2017, 10:31 AM

No, that sounds fair, Phil should resend (as a new differential ID) when the issue is sorted.

phillco added a comment.Oct 17 2017, 3:28 PM

Whoops, sorry, I forgot to run tests on the most recent version. Will fix & resend.

phillco updated this revision to Diff 2941.Oct 17 2017, 3:40 PM
phillco added a comment.Oct 17 2017, 3:42 PM

I've sent D1165, which is the corrected version.

Revision Contents
Changeset List

PathPackages
Mmercurial/context.py (8 lines)
AMtests/test-arbitraryfilectx.t (58 lines)

Diff 2878

mercurial/context.py

location on disk, possibly not in the working directory. location on disk, possibly not in the working directory.
""" """
def __init__(self, path, repo=None): def __init__(self, path, repo=None):
# Repo is optional because contrib/simplemerge uses this class. # Repo is optional because contrib/simplemerge uses this class.
self._repo = repo self._repo = repo
self._path = path self._path = path
def cmp(self, fctx): def cmp(self, fctx):
if isinstance(fctx, workingfilectx) and self._repo: # filecmp follows symlinks whereas `cmp` should not, so skip the fast
# path if either side is a symlink.
symlinks = ('l' in self.flags() or 'l' in fctx.flags())
if isinstance(fctx, workingfilectx) and self._repo and not symlinks:
# Add a fast-path for merge if both sides are disk-backed. # Add a fast-path for merge if both sides are disk-backed.
# Note that filecmp uses the opposite return values as cmp. # Note that filecmp uses the opposite return values (True if same)
# as our ``cmp`` functions (True if different).
return not filecmp.cmp(self.path(), self._repo.wjoin(fctx.path())) return not filecmp.cmp(self.path(), self._repo.wjoin(fctx.path()))
return self.data() != fctx.data() return self.data() != fctx.data()
def path(self): def path(self):
return self._path return self._path
def flags(self): def flags(self):
return '' return ''

tests/test-arbitraryfilectx.t

  • This file was added.
Setup:
$ cat > eval.py <<EOF
> from __future__ import absolute_import
> from mercurial import context, commands, registrar
> import filecmp
> cmdtable = {}
> command = registrar.command(cmdtable)
> @command(b'eval', [], 'hg eval CMD')
> def eval_(ui, repo, *cmds, **opts):
> cmd = " ".join(cmds)
> res = str(eval(cmd, globals(), locals()))
> ui.warn("%s" % res)
> EOF
$ echo "[extensions]" >> $HGRCPATH
$ echo "eval=`pwd`/eval.py" >> $HGRCPATH
Arbitraryfilectx.cmp does not follow symlinks:
$ mkdir case1
$ cd case1
$ hg init
$ hg eval "repo[None]['real_A'].write('A', '')"
None (no-eol)
$ echo -n "foo" > A
$ echo -n "foo" > B
$ ln -s A sym_A
$ hg add .
adding A
adding B
adding real_A
adding sym_A
$ hg commit -m "base"
These files are different and should return True (different):
(Note that filecmp.cmp's return semantics are inverted from ours, so we invert
for simplicity):
$ hg eval "context.arbitraryfilectx('A', repo).cmp(repo[None]['real_A'])"
True (no-eol)
$ hg eval "not filecmp.cmp('A', 'real_A')"
True (no-eol)
These files are identical and should return False (same):
$ hg eval "context.arbitraryfilectx('A', repo).cmp(repo[None]['A'])"
False (no-eol)
$ hg eval "context.arbitraryfilectx('A', repo).cmp(repo[None]['B'])"
False (no-eol)
$ hg eval "not filecmp.cmp('A', 'B')"
False (no-eol)
This comparison should also return False, since A and sym_A are substantially
the same in the eyes of ``filectx.cmp``, which looks at data only.
$ hg eval "context.arbitraryfilectx('real_A', repo).cmp(repo[None]['sym_A'])"
False (no-e ol)
ryanmceUnsubmitted
Not Done

why is there a space after the e here?

ryanmce: why is there a space after the e here?
A naive use of filecmp on those two would wrongly return True, since it follows
the symlink to "A", which has different contents.
$ hg eval "not filecmp.cmp('real_A', 'sym_A')"
True (no-eol)