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

# HG changeset patch
# User Denis Laxalde <denis.laxalde@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)

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

Drop no longer used "experimental.revertalternateinteractivemode"
configuration option. (Keeping it would lead to inconsistent prompt message
vs. hunks display.)

On Mon, Nov 7, 2016 at 7:17 AM, Denis Laxalde <denis.laxalde@logilab.fr> wrote:
> # HG changeset patch
> # User Denis Laxalde <denis.laxalde@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)

Still looks great to me. Pierre-Yves, did you want discuss this?
Otherwise I'm more than happy to queue this now. I think this is the
obviously correct way of showing the hunks.

>
> 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...
>
> Drop no longer used "experimental.revertalternateinteractivemode"
> configuration option. (Keeping it would lead to inconsistent prompt message
> vs. hunks display.)
>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -3291,15 +3291,17 @@ def _performrevert(repo, parents, ctx, a
>          diffopts = patch.difffeatureopts(repo.ui, whitespace=True)
>          diffopts.nodates = True
>          diffopts.git = True
> -        reversehunks = repo.ui.configbool('experimental',
> -                                          'revertalternateinteractivemode',
> -                                          True)
> +        if node == parent:
> +            operation = 'discard'
> +            reversehunks = True
> +        else:
> +            operation = 'apply'
> +            reversehunks = False
>          if reversehunks:
>              diff = patch.diff(repo, ctx.node(), None, m, opts=diffopts)
>          else:
>              diff = patch.diff(repo, None, ctx.node(), m, opts=diffopts)
>          originalchunks = patch.parsepatch(diff)
> -        operation = 'discard' if node == parent else 'revert'
>
>          try:
>
> diff --git a/mercurial/patch.py b/mercurial/patch.py
> --- a/mercurial/patch.py
> +++ b/mercurial/patch.py
> @@ -980,14 +980,14 @@ def filterpatch(ui, headers, operation=N
>          operation = 'record'
>      messages = {
>          'multiple': {
> +            'apply': _("apply change %d/%d to '%s'?"),
>              'discard': _("discard change %d/%d to '%s'?"),
>              'record': _("record change %d/%d to '%s'?"),
> -            'revert': _("revert change %d/%d to '%s'?"),
>          }[operation],
>          'single': {
> +            'apply': _("apply this change to '%s'?"),
>              'discard': _("discard this change to '%s'?"),
>              'record': _("record this change to '%s'?"),
> -            'revert': _("revert this change to '%s'?"),
>          }[operation],
>      }
>
> diff --git a/tests/test-revert-interactive.t b/tests/test-revert-interactive.t
> --- a/tests/test-revert-interactive.t
> +++ b/tests/test-revert-interactive.t
> @@ -57,45 +57,45 @@ 10 run the same test than 8 from within
>    2 hunks, 2 lines changed
>    examine changes to 'f'? [Ynesfdaq?] y
>
> -  @@ -1,5 +1,6 @@
> -  +a
> -   1
> -   2
> -   3
> -   4
> -   5
> -  revert change 1/6 to 'f'? [Ynesfdaq?] y
> -
> -  @@ -1,5 +2,6 @@
> +  @@ -1,6 +1,5 @@
> +  -a
>     1
>     2
>     3
>     4
>     5
> -  +b
> -  revert change 2/6 to 'f'? [Ynesfdaq?] y
> +  apply change 1/6 to 'f'? [Ynesfdaq?] y
> +
> +  @@ -2,6 +1,5 @@
> +   1
> +   2
> +   3
> +   4
> +   5
> +  -b
> +  apply change 2/6 to 'f'? [Ynesfdaq?] y
>
>    diff --git a/folder1/g b/folder1/g
>    2 hunks, 2 lines changed
>    examine changes to 'folder1/g'? [Ynesfdaq?] y
>
> -  @@ -1,5 +1,6 @@
> -  +c
> +  @@ -1,6 +1,5 @@
> +  -c
>     1
>     2
>     3
>     4
>     5
> -  revert change 3/6 to 'folder1/g'? [Ynesfdaq?] y
> +  apply change 3/6 to 'folder1/g'? [Ynesfdaq?] y
>
> -  @@ -1,5 +2,6 @@
> +  @@ -2,6 +1,5 @@
>     1
>     2
>     3
>     4
>     5
> -  +d
> -  revert change 4/6 to 'folder1/g'? [Ynesfdaq?] n
> +  -d
> +  apply change 4/6 to 'folder1/g'? [Ynesfdaq?] n
>
>    diff --git a/folder2/h b/folder2/h
>    2 hunks, 2 lines changed
> @@ -143,12 +143,12 @@ Test that a noop revert doesn't do an un
>    1 hunks, 1 lines changed
>    examine changes to 'folder1/g'? [Ynesfdaq?] y
>
> -  @@ -3,3 +3,4 @@
> +  @@ -3,4 +3,3 @@
>     3
>     4
>     5
> -  +d
> -  revert this change to 'folder1/g'? [Ynesfdaq?] n
> +  -d
> +  apply this change to 'folder1/g'? [Ynesfdaq?] n
>
>    $ ls folder1/
>    g
> @@ -159,12 +159,12 @@ Test --no-backup
>    1 hunks, 1 lines changed
>    examine changes to 'folder1/g'? [Ynesfdaq?] y
>
> -  @@ -3,3 +3,4 @@
> +  @@ -3,4 +3,3 @@
>     3
>     4
>     5
> -  +d
> -  revert this change to 'folder1/g'? [Ynesfdaq?] y
> +  -d
> +  apply this change to 'folder1/g'? [Ynesfdaq?] y
>
>    $ ls folder1/
>    g
> @@ -190,45 +190,45 @@ Test --no-backup
>    2 hunks, 2 lines changed
>    examine changes to 'f'? [Ynesfdaq?] y
>
> -  @@ -1,5 +1,6 @@
> -  +a
> -   1
> -   2
> -   3
> -   4
> -   5
> -  revert change 1/6 to 'f'? [Ynesfdaq?] y
> -
> -  @@ -1,5 +2,6 @@
> +  @@ -1,6 +1,5 @@
> +  -a
>     1
>     2
>     3
>     4
>     5
> -  +b
> -  revert change 2/6 to 'f'? [Ynesfdaq?] y
> +  apply change 1/6 to 'f'? [Ynesfdaq?] y
> +
> +  @@ -2,6 +1,5 @@
> +   1
> +   2
> +   3
> +   4
> +   5
> +  -b
> +  apply change 2/6 to 'f'? [Ynesfdaq?] y
>
>    diff --git a/folder1/g b/folder1/g
>    2 hunks, 2 lines changed
>    examine changes to 'folder1/g'? [Ynesfdaq?] y
>
> -  @@ -1,5 +1,6 @@
> -  +c
> +  @@ -1,6 +1,5 @@
> +  -c
>     1
>     2
>     3
>     4
>     5
> -  revert change 3/6 to 'folder1/g'? [Ynesfdaq?] y
> +  apply change 3/6 to 'folder1/g'? [Ynesfdaq?] y
>
> -  @@ -1,5 +2,6 @@
> +  @@ -2,6 +1,5 @@
>     1
>     2
>     3
>     4
>     5
> -  +d
> -  revert change 4/6 to 'folder1/g'? [Ynesfdaq?] n
> +  -d
> +  apply change 4/6 to 'folder1/g'? [Ynesfdaq?] n
>
>    diff --git a/folder2/h b/folder2/h
>    2 hunks, 2 lines changed
> @@ -378,29 +378,29 @@ Check the experimental config to invert
>    3 hunks, 3 lines changed
>    examine changes to 'folder1/g'? [Ynesfdaq?] y
>
> -  @@ -1,5 +1,4 @@
> -  -firstline
> +  @@ -1,4 +1,5 @@
> +  +firstline
>     c
>     1
>     2
>     3
>    discard change 1/3 to 'folder1/g'? [Ynesfdaq?] y
>
> -  @@ -2,7 +1,7 @@
> +  @@ -1,7 +2,7 @@
>     c
>     1
>     2
>     3
> -  - 3
> -  +4
> +  -4
> +  + 3
>     5
>     d
>    discard change 2/3 to 'folder1/g'? [Ynesfdaq?] y
>
> -  @@ -7,3 +6,2 @@
> +  @@ -6,2 +7,3 @@
>     5
>     d
> -  -lastline
> +  +lastline
>    discard change 3/3 to 'folder1/g'? [Ynesfdaq?] n
>
>    $ hg diff --nodates
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

On Tue, Nov 08, 2016 at 09:21:19AM -0800, Martin von Zweigbergk via Mercurial-devel wrote:
> On Mon, Nov 7, 2016 at 7:17 AM, Denis Laxalde <denis.laxalde@logilab.fr> wrote:
> > # HG changeset patch
> > # User Denis Laxalde <denis.laxalde@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)
>
> Still looks great to me. Pierre-Yves, did you want discuss this?
> Otherwise I'm more than happy to queue this now. I think this is the
> obviously correct way of showing the hunks.

I also agree this is a significant clarity improvement.

>
> >
> > 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...
> >
> > Drop no longer used "experimental.revertalternateinteractivemode"
> > configuration option. (Keeping it would lead to inconsistent prompt message
> > vs. hunks display.)
> >
> > diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> > --- a/mercurial/cmdutil.py
> > +++ b/mercurial/cmdutil.py
> > @@ -3291,15 +3291,17 @@ def _performrevert(repo, parents, ctx, a
> >          diffopts = patch.difffeatureopts(repo.ui, whitespace=True)
> >          diffopts.nodates = True
> >          diffopts.git = True
> > -        reversehunks = repo.ui.configbool('experimental',
> > -                                          'revertalternateinteractivemode',
> > -                                          True)
> > +        if node == parent:
> > +            operation = 'discard'
> > +            reversehunks = True
> > +        else:
> > +            operation = 'apply'
> > +            reversehunks = False
> >          if reversehunks:
> >              diff = patch.diff(repo, ctx.node(), None, m, opts=diffopts)
> >          else:
> >              diff = patch.diff(repo, None, ctx.node(), m, opts=diffopts)
> >          originalchunks = patch.parsepatch(diff)
> > -        operation = 'discard' if node == parent else 'revert'
> >
> >          try:
> >
> > diff --git a/mercurial/patch.py b/mercurial/patch.py
> > --- a/mercurial/patch.py
> > +++ b/mercurial/patch.py
> > @@ -980,14 +980,14 @@ def filterpatch(ui, headers, operation=N
> >          operation = 'record'
> >      messages = {
> >          'multiple': {
> > +            'apply': _("apply change %d/%d to '%s'?"),
> >              'discard': _("discard change %d/%d to '%s'?"),
> >              'record': _("record change %d/%d to '%s'?"),
> > -            'revert': _("revert change %d/%d to '%s'?"),
> >          }[operation],
> >          'single': {
> > +            'apply': _("apply this change to '%s'?"),
> >              'discard': _("discard this change to '%s'?"),
> >              'record': _("record this change to '%s'?"),
> > -            'revert': _("revert this change to '%s'?"),
> >          }[operation],
> >      }
> >
> > diff --git a/tests/test-revert-interactive.t b/tests/test-revert-interactive.t
> > --- a/tests/test-revert-interactive.t
> > +++ b/tests/test-revert-interactive.t
> > @@ -57,45 +57,45 @@ 10 run the same test than 8 from within
> >    2 hunks, 2 lines changed
> >    examine changes to 'f'? [Ynesfdaq?] y
> >
> > -  @@ -1,5 +1,6 @@
> > -  +a
> > -   1
> > -   2
> > -   3
> > -   4
> > -   5
> > -  revert change 1/6 to 'f'? [Ynesfdaq?] y
> > -
> > -  @@ -1,5 +2,6 @@
> > +  @@ -1,6 +1,5 @@
> > +  -a
> >     1
> >     2
> >     3
> >     4
> >     5
> > -  +b
> > -  revert change 2/6 to 'f'? [Ynesfdaq?] y
> > +  apply change 1/6 to 'f'? [Ynesfdaq?] y
> > +
> > +  @@ -2,6 +1,5 @@
> > +   1
> > +   2
> > +   3
> > +   4
> > +   5
> > +  -b
> > +  apply change 2/6 to 'f'? [Ynesfdaq?] y
> >
> >    diff --git a/folder1/g b/folder1/g
> >    2 hunks, 2 lines changed
> >    examine changes to 'folder1/g'? [Ynesfdaq?] y
> >
> > -  @@ -1,5 +1,6 @@
> > -  +c
> > +  @@ -1,6 +1,5 @@
> > +  -c
> >     1
> >     2
> >     3
> >     4
> >     5
> > -  revert change 3/6 to 'folder1/g'? [Ynesfdaq?] y
> > +  apply change 3/6 to 'folder1/g'? [Ynesfdaq?] y
> >
> > -  @@ -1,5 +2,6 @@
> > +  @@ -2,6 +1,5 @@
> >     1
> >     2
> >     3
> >     4
> >     5
> > -  +d
> > -  revert change 4/6 to 'folder1/g'? [Ynesfdaq?] n
> > +  -d
> > +  apply change 4/6 to 'folder1/g'? [Ynesfdaq?] n
> >
> >    diff --git a/folder2/h b/folder2/h
> >    2 hunks, 2 lines changed
> > @@ -143,12 +143,12 @@ Test that a noop revert doesn't do an un
> >    1 hunks, 1 lines changed
> >    examine changes to 'folder1/g'? [Ynesfdaq?] y
> >
> > -  @@ -3,3 +3,4 @@
> > +  @@ -3,4 +3,3 @@
> >     3
> >     4
> >     5
> > -  +d
> > -  revert this change to 'folder1/g'? [Ynesfdaq?] n
> > +  -d
> > +  apply this change to 'folder1/g'? [Ynesfdaq?] n
> >
> >    $ ls folder1/
> >    g
> > @@ -159,12 +159,12 @@ Test --no-backup
> >    1 hunks, 1 lines changed
> >    examine changes to 'folder1/g'? [Ynesfdaq?] y
> >
> > -  @@ -3,3 +3,4 @@
> > +  @@ -3,4 +3,3 @@
> >     3
> >     4
> >     5
> > -  +d
> > -  revert this change to 'folder1/g'? [Ynesfdaq?] y
> > +  -d
> > +  apply this change to 'folder1/g'? [Ynesfdaq?] y
> >
> >    $ ls folder1/
> >    g
> > @@ -190,45 +190,45 @@ Test --no-backup
> >    2 hunks, 2 lines changed
> >    examine changes to 'f'? [Ynesfdaq?] y
> >
> > -  @@ -1,5 +1,6 @@
> > -  +a
> > -   1
> > -   2
> > -   3
> > -   4
> > -   5
> > -  revert change 1/6 to 'f'? [Ynesfdaq?] y
> > -
> > -  @@ -1,5 +2,6 @@
> > +  @@ -1,6 +1,5 @@
> > +  -a
> >     1
> >     2
> >     3
> >     4
> >     5
> > -  +b
> > -  revert change 2/6 to 'f'? [Ynesfdaq?] y
> > +  apply change 1/6 to 'f'? [Ynesfdaq?] y
> > +
> > +  @@ -2,6 +1,5 @@
> > +   1
> > +   2
> > +   3
> > +   4
> > +   5
> > +  -b
> > +  apply change 2/6 to 'f'? [Ynesfdaq?] y
> >
> >    diff --git a/folder1/g b/folder1/g
> >    2 hunks, 2 lines changed
> >    examine changes to 'folder1/g'? [Ynesfdaq?] y
> >
> > -  @@ -1,5 +1,6 @@
> > -  +c
> > +  @@ -1,6 +1,5 @@
> > +  -c
> >     1
> >     2
> >     3
> >     4
> >     5
> > -  revert change 3/6 to 'folder1/g'? [Ynesfdaq?] y
> > +  apply change 3/6 to 'folder1/g'? [Ynesfdaq?] y
> >
> > -  @@ -1,5 +2,6 @@
> > +  @@ -2,6 +1,5 @@
> >     1
> >     2
> >     3
> >     4
> >     5
> > -  +d
> > -  revert change 4/6 to 'folder1/g'? [Ynesfdaq?] n
> > +  -d
> > +  apply change 4/6 to 'folder1/g'? [Ynesfdaq?] n
> >
> >    diff --git a/folder2/h b/folder2/h
> >    2 hunks, 2 lines changed
> > @@ -378,29 +378,29 @@ Check the experimental config to invert
> >    3 hunks, 3 lines changed
> >    examine changes to 'folder1/g'? [Ynesfdaq?] y
> >
> > -  @@ -1,5 +1,4 @@
> > -  -firstline
> > +  @@ -1,4 +1,5 @@
> > +  +firstline
> >     c
> >     1
> >     2
> >     3
> >    discard change 1/3 to 'folder1/g'? [Ynesfdaq?] y
> >
> > -  @@ -2,7 +1,7 @@
> > +  @@ -1,7 +2,7 @@
> >     c
> >     1
> >     2
> >     3
> > -  - 3
> > -  +4
> > +  -4
> > +  + 3
> >     5
> >     d
> >    discard change 2/3 to 'folder1/g'? [Ynesfdaq?] y
> >
> > -  @@ -7,3 +6,2 @@
> > +  @@ -6,2 +7,3 @@
> >     5
> >     d
> > -  -lastline
> > +  +lastline
> >    discard change 3/3 to 'folder1/g'? [Ynesfdaq?] n
> >
> >    $ hg diff --nodates
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

On Wed, 9 Nov 2016 11:43:58 -0500, Augie Fackler wrote:
> On Tue, Nov 08, 2016 at 09:21:19AM -0800, Martin von Zweigbergk via Mercurial-devel wrote:
> > On Mon, Nov 7, 2016 at 7:17 AM, Denis Laxalde <denis.laxalde@logilab.fr> wrote:
> > > # HG changeset patch
> > > # User Denis Laxalde <denis.laxalde@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)
> >
> > Still looks great to me. Pierre-Yves, did you want discuss this?
> > Otherwise I'm more than happy to queue this now. I think this is the
> > obviously correct way of showing the hunks.
> 
> I also agree this is a significant clarity improvement.

+1. I want to see the diff what I'm going to apply.

On 11/07/2016 03:17 PM, Denis Laxalde wrote:
> # HG changeset patch
> # User Denis Laxalde <denis.laxalde@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.

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.
This is a smaller step without negative impact, if after a while user 
are pressing for changing the default, we could revisit the question.

Cheers,

On Thu, Nov 10, 2016 at 9:19 AM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
> On 11/07/2016 03:17 PM, Denis Laxalde wrote:
>>
>> # HG changeset patch
>> # User Denis Laxalde <denis.laxalde@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.

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

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

>
> Cheers,
>
> --
> Pierre-Yves David
>
> [1] that one is unchanged in this patch.
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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@ens-lyon.org> wrote:
>> On 11/07/2016 03:17 PM, Denis Laxalde wrote:
>>>
>>> # HG changeset patch
>>> # User Denis Laxalde <denis.laxalde@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,

Augie Fackler <raf@durin42.com> writes:

> On Thu, Nov 10, 2016 at 9:19 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>> On 11/07/2016 03:17 PM, Denis Laxalde wrote:
>>>
>>> # HG changeset patch
>>> # User Denis Laxalde <denis.laxalde@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.

Yeah, I agree with Augie here. Wholeheartedly, in fact.

On Thu, Nov 10, 2016 at 10:11 AM, Sean Farley <sean@farley.io> wrote:
> Augie Fackler <raf@durin42.com> writes:
>
>> On Thu, Nov 10, 2016 at 9:19 AM, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org> wrote:
>>> On 11/07/2016 03:17 PM, Denis Laxalde wrote:
>>>>
>>>> # HG changeset patch
>>>> # User Denis Laxalde <denis.laxalde@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.

I do agree with this point. How about introducing a "hg apply -r $rev"
that's a synonym for "hg revert -r $rev", except that it uses the
"apply" direction in interactive mode? I was positively surprised that
"hg apply" is not yet taken. Then we can consider even rejecting "hg
revert -ir $rev" unless $rev is ".".

>>>
>>> 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.
>
> Yeah, I agree with Augie here. Wholeheartedly, in fact.

On Thu, Nov 10, 2016 at 3:20 PM, Martin von Zweigbergk
<martinvonz@google.com> wrote:
> On Thu, Nov 10, 2016 at 10:11 AM, Sean Farley <sean@farley.io> wrote:
>> Augie Fackler <raf@durin42.com> writes:
>>
>>> On Thu, Nov 10, 2016 at 9:19 AM, Pierre-Yves David
>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>> On 11/07/2016 03:17 PM, Denis Laxalde wrote:
>>>>>
>>>>> # HG changeset patch
>>>>> # User Denis Laxalde <denis.laxalde@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.
>
> I do agree with this point. How about introducing a "hg apply -r $rev"
> that's a synonym for "hg revert -r $rev", except that it uses the
> "apply" direction in interactive mode? I was positively surprised that
> "hg apply" is not yet taken. Then we can consider even rejecting "hg
> revert -ir $rev" unless $rev is ".".

No other voices were raised, and JordiGH says "revert" sounds right to
him, so patch queued. Thanks!

>
>>>>
>>>> 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.
>>
>> Yeah, I agree with Augie here. Wholeheartedly, in fact.

On 11/14/2016 10:36 PM, Martin von Zweigbergk via Mercurial-devel wrote:
> On Thu, Nov 10, 2016 at 3:20 PM, Martin von Zweigbergk
> <martinvonz@google.com> wrote:
>> On Thu, Nov 10, 2016 at 10:11 AM, Sean Farley <sean@farley.io> wrote:
>>> Augie Fackler <raf@durin42.com> writes:
>>>
>>>> On Thu, Nov 10, 2016 at 9:19 AM, Pierre-Yves David
>>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>>> On 11/07/2016 03:17 PM, Denis Laxalde wrote:
>>>>>>
>>>>>> # HG changeset patch
>>>>>> # User Denis Laxalde <denis.laxalde@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.
>>
>> I do agree with this point. How about introducing a "hg apply -r $rev"
>> that's a synonym for "hg revert -r $rev", except that it uses the
>> "apply" direction in interactive mode? I was positively surprised that
>> "hg apply" is not yet taken. Then we can consider even rejecting "hg
>> revert -ir $rev" unless $rev is ".".
>
> No other voices were raised, and JordiGH says "revert" sounds right to
> him, so patch queued. Thanks!

I'm not too sure bout what was the conclusion and what was queued.

On Mon, Nov 14, 2016 at 2:49 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 11/14/2016 10:36 PM, Martin von Zweigbergk via Mercurial-devel wrote:
>>
>> On Thu, Nov 10, 2016 at 3:20 PM, Martin von Zweigbergk
>> <martinvonz@google.com> wrote:
>>>
>>> On Thu, Nov 10, 2016 at 10:11 AM, Sean Farley <sean@farley.io> wrote:
>>>>
>>>> Augie Fackler <raf@durin42.com> writes:
>>>>
>>>>> On Thu, Nov 10, 2016 at 9:19 AM, Pierre-Yves David
>>>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>>>>
>>>>>> On 11/07/2016 03:17 PM, Denis Laxalde wrote:
>>>>>>>
>>>>>>>
>>>>>>> # HG changeset patch
>>>>>>> # User Denis Laxalde <denis.laxalde@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.
>>>
>>>
>>> I do agree with this point. How about introducing a "hg apply -r $rev"
>>> that's a synonym for "hg revert -r $rev", except that it uses the
>>> "apply" direction in interactive mode? I was positively surprised that
>>> "hg apply" is not yet taken. Then we can consider even rejecting "hg
>>> revert -ir $rev" unless $rev is ".".
>>
>>
>> No other voices were raised, and JordiGH says "revert" sounds right to
>> him, so patch queued. Thanks!
>
>
> I'm not too sure bout what was the conclusion and what was queued.

The conclusion was that most people want "hg revert -i -r $rev" to
show the hunks to apply, which is what the patch at the start of this
thread does. I said I "revert" doesn't sound natural to me for
anything but reverting to the parent of working copy, but no one
seemed to agree, so I queued what was proposed in the patch.

>
> --
> Pierre-Yves David

On Mon, Nov 14, 2016 at 2:52 PM, Martin von Zweigbergk
<martinvonz@google.com> wrote:
> On Mon, Nov 14, 2016 at 2:49 PM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>>
>>
>> On 11/14/2016 10:36 PM, Martin von Zweigbergk via Mercurial-devel wrote:
>>>
>>> On Thu, Nov 10, 2016 at 3:20 PM, Martin von Zweigbergk
>>> <martinvonz@google.com> wrote:
>>>>
>>>> On Thu, Nov 10, 2016 at 10:11 AM, Sean Farley <sean@farley.io> wrote:
>>>>>
>>>>> Augie Fackler <raf@durin42.com> writes:
>>>>>
>>>>>> On Thu, Nov 10, 2016 at 9:19 AM, Pierre-Yves David
>>>>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>>>>>
>>>>>>> On 11/07/2016 03:17 PM, Denis Laxalde wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> # HG changeset patch
>>>>>>>> # User Denis Laxalde <denis.laxalde@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.
>>>>
>>>>
>>>> I do agree with this point. How about introducing a "hg apply -r $rev"
>>>> that's a synonym for "hg revert -r $rev", except that it uses the
>>>> "apply" direction in interactive mode? I was positively surprised that
>>>> "hg apply" is not yet taken. Then we can consider even rejecting "hg
>>>> revert -ir $rev" unless $rev is ".".
>>>
>>>
>>> No other voices were raised, and JordiGH says "revert" sounds right to
>>> him, so patch queued. Thanks!
>>
>>
>> I'm not too sure bout what was the conclusion and what was queued.
>
> The conclusion was that most people want "hg revert -i -r $rev" to
> show the hunks to apply, which is what the patch at the start of this
> thread does. I said I "revert" doesn't sound natural to me for
> anything but reverting to the parent of working copy, but no one
> seemed to agree, so I queued what was proposed in the patch.

Pierre-Yves reached out and reminded me that he actually frequently
uses "hg revert -i -r .~1" to undo changes since that commit. Also, I
still personally feel that the people who want to undo changes since a
revision and the people who want to get changes from a revision are
thinking about it differently enough that they should be using
different commands. I think it would be good if we could find good
names for both of those operations. In my mind, "revert" is a good
word for the former (i.e. undoing), and I'm not sure what's a good
word for the latter (i.e. getting state from). It's still unclear to
me if others were turned off mostly by my proposed name ("apply") or
the idea of them being different operations.

I'll de-queue this patch for now, and we can talk (even) more about it :-/

>
>>
>> --
>> Pierre-Yves David

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

For me, as a user, REV being in the "background" or not is not
particularly relevant. 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). 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.

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

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


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

Which "softer way"? We could not drop the experimental option to let the
users you refer to have the direction "reversed", I guess.

On 11/15/2016 08:24 AM, Denis Laxalde wrote:
> Pierre-Yves David a écrit :
>>
 >> […]

Skipping most of the content. I am currently traveling and will spend 
more time on this next week.

 >>
>>> 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.
>
> Which "softer way"? We could not drop the experimental option to let the
> users you refer to have the direction "reversed", I guess.

My proposal is to add an "action" to the revert UI to switch the 
direction during the revert. That would be a good first step and allow 
each type of user to eventually get what they need whatever the final 
default we pick.

Cheers,

> On Nov 15, 2016, at 09:11, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> 
> 
> On 11/15/2016 08:24 AM, Denis Laxalde wrote:
>> Pierre-Yves David a écrit :
>>> 
> >> […]
> 
> Skipping most of the content. I am currently traveling and will spend more time on this next week.
> 
> >>
>>>> 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.
>> 
>> Which "softer way"? We could not drop the experimental option to let the
>> users you refer to have the direction "reversed", I guess.
> 
> My proposal is to add an "action" to the revert UI to switch the direction during the revert. That would be a good first step and allow each type of user to eventually get what they need whatever the final default we pick.

Alternative: we add --no-commit to graft, and then --interactive. Then the user could spell this as 'hg graft --no-commit -i FOO' instead of 'hg revert -r FOO -i' and it states their intent a little more clearly. This is similar to the 'hg apply' concept that came up earlier, but uses an existing command.

I have no idea if this is a good idea or not, but it occurs to me that it might be more correct from a UX perspective. Typically what I'm doing when I get confused by the "reverse"[0] diffs in revert it's because I'm doing something like 'hg co FOO^ && hg revert -ir FOO' to pick up part of FOO's changes in the working copy (I'm not sure it's ever been for grabbing hunks from multiple changes, but even if it was interactive graft could accept multiple revisions...

Just a thought.
AF

0: scare quotes because the diff direction in revert always matches the output of 'hg diff -r FOO', so that's actually some nice consistency.

On Wed, Nov 16, 2016 at 7:50 AM, Augie Fackler <raf@durin42.com> wrote:
>
>> On Nov 15, 2016, at 09:11, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>
>>
>>
>> On 11/15/2016 08:24 AM, Denis Laxalde wrote:
>>> Pierre-Yves David a écrit :
>>>>
>> >> […]
>>
>> Skipping most of the content. I am currently traveling and will spend more time on this next week.
>>
>> >>
>>>>> 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.
>>>
>>> Which "softer way"? We could not drop the experimental option to let the
>>> users you refer to have the direction "reversed", I guess.
>>
>> My proposal is to add an "action" to the revert UI to switch the direction during the revert. That would be a good first step and allow each type of user to eventually get what they need whatever the final default we pick.
>
> Alternative: we add --no-commit to graft, and then --interactive. Then the user could spell this as 'hg graft --no-commit -i FOO' instead of 'hg revert -r FOO -i' and it states their intent a little more clearly. This is similar to the 'hg apply' concept that came up earlier, but uses an existing command.

"hg graft -r $rev" is about applying the changes between $rev and
$rev^ to the working copy. "hg revert -r $rev" is about applying the
changes between the working copy and $rev to the working copy. So "hg
graft" doesn't seem like a good choice.

Note that I described revert's behavior backwards there on purpose,
just to match graft's behavior. I think most of us think of it as
discarding changes from $rev to the working copy, at least when $rev
is ".".

Other than "hg revert", the command I can think of that's closest is
"hg checkout". Note that that's what git uses. I think that's too much
overloading for the command, so I don't think we should follow their
lead.

>
> I have no idea if this is a good idea or not, but it occurs to me that it might be more correct from a UX perspective. Typically what I'm doing when I get confused by the "reverse"[0] diffs in revert it's because I'm doing something like 'hg co FOO^ && hg revert -ir FOO' to pick up part of FOO's changes in the working copy (I'm not sure it's ever been for grabbing hunks from multiple changes, but even if it was interactive graft could accept multiple revisions...

Yes, that's exactly why I suggested a different command -- because my
mindset is different when I want to apply changes to the working copy
rather than to discard them. Also note that git uses "git apply" to
mean something like "hg import", and maybe that's why some people
think "apply" sounds like "graft".

>
> Just a thought.
> AF
>
> 0: scare quotes because the diff direction in revert always matches the output of 'hg diff -r FOO', so that's actually some nice consistency.

Excerpts from Martin von Zweigbergk's message of 2016-11-14 15:57:30 -0800:
> Pierre-Yves reached out and reminded me that he actually frequently
> uses "hg revert -i -r .~1" to undo changes since that commit. Also, I
> still personally feel that the people who want to undo changes since a
> revision and the people who want to get changes from a revision are
> thinking about it differently enough that they should be using
> different commands. I think it would be good if we could find good

There are other commands:
- For picking what should be undone: "hg uncommit -i"
- For picking what to kept: "hg split". (We can enhance it so it's possible
  to abort and drop the remaining changes)

Those commands sound more intuitive to me and could probably fit marmoute's
use-case.

> names for both of those operations. In my mind, "revert" is a good
> word for the former (i.e. undoing), and I'm not sure what's a good
> word for the latter (i.e. getting state from). It's still unclear to
> me if others were turned off mostly by my proposed name ("apply") or
> the idea of them being different operations.
> 
> I'll de-queue this patch for now, and we can talk (even) more about it :-/

An internal user just complained about the confusing behavior. I think an
easy change we would all agree is to make the curses UI to use the same verb
as the text UI does.

Before:

  SELECT CHUNKS: .... toggle hunk/all; (e)dit hunk;
   (f)old/unfold; (c)onfirm applied; ... [X]=hunk applied ....
                            ^^^^^^^               ^^^^^^^
After:

  SELECT CHUNKS: .... toggle hunk/all; (e)dit hunk;
   (f)old/unfold; (c)onfirm revert; ... [X]=hunk to revert ....
                            ^^^^^^               ^^^^^^^^^

If there is no objection, I could prepare the patch.

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,

On 11/23/2016 04:01 PM, Jun Wu wrote:
> Excerpts from Martin von Zweigbergk's message of 2016-11-14 15:57:30 -0800:
>> Pierre-Yves reached out and reminded me that he actually frequently
>> uses "hg revert -i -r .~1" to undo changes since that commit. Also, I
>> still personally feel that the people who want to undo changes since a
>> revision and the people who want to get changes from a revision are
>> thinking about it differently enough that they should be using
>> different commands. I think it would be good if we could find good
>
> There are other commands:
> - For picking what should be undone: "hg uncommit -i"
> - For picking what to kept: "hg split". (We can enhance it so it's possible
>   to abort and drop the remaining changes)
>
> Those commands sound more intuitive to me and could probably fit marmoute's
> use-case.

These does not really fits my usecase, a frequent use of 'hg revert -i 
-r .~x' to temporarily revert some change and check if it responsible 
for a crash/change in test. I don't want to touch history yet at that 
point. I want to check the check between my current state and an earlier 
point and revert some of these change. So neither 'uncommit' or 'split' 
are adequate here.


I also sometime use it to just drop a hunk I commited by mistake, in 
this case a combination of 'hg uncommit -i + hg revert -i' (or some kind 
of 'hg uncommit -i --revert') could do it. Not that we do not have '-i' 
for 'hg uncommit' yet (that would be handy).



On the contrary, it seems to me 'hg split' could cover some of the 
usecase of other people in this thread.


>> names for both of those operations. In my mind, "revert" is a good
>> word for the former (i.e. undoing), and I'm not sure what's a good
>> word for the latter (i.e. getting state from). It's still unclear to
>> me if others were turned off mostly by my proposed name ("apply") or
>> the idea of them being different operations.
>>
>> I'll de-queue this patch for now, and we can talk (even) more about it :-/
>
> An internal user just complained about the confusing behavior. I think an
> easy change we would all agree is to make the curses UI to use the same verb
> as the text UI does.
>
> Before:
>
>   SELECT CHUNKS: .... toggle hunk/all; (e)dit hunk;
>    (f)old/unfold; (c)onfirm applied; ... [X]=hunk applied ....
>                             ^^^^^^^               ^^^^^^^
> After:
>
>   SELECT CHUNKS: .... toggle hunk/all; (e)dit hunk;
>    (f)old/unfold; (c)onfirm revert; ... [X]=hunk to revert ....
>                             ^^^^^^               ^^^^^^^^^
>
> If there is no objection, I could prepare the patch.

+1 for having curse using the proper action/verb, that would for sure 
help the situation.

Cheers,