[PATCH RESEND] revert: do not reverse hunks in interactive when REV is not parent (issue5096)

[Pierre-Yves David]

On 11/10/2016 03:34 PM, Augie Fackler wrote:
> On Thu, Nov 10, 2016 at 9:19 AM, Pierre-Yves David
> <pierre-yves.david at ens-lyon.org> wrote:
>> On 11/07/2016 03:17 PM, Denis Laxalde wrote:
>>>
>>> # HG changeset patch
>>> # User Denis Laxalde <denis.laxalde at logilab.fr>
>>> # Date 1475237490 -7200
>>> #      Fri Sep 30 14:11:30 2016 +0200
>>> # Node ID 4f7b7750403ce48e78d0f361236f65ac03584c3c
>>> # Parent  d06c049695e6ad3219e7479c65ce98a2f123e878
>>> revert: do not reverse hunks in interactive when REV is not parent
>>> (issue5096)
>>
>>
>> (note, we should make this a BC)
>>
>>>
>>> And introduce a new "apply" operation verb for this case as suggested in
>>> issue5096. This replaces the no longer used "revert" operation.
>>>
>>> In interactive revert, when reverting to something else that the parent
>>> revision, display an "apply this change" message with a diff that is not
>>> reversed.
>>>
>>> The rationale is that `hg revert -i -r REV` will show hunks of the diff
>>> from
>>> the working directory to REV and prompt the user to select them for
>>> applying
>>> (to working directory). This somehow contradicts dcc56e10c23b in which it
>>> was
>>> decided to have the "direction" of prompted hunks reversed...
>>
>>
>> I still think this is not the right thing to
>>
>> To me, Having `hg diff`, `hg commit/amend/shelve -i` and `hg revert` showing
>> the same hunk make sense and is really useful. In particular showing the
>> change I'm about to revert makes a lot of sense when I revert to a parent¹
>> or an ancestors, or anything in the background of the patch (precursors, or
>> ancestors, or combination of these). I check the change I introduce since a
>> given point and revert some. To me using a revert to fetch content from
>> arbitrary unrelated changeset is a semantic drift of the command. Such
>> semantic drift should not motivate change that alter the core goal of the
>> commands.
>>
>> Having a different behavior depending of the target (with or without -r, -r
>> on a background changeset or not) seems too confusing for me. The change in
>> the action verb (revert/apply) seems too subtle to me this is going to bite
>> user.
>
> I've got 2 committers beyond me that agree the current behavior of
> revert is confusing, plus at least one contributor from Logilab. I
> realize you worry this is confusing, but I have had to experiment
> multiple times on a test repo before being able to safely use 'hg
> revert -ir' and this patch resolves that usability problem for me.
> Today, we say "discard" or "revert" based on the value of -r already,
> but the word "revert" has to be parsed and then understood to mean
> "I'm going to patch -R this if you say yes". The double negative
> requires careful thought on my part, and has the same net meaning.

We can extract two main points in the section you reply to and I'm not 
sure which one you address in your reply. I'm going to re-emit them in a 
simplified/modified form to clarify

First important point
=====================

In the following case:
* `hg revert -i`
* `hg revert -i -r ancestor`
* `hg revert -i -r precursor`
* `hg revert -i -r background` #combination of the above,

Seeing the hunk the way they are currently is exactly what I want to 
see. These hunks are shown that way by all other commands (diff, commit, 
amend, shelve) and not seeing the same thing in revert is super 
confusing to me. (typical, (use hg diff, see something to drop; use hg 
revert, drop the same thing at the same spot).

An excellent example for such case is `hg revert -i -r .~1`, associate 
with `hg pdif `(hg diff -r .~1) and amend I'm a bit user of it.

In that case, I'm not "worried it will be confusing" I know from 
experience that I'm greatly confused by it. This was one of the 
motivation of dcc56e10c23b;

The fact "hg revert -i" display the right thing in these situation is 
partly validated by the fact this patch for not change the direction for 
the first item in my list (plain `hg revert -i`)

The second import point
=======================

Having different direction given invocation is confusing.

Case one (this exact patch):
----------------------------

We have different direction between

* hg revert -i

and

* hg revert -i -r any

I expect some confusion out of that and as pointed in the previous 
section I think the behavior for '--rev background' is the wrong one.

Case two (keep current state for case point in the first section)
-----------------------------------------------------------------

We have different direction between

* `hg revert -i`
* `hg revert -i -r ancestor`
* `hg revert -i -r precursor`
* `hg revert -i -r background` #combination of the above,

and

* `hg revert -i -r other`

I expect major confusion about diff direction and would feel embarrassed 
to explain what trigger the different to user.

>> As I expressed the first time this was discussed I think we should start by
>> introducing a way to revert the direction during the revert itself so that
>> people switch to "apply-diff" when needed without touching the default
>> behavior.
>
> I don't understand this proposal, so maybe you could reply-all with a
> sample invocation dummied out for us to consider?


Revert ask the user for actions (the famous "[Ynesfdaq?]" for old record 
and the top bar for crecord). I think we should have an action "foo" 
that change the direction of the diff for the rest of the record, this 
will give a way to the user to have diff in the right direction what 
situation he is in.

This is similar to the proposal of having a "show is curse" action for 
old record to allow user to jump into the more modern view.

>> This is a smaller step without negative impact, if after a while user are
>> pressing for changing the default, we could revisit the question.
>
> I'm still strongly inclined to take this patch, because you're the
> only one opposed.

This is a backward compatibility breaking change, confusing some users, 
and with currently no way to restore the previous experience. I greatly 
advice we move with caution here, especially since we have a softer way 
to more forward on the topic.

Cheers,

-- 
Pierre-Yves David