Patchwork phases: return zero for no-op operations (issue4751)

login
register
mail settings
Submitter Jordi Gutiérrez Hermoso
Date Sept. 14, 2015, 11:26 p.m.
Message ID <a247eb167de1ee9f447b.1442273186@Iris>
Download mbox | patch
Permalink /patch/10506/
State Accepted
Delegated to: Augie Fackler
Headers show

Comments

Jordi Gutiérrez Hermoso - Sept. 14, 2015, 11:26 p.m.
# HG changeset patch
# User Jordi Gutiérrez Hermoso <jordigh@octave.org>
# Date 1442273134 14400
#      Mon Sep 14 19:25:34 2015 -0400
# Node ID a247eb167de1ee9f447b7e0bbab0fc98babe3d7a
# Parent  6c962145f523e6e0ed1c94eb6764bf198a92917b
phases: return zero for no-op operations (issue4751)

It is rather unhelpful to return 1 if there were no changes because
the request matches the current state of phases. So we just undo that.
Julien Cristau - Sept. 15, 2015, 12:05 p.m.
On Mon, Sep 14, 2015 at 19:26:26 -0400, Jordi Gutiérrez Hermoso wrote:

> # HG changeset patch
> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> # Date 1442273134 14400
> #      Mon Sep 14 19:25:34 2015 -0400
> # Node ID a247eb167de1ee9f447b7e0bbab0fc98babe3d7a
> # Parent  6c962145f523e6e0ed1c94eb6764bf198a92917b
> phases: return zero for no-op operations (issue4751)
> 
> It is rather unhelpful to return 1 if there were no changes because
> the request matches the current state of phases. So we just undo that.
> 
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -5032,8 +5032,7 @@ def phase(ui, repo, *revs, **opts):
>  
>          public < draft < secret
>  
> -    Returns 0 on success, 1 if no phases were changed or some could not
> -    be changed.
> +    Returns 0 on success, 1 if some phases could not be changed.
>  
>      (For more information about the phases concept, see :hg:`help phases`.)
>      """
> @@ -5102,7 +5101,6 @@ def phase(ui, repo, *revs, **opts):
>                  ui.note(msg)
>          else:
>              ui.warn(_('no phases changed\n'))
> -            ret = 1

This should probably be a note instead of warn?

>      return ret
>  
>  def postincoming(ui, repo, modheads, optupdate, checkout):
> diff --git a/tests/test-commandserver.t b/tests/test-commandserver.t
> --- a/tests/test-commandserver.t
> +++ b/tests/test-commandserver.t
> @@ -322,7 +322,6 @@ check that local configs for the cached 
>    ...     runcommand(server, ['phase', '-r', '.'])
>    *** runcommand phase -r . -p
>    no phases changed
> -   [1]
>    *** runcommand commit -Am.
>    *** runcommand rollback
>    repository tip rolled back to revision 3 (undo commit)

Cheers,
Julien
Jordi Gutiérrez Hermoso - Sept. 15, 2015, 5:52 p.m.
On Tue, 2015-09-15 at 14:05 +0200, Julien Cristau wrote:
> On Mon, Sep 14, 2015 at 19:26:26 -0400, Jordi Gutiérrez Hermoso wrote:
> 
> > # HG changeset patch
> > # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> > # Date 1442273134 14400
> > #      Mon Sep 14 19:25:34 2015 -0400
> > # Node ID a247eb167de1ee9f447b7e0bbab0fc98babe3d7a
> > # Parent  6c962145f523e6e0ed1c94eb6764bf198a92917b
> > phases: return zero for no-op operations (issue4751)
> > 
> > It is rather unhelpful to return 1 if there were no changes because
> > the request matches the current state of phases. So we just undo that.
> > 
> > diff --git a/mercurial/commands.py b/mercurial/commands.py
> > --- a/mercurial/commands.py
> > +++ b/mercurial/commands.py
> > @@ -5032,8 +5032,7 @@ def phase(ui, repo, *revs, **opts):
> >  
> >          public < draft < secret
> >  
> > -    Returns 0 on success, 1 if no phases were changed or some could not
> > -    be changed.
> > +    Returns 0 on success, 1 if some phases could not be changed.
> >  
> >      (For more information about the phases concept, see :hg:`help phases`.)
> >      """
> > @@ -5102,7 +5101,6 @@ def phase(ui, repo, *revs, **opts):
> >                  ui.note(msg)
> >          else:
> >              ui.warn(_('no phases changed\n'))
> > -            ret = 1
> 
> This should probably be a note instead of warn?

