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

This is an archive of the discontinued Mercurial Phabricator instance.

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

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

Details

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
Lint Skipped
Unit
Unit Tests Skipped

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
2578

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

Excellent comment overall, nonetheless

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

8–13

What a terrible, evil extension!

This revision is now accepted and ready to land.Oct 17 2017, 5:57 PM
This revision was automatically updated to reflect the committed changes.
phillco added inline comments.Oct 17 2017, 6:06 PM
mercurial/context.py
2578

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
4–5

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

8–13

You misspelled "glorious"!

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

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.