( )⚙ D8504 diff: add experimental support for "merge diffs"

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8504

diff: add experimental support for "merge diffs"
ClosedPublic

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

Details

Reviewers
marmoute
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG1a7d12c82057: diff: add experimental support for "merge diffs"
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
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

durin42 created this revision.May 7 2020, 5:26 PM
martinvonz added a subscriber: martinvonz.May 8 2020, 12:24 PM

Looking forward to using this!

mercurial/commands.py
2413

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

2502–2517

Seems like it would be cleaner to combine these two.

2505–2506

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

2511

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

tests/test-diff-change.t
187

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
2413

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

tests/test-diff-change.t
187

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.
durin42 added a comment.May 11 2020, 6:49 PM

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
2501

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
187

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
marmoute added a subscriber: marmoute.Jun 8 2020, 2:07 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 added a child revision: D8623: rebase: add a config knob for forcing in-memory rebasing.Oct 2 2020, 2:45 PM
durin42 planned changes to this revision.
durin42 updated this revision to Diff 23028.
durin42 removed a child revision: D8623: rebase: add a config knob for forcing in-memory rebasing.Jan 19 2021, 1:53 PM
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
155

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
155

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

martinvonz added a comment.Jan 29 2021, 1:12 PM

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

mercurial/commands.py
2501

Ping on this

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

Yep, agreed.

durin42 marked 6 inline comments as done.Feb 1 2021, 12:01 PM
durin42 added a child revision: D9939: diff: suppress `merging foo` output lines when performing a merge diff.
durin42 updated this revision to Diff 25423.
martinvonz accepted this revision.Feb 1 2021, 12:49 PM
martinvonz added inline comments.
mercurial/commands.py
2501

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
durin42 added a commit: rHG1a7d12c82057: diff: add experimental support for "merge diffs".Feb 1 2021, 12:50 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG1a7d12c82057: diff: add experimental support for "merge diffs" (authored by durin42). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/commands.py (28 lines)
Mtests/test-completion.t (2 lines)
Mtests/test-diff-change.t (101 lines)

Diff 21342

