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

[Pierre-Yves David]

On 11/15/2016 09:24 AM, Denis Laxalde wrote:
> Pierre-Yves David a écrit :
>> 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.
>
> There's an essential different with these "other commands"
> (commit/shelve) and revert: the former operate on the repository history
> while the latter operates on the working directory. In this respect,
> when interactively applying changes to the repository (commit-like
> commands, former case), one gets prompted with a diff as it would apply
> to the current revision. This patch essentially proposes to apply the
> same logic to the revert command (i.e. prompt the user with a change
> that would be applied to the working directory) and to prompt the user
> with an intelligible action (avoid double negation).

I don't really follow all your logic here. For example `hg shelve` is 
touching the working directory (by removing change in it, much like what 
`hg revert` does).

To me, for the way I use `hg revert -i --rev` the current direction is 
an intelligible action. I understand and recognize that some other 
people use it in a way where the other direction is more intelligible to 
them. This is why I suggest we get a way for the UI to offer both 
direction, there is probably always going to be a case where someone 
legitimately wants one of the two specific directions.

> For me, as a user, REV being in the "background" or not is not
> particularly relevant.

To me, REV being in the background is relevant (as I'm currently using 
it that way) and seeing the diff that I discard is the information 
intelligible to me. It was the other way for a brief period, that was 
confusing and made the command unhelpful to me. After discussing it with 
Laurent he made dcc56e10c23b. The commit you are trying to revert here.

> I sometimes use -r REV with REV being on some
> "unrelated" branch. More often do I use -r REV with REV being a
> descendant changeset (for instance to extract a refactoring that I
> committed together with a functional change -- hg up REV^; hg rev -i -r
> REV; then commit/rebase).

By the way, did you gave a shot to `hg split` for this very usecase?

> I can't figure out how your reasoning would
> apply in this case, but I'm certain that being prompted with changes as
> they would apply to the working directory won't be confusing for me.

The things I'm pointing out as confusing¹ here is the change in diff 
direction given the command invocation:

For example `hg revert -i` show different diff that `hg revert -i -r .`, 
And if we keep the direction for `--rev BACKGROUND` we end up with even 
subtler change from.

This is explained in more detailed in my previous email.


>>  (typical, (use hg diff, see something to drop; use hg
>> revert, drop the same thing at the same spot).
>
> That one (which corresponds to `hg revert -i [-r .]`) already works that
> way, only it shows a "discard this hunk" prompt.
>
>
>> 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.

These two sentences of mine works together:

Your proposal breaks it for `hg diff -r .~1 # hg pdiff` and associated 
`hg revert -i --rev .~1`. (or any further ancestors/precursors). 
Something I'm already using very frequently while developing.

(also, reiteration of my point that the switch in direction between even 
`hg revert -i` and `hg revert -ir .` is too subtle with just a small 
wording changes. I don't expect user to read wording in place they are 
familiar with.

>> 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`)
>
> Without -r REV, `hg revert -i` is basically a "discard uncommitted
> changes" command. So ones gets prompted with these uncommitted changes
> and whether to discard them or not. Sure the diff is reversed with
> respected to other cases, but the fundamental idea is that the
> combination of "diff direction" and "operation" (discard or apply) is
> consistent.

Just to make sure I'm clear:

  * I use `hg revert -i -r BACKGROUND` to 'discard' hunk. I need the 
appropriate direction and action to do so.

  * I think the 'operation' change is going to be too subtle for user to 
avoid large amount of confusion.

>> 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
>
> Again, if we consider the working directory as the reference to compare
> to, the direction is not reversed in either case. We'll always be
> prompted to accept changes (be they a "discard" or an "apply" operation)
> from either the parent (first case, diff hunks as outputted by `hg diff
> -r .`) or this "any" revision (second case, diff hunks as outputted by
> `hg diff -r . -r any`) to the working directory.

I get there is some logic in the change proposed by this patch (and I 
also recognize there is some usecase).

However a facette of the point I make can probably be summarized as the 
fact we have:

   'hg diff' = 'hg diff --rev .'

So it will be more consistent to have:

   'hg revert -i' = 'hg revert -i -r .'

>> 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`
>
> Unless I missed something, directions should be the same for all cases
> where -r is specified (i.e. `hg diff -r . -r REV`).

cf the whole section where I explain that the current direction make 
sense for `hg revert -i -r BACKGROUND` and that I actually use it that 
way on a regular basis (and already tried the other way around). So to 
me we should not change it at least for the background case. From there 
I'm exploring the idea of changing it for some value of --rev (and not 
other). (As the result of this exploration I find the idea even more 
confusing for user)

>> I greatly
>> advice we move with caution here, especially since we have a softer way
>> to more forward on the topic.
>
> Which "softer way"? We could not drop the experimental option to let the
> users you refer to have the direction "reversed", I guess.

cf: 
https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-November/090556.html


If my position is still unclear to you after this email, we should 
probably get on the phone and discuss it synchronously.

Cheers,

-- 
Pierre-Yves David

[1] in addition to the fact I think `hg revert -i -r BACKGROUND` is 
currently displaying diff in the right other.