Hm, it should be a warning if there was a problem and a note
otherwise, right? If you feel strongly about it, I'll submit a V2.
Augie Fackler - Sept. 15, 2015, 6:32 p.m.
On Tue, Sep 15, 2015 at 1:52 PM, Jordi Gutiérrez Hermoso
<jordigh@octave.org> wrote:
> On Tue, 2015-09-15 at 14:05 +0200, Julien Cristau wrote:
>> On Mon, Sep 14, 2015 at 19:26:26 -0400, Jordi Gutiérrez Hermoso wrote:
>>
>> > # HG changeset patch
>> > # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
>> > # Date 1442273134 14400
>> > #      Mon Sep 14 19:25:34 2015 -0400
>> > # Node ID a247eb167de1ee9f447b7e0bbab0fc98babe3d7a
>> > # Parent  6c962145f523e6e0ed1c94eb6764bf198a92917b
>> > phases: return zero for no-op operations (issue4751)
>> >
>> > It is rather unhelpful to return 1 if there were no changes because
>> > the request matches the current state of phases. So we just undo that.
>> >
>> > diff --git a/mercurial/commands.py b/mercurial/commands.py
>> > --- a/mercurial/commands.py
>> > +++ b/mercurial/commands.py
>> > @@ -5032,8 +5032,7 @@ def phase(ui, repo, *revs, **opts):
>> >
>> >          public < draft < secret
>> >
>> > -    Returns 0 on success, 1 if no phases were changed or some could not
>> > -    be changed.
>> > +    Returns 0 on success, 1 if some phases could not be changed.
>> >
>> >      (For more information about the phases concept, see :hg:`help phases`.)
>> >      """
>> > @@ -5102,7 +5101,6 @@ def phase(ui, repo, *revs, **opts):
>> >                  ui.note(msg)
>> >          else:
>> >              ui.warn(_('no phases changed\n'))
>> > -            ret = 1
>>
>> This should probably be a note instead of warn?
>
> Hm, it should be a warning if there was a problem and a note
> otherwise, right? If you feel strongly about it, I'll submit a V2.


I think "normal" UNIXy programs report silent success if you request a noop?

(I don't feel strongly, but the requested end state does exist. OTOH,
'rm' is a counterexample, so I'm torn.)
Brandon McCaig - Sept. 15, 2015, 7:01 p.m.
On Tue, Sep 15, 2015 at 2:32 PM, Augie Fackler <raf@durin42.com> wrote:
> I think "normal" UNIXy programs report silent success if you request a noop?
>
> (I don't feel strongly, but the requested end state does exist. OTOH,
> 'rm' is a counterexample, so I'm torn.)

My 2 cents would be that STDERR seems appropriate. The user requested
something to be done which was not needed. The program should rightly
"succeed" because the end result is the same as what the user
requested (and harmless in every way), but it's helpful to alert the
user to their mistake to potentially signal the wrong changeset(s)
being modified.

Regards,
Julien Cristau - Sept. 18, 2015, 7:49 a.m.
On Tue, Sep 15, 2015 at 13:52:42 -0400, Jordi Gutiérrez Hermoso wrote:

> On Tue, 2015-09-15 at 14:05 +0200, Julien Cristau wrote:
> > On Mon, Sep 14, 2015 at 19:26:26 -0400, Jordi Gutiérrez Hermoso wrote:
> > 
> > > # HG changeset patch
> > > # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> > > # Date 1442273134 14400
> > > #      Mon Sep 14 19:25:34 2015 -0400
> > > # Node ID a247eb167de1ee9f447b7e0bbab0fc98babe3d7a
> > > # Parent  6c962145f523e6e0ed1c94eb6764bf198a92917b
> > > phases: return zero for no-op operations (issue4751)
> > > 
> > > It is rather unhelpful to return 1 if there were no changes because
> > > the request matches the current state of phases. So we just undo that.
> > > 
> > > diff --git a/mercurial/commands.py b/mercurial/commands.py
> > > --- a/mercurial/commands.py
> > > +++ b/mercurial/commands.py
> > > @@ -5032,8 +5032,7 @@ def phase(ui, repo, *revs, **opts):
> > >  
> > >          public < draft < secret
> > >  
> > > -    Returns 0 on success, 1 if no phases were changed or some could not
> > > -    be changed.
> > > +    Returns 0 on success, 1 if some phases could not be changed.
> > >  
> > >      (For more information about the phases concept, see :hg:`help phases`.)
> > >      """
> > > @@ -5102,7 +5101,6 @@ def phase(ui, repo, *revs, **opts):
> > >                  ui.note(msg)
> > >          else:
> > >              ui.warn(_('no phases changed\n'))
> > > -            ret = 1
> > 
> > This should probably be a note instead of warn?
> 
> Hm, it should be a warning if there was a problem and a note
> otherwise, right? If you feel strongly about it, I'll submit a V2.
> 
I don't care all that much, I can live with the warning as long as the
exit code gets fixed.  Thanks.

