This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9956

diff: replace --merge option by config option
ClosedPublic

Authored by martinvonz on Feb 5 2021, 3:04 AM.

Details

Reviewers
marmoute
pulkit
Group Reviewers
hg-reviewers
Commits
rHGd57e607d9e33: diff: replace --merge option by config option
rHG6bb80672d268: diff: replace --merge option by config option
Summary

I can't think of any reason you'd want to enable the merge diff on a
run-to-run basis; you'd probably either always or never want it set
(though I can't see why you'd never want it set). If you have it set,
you'll probably also want the same output in hg log -p
output. Having a single config option for the feature makes sense.

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

martinvonz created this revision.Feb 5 2021, 3:04 AM
marmoute requested changes to this revision.Feb 5 2021, 7:13 AM
marmoute added a subscriber: marmoute.

This seems too premature for a quite new feature we don't have much experience with.

The current behavior for diff on merge is well established and have valid usecase (eg: when doing traversal following first parents). Keeping this as an explicit flag for now seems preferable.

This revision now requires changes to proceed.Feb 5 2021, 7:13 AM
martinvonz added a comment.Feb 5 2021, 11:13 AM
In D9956#150835, @marmoute wrote:

This seems too premature for a quite new feature we don't have much experience with.
The current behavior for diff on merge is well established and have valid usecase (eg: when doing traversal following first parents). Keeping this as an explicit flag for now seems preferable.

I'm confused about your comment. Do you plan to test it frequently enough that you want to be able to turn it on and off with --merge instead of --config diff.merge=yes? You noticed that it's an experimental flag that turned into an experimental config, right?

martinvonz requested review of this revision.Feb 5 2021, 11:13 AM
marmoute added a comment.Feb 5 2021, 5:11 PM
In D9956#150837, @martinvonz wrote:
In D9956#150835, @marmoute wrote:

This seems too premature for a quite new feature we don't have much experience with.
The current behavior for diff on merge is well established and have valid usecase (eg: when doing traversal following first parents). Keeping this as an explicit flag for now seems preferable.

I'm confused about your comment. Do you plan to test it frequently enough that you want to be able to turn it on and off with --merge instead of --config diff.merge=yes? You noticed that it's an experimental flag that turned into an experimental config, right?

Yes. The flag (with --merge and --no-merge variant) seems like a good idea, at least for a while. We can have a config to change the default flag value (while keeping the flag).

martinvonz added a comment.Feb 5 2021, 5:17 PM
In D9956#150840, @marmoute wrote:
In D9956#150837, @martinvonz wrote:
In D9956#150835, @marmoute wrote:

This seems too premature for a quite new feature we don't have much experience with.
The current behavior for diff on merge is well established and have valid usecase (eg: when doing traversal following first parents). Keeping this as an explicit flag for now seems preferable.

I'm confused about your comment. Do you plan to test it frequently enough that you want to be able to turn it on and off with --merge instead of --config diff.merge=yes? You noticed that it's an experimental flag that turned into an experimental config, right?

Yes. The flag (with --merge and --no-merge variant) seems like a good idea, at least for a while. We can have a config to change the default flag value (while keeping the flag).

I assume I can't convince you to use hg diff --from foo^1 --to foo instead. Do you want me to add the --merge flag to hg log -p, hg incoming, and hg outgoing as well or do you think you'll only be experimenting with the feature on hg diff -c?

pulkit accepted this revision.Feb 10 2021, 7:21 AM
martinvonz added a commit: rHG6bb80672d268: diff: replace --merge option by config option.Feb 11 2021, 11:34 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG6bb80672d268: diff: replace --merge option by config option (authored by martinvonz). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 25554

mercurial/commands.py

