Patchwork [RFC] revert: no longer mark --interactive as experimental

login
register
mail settings
Submitter Augie Fackler
Date Nov. 1, 2017, 11:32 p.m.
Message ID <a2fc2086ac3116c63ab8.1509579147@augie-macbookpro2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/25324/
State Accepted
Headers show

Comments

Augie Fackler - Nov. 1, 2017, 11:32 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1509399967 14400
#      Mon Oct 30 17:46:07 2017 -0400
# Node ID a2fc2086ac3116c63ab890180697b2695c50d9f4
# Parent  3ce0e4b51f789eff195ec900a07c1fa5e8d5c5f2
revert: no longer mark --interactive as experimental

We seem to have settled down on behavior changes here (nothing
matching revset `keyword(revert) and keyword(interactive)` since 4.2
was released), so let's go ahead and advertise this excellent feature.

.. feature:: revert --interactive

  The revert command now accepts the flag --interactive to allow reverting
  only some of the changes to the specified files.
Denis Laxalde - Nov. 2, 2017, 9:03 a.m.
Augie Fackler a écrit :
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1509399967 14400
> #      Mon Oct 30 17:46:07 2017 -0400
> # Node ID a2fc2086ac3116c63ab890180697b2695c50d9f4
> # Parent  3ce0e4b51f789eff195ec900a07c1fa5e8d5c5f2
> revert: no longer mark --interactive as experimental
> 
> We seem to have settled down on behavior changes here (nothing
> matching revset `keyword(revert) and keyword(interactive)` since 4.2
> was released), so let's go ahead and advertise this excellent feature.

+1

> .. feature:: revert --interactive
> 
>    The revert command now accepts the flag --interactive to allow reverting
>    only some of the changes to the specified files.
> 
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -4517,8 +4517,7 @@ def resolve(ui, repo, *pats, **opts):
>       ('d', 'date', '', _('tipmost revision matching date'), _('DATE')),
>       ('r', 'rev', '', _('revert to the specified revision'), _('REV')),
>       ('C', 'no-backup', None, _('do not save backup copies of files')),
> -    ('i', 'interactive', None,
> -            _('interactively select the changes (EXPERIMENTAL)')),
> +    ('i', 'interactive', None, _('interactively select the changes')),
>       ] + walkopts + dryrunopts,
>       _('[OPTION]... [-r REV] [NAME]...'))
>   def revert(ui, repo, *pats, **opts):
Yuya Nishihara - Nov. 2, 2017, 12:41 p.m.
On Thu, 2 Nov 2017 10:03:44 +0100, Denis Laxalde wrote:
> Augie Fackler a écrit :
> > # HG changeset patch
> > # User Augie Fackler <augie@google.com>
> > # Date 1509399967 14400
> > #      Mon Oct 30 17:46:07 2017 -0400
> > # Node ID a2fc2086ac3116c63ab890180697b2695c50d9f4
> > # Parent  3ce0e4b51f789eff195ec900a07c1fa5e8d5c5f2
> > revert: no longer mark --interactive as experimental
> > 
> > We seem to have settled down on behavior changes here (nothing
> > matching revset `keyword(revert) and keyword(interactive)` since 4.2
> > was released), so let's go ahead and advertise this excellent feature.
> 
> +1

+1, too. Queued, thanks.
via Mercurial-devel - Nov. 2, 2017, 1:39 p.m.
Can we still change the behavior of "hg revert -i -r" to show a to-apply
diff, not a to-revert diff? (There's a bug number I'm too lazy to look up
from mobile.)

On Thu, Nov 2, 2017, 05:47 Yuya Nishihara <yuya@tcha.org> wrote:

> On Thu, 2 Nov 2017 10:03:44 +0100, Denis Laxalde wrote:
> > Augie Fackler a écrit :
> > > # HG changeset patch
> > > # User Augie Fackler <augie@google.com>
> > > # Date 1509399967 14400
> > > #      Mon Oct 30 17:46:07 2017 -0400
> > > # Node ID a2fc2086ac3116c63ab890180697b2695c50d9f4
> > > # Parent  3ce0e4b51f789eff195ec900a07c1fa5e8d5c5f2
> > > revert: no longer mark --interactive as experimental
> > >
> > > We seem to have settled down on behavior changes here (nothing
> > > matching revset `keyword(revert) and keyword(interactive)` since 4.2
> > > was released), so let's go ahead and advertise this excellent feature.
> >
> > +1
>
> +1, too. Queued, thanks.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Yuya Nishihara - Nov. 2, 2017, 1:47 p.m.
On Thu, 02 Nov 2017 13:39:59 +0000, Martin von Zweigbergk wrote:
> Can we still change the behavior of "hg revert -i -r" to show a to-apply
> diff, not a to-revert diff? (There's a bug number I'm too lazy to look up
> from mobile.)