Cheers,
Julien
Jordi Gutiérrez Hermoso - Sept. 25, 2015, 2:20 p.m.
On Fri, 2015-09-18 at 09:49 +0200, Julien Cristau wrote:
> On Tue, Sep 15, 2015 at 13:52:42 -0400, Jordi Gutiérrez Hermoso wrote:
> 
> > On Tue, 2015-09-15 at 14:05 +0200, Julien Cristau wrote:
> > > On Mon, Sep 14, 2015 at 19:26:26 -0400, Jordi Gutiérrez Hermoso wrote:
> > > 
> > > > # HG changeset patch
> > > > # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> > > > # Date 1442273134 14400
> > > > #      Mon Sep 14 19:25:34 2015 -0400
> > > > # Node ID a247eb167de1ee9f447b7e0bbab0fc98babe3d7a
> > > > # Parent  6c962145f523e6e0ed1c94eb6764bf198a92917b
> > > > phases: return zero for no-op operations (issue4751)
> > > > 
> > > > It is rather unhelpful to return 1 if there were no changes because
> > > > the request matches the current state of phases. So we just undo that.
> > > > 
> > > > diff --git a/mercurial/commands.py b/mercurial/commands.py
> > > > --- a/mercurial/commands.py
> > > > +++ b/mercurial/commands.py
> > > > @@ -5032,8 +5032,7 @@ def phase(ui, repo, *revs, **opts):
> > > >  
> > > >          public < draft < secret
> > > >  
> > > > -    Returns 0 on success, 1 if no phases were changed or some could not
> > > > -    be changed.
> > > > +    Returns 0 on success, 1 if some phases could not be changed.
> > > >  
> > > >      (For more information about the phases concept, see :hg:`help phases`.)
> > > >      """
> > > > @@ -5102,7 +5101,6 @@ def phase(ui, repo, *revs, **opts):
> > > >                  ui.note(msg)
> > > >          else:
> > > >              ui.warn(_('no phases changed\n'))
> > > > -            ret = 1
> > > 
> > > This should probably be a note instead of warn?
> > 
> > Hm, it should be a warning if there was a problem and a note
> > otherwise, right? If you feel strongly about it, I'll submit a V2.
> > 
> I don't care all that much, I can live with the warning as long as the
> exit code gets fixed.  Thanks.

If you don't care, I don't either. Can we queue this patch, then?
Augie Fackler - Sept. 25, 2015, 5:21 p.m.
On Fri, Sep 25, 2015 at 10:20:51AM -0400, Jordi Gutiérrez Hermoso wrote:
> On Fri, 2015-09-18 at 09:49 +0200, Julien Cristau wrote:
> > On Tue, Sep 15, 2015 at 13:52:42 -0400, Jordi Gutiérrez Hermoso wrote:
> >
> > > On Tue, 2015-09-15 at 14:05 +0200, Julien Cristau wrote:
> > > > On Mon, Sep 14, 2015 at 19:26:26 -0400, Jordi Gutiérrez Hermoso wrote:
> > > >
> > > > > # HG changeset patch
> > > > > # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> > > > > # Date 1442273134 14400
> > > > > #      Mon Sep 14 19:25:34 2015 -0400
> > > > > # Node ID a247eb167de1ee9f447b7e0bbab0fc98babe3d7a
> > > > > # Parent  6c962145f523e6e0ed1c94eb6764bf198a92917b
> > > > > phases: return zero for no-op operations (issue4751)
> > > > >
> > > > > It is rather unhelpful to return 1 if there were no changes because
> > > > > the request matches the current state of phases. So we just undo that.
> > > > >
> > > > > diff --git a/mercurial/commands.py b/mercurial/commands.py
> > > > > --- a/mercurial/commands.py
> > > > > +++ b/mercurial/commands.py
> > > > > @@ -5032,8 +5032,7 @@ def phase(ui, repo, *revs, **opts):
> > > > >
> > > > >          public < draft < secret
> > > > >
> > > > > -    Returns 0 on success, 1 if no phases were changed or some could not
> > > > > -    be changed.
> > > > > +    Returns 0 on success, 1 if some phases could not be changed.
> > > > >
> > > > >      (For more information about the phases concept, see :hg:`help phases`.)
> > > > >      """
> > > > > @@ -5102,7 +5101,6 @@ def phase(ui, repo, *revs, **opts):
> > > > >                  ui.note(msg)
> > > > >          else:
> > > > >              ui.warn(_('no phases changed\n'))
> > > > > -            ret = 1
> > > >
> > > > This should probably be a note instead of warn?
> > >
> > > Hm, it should be a warning if there was a problem and a note
> > > otherwise, right? If you feel strongly about it, I'll submit a V2.
> > >
> > I don't care all that much, I can live with the warning as long as the
> > exit code gets fixed.  Thanks.
>
> If you don't care, I don't either. Can we queue this patch, then?

