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

login
register
mail settings
Submitter phabricator
Date Oct. 17, 2017, 7:42 p.m.
Message ID <differential-rev-PHID-DREV-7prbuknjsmrga2cumiza-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/25139/
State Superseded
Headers show

Comments

phabricator - Oct. 17, 2017, 7:42 p.m.
phillco created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION 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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1165

AFFECTED FILES
  mercurial/context.py
  tests/test-arbitraryfilectx.t

CHANGE DETAILS




To: phillco, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 17, 2017, 9:57 p.m.
ryanmce accepted this revision.
ryanmce added a comment.
This revision is now accepted and ready to land.


  queued

INLINE COMMENTS

> context.py:2578
> +            # 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()))

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

Excellent comment overall, nonetheless

> test-arbitraryfilectx.t:4-5
> +  > from __future__ import absolute_import
> +  > from mercurial import context, commands, registrar
> +  > import filecmp
> +  > cmdtable = {}

--- /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?

> test-arbitraryfilectx.t:8-13
> +  > @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

What a terrible, evil extension!

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1165

To: phillco, #hg-reviewers, ryanmce
Cc: ryanmce, mercurial-devel
phabricator - Oct. 17, 2017, 10:06 p.m.
phillco added inline comments.

INLINE COMMENTS

> ryanmce wrote in test-arbitraryfilectx.t:4-5
>   --- /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?

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

> ryanmce wrote in test-arbitraryfilectx.t:8-13
> What a terrible, evil extension!

You misspelled "glorious"!

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1165

To: phillco, #hg-reviewers, ryanmce
Cc: ryanmce, mercurial-devel
phabricator - Oct. 17, 2017, 10:07 p.m.
phillco added inline comments.

INLINE COMMENTS

> ryanmce wrote in context.py:2578
> I will remove the backticks in-flight; I think this are discouraged in comments still
> 
> Excellent comment overall, nonetheless

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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1165

To: phillco, #hg-reviewers, ryanmce
Cc: ryanmce, mercurial-devel
phabricator - Oct. 17, 2017, 10:17 p.m.
phillco added inline comments.

INLINE COMMENTS

> phillco wrote in test-arbitraryfilectx.t:4-5
> hm, something might be up with my setup, definitely got a green on everything. I'll look into it.

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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1165

To: phillco, #hg-reviewers, ryanmce
Cc: ryanmce, mercurial-devel

Patch

diff --git a/tests/test-arbitraryfilectx.t b/tests/test-arbitraryfilectx.t
new file mode 100644
--- /dev/null
+++ b/tests/test-arbitraryfilectx.t
@@ -0,0 +1,57 @@ 
+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
+  $ 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)
diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -2569,9 +2569,13 @@ 
         self._path = path
 
     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.
-            # 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 self.data() != fctx.data()