This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9554

diff: add --from and --to flags as clearer alternative to -r -r
ClosedPublic

Authored by martinvonz on Dec 9 2020, 10:09 PM.

Details

Reviewers
mharbison72
pulkit
Group Reviewers
hg-reviewers
Summary

I think it was mistake to let the -r flag accept two revisions in
hg diff in 98633e60067c (Support for 0, 1, or 2 diff revs,
2005-05-07). The command clearly acts on two revisions and having a
single flag to indicate which those are is unclear. It got worse when
it started accepting revsets as input.

This patch introduces --from and --to flags, each taking a single
revision and each defaulting to the working copy. That means that `hg
diff --from . behaves like hg diff` and hg diff --to . behaves
like hg diff --reverse.

In addition to clarifying the direction, it also makes it so one won't
make the mistake of thinking that hg diff -r "date('yesterday')"
will show differences since (some commit) yesterday when it will
actually depend on the how many commits were made yesterday (it works
if exactly one commit was made). (Fun fact: hg help diff -v includes
that broken case, incorrectly claiming that it shows changes "relative
to the last change on some date".)

I think -r should be deprecated, but I understand that it's used in
way too much documentation and in people's muscle memory for that to
be realistic.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

martinvonz created this revision.Dec 9 2020, 10:09 PM
martinvonz updated this revision to Diff 24150.Dec 9 2020, 10:17 PM
mharbison72 added a subscriber: mharbison72.Dec 9 2020, 11:45 PM

I haven't followed all of the UI enhancement discussions; it looks familiar, so is this something planned for other commands (e.g. status, extdiff)?

mercurial/commands.py
2548–2549

Should this be something like scmutil.revrange(), and then raise an InputError here if what was passed isn't len() == 1 for each? It would avoid awkwardness like --from a::b.

martinvonz added a comment.Dec 9 2020, 11:54 PM

I haven't followed all of the UI enhancement discussions; it looks familiar, so is this something planned for other commands (e.g. status, extdiff)?

I had not planned to do it for those, but it would make sense (expect that I think it's another mistake that hg status can work on anything but the working copy to start with).

mercurial/commands.py
2548–2549

I wish revingle() made that check but since it doesn't, I think it's better than we still use revingle()'s current behavior for consistency. See example with date() revset in the next patch.

mharbison72 added a comment.Dec 10 2020, 12:07 AM

I wish revingle() made that check but since it doesn't, I think it's better than we still use revingle()'s current behavior for consistency. See example with date() revset in the next patch.

Sounds reasonable to me.

mharbison72 accepted this revision.Dec 10 2020, 12:17 AM
This revision is now accepted and ready to land.Dec 10 2020, 12:17 AM
pulkit accepted this revision.Dec 10 2020, 3:41 AM
pulkit added a subscriber: pulkit.
pulkit added inline comments.
mercurial/commands.py
2458

How about deprecating the --rev flag as a followup?

mharbison72 added inline comments.Dec 10 2020, 8:47 AM
mercurial/commands.py
2458

Or (ADVANCED) so people don’t think it’s going to be removed.

pulkit added inline comments.Dec 10 2020, 8:50 AM
mercurial/commands.py
2458

Generally things marked as ADVANCED are not for general workflow and might be experimental also. I think absorb was/is marked as advanced.

Whereas, here we want to convey that this is not the flag to use now and there are better replacements.

Also I think, we don't remove DEPRECATED stuff due to BC reasons.

martinvonz added inline comments.Dec 10 2020, 12:32 PM
mercurial/commands.py
2458

I agree that (DEPRECATED) is better. IMO, (ADVANCED) is for when the feature is useful for some unusual workflows, which I don't think is the case here -- I think you can always rewrite your hg diff -r ... [-r ...] command to use hg diff [--from ...] [--to ...], including hg diff -r 'thing()' as hg diff --from 'min(thing())' --to 'max(thing())'.

Alphare added a subscriber: Alphare.Dec 14 2020, 5:00 AM

+1 for DEPRECATED

pulkit added a comment.Dec 18 2020, 3:52 AM

IIUC, this one was pushed. We should abandon it.

martinvonz added a comment.Dec 18 2020, 7:58 AM
In D9554#145030, @pulkit wrote:

IIUC, this one was pushed. We should abandon it.

This is for hg diff. I think you're thinking of the same thing for hg status. I haven't written a patch for that.

pulkit added a comment.Dec 18 2020, 8:04 AM
In D9554#145072, @martinvonz wrote:
In D9554#145030, @pulkit wrote:

IIUC, this one was pushed. We should abandon it.

This is for hg diff. I think you're thinking of the same thing for hg status. I haven't written a patch for that.

I mean this patch was pushed as 64292addbe67bf4f089704cfcaf868aba7a5443b. It seems somehow the commit message got trimmed which led to this differential not closing automatically.

martinvonz added a comment.Dec 18 2020, 11:27 AM
In D9554#145074, @pulkit wrote:
In D9554#145072, @martinvonz wrote:
In D9554#145030, @pulkit wrote:

IIUC, this one was pushed. We should abandon it.

This is for hg diff. I think you're thinking of the same thing for hg status. I haven't written a patch for that.

I mean this patch was pushed as 64292addbe67bf4f089704cfcaf868aba7a5443b. It seems somehow the commit message got trimmed which led to this differential not closing automatically.

Haha, I bet the answer to the "somehow" is that the next line started with "diff --" ("diff --from ." to be specific). It feels like there must be a more modern solution. @mharbison72: Does your hg phabimport solve the problem (by getting more structured data from the server)?

martinvonz closed this revision.Dec 18 2020, 11:28 AM
mharbison72 added a comment.Dec 18 2020, 8:30 PM
In D9554#145175, @martinvonz wrote:
In D9554#145074, @pulkit wrote:
In D9554#145072, @martinvonz wrote:
In D9554#145030, @pulkit wrote:

IIUC, this one was pushed. We should abandon it.

This is for hg diff. I think you're thinking of the same thing for hg status. I haven't written a patch for that.

I mean this patch was pushed as 64292addbe67bf4f089704cfcaf868aba7a5443b. It seems somehow the commit message got trimmed which led to this differential not closing automatically.

Haha, I bet the answer to the "somehow" is that the next line started with "diff --" ("diff --from ." to be specific). It feels like there must be a more modern solution. @mharbison72: Does your hg phabimport solve the problem (by getting more structured data from the server)?

I think you're correct, and sadly, no- hg phabimport doesn't help because it seems that the core import functionality isn't capable of handling this. I read the patch to a file, ran hg import $patch, and got the same result. (In fact, it unhid the original import I had, which momentarily confused me.) IDK if there are more options that we can/should throw at the core import (internally). It should always be a git patch that is stuffed into the phab metadata, and since this wasn't updated by the commit, you can hg phabread it and see that it is correct. But the import machinery truncates there. I'm a little surprised that it didn't complain that it didn't know what --from meant.

This is the second time this has happened in the last couple of weeks, so I think we should probably figure out how to address it. (IDR which was the original where it happened, but the commit update may have changed the diff such that it doesn't have the original state now.)

Revision Contents
Changeset List

PathPackages
Mmercurial/commands.py (10 lines)
Mrelnotes/next (3 lines)
Mtests/test-completion.t (2 lines)
Mtests/test-diff-change.t (55 lines)
Mtests/test-help.t (2 lines)

Diff 24150

mercurial/commands.py

if ui.verbose: if ui.verbose:
cmdlist = [b' '.join(c[0]) for c in cmdlist.values()] cmdlist = [b' '.join(c[0]) for c in cmdlist.values()]
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')),
pulkitUnsubmitted
Not Done

How about deprecating the --rev flag as a followup?

pulkit: How about deprecating the `--rev` flag as a followup?
mharbison72Unsubmitted
Not Done

Or (ADVANCED) so people don’t think it’s going to be removed.

mharbison72: Or `(ADVANCED)` so people don’t think it’s going to be removed.
pulkitUnsubmitted
Not Done

Generally things marked as ADVANCED are not for general workflow and might be experimental also. I think absorb was/is marked as advanced.

Whereas, here we want to convey that this is not the flag to use now and there are better replacements.

Also I think, we don't remove DEPRECATED stuff due to BC reasons.

pulkit: Generally things marked as `ADVANCED` are not for general workflow and might be experimental…
martinvonzAuthorUnsubmitted
Done

I agree that (DEPRECATED) is better. IMO, (ADVANCED) is for when the feature is useful for some unusual workflows, which I don't think is the case here -- I think you can always rewrite your hg diff -r ... [-r ...] command to use hg diff [--from ...] [--to ...], including hg diff -r 'thing()' as hg diff --from 'min(thing())' --to 'max(thing())'.

martinvonz: I agree that `(DEPRECATED)` is better. IMO, `(ADVANCED)` is for when the feature is useful for…
(b'', b'from', b'', _(b'revision to diff from'), _(b'REV')),
(b'', b'to', b'', _(b'revision to diff to'), _(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')),
] ]
+ 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,
Returns 0 on success. Returns 0 on success.
""" """
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')
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')
cmdutil.check_incompatible_arguments(opts, b'from', [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)
ctx1 = ctx2.p1() ctx1 = ctx2.p1()
elif from_rev or to_rev:
repo = scmutil.unhidehashlikerevs(repo, [from_rev] + [to_rev], b'nowarn')
ctx1 = scmutil.revsingle(repo, from_rev, None)
ctx2 = scmutil.revsingle(repo, to_rev, None)
mharbison72Unsubmitted
Not Done

Should this be something like scmutil.revrange(), and then raise an InputError here if what was passed isn't len() == 1 for each? It would avoid awkwardness like --from a::b.

mharbison72: Should this be something like `scmutil.revrange()`, and then raise an InputError here if what…
martinvonzAuthorUnsubmitted
Done

I wish revingle() made that check but since it doesn't, I think it's better than we still use revingle()'s current behavior for consistency. See example with date() revset in the next patch.

martinvonz: I wish revingle() made that check but since it doesn't, I think it's better than we still use…
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)
if reverse: if reverse:
ctxleft = ctx2 ctxleft = ctx2
ctxright = ctx1 ctxright = ctx1
else: else:

relnotes/next

can be overridden per command with can be overridden per command with
`command-templates.oneline-summary.<command>`, where `<command>` `command-templates.oneline-summary.<command>`, where `<command>`
can be e.g. `rebase`. As part of this effort, the default format can be e.g. `rebase`. As part of this effort, the default format
from `hg rebase` was reorganized a bit. from `hg rebase` was reorganized a bit.
* `hg strip`, from the strip extension, is now a core command, `hg * `hg strip`, from the strip extension, is now a core command, `hg
debugstrip`. The extension remains for compatibility. debugstrip`. The extension remains for compatibility.
* `hg diff` now supports `--from <rev>` and `--to <rev>` arguments as
clearer alternatives to `-r <revs>`.
* The memory footprint per changeset during pull/unbundle * The memory footprint per changeset during pull/unbundle
operations has been further reduced. operations has been further reduced.
== New Experimental Features == == New Experimental Features ==
== Bug Fixes == == Bug Fixes ==

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, 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, 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

Testing diff --change Testing diff --change, --from, --to
$ hg init a $ hg init a
$ cd a $ cd a
$ echo "first" > file.txt $ echo "first" > file.txt
$ hg add file.txt $ hg add file.txt
$ hg commit -m 'first commit' # 0 $ hg commit -m 'first commit' # 0
$ hg diff --change e9b286083166 $ hg diff --change e9b286083166
diff -r 4bb65dda5db4 -r e9b286083166 file.txt diff -r 4bb65dda5db4 -r e9b286083166 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
@@ -1,1 +1,1 @@ @@ -1,1 +1,1 @@
-first -first
+second +second
Test --from and --to
$ hg diff --from . --rev .
abort: cannot specify both --from and --rev
[10]
$ hg diff --to . --rev .
abort: cannot specify both --to and --rev
[10]
$ hg diff --from . --change .
abort: cannot specify both --from and --change
[10]
$ hg diff --to . --change .
abort: cannot specify both --to and --change
[10]
$ echo dirty > file.txt
$ hg diff --from .
diff -r bf5ff72eb7e0 file.txt
--- a/file.txt Thu Jan 01 00:00:00 1970 +0000
+++ b/file.txt Thu Jan 01 00:00:00 1970 +0000
@@ -1,1 +1,1 @@
-third
+dirty
$ hg diff --from . --reverse
diff -r bf5ff72eb7e0 file.txt
--- a/file.txt Thu Jan 01 00:00:00 1970 +0000
+++ b/file.txt Thu Jan 01 00:00:00 1970 +0000
@@ -1,1 +1,1 @@
-dirty
+third
$ hg diff --to .
diff -r bf5ff72eb7e0 file.txt
--- a/file.txt Thu Jan 01 00:00:00 1970 +0000
+++ b/file.txt Thu Jan 01 00:00:00 1970 +0000
@@ -1,1 +1,1 @@
-dirty
+third
$ hg diff --from 0 --to 2
diff -r 4bb65dda5db4 -r bf5ff72eb7e0 file.txt
--- a/file.txt Thu Jan 01 00:00:00 1970 +0000
+++ b/file.txt Thu Jan 01 00:00:00 1970 +0000
@@ -1,1 +1,1 @@
-first
+third
$ hg diff --from 2 --to 0
diff -r bf5ff72eb7e0 -r 4bb65dda5db4 file.txt
--- a/file.txt Thu Jan 01 00:00:00 1970 +0000
+++ b/file.txt Thu Jan 01 00:00:00 1970 +0000
@@ -1,1 +1,1 @@
-third
+first
$ hg co -C .
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ cd .. $ cd ..
Test dumb revspecs: top-level "x:y", "x:", ":y" and ":" ranges should be handled Test dumb revspecs: top-level "x:y", "x:", ":y" and ":" ranges should be handled
as pairs even if x == y, but not for "f(x:y)" nor "x::y" (issue3474, issue4774) as pairs even if x == y, but not for "f(x:y)" nor "x::y" (issue3474, issue4774)
$ hg clone -q a dumbspec $ hg clone -q a dumbspec
$ cd dumbspec $ cd dumbspec
$ echo "wdir" > file.txt $ echo "wdir" > file.txt

tests/test-help.t

Use the -g/--git option to generate diffs in the git extended diff format. Use the -g/--git option to generate diffs in the git extended diff format.
For more information, read 'hg help diffs'. For more information, read 'hg help diffs'.
Returns 0 on success. Returns 0 on success.
options ([+] can be repeated): options ([+] can be repeated):
-r --rev REV [+] revision -r --rev REV [+] revision
--from REV revision to diff from
--to REV revision to diff to
-c --change REV change made by revision -c --change REV change made by revision
-a --text treat all files as text -a --text treat all files as text
-g --git use git extended diff format -g --git use git extended diff format
--binary generate binary diffs in git mode (default) --binary generate binary diffs in git mode (default)
--nodates omit dates from diff headers --nodates omit dates from diff headers
--noprefix omit a/ and b/ prefixes from filenames --noprefix omit a/ and b/ prefixes from filenames
-p --show-function show which function each change is in -p --show-function show which function each change is in
--reverse produce a diff that undoes the changes --reverse produce a diff that undoes the changes