Page MenuHomePhabricator

diff: add experimental support for "merge diffs"
ClosedPublic

Authored by durin42 on May 7 2020, 5:26 PM.

Details

Summary

The way this works is it re-runs the merge and "stores" conflicts, and then
diffs against the conflicted result. In a normal merge, you should only see
diffs against conflicted regions or in cases where there was a semantic
conflict but not a textual one. This makes it easier to detect "evil merges"
that contain substantial new work embedded in the merge commit.

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

durin42 created this revision.May 7 2020, 5:26 PM

Looking forward to using this!

mercurial/commands.py
2471

How about making this boolean-valued instead? I'd expect hg diff --merge -c <some merge commit> to work. That will also make it easy to provide a config option to always enable it. (I don't ever want the current behavior for -c with merge commits.)

2565–2580

Seems like it would be cleaner to combine these two.

2568–2569

Or should we allow non-merge commits so one can always pass --merge without thinking (e.g. in [defaults])?

2574

Want to set a specific merge tool here while running this?

tests/test-diff-change.t
240

We should have at least one test showing a "four-way diff" with base, left, right, and resolved states.

durin42 marked 3 inline comments as done.May 11 2020, 5:34 PM
durin42 updated this revision to Diff 21341.
durin42 added inline comments.May 11 2020, 5:46 PM
mercurial/commands.py
2471

Wow, that feels obvious in hindsight. And then we can also default-true when tweakdefaults is on.

tests/test-diff-change.t
240

Not sure what you mean. Can you elaborate?

durin42 edited the summary of this revision. (Show Details)May 11 2020, 6:21 PM
durin42 updated this revision to Diff 21342.

For anyone curious what's left, the test currently fails like this:

--- tests/test-diff-change.t
+++ tests/test-diff-change.t.err
@@ -241,9 +241,15 @@
   ->>>>>>> other: d9e7de69eac3 - test: new file
    9
    10
+  diff -r 5010caab09f6 new-file-p2.txt
+  --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/new-file-p2.txt	Thu Jan 01 00:00:00 1970 +0000
+  @@ -0,0 +1,1 @@
+  +this file is new in p2 of the merge
 
 There must _NOT_ be a .hg/merge directory leftover.
   $ test ! -e .hg/merge
-
-
-  $ cd ..
+  [1]
+
+
+  $ cd ..
martinvonz added inline comments.May 11 2020, 10:57 PM
mercurial/commands.py
2566

How about calling these "pctx1" and "pctx2" instead so the same name doesn't change meaning and so you don't have to reassign "ctx2"?

tests/test-diff-change.t
240

Like the test case you added below. There, base is "8", left is "z", right is "y", and resolved is "z", if I'm reading that right.

durin42 updated this revision to Diff 21449.May 18 2020, 6:08 PM
durin42 updated this revision to Diff 21515.May 28 2020, 4:27 PM

I like the idea a lot :-)

durin42 updated this revision to Diff 21596.Jun 9 2020, 4:52 PM
marmoute requested changes to this revision.Jul 31 2020, 12:03 PM

According to IRC discussion, this series is not ready for review, so moving it out of yadda.

This revision now requires changes to proceed.Jul 31 2020, 12:03 PM
durin42 planned changes to this revision.
durin42 updated this revision to Diff 23028.
durin42 planned changes to this revision.
durin42 updated this revision to Diff 25160.
pulkit added a subscriber: pulkit.Jan 21 2021, 6:18 AM

Looks good to me but the commit description needs to be updated and an entry in relnotes/next will be helpful.

martinvonz added inline comments.Jan 29 2021, 1:06 PM
tests/test-diff-change.t
208

Would be nice to silence this :)

durin42 edited the summary of this revision. (Show Details)Jan 29 2021, 1:07 PM
martinvonz added inline comments.Jan 29 2021, 1:08 PM
tests/test-diff-change.t
208

Sorry, I meant the one on line 231, of course

This is definitely releases-notes-worthy, so can you mention it there?

mercurial/commands.py
2566

Ping on this

pulkit added inline comments.Jan 29 2021, 3:05 PM
tests/test-diff-change.t
208

Yep, agreed.

durin42 marked 6 inline comments as done.Feb 1 2021, 12:01 PM
durin42 updated this revision to Diff 25423.
martinvonz accepted this revision.Feb 1 2021, 12:49 PM
martinvonz added inline comments.
mercurial/commands.py
2566

I'll do this in flight:

  • Replace ctx1 by pctx1 on line 2570 (pushing down fix D9939 to here)
  • Replace mctx uses by ctx2
  • Drop mctx
  • Drop reassignment to ctx2 on line 2582
  • Delete assignment to unused stats variable
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.