Oh, I thought "revert -i" would already work that way. I think BC of
interactive outputs is acceptable.
Augie Fackler - Nov. 2, 2017, 2 p.m.
> On Nov 2, 2017, at 09:39, Martin von Zweigbergk via Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
> 
> Can we still change the behavior of "hg revert -i -r" to show a to-apply diff, not a to-revert diff? (There's a bug number I'm too lazy to look up from mobile.)

I thought we had already done the patch-reversing that we felt was required...

> 
> On Thu, Nov 2, 2017, 05:47 Yuya Nishihara <yuya@tcha.org> wrote:
> On Thu, 2 Nov 2017 10:03:44 +0100, Denis Laxalde wrote:
> > Augie Fackler a écrit :
> > > # HG changeset patch
> > > # User Augie Fackler <augie@google.com>
> > > # Date 1509399967 14400
> > > #      Mon Oct 30 17:46:07 2017 -0400
> > > # Node ID a2fc2086ac3116c63ab890180697b2695c50d9f4
> > > # Parent  3ce0e4b51f789eff195ec900a07c1fa5e8d5c5f2
> > > revert: no longer mark --interactive as experimental
> > >
> > > We seem to have settled down on behavior changes here (nothing
> > > matching revset `keyword(revert) and keyword(interactive)` since 4.2
> > > was released), so let's go ahead and advertise this excellent feature.
> >
> > +1
> 
> +1, too. Queued, thanks.
> _______________________________________________
> 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
Denis Laxalde - Nov. 2, 2017, 2:16 p.m.
Augie Fackler a écrit :
> 
>> On Nov 2, 2017, at 09:39, Martin von Zweigbergk via Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
>>
>> Can we still change the behavior of "hg revert -i -r" to show a to-apply diff, not a to-revert diff? (There's a bug number I'm too lazy to look up from mobile.)
> 
> I thought we had already done the patch-reversing that we felt was required...

The last discussion ended with a status quo:

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

Since then, I set "experimental.revertalternateinteractivemode=false" to
have a behavior that I find meaningful in most situations. The only case
it does not work well is "hg revert -i -r .^" (which I think was a major
motivation for the current behavior). On the other hand, now "hg
uncommit" in evolve extension has a --interactive flag that could be
used instead.

> 
>>
>> On Thu, Nov 2, 2017, 05:47 Yuya Nishihara <yuya@tcha.org> wrote:
>> On Thu, 2 Nov 2017 10:03:44 +0100, Denis Laxalde wrote:
>>> Augie Fackler a écrit :
>>>> # HG changeset patch
>>>> # User Augie Fackler <augie@google.com>
>>>> # Date 1509399967 14400
>>>> #      Mon Oct 30 17:46:07 2017 -0400
>>>> # Node ID a2fc2086ac3116c63ab890180697b2695c50d9f4
>>>> # Parent  3ce0e4b51f789eff195ec900a07c1fa5e8d5c5f2
>>>> revert: no longer mark --interactive as experimental
>>>>
>>>> We seem to have settled down on behavior changes here (nothing
>>>> matching revset `keyword(revert) and keyword(interactive)` since 4.2
>>>> was released), so let's go ahead and advertise this excellent feature.
>>>
>>> +1
>>
>> +1, too. Queued, thanks.
>> _______________________________________________
>> 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
>
via Mercurial-devel - Nov. 2, 2017, 3:34 p.m.
On Thu, Nov 2, 2017 at 7:16 AM, Denis Laxalde <denis@laxalde.org> wrote:

> Augie Fackler a écrit :
>
>>
>> On Nov 2, 2017, at 09:39, Martin von Zweigbergk via Mercurial-devel <
>>> mercurial-devel@mercurial-scm.org> wrote:
>>>
>>> Can we still change the behavior of "hg revert -i -r" to show a to-apply
>>> diff, not a to-revert diff? (There's a bug number I'm too lazy to look up
>>> from mobile.)
>>>
>>
>> I thought we had already done the patch-reversing that we felt was
>> required...
>>
>
> The last discussion ended with a status quo:
>
> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016
> -November/090142.html
>
> Since then, I set "experimental.revertalternateinteractivemode=false" to
> have a behavior that I find meaningful in most situations.