mercurial/commands.py

) )
from .pycompat import open from .pycompat import open
from . import ( from . import (
archival, archival,
bookmarks, bookmarks,
bundle2, bundle2,
changegroup, changegroup,
cmdutil, cmdutil,
context as contextmod,
copies, copies,
debugcommands as debugcommandsmod, debugcommands as debugcommandsmod,
destutil, destutil,
dirstateguard, dirstateguard,
discovery, discovery,
encoding, encoding,
error, error,
exchange, exchange,
ui.write(b"%s\n" % b"\n".join(sorted(cmdlist))) ui.write(b"%s\n" % b"\n".join(sorted(cmdlist)))
@command( @command(
b'diff', b'diff',
[ [
(b'r', b'rev', [], _(b'revision'), _(b'REV')), (b'r', b'rev', [], _(b'revision'), _(b'REV')),
(b'c', b'change', b'', _(b'change made by revision'), _(b'REV')), (b'c', b'change', b'', _(b'change made by revision'), _(b'REV')),
(
b'',
b'merge',
False,
martinvonzUnsubmitted
Done

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

martinvonz: How about making this boolean-valued instead? I'd expect `hg diff --merge -c <some merge…
durin42AuthorUnsubmitted
Done

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

durin42: Wow, that feels obvious in hindsight. And then we can also default-true when tweakdefaults is…
_(
b'show difference between auto-merge and committed '
b'merge for merge commits (EXPERIMENTAL)'
),
_(b'REV'),
),
] ]
+ diffopts + diffopts
+ diffopts2 + diffopts2
+ walkopts + walkopts
+ subrepoopts, + subrepoopts,
_(b'[OPTION]... ([-c REV] | [-r REV1 [-r REV2]]) [FILE]...'), _(b'[OPTION]... ([-c REV] | [-r REV1 [-r REV2]]) [FILE]...'),
helpcategory=command.CATEGORY_FILE_CONTENTS, helpcategory=command.CATEGORY_FILE_CONTENTS,
helpbasic=True, helpbasic=True,
""" """
cmdutil.check_at_most_one_arg(opts, 'rev', 'change') cmdutil.check_at_most_one_arg(opts, 'rev', 'change')
opts = pycompat.byteskwargs(opts) opts = pycompat.byteskwargs(opts)
revs = opts.get(b'rev') revs = opts.get(b'rev')
change = opts.get(b'change') change = opts.get(b'change')
stat = opts.get(b'stat') stat = opts.get(b'stat')
reverse = opts.get(b'reverse') reverse = opts.get(b'reverse')
diffmerge = opts.get(b'merge')
if change: if change:
repo = scmutil.unhidehashlikerevs(repo, [change], b'nowarn') repo = scmutil.unhidehashlikerevs(repo, [change], b'nowarn')
ctx2 = scmutil.revsingle(repo, change, None) ctx2 = scmutil.revsingle(repo, change, None)
if diffmerge and ctx2.p2().node() != nullid:
mctx = ctx2
martinvonzUnsubmitted
Done

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

martinvonz: How about calling these "pctx1" and "pctx2" instead so the same name doesn't change meaning and…
martinvonzUnsubmitted
Done

Ping on this

martinvonz: Ping on this
martinvonzUnsubmitted
Not Done

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
martinvonz: I'll do this in flight: * Replace `ctx1` by `pctx1` on line 2570 (pushing down fix D9939 to…
ctx1 = mctx.p1()
ctx2 = mctx.p2()
wctx = contextmod.overlayworkingctx(repo)
wctx.setbase(ctx1)
with ui.configoverride({
martinvonzUnsubmitted
Done

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

martinvonz: Or should we allow non-merge commits so one can always pass `--merge` without thinking (e.g. in…
(b'ui', b'forcemerge'): b'internal:merge',
},
b'diff --merge'):
stats = mergemod.merge(ctx2, wc=wctx)
ctx1 = wctx
martinvonzUnsubmitted
Done

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

martinvonz: Want to set a specific merge tool here while running this?
ctx2 = mctx
else:
ctx1 = ctx2.p1() ctx1 = ctx2.p1()
else: else:
repo = scmutil.unhidehashlikerevs(repo, revs, b'nowarn') repo = scmutil.unhidehashlikerevs(repo, revs, b'nowarn')
ctx1, ctx2 = scmutil.revpair(repo, revs) ctx1, ctx2 = scmutil.revpair(repo, revs)
martinvonzUnsubmitted
Done

Seems like it would be cleaner to combine these two.

martinvonz: Seems like it would be cleaner to combine these two.
node1, node2 = ctx1.node(), ctx2.node() node1, node2 = ctx1.node(), ctx2.node()
if reverse: if reverse:
ctxleft = ctx2 ctxleft = ctx2
ctxright = ctx1 ctxright = ctx1
else: else:
ctxleft = ctx1 ctxleft = ctx1
ctxright = ctx2 ctxright = ctx2

tests/test-completion.t

debuguigetpass: prompt debuguigetpass: prompt
debuguiprompt: prompt debuguiprompt: prompt
debugupdatecaches: debugupdatecaches:
debugupgraderepo: optimize, run, backup, changelog, manifest debugupgraderepo: optimize, run, backup, changelog, manifest
debugwalk: include, exclude debugwalk: include, exclude
debugwhyunstable: debugwhyunstable:
debugwireargs: three, four, five, ssh, remotecmd, insecure debugwireargs: three, four, five, ssh, remotecmd, insecure
debugwireproto: localssh, peer, noreadstderr, nologhandshake, ssh, remotecmd, insecure debugwireproto: localssh, peer, noreadstderr, nologhandshake, ssh, remotecmd, insecure
diff: rev, change, text, git, binary, nodates, noprefix, show-function, reverse, ignore-all-space, ignore-space-change, ignore-blank-lines, ignore-space-at-eol, unified, stat, root, include, exclude, subrepos diff: rev, change, merge, text, git, binary, nodates, noprefix, show-function, reverse, ignore-all-space, ignore-space-change, ignore-blank-lines, ignore-space-at-eol, unified, stat, root, include, exclude, subrepos
export: bookmark, output, switch-parent, rev, text, git, binary, nodates, template export: bookmark, output, switch-parent, rev, text, git, binary, nodates, template
files: rev, print0, include, exclude, template, subrepos files: rev, print0, include, exclude, template, subrepos
forget: interactive, include, exclude, dry-run forget: interactive, include, exclude, dry-run
graft: rev, base, continue, stop, abort, edit, log, no-commit, force, currentdate, currentuser, date, user, tool, dry-run graft: rev, base, continue, stop, abort, edit, log, no-commit, force, currentdate, currentuser, date, user, tool, dry-run
grep: print0, all, diff, text, follow, ignore-case, files-with-matches, line-number, rev, all-files, user, date, template, include, exclude grep: print0, all, diff, text, follow, ignore-case, files-with-matches, line-number, rev, all-files, user, date, template, include, exclude
heads: rev, topo, active, closed, style, template heads: rev, topo, active, closed, style, template
help: extension, command, keyword, system help: extension, command, keyword, system
identify: rev, num, id, branch, tags, bookmarks, ssh, remotecmd, insecure, template identify: rev, num, id, branch, tags, bookmarks, ssh, remotecmd, insecure, template

tests/test-diff-change.t

5 5
6 6
7 7
-8 -8
+y +y
9 9
10 10
merge diff should show only manual edits to a merge:
$ hg diff --merge -c 6
merging file.txt
(no diff output is expected here)
Construct an "evil merge" that does something other than just the merge.
$ hg co ".^"
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg merge -r 5
merging file.txt
martinvonzUnsubmitted
Done

Would be nice to silence this :)

martinvonz: Would be nice to silence this :)
martinvonzUnsubmitted
Done

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

martinvonz: Sorry, I meant the one on line 231, of course
pulkitUnsubmitted
Done

Yep, agreed.

pulkit: Yep, agreed.
0 files updated, 1 files merged, 0 files removed, 0 files unresolved
(branch merge, don't forget to commit)
$ echo 11 >> file.txt
$ hg ci -m 'merge 8 to y with manual edit of 11' # 7
created new head
$ hg diff -c 7
diff -r 273b50f17c6d -r 8ad85e839ba7 file.txt
--- a/file.txt Thu Jan 01 00:00:00 1970 +0000
+++ b/file.txt Thu Jan 01 00:00:00 1970 +0000
@@ -6,6 +6,7 @@
5
6
7
-8
+y
9
10
+11
Contrast with the `hg diff -c 7` version above: only the manual edit shows
up, making it easy to identify changes someone is otherwise trying to sneak
into a merge.
$ hg diff --merge -c 7
merging file.txt
diff -r 8ad85e839ba7 file.txt
--- a/file.txt Thu Jan 01 00:00:00 1970 +0000
+++ b/file.txt Thu Jan 01 00:00:00 1970 +0000
@@ -9,3 +9,4 @@
y
9
10
+11
martinvonzUnsubmitted
Not Done

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

martinvonz: We should have at least one test showing a "four-way diff" with base, left, right, and resolved…
durin42AuthorUnsubmitted
Done

Not sure what you mean. Can you elaborate?

durin42: Not sure what you mean. Can you elaborate?
martinvonzUnsubmitted
Not Done

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.

martinvonz: Like the test case you added below. There, base is "8", left is "z", right is "y", and resolved…
Set up a conflict.
$ hg co ".^"
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ sed -e 's,^8$,z,' file.txt > file.txt.tmp
$ mv file.txt.tmp file.txt
$ hg ci -m 'conflicting edit: 8 to z'
created new head
$ echo "this file is new in p1 of the merge" > new-file-p1.txt
$ hg ci -Am 'new file' new-file-p1.txt
$ hg log -r . --template 'p1 will be rev {rev}\n'
p1 will be rev 9
$ hg co 5
1 files updated, 0 files merged, 1 files removed, 0 files unresolved
$ echo "this file is new in p2 of the merge" > new-file-p2.txt
$ hg ci -Am 'new file' new-file-p2.txt
created new head
$ hg log -r . --template 'p2 will be rev {rev}\n'
p2 will be rev 10
$ hg co -- 9
2 files updated, 0 files merged, 1 files removed, 0 files unresolved
$ hg merge -r 10
merging file.txt
warning: conflicts while merging file.txt! (edit, then use 'hg resolve --mark')
1 files updated, 0 files merged, 0 files removed, 1 files unresolved
use 'hg resolve' to retry unresolved file merges or 'hg merge --abort' to abandon
[1]
$ hg revert file.txt -r .
$ hg resolve -ma
(no more unresolved files)
$ hg commit -m 'merge conflicted edit'
Without --merge, it's a diff against p1
$ hg diff --no-merge -c 11
diff -r fd1f17c90d7c -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
With --merge, it's a diff against the conflicted content.
$ hg diff --merge -c 11
diff -r 273b50f17c6d -r cd8ec38ff23b file.txt
--- a/file.txt Thu Jan 01 00:00:00 1970 +0000
+++ b/file.txt Thu Jan 01 00:00:00 1970 +0000
@@ -6,10 +6,6 @@
5
6
7
-<<<<<<< working copy: fd1f17c90d7c - test: new file
z
-=======
-y
->>>>>>> merge rev: 9085c5c02e52 - test: change 8 to y
9
10
There must _NOT_ be a .hg/merge directory leftover.
$ test ! -e .hg/merge
$ cd .. $ cd ..