| 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
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
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
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
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()