So do I. The problem is that new users won't have that. If we're graduating
--interactive now, then this seems like a good time (at the latest) to
switch the default of that flag.


> The only case
> it does not work well is "hg revert -i -r .^" (which I think was a major
> motivation for the current behavior).


I prefer the forward direction even when reverting to a parent. We could
add --no-forward-patch flag or something, or we could add a "hg grab" that
grabs the file content from a revision and is equivalent to "hg revert"
except that the patch is always forward. But last time I suggested that, no
one seemed interested, so I'm not very optimistic. So probably just switch
the default of revertalternateinteractivemode to false?


> On the other hand, now "hg
> uncommit" in evolve extension has a --interactive flag that could be
> used instead.
>
>
>
>>
>>> On Thu, Nov 2, 2017, 05:47 Yuya Nishihara <yuya@tcha.org> wrote:
>>> On Thu, 2 Nov 2017 10:03:44 +0100, Denis Laxalde wrote:
>>>
>>>> Augie Fackler a écrit :
>>>>
>>>>> # HG changeset patch
>>>>> # User Augie Fackler <augie@google.com>
>>>>> # Date 1509399967 14400
>>>>> #      Mon Oct 30 17:46:07 2017 -0400
>>>>> # Node ID a2fc2086ac3116c63ab890180697b2695c50d9f4
>>>>> # Parent  3ce0e4b51f789eff195ec900a07c1fa5e8d5c5f2
>>>>> revert: no longer mark --interactive as experimental
>>>>>
>>>>> We seem to have settled down on behavior changes here (nothing
>>>>> matching revset `keyword(revert) and keyword(interactive)` since 4.2
>>>>> was released), so let's go ahead and advertise this excellent feature.
>>>>>
>>>>
>>>> +1
>>>>
>>>
>>> +1, too. Queued, thanks.
>>> _______________________________________________
>>> 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
>>>
>>
>>
>
Denis Laxalde - Nov. 3, 2017, 1:57 p.m.
Martin von Zweigbergk a écrit :
> On Thu, Nov 2, 2017 at 7:16 AM, Denis Laxalde <denis@laxalde.org> wrote:
> 
>> Augie Fackler a écrit :
>>
>>>
>>> On Nov 2, 2017, at 09:39, Martin von Zweigbergk via Mercurial-devel <
>>>> mercurial-devel@mercurial-scm.org> wrote:
>>>>
>>>> Can we still change the behavior of "hg revert -i -r" to show a to-apply
>>>> diff, not a to-revert diff? (There's a bug number I'm too lazy to look up
>>>> from mobile.)
>>>>
>>>
>>> I thought we had already done the patch-reversing that we felt was
>>> required...
>>>
>>
>> The last discussion ended with a status quo:
>>
>> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-November/090142.html
>>
>> Since then, I set "experimental.revertalternateinteractivemode=false" to
>> have a behavior that I find meaningful in most situations.
> 
> 
> So do I. The problem is that new users won't have that. If we're graduating
> --interactive now, then this seems like a good time (at the latest) to
> switch the default of that flag.
> 
> 
>> The only case
>> it does not work well is "hg revert -i -r .^" (which I think was a major
>> motivation for the current behavior).
> 
> 
> I prefer the forward direction even when reverting to a parent. We could
> add --no-forward-patch flag or something, or we could add a "hg grab" that
> grabs the file content from a revision and is equivalent to "hg revert"
> except that the patch is always forward. But last time I suggested that, no
> one seemed interested, so I'm not very optimistic. So probably just switch
> the default of revertalternateinteractivemode to false?

I can resurrect the patch above linked above which drops the option and
use the "apply" verb instead of "revert". Just let me know.
via Mercurial-devel - Nov. 3, 2017, 2:35 p.m.
Yes, please! I'd be very much in favor of that. (Sorry, I misunderstood
what the patch was about when I looked last time.)

On Fri, Nov 3, 2017, 06:57 Denis Laxalde <denis@laxalde.org> wrote:

> Martin von Zweigbergk a écrit :
> > On Thu, Nov 2, 2017 at 7:16 AM, Denis Laxalde <denis@laxalde.org> wrote:
> >
> >> Augie Fackler a écrit :
> >>
> >>>
> >>> On Nov 2, 2017, at 09:39, Martin von Zweigbergk via Mercurial-devel <
> >>>> mercurial-devel@mercurial-scm.org> wrote:
> >>>>
> >>>> Can we still change the behavior of "hg revert -i -r" to show a
> to-apply
> >>>> diff, not a to-revert diff? (There's a bug number I'm too lazy to
> look up
> >>>> from mobile.)
> >>>>
> >>>
> >>> I thought we had already done the patch-reversing that we felt was
> >>> required...
> >>>
> >>
> >> The last discussion ended with a status quo:
> >>
> >>
> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-November/090142.html
> >>
> >> Since then, I set "experimental.revertalternateinteractivemode=false" to
> >> have a behavior that I find meaningful in most situations.
> >
> >
> > So do I. The problem is that new users won't have that. If we're
> graduating
> > --interactive now, then this seems like a good time (at the latest) to
> > switch the default of that flag.
> >
> >
> >> The only case
> >> it does not work well is "hg revert -i -r .^" (which I think was a major
> >> motivation for the current behavior).
> >
> >
> > I prefer the forward direction even when reverting to a parent. We could
> > add --no-forward-patch flag or something, or we could add a "hg grab"
> that
> > grabs the file content from a revision and is equivalent to "hg revert"
> > except that the patch is always forward. But last time I suggested that,
> no
> > one seemed interested, so I'm not very optimistic. So probably just
> switch
> > the default of revertalternateinteractivemode to false?
>
> I can resurrect the patch above linked above which drops the option and
> use the "apply" verb instead of "revert". Just let me know.
>
Boris Feld - Nov. 3, 2017, 7:29 p.m.
On Fri, 2017-11-03 at 14:57 +0100, Denis Laxalde wrote:
> Martin von Zweigbergk a écrit :
> > On Thu, Nov 2, 2017 at 7:16 AM, Denis Laxalde <denis@laxalde.org>
> > wrote:
> > 
> > > Augie Fackler a écrit :
> > > 
> > > > 
> > > > On Nov 2, 2017, at 09:39, Martin von Zweigbergk via Mercurial-
> > > > devel <
> > > > > mercurial-devel@mercurial-scm.org> wrote:
> > > > > 
> > > > > Can we still change the behavior of "hg revert -i -r" to show
> > > > > a to-apply
> > > > > diff, not a to-revert diff? (There's a bug number I'm too
> > > > > lazy to look up
> > > > > from mobile.)
> > > > > 
> > > > 
> > > > I thought we had already done the patch-reversing that we felt
> > > > was
> > > > required...
> > > > 
> > > 
> > > The last discussion ended with a status quo:
> > > 
> > > https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-Nove
> > > mber/090142.html
> > > 
> > > Since then, I set
> > > "experimental.revertalternateinteractivemode=false" to
> > > have a behavior that I find meaningful in most situations.
> > 
> > 
> > So do I. The problem is that new users won't have that. If we're
> > graduating
> > --interactive now, then this seems like a good time (at the latest)
> > to
> > switch the default of that flag.
> > 
> > 
> > > The only case
> > > it does not work well is "hg revert -i -r .^" (which I think was
> > > a major
> > > motivation for the current behavior).

