D1165: arbitraryfilecontext: skip the cmp fast path if any side is a symlink
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1165: arbitraryfilecontext: skip the cmp fast path if any side is a symlink
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1165: arbitraryfilecontext: skip the cmp fast path if any side is a symlink
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1165: arbitraryfilecontext: skip the cmp fast path if any side is a symlink
This revision was automatically updated to reflect the committed changes.
Closed by commit rHG14c87708f432: arbitraryfilecontext: skip the cmp fast path
if any side is a symlink (authored by phillco, committed by ).
CHANGED PRIOR TO COMMIT
https://phab.mercurial-scm.org/D1165?vs=2942=2954#toc
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D1165?vs=2942=2954
REVISION DETAIL
https://phab.mercurial-scm.org/D1165
AFFECTED FILES
mercurial/context.py
tests/test-arbitraryfilectx.t
CHANGE DETAILS
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 < from __future__ import absolute_import
+ > import filecmp
+ > from mercurial import commands, context, registrar
+ > 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
@@ -2570,9 +2570,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 not symlinks and isinstance(fctx, workingfilectx) and self._repo:
# 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).
return not filecmp.cmp(self.path(), self._repo.wjoin(fctx.path()))
return self.data() != fctx.data()
To: phillco, #hg-reviewers, ryanmce
Cc: ryanmce, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1165: arbitraryfilecontext: skip the cmp fast path if any side is a symlink
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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D1165: arbitraryfilecontext: skip the cmp fast path if any side is a symlink
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
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 < 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()
To: phillco, #hg-reviewers
Cc: mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