@command( @command(
b'diff', b'diff',
[ [
(b'r', b'rev', [], _(b'revision (DEPRECATED)'), _(b'REV')), (b'r', b'rev', [], _(b'revision (DEPRECATED)'), _(b'REV')),
(b'', b'from', b'', _(b'revision to diff from'), _(b'REV1')), (b'', b'from', b'', _(b'revision to diff from'), _(b'REV1')),
(b'', b'to', b'', _(b'revision to diff to'), _(b'REV2')), (b'', b'to', b'', _(b'revision to diff to'), _(b'REV2')),
(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,
_(
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] | [--from REV1] [--to REV2]) [FILE]...'), _(b'[OPTION]... ([-c REV] | [--from REV1] [--to 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')
from_rev = opts.get(b'from') from_rev = opts.get(b'from')
to_rev = opts.get(b'to') to_rev = opts.get(b'to')
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') diffmerge = ui.configbool(b'diff', b'merge')
cmdutil.check_incompatible_arguments(opts, b'from', [b'rev', b'change']) cmdutil.check_incompatible_arguments(opts, b'from', [b'rev', b'change'])
cmdutil.check_incompatible_arguments(opts, b'to', [b'rev', b'change']) cmdutil.check_incompatible_arguments(opts, b'to', [b'rev', b'change'])
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: if diffmerge and ctx2.p2().node() != nullid:
pctx1 = ctx2.p1() pctx1 = ctx2.p1()

mercurial/configitems.py

# Control the initial size of the discovery for initial change # Control the initial size of the discovery for initial change
coreconfigitem( coreconfigitem(
b'devel', b'devel',
b'discovery.sample-size.initial', b'discovery.sample-size.initial',
default=100, default=100,
) )
_registerdiffopts(section=b'diff') _registerdiffopts(section=b'diff')
coreconfigitem( coreconfigitem(
b'diff',
b'merge',
default=False,
experimental=True,
)
coreconfigitem(
b'email', b'email',
b'bcc', b'bcc',
default=None, default=None,
) )
coreconfigitem( coreconfigitem(
b'email', b'email',
b'cc', b'cc',
default=None, default=None,

mercurial/filemerge.py

b'merge3-lie-about-conflicts', b'merge3-lie-about-conflicts',
fullmerge, fullmerge,
b'', b'',
precheck=_mergecheck, precheck=_mergecheck,
) )
def _imerge3alwaysgood(*args, **kwargs): def _imerge3alwaysgood(*args, **kwargs):
# Like merge3, but record conflicts as resolved with markers in place. # Like merge3, but record conflicts as resolved with markers in place.
# #
# This is used for `hg diff --merge` to show the differences between # This is used for `diff.merge` to show the differences between
# the auto-merge state and the committed merge state. It may be # the auto-merge state and the committed merge state. It may be
# useful for other things. # useful for other things.
b1, junk, b2 = _imerge3(*args, **kwargs) b1, junk, b2 = _imerge3(*args, **kwargs)
# TODO is this right? I'm not sure what these return values mean, # TODO is this right? I'm not sure what these return values mean,
# but as far as I can tell this will indicate to callers tha the # but as far as I can tell this will indicate to callers tha the
# merge succeeded. # merge succeeded.
return b1, False, b2 return b1, False, b2

relnotes/next

== New Features == == New Features ==
* `hg purge` is now a core command using `--confirm` by default. * `hg purge` is now a core command using `--confirm` by default.
* The `rev-branch-cache` is now updated incrementally whenever changesets * The `rev-branch-cache` is now updated incrementally whenever changesets
are added. are added.
== New Experimental Features == == New Experimental Features ==
* `hg diff` now takes an experimental `--merge` flag which causes `hg * There's a new `diff.merge` config option to show the changes
diff --change` to show the changes relative to an automerge for relative to an automerge for merge changesets. This makes it
merge changesets. This makes it easier to detect and review manual easier to detect and review manual changes performed in merge
changes performed in merge changesets. changesets. It is only supported by `hg diff --change` so far.
== Bug Fixes == == Bug Fixes ==
== Backwards Compatibility Changes == == Backwards Compatibility Changes ==
== Internal API Changes == == Internal API Changes ==
* `changelog.branchinfo` is deprecated and will be removed after 5.8. * `changelog.branchinfo` is deprecated and will be removed after 5.8.
It is superseded by `changelogrevision.branchinfo`. It is superseded by `changelogrevision.branchinfo`.

tests/test-completion.t

debuguigetpass: prompt debuguigetpass: prompt
debuguiprompt: prompt debuguiprompt: prompt
debugupdatecaches: debugupdatecaches:
debugupgraderepo: optimize, run, backup, changelog, manifest, filelogs debugupgraderepo: optimize, run, backup, changelog, manifest, filelogs
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, from, to, 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 diff: rev, from, to, 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
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

7 7
-8 -8
+y +y
9 9
10 10
merge diff should show only manual edits to a merge: merge diff should show only manual edits to a merge:
$ hg diff --merge -c 6 $ hg diff --config diff.merge=yes -c 6
(no diff output is expected here) (no diff output is expected here)
Construct an "evil merge" that does something other than just the merge. Construct an "evil merge" that does something other than just the merge.
$ hg co ".^" $ hg co ".^"
1 files updated, 0 files merged, 0 files removed, 0 files unresolved 1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg merge -r 5 $ hg merge -r 5
merging file.txt merging file.txt
-8 -8
+y +y
9 9
10 10
+11 +11
Contrast with the `hg diff -c 7` version above: only the manual edit shows 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 up, making it easy to identify changes someone is otherwise trying to sneak
into a merge. into a merge.
$ hg diff --merge -c 7 $ hg diff --config diff.merge=yes -c 7
diff -r 8ad85e839ba7 file.txt diff -r 8ad85e839ba7 file.txt
--- a/file.txt Thu Jan 01 00:00:00 1970 +0000 --- a/file.txt Thu Jan 01 00:00:00 1970 +0000
+++ b/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 @@ @@ -9,3 +9,4 @@
y y
9 9
10 10
+11 +11
warning: conflicts while merging file.txt! (edit, then use 'hg resolve --mark') warning: conflicts while merging file.txt! (edit, then use 'hg resolve --mark')
1 files updated, 0 files merged, 0 files removed, 1 files unresolved 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 use 'hg resolve' to retry unresolved file merges or 'hg merge --abort' to abandon
[1] [1]
$ hg revert file.txt -r . $ hg revert file.txt -r .
$ hg resolve -ma $ hg resolve -ma
(no more unresolved files) (no more unresolved files)
$ hg commit -m 'merge conflicted edit' $ hg commit -m 'merge conflicted edit'
Without --merge, it's a diff against p1 Without diff.merge, it's a diff against p1
$ hg diff --no-merge -c 11 $ hg diff --config diff.merge=no -c 11
diff -r fd1f17c90d7c -r 5010caab09f6 new-file-p2.txt diff -r fd1f17c90d7c -r 5010caab09f6 new-file-p2.txt
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/new-file-p2.txt 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 @@ @@ -0,0 +1,1 @@
+this file is new in p2 of the merge +this file is new in p2 of the merge
With --merge, it's a diff against the conflicted content. With diff.merge, it's a diff against the conflicted content.
$ hg diff --merge -c 11 $ hg diff --config diff.merge=yes -c 11
diff -r 5010caab09f6 file.txt diff -r 5010caab09f6 file.txt
--- a/file.txt Thu Jan 01 00:00:00 1970 +0000 --- a/file.txt Thu Jan 01 00:00:00 1970 +0000
+++ b/file.txt Thu Jan 01 00:00:00 1970 +0000 +++ b/file.txt Thu Jan 01 00:00:00 1970 +0000
@@ -6,12 +6,6 @@ @@ -6,12 +6,6 @@
5 5
6 6
7 7
-<<<<<<< local: fd1f17c90d7c - test: new file -<<<<<<< local: fd1f17c90d7c - test: new file