Page MenuHomePhabricator

revert: option to choose what to keep, not what to discard

Authored by martinvonz on Mar 12 2019, 6:13 PM.



I know the you (the reader) are probably tired of discussing how `hg
revert -i -r .` should behave and so am I. And I know I'm one of the
people who argued that showing the diff from the working copy to the
parent was confusing. I think it is less confusing now that we show
the diff from the parent to the working copy, but I still find it
confusing. I think showing the diff of hunks to keep might make it
easier to understand. So that's what this patch provides an option

One argument doing it this way is that most people seem to find `hg
split` natural. I suspect that is because it shows the forward diff
(from parent commit to the commit) and asks you what to put in the
first commit. I think the new "keep" mode for revert (this patch)
matches that.

In "keep" mode, all the changes are still selected by default. That
means that hg revert -i followed by 'A' (keep all) (or 'c' in
curses) will be different from hg revert -a. That's mostly because
that was simplest. It can also be argued that it's safest. But it can
also be argued that it should be consistent with hg revert -a.

Note that in this mode, you can edit the hunks and it will do what you
expect (e.g. add new lines to your file if you added a new lines when
editing). The test case shows that that works.

Diff Detail

rHG Mercurial
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.Mar 12 2019, 6:13 PM

I understand that it's not at all clear if we want to make this change by default, but I'd appreciate if we can queue this patch as is so I and others can easily start testing it (it's pretty deep inside _performrevert(), so it's hard to do it cleanly in an extension). I don't believe I'm changing the behavior for anyone who has not set the new config.

pulkit added a subscriber: pulkit.Mar 19 2019, 11:07 AM

I like what this patch does. I am not sure why this needs to be experimental since hg revert -i is not. Also, let's document this config option in hg help revert.


Is there any reason for it to be experimental? I see hg revert -i is not interactive.

martinvonz added inline comments.Mar 19 2019, 11:12 AM

I made it experimental so we can test it for a bit and see if we prefer this UX. I don't want to change the behavior for everyone without testing it a bit first, and it's hard to test this on more than just myself if it's not in core.

pulkit accepted this revision.Mar 19 2019, 2:07 PM
This revision was automatically updated to reflect the committed changes.