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

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1165

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

Authored by phillco on Oct 17 2017, 3:42 PM.

Details

Reviewers
ryanmce
Group Reviewers
hg-reviewers
Commits
rHG14c87708f432: 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.

This is a corrected version of D1122.

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 17 2017, 3:42 PM
phillco edited the summary of this revision. (Show Details)Oct 17 2017, 4:57 PM
ryanmce accepted this revision.Oct 17 2017, 5:57 PM
ryanmce added a subscriber: ryanmce.

queued

mercurial/context.py
2579

I will remove the backticks in-flight; I think this are discouraged in comments still

Excellent comment overall, nonetheless

tests/test-arbitraryfilectx.t
5–6
--- /data/users/rmcelroy/mercurial/hg/tests/test-check-module-imports.t
+++ /data/users/rmcelroy/mercurial/hg/tests/test-check-module-imports.t.err
@@ -42,3 +42,6 @@
   > -X tests/test-lock.py \
   > -X tests/test-verify-repo-operations.py \
   > | sed 's-\\-/-g' | $PYTHON "$import_checker" -
+  tests/test-arbitraryfilectx.t:4: imports from mercurial not lexically sorted: commands < context
+  tests/test-arbitraryfilectx.t:5: stdlib import "filecmp" follows local import: mercurial
+  [1]

I can fix this in flight. Are you running all the test-check-*.t tests locally?

9–14

What a terrible, evil extension!

This revision is now accepted and ready to land.Oct 17 2017, 5:57 PM
Closed by commit rHG14c87708f432: arbitraryfilecontext: skip the cmp fast path if any side is a symlink (authored by phillco). · Explain WhyOct 17 2017, 5:58 PM
This revision was automatically updated to reflect the committed changes.
phillco added inline comments.Oct 17 2017, 6:06 PM
mercurial/context.py
2579

Is there one standard for indicating code snippets? I've seen backticks, double backticks, or nothing, but I think anything is better than nothing.

tests/test-arbitraryfilectx.t
5–6

hm, something might be up with my setup, definitely got a green on everything. I'll look into it.

9–14

You misspelled "glorious"!

phillco added inline comments.Oct 17 2017, 6:17 PM
tests/test-arbitraryfilectx.t
5–6

Ah, mea culpa. I had exempted those from my test system while on the long-lived in-memory merge branch (which has a lot of temp commits0 and forgot to switch them back on.

Revision Contents
Changeset List

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

Diff 2954

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 not symlinks and isinstance(fctx, workingfilectx) and self._repo:
# 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)
# from our cmp functions (True if different).
ryanmceUnsubmitted
Not Done

I will remove the backticks in-flight; I think this are discouraged in comments still

Excellent comment overall, nonetheless

ryanmce: I will remove the backticks in-flight; I think this are discouraged in comments still…
phillcoAuthorUnsubmitted
Not Done

Is there one standard for indicating code snippets? I've seen backticks, double backticks, or nothing, but I think anything is better than nothing.

phillco: Is there one standard for indicating code snippets? I've seen backticks, double backticks, or…
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
> import filecmp
> from mercurial import commands, context, registrar
> cmdtable = {}
ryanmceUnsubmitted
Not Done
--- /data/users/rmcelroy/mercurial/hg/tests/test-check-module-imports.t
+++ /data/users/rmcelroy/mercurial/hg/tests/test-check-module-imports.t.err
@@ -42,3 +42,6 @@
   > -X tests/test-lock.py \
   > -X tests/test-verify-repo-operations.py \
   > | sed 's-\\-/-g' | $PYTHON "$import_checker" -
+  tests/test-arbitraryfilectx.t:4: imports from mercurial not lexically sorted: commands < context
+  tests/test-arbitraryfilectx.t:5: stdlib import "filecmp" follows local import: mercurial
+  [1]

I can fix this in flight. Are you running all the test-check-*.t tests locally?

ryanmce: ``` --- /data/users/rmcelroy/mercurial/hg/tests/test-check-module-imports.t +++…
phillcoAuthorUnsubmitted
Not Done

hm, something might be up with my setup, definitely got a green on everything. I'll look into it.

phillco: hm, something might be up with my setup, definitely got a green on everything. I'll look into…
phillcoAuthorUnsubmitted
Not Done

Ah, mea culpa. I had exempted those from my test system while on the long-lived in-memory merge branch (which has a lot of temp commits0 and forgot to switch them back on.

phillco: Ah, mea culpa. I had exempted those from my test system while on the long-lived in-memory merge…
> 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
ryanmceUnsubmitted
Not Done

What a terrible, evil extension!

ryanmce: What a terrible, evil extension!
phillcoAuthorUnsubmitted
Not Done

You misspelled "glorious"!

phillco: You misspelled "glorious"!
$ echo "[extensions]" >> $HGRCPATH
$ echo "eval=`pwd`/eval.py" >> $HGRCPATH
Arbitraryfilectx.cmp does not follow symlinks:
$ mkdir case1
$ cd case1
$ hg init
$ printf "A" > real_A
$ printf "foo" > A
$ printf "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-eol)
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)