I'm fine with this patch as stated. Pierre-yves, do you object, or
shall I take it?

>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Sept. 25, 2015, 9:58 p.m.
On 09/25/2015 10:21 AM, Augie Fackler wrote:
> On Fri, Sep 25, 2015 at 10:20:51AM -0400, Jordi Gutiérrez Hermoso wrote:
>> On Fri, 2015-09-18 at 09:49 +0200, Julien Cristau wrote:
>>> On Tue, Sep 15, 2015 at 13:52:42 -0400, Jordi Gutiérrez Hermoso wrote:
>>>
>>>> On Tue, 2015-09-15 at 14:05 +0200, Julien Cristau wrote:
>>>>> On Mon, Sep 14, 2015 at 19:26:26 -0400, Jordi Gutiérrez Hermoso wrote:
>>>>>
>>>>>> # HG changeset patch
>>>>>> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
>>>>>> # Date 1442273134 14400
>>>>>> #      Mon Sep 14 19:25:34 2015 -0400
>>>>>> # Node ID a247eb167de1ee9f447b7e0bbab0fc98babe3d7a
>>>>>> # Parent  6c962145f523e6e0ed1c94eb6764bf198a92917b
>>>>>> phases: return zero for no-op operations (issue4751)
>>>>>>
>>>>>> It is rather unhelpful to return 1 if there were no changes because
>>>>>> the request matches the current state of phases. So we just undo that.
>>>>>>
>>>>>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>>>>>> --- a/mercurial/commands.py
>>>>>> +++ b/mercurial/commands.py
>>>>>> @@ -5032,8 +5032,7 @@ def phase(ui, repo, *revs, **opts):
>>>>>>
>>>>>>           public < draft < secret
>>>>>>
>>>>>> -    Returns 0 on success, 1 if no phases were changed or some could not
>>>>>> -    be changed.
>>>>>> +    Returns 0 on success, 1 if some phases could not be changed.
>>>>>>
>>>>>>       (For more information about the phases concept, see :hg:`help phases`.)
>>>>>>       """
>>>>>> @@ -5102,7 +5101,6 @@ def phase(ui, repo, *revs, **opts):
>>>>>>                   ui.note(msg)
>>>>>>           else:
>>>>>>               ui.warn(_('no phases changed\n'))
>>>>>> -            ret = 1
>>>>>
>>>>> This should probably be a note instead of warn?
>>>>
>>>> Hm, it should be a warning if there was a problem and a note
>>>> otherwise, right? If you feel strongly about it, I'll submit a V2.
>>>>
>>> I don't care all that much, I can live with the warning as long as the
>>> exit code gets fixed.  Thanks.
>>
>> If you don't care, I don't either. Can we queue this patch, then?
>
> I'm fine with this patch as stated. Pierre-yves, do you object, or
> shall I take it?

I do not have a strong opinion. I'm not usre it make sense to issue the 
warning at all if the change is a no-op but valid. (eg: moving from 
draft to draft)
Augie Fackler - Sept. 26, 2015, 1 a.m.
> On Sep 25, 2015, at 5:58 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> 
> 
> On 09/25/2015 10:21 AM, Augie Fackler wrote:
>> On Fri, Sep 25, 2015 at 10:20:51AM -0400, Jordi Gutiérrez Hermoso wrote:
>>> On Fri, 2015-09-18 at 09:49 +0200, Julien Cristau wrote:
>>>> On Tue, Sep 15, 2015 at 13:52:42 -0400, Jordi Gutiérrez Hermoso wrote:
>>>> 
>>>>> On Tue, 2015-09-15 at 14:05 +0200, Julien Cristau wrote:
>>>>>> On Mon, Sep 14, 2015 at 19:26:26 -0400, Jordi Gutiérrez Hermoso wrote:
>>>>>> 
>>>>>>> # HG changeset patch
>>>>>>> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
>>>>>>> # Date 1442273134 14400
>>>>>>> #      Mon Sep 14 19:25:34 2015 -0400
>>>>>>> # Node ID a247eb167de1ee9f447b7e0bbab0fc98babe3d7a
>>>>>>> # Parent  6c962145f523e6e0ed1c94eb6764bf198a92917b
>>>>>>> phases: return zero for no-op operations (issue4751)
>>>>>>> 
>>>>>>> It is rather unhelpful to return 1 if there were no changes because
>>>>>>> the request matches the current state of phases. So we just undo that.
>>>>>>> 
>>>>>>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>>>>>>> --- a/mercurial/commands.py
>>>>>>> +++ b/mercurial/commands.py
>>>>>>> @@ -5032,8 +5032,7 @@ def phase(ui, repo, *revs, **opts):
>>>>>>> 
>>>>>>>          public < draft < secret
>>>>>>> 
>>>>>>> -    Returns 0 on success, 1 if no phases were changed or some could not
>>>>>>> -    be changed.
>>>>>>> +    Returns 0 on success, 1 if some phases could not be changed.
>>>>>>> 
>>>>>>>      (For more information about the phases concept, see :hg:`help phases`.)
>>>>>>>      """
>>>>>>> @@ -5102,7 +5101,6 @@ def phase(ui, repo, *revs, **opts):
>>>>>>>                  ui.note(msg)
>>>>>>>          else:
>>>>>>>              ui.warn(_('no phases changed\n'))
>>>>>>> -            ret = 1
>>>>>> 
>>>>>> This should probably be a note instead of warn?
>>>>> 
>>>>> Hm, it should be a warning if there was a problem and a note
>>>>> otherwise, right? If you feel strongly about it, I'll submit a V2.
>>>>> 
>>>> I don't care all that much, I can live with the warning as long as the
>>>> exit code gets fixed.  Thanks.
>>> 
>>> If you don't care, I don't either. Can we queue this patch, then?
>> 
>> I'm fine with this patch as stated. Pierre-yves, do you object, or
>> shall I take it?
> 
> I do not have a strong opinion. I'm not usre it make sense to issue the warning at all if the change is a no-op but valid. (eg: moving from draft to draft)

Queued, with a (BC) added. Thanks!

> 
> --
> Pierre-Yves David

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -5032,8 +5032,7 @@  def phase(ui, repo, *revs, **opts):
 
         public < draft < secret
 
-    Returns 0 on success, 1 if no phases were changed or some could not
-    be changed.
+    Returns 0 on success, 1 if some phases could not be changed.
 
     (For more information about the phases concept, see :hg:`help phases`.)
     """
@@ -5102,7 +5101,6 @@  def phase(ui, repo, *revs, **opts):
                 ui.note(msg)
         else:
             ui.warn(_('no phases changed\n'))
-            ret = 1
     return ret
 
 def postincoming(ui, repo, modheads, optupdate, checkout):
diff --git a/tests/test-commandserver.t b/tests/test-commandserver.t
--- a/tests/test-commandserver.t
+++ b/tests/test-commandserver.t
@@ -322,7 +322,6 @@  check that local configs for the cached 
   ...     runcommand(server, ['phase', '-r', '.'])
   *** runcommand phase -r . -p
   no phases changed
-   [1]
   *** runcommand commit -Am.
   *** runcommand rollback
   repository tip rolled back to revision 3 (undo commit)