We think that `hg revert -i and `hg uncommit -i` are both useful and
couldn't be replace by the other. They are different in 3 points:
    
    - hg uncommit works on the current directory parent while hg revert
-i -r .^ could works on other changesets.
    - hg uncommit modify the current directory parent and keep the
changes in the current working directory while hg revert is only
modifying the current working directory
    - hg uncommit use-case is to remove something from a commit (like
some parts that really should be in their own changeset) while hg
revert use-case is try to revert a changeset in your history and see
the impact (like are the tests passing now that I reverted these two
lines).

> > 
> > 
> > I prefer the forward direction even when reverting to a parent. We
> > could
> > add --no-forward-patch flag or something, or we could add a "hg
> > grab" that
> > grabs the file content from a revision and is equivalent to "hg
> > revert"
> > except that the patch is always forward. But last time I suggested
> > that, no
> > one seemed interested, so I'm not very optimistic. So probably just
> > switch
> > the default of revertalternateinteractivemode to false?
> 
> I can resurrect the patch above linked above which drops the option
> and
> use the "apply" verb instead of "revert". Just let me know.


We think that keeping both `hg revert -i` and `hg uncommit -i` in their
current state is the way to go, as they works on a different target,
they solves different use-cases and one is the not the subset of the
another.

Another dedicated command seems semantically better for this specific
use-case, maybe grab is not the best name. It is used by the evolve
extension for another purpose.

If not a new command, a new command-line flag or something in the
interface would be good enough to be discoverable by users.

If there are a command line flag. It make sense to have the associated
boolean config in the [commands] section. Maybe revert.interactive-
patch-direction.
via Mercurial-devel - Nov. 3, 2017, 7:36 p.m.
On Fri, Nov 3, 2017 at 12:29 PM, Boris Feld <boris.feld@octobus.net> wrote:

> On Fri, 2017-11-03 at 14:57 +0100, Denis Laxalde wrote:
> > Martin von Zweigbergk a écrit :
> > > On Thu, Nov 2, 2017 at 7:16 AM, Denis Laxalde <denis@laxalde.org>
> > > wrote:
> > >
> > > > Augie Fackler a écrit :
> > > >
> > > > >
> > > > > On Nov 2, 2017, at 09:39, Martin von Zweigbergk via Mercurial-
> > > > > devel <
> > > > > > mercurial-devel@mercurial-scm.org> wrote:
> > > > > >
> > > > > > Can we still change the behavior of "hg revert -i -r" to show
> > > > > > a to-apply
> > > > > > diff, not a to-revert diff? (There's a bug number I'm too
> > > > > > lazy to look up
> > > > > > from mobile.)
> > > > > >
> > > > >
> > > > > I thought we had already done the patch-reversing that we felt
> > > > > was
> > > > > required...
> > > > >
> > > >
> > > > The last discussion ended with a status quo:
> > > >
> > > > https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-Nove
> > > > mber/090142.html
> > > >
> > > > Since then, I set
> > > > "experimental.revertalternateinteractivemode=false" to
> > > > have a behavior that I find meaningful in most situations.
> > >
> > >
> > > So do I. The problem is that new users won't have that. If we're
> > > graduating
> > > --interactive now, then this seems like a good time (at the latest)
> > > to
> > > switch the default of that flag.
> > >
> > >
> > > > The only case
> > > > it does not work well is "hg revert -i -r .^" (which I think was
> > > > a major
> > > > motivation for the current behavior).
>
> We think that `hg revert -i and `hg uncommit -i` are both useful and
> couldn't be replace by the other. They are different in 3 points:
>
>     - hg uncommit works on the current directory parent while hg revert
> -i -r .^ could works on other changesets.
>     - hg uncommit modify the current directory parent and keep the
> changes in the current working directory while hg revert is only
> modifying the current working directory
>     - hg uncommit use-case is to remove something from a commit (like
> some parts that really should be in their own changeset) while hg
> revert use-case is try to revert a changeset in your history and see
> the impact (like are the tests passing now that I reverted these two
> lines).
>
> > >
> > >
> > > I prefer the forward direction even when reverting to a parent. We
> > > could
> > > add --no-forward-patch flag or something, or we could add a "hg
> > > grab" that
> > > grabs the file content from a revision and is equivalent to "hg
> > > revert"
> > > except that the patch is always forward. But last time I suggested
> > > that, no
> > > one seemed interested, so I'm not very optimistic. So probably just
> > > switch
> > > the default of revertalternateinteractivemode to false?
> >
> > I can resurrect the patch above linked above which drops the option
> > and
> > use the "apply" verb instead of "revert". Just let me know.
>
>
> We think that keeping both `hg revert -i` and `hg uncommit -i` in their
> current state is the way to go, as they works on a different target,
> they solves different use-cases and one is the not the subset of the
> another.
>
> Another dedicated command seems semantically better for this specific
> use-case, maybe grab is not the best name. It is used by the evolve
> extension for another purpose.
>
> If not a new command, a new command-line flag or something in the
> interface would be good enough to be discoverable by users.
>

As I said, I suggested "hg grab" a long time ago and no one seemed
interested. So I'd say consensus (i.e. that most people, not all, agree) is
the patch from Denis that I queued earlier today.


>
> If there are a command line flag. It make sense to have the associated
> boolean config in the [commands] section. Maybe revert.interactive-
> patch-direction.
>

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -4517,8 +4517,7 @@  def resolve(ui, repo, *pats, **opts):
     ('d', 'date', '', _('tipmost revision matching date'), _('DATE')),
     ('r', 'rev', '', _('revert to the specified revision'), _('REV')),
     ('C', 'no-backup', None, _('do not save backup copies of files')),
-    ('i', 'interactive', None,
-            _('interactively select the changes (EXPERIMENTAL)')),
+    ('i', 'interactive', None, _('interactively select the changes')),
     ] + walkopts + dryrunopts,
     _('[OPTION]... [-r REV] [NAME]...'))
 def revert(ui, repo, *pats, **opts):