Page MenuHomePhabricator

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

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.

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.

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

+1 for DEPRECATED

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

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.

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.

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

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