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

login
register
mail settings
Submitter Denis Laxalde
Date Sept. 30, 2016, 12:39 p.m.
Message ID <992c369a0dbc96f8276e.1475239140@sh77.tls.logilab.fr>
Download mbox | patch
Permalink /patch/16808/
State Deferred
Headers show

Comments

Denis Laxalde - Sept. 30, 2016, 12:39 p.m.
# HG changeset patch
# User Denis Laxalde <denis.laxalde@logilab.fr>
# Date 1475237490 -7200
#      Fri Sep 30 14:11:30 2016 +0200
# Node ID 992c369a0dbc96f8276e0585893c76dbdf68834d
# Parent  e83f89d3b1f733d0ee5f23f6a2293279a17fbbfb
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.)
via Mercurial-devel - Sept. 30, 2016, 4:39 p.m.
This looks great to me. I think Pierre-Yves wanted to discuss this in
person at the sprint, and since the sprint is only a week away, I can
wait. I won't be at the sprint, but Augie/durin42 will be and I saw
that he also was confused about the current behavior. I still haven't
heard from anyone who has used the feature who prefers the way it
currently works.

On Fri, Sep 30, 2016 at 5:39 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 992c369a0dbc96f8276e0585893c76dbdf68834d
> # Parent  e83f89d3b1f733d0ee5f23f6a2293279a17fbbfb
> 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.)
>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -3266,15 +3266,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
Pierre-Yves David - Oct. 8, 2016, 9:49 a.m.
On 09/30/2016 06:39 PM, Martin von Zweigbergk via Mercurial-devel wrote:
> This looks great to me. I think Pierre-Yves wanted to discuss this in
> person at the sprint, and since the sprint is only a week away, I can
> wait. I won't be at the sprint, but Augie/durin42 will be and I saw
> that he also was confused about the current behavior. I still haven't
> heard from anyone who has used the feature who prefers the way it
> currently works.

I'm still not very enthusiastic about this, but lets discuss it today or 
tomorrow.
Augie Fackler - Oct. 8, 2016, 9:57 a.m.
> On Sep 30, 2016, at 18:39, Martin von Zweigbergk via Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
> 
> This looks great to me. I think Pierre-Yves wanted to discuss this in
> person at the sprint, and since the sprint is only a week away, I can
> wait. I won't be at the sprint, but Augie/durin42 will be and I saw
> that he also was confused about the current behavior. I still haven't
> heard from anyone who has used the feature who prefers the way it
> currently works.

I agree, this appears to be clearer.
Augie Fackler - Oct. 18, 2016, 1:13 p.m.
On Sat, Oct 08, 2016 at 11:49:54AM +0200, Pierre-Yves David wrote:
>
>
> On 09/30/2016 06:39 PM, Martin von Zweigbergk via Mercurial-devel wrote:
> >This looks great to me. I think Pierre-Yves wanted to discuss this in
> >person at the sprint, and since the sprint is only a week away, I can
> >wait. I won't be at the sprint, but Augie/durin42 will be and I saw
> >that he also was confused about the current behavior. I still haven't
> >heard from anyone who has used the feature who prefers the way it
> >currently works.
>
> I'm still not very enthusiastic about this, but lets discuss it today or
> tomorrow.

I'm still very much a fan of this patch. Did your discussion ever happen?

>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - Oct. 18, 2016, 1:14 p.m.
On 10/18/2016 03:13 PM, Augie Fackler wrote:
> On Sat, Oct 08, 2016 at 11:49:54AM +0200, Pierre-Yves David wrote:
>>
>>
>> On 09/30/2016 06:39 PM, Martin von Zweigbergk via Mercurial-devel wrote:
>>> This looks great to me. I think Pierre-Yves wanted to discuss this in
>>> person at the sprint, and since the sprint is only a week away, I can
>>> wait. I won't be at the sprint, but Augie/durin42 will be and I saw
>>> that he also was confused about the current behavior. I still haven't
>>> heard from anyone who has used the feature who prefers the way it
>>> currently works.
>>
>> I'm still not very enthusiastic about this, but lets discuss it today or
>> tomorrow.
>
> I'm still very much a fan of this patch. Did your discussion ever happen?

No the discussion did not happened. Lets try to find an efficient way to 
discuss this in the next cycle.

Cheers,
Augie Fackler - Oct. 18, 2016, 1:18 p.m.
On Tue, Oct 18, 2016 at 9:14 AM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 10/18/2016 03:13 PM, Augie Fackler wrote:
>>
>> On Sat, Oct 08, 2016 at 11:49:54AM +0200, Pierre-Yves David wrote:
>>>
>>>
>>>
>>> On 09/30/2016 06:39 PM, Martin von Zweigbergk via Mercurial-devel wrote:
>>>>
>>>> This looks great to me. I think Pierre-Yves wanted to discuss this in
>>>> person at the sprint, and since the sprint is only a week away, I can
>>>> wait. I won't be at the sprint, but Augie/durin42 will be and I saw
>>>> that he also was confused about the current behavior. I still haven't
>>>> heard from anyone who has used the feature who prefers the way it
>>>> currently works.
>>>
>>>
>>> I'm still not very enthusiastic about this, but lets discuss it today or
>>> tomorrow.
>>
>>
>> I'm still very much a fan of this patch. Did your discussion ever happen?
>
>
> No the discussion did not happened. Lets try to find an efficient way to
> discuss this in the next cycle.

Indeed. Please resend this on about November 2nd and we'll see what
progress we can make.

>
> Cheers,
>
> --
> Pierre-Yves David

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -3266,15 +3266,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