Patchwork rebase: clarify --source and --base documentation so it's less ambiguous

login
register
mail settings
Submitter Augie Fackler
Date Oct. 8, 2014, 5:43 p.m.
Message ID <205bd6e5778956a22663.1412790212@arthedain.pit.corp.google.com>
Download mbox | patch
Permalink /patch/6160/
State Accepted
Headers show

Comments

Augie Fackler - Oct. 8, 2014, 5:43 p.m.
# HG changeset patch
# User Augie Fackler <raf@durin42.com>
# Date 1412026314 14400
#      Mon Sep 29 17:31:54 2014 -0400
# Node ID 205bd6e5778956a22663ec1d81542c6bcdd40522
# Parent  a1eb21f5caea4366310e32aa85248791d5bbfa0c
rebase: clarify --source and --base documentation so it's less ambiguous

I've been burned by these descriptions before, and others have as
well. Hopefully this is more universally understandable.
Mads Kiilerich - Oct. 8, 2014, 6:09 p.m.
On 10/08/2014 07:43 PM, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <raf@durin42.com>
> # Date 1412026314 14400
> #      Mon Sep 29 17:31:54 2014 -0400
> # Node ID 205bd6e5778956a22663ec1d81542c6bcdd40522
> # Parent  a1eb21f5caea4366310e32aa85248791d5bbfa0c
> rebase: clarify --source and --base documentation so it's less ambiguous
>
> I've been burned by these descriptions before, and others have as
> well. Hopefully this is more universally understandable.
>
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -50,10 +50,10 @@
>   
>   @command('rebase',
>       [('s', 'source', '',
> -     _('rebase from the specified changeset'), _('REV')),
> +     _('rebase the specified changeset and its descendants'), _('REV')),

+1

>       ('b', 'base', '',
> -     _('rebase the tree around the specified changeset without '
> -       'ancestors of dest'),
> +     _('rebase specified changeset and all ancestors which are not already'
> +       ' ancestors of the destination'),

I agree it is more clear, but it also leaves essential facts out so it 
is more clearly incorrect.

To make it correct, we have to add "and all descendants of these 
ancestors". Without that I'm -1. With that addition I'm +0, as I don't 
think it makes it significantly more understandable despite having more 
details.

Looking at previous phrasings of this description can perhaps help 
making sure we are converging towards an optimum.

My gut feeling was that the brief description had to be less than 80 
characters ... but in this case we have good use for extra characters.

/Mads
Sean Farley - Oct. 8, 2014, 6:28 p.m.
Mads Kiilerich writes:

> On 10/08/2014 07:43 PM, Augie Fackler wrote:
>> # HG changeset patch
>> # User Augie Fackler <raf@durin42.com>
>> # Date 1412026314 14400
>> #      Mon Sep 29 17:31:54 2014 -0400
>> # Node ID 205bd6e5778956a22663ec1d81542c6bcdd40522
>> # Parent  a1eb21f5caea4366310e32aa85248791d5bbfa0c
>> rebase: clarify --source and --base documentation so it's less ambiguous
>>
>> I've been burned by these descriptions before, and others have as
>> well. Hopefully this is more universally understandable.
>>
>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>> --- a/hgext/rebase.py
>> +++ b/hgext/rebase.py
>> @@ -50,10 +50,10 @@
>>   
>>   @command('rebase',
>>       [('s', 'source', '',
>> -     _('rebase from the specified changeset'), _('REV')),
>> +     _('rebase the specified changeset and its descendants'), _('REV')),
>
> +1

+1

>>       ('b', 'base', '',
>> -     _('rebase the tree around the specified changeset without '
>> -       'ancestors of dest'),
>> +     _('rebase specified changeset and all ancestors which are not already'
>> +       ' ancestors of the destination'),
>
> I agree it is more clear, but it also leaves essential facts out so it 
> is more clearly incorrect.

For once, I actually might agree with Mads ;-) Though, I'm not a
strongly opinionated as him on this phrasing. We could emphasize the
'finding' aspect of this:

'find the base of the specified changeset and rebase all descendants'

which is under 80 characters! We could also use 'branchpoint' (and maybe
other keywords from revsets?) to clarify what 'base' means:

'find the base (branchpoint) of the specified changeset and rebase all
descendants'

A little longer, but just a thought.
Mads Kiilerich - Oct. 8, 2014, 6:47 p.m.
On 10/08/2014 08:28 PM, Sean Farley wrote:
> Mads Kiilerich writes:
>
>> On 10/08/2014 07:43 PM, Augie Fackler wrote:
>>> # HG changeset patch
>>> # User Augie Fackler <raf@durin42.com>
>>> # Date 1412026314 14400
>>> #      Mon Sep 29 17:31:54 2014 -0400
>>> # Node ID 205bd6e5778956a22663ec1d81542c6bcdd40522
>>> # Parent  a1eb21f5caea4366310e32aa85248791d5bbfa0c
>>> rebase: clarify --source and --base documentation so it's less ambiguous
>>>
>>> I've been burned by these descriptions before, and others have as
>>> well. Hopefully this is more universally understandable.
>>>
>>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>>> --- a/hgext/rebase.py
>>> +++ b/hgext/rebase.py
>>> @@ -50,10 +50,10 @@
>>>    
>>>    @command('rebase',
>>>        [('s', 'source', '',
>>> -     _('rebase from the specified changeset'), _('REV')),
>>> +     _('rebase the specified changeset and its descendants'), _('REV')),
>> +1
> +1
>
>>>        ('b', 'base', '',
>>> -     _('rebase the tree around the specified changeset without '
>>> -       'ancestors of dest'),
>>> +     _('rebase specified changeset and all ancestors which are not already'
>>> +       ' ancestors of the destination'),
>> I agree it is more clear, but it also leaves essential facts out so it
>> is more clearly incorrect.
> For once, I actually might agree with Mads ;-) Though, I'm not a
> strongly opinionated as him on this phrasing. We could emphasize the
> 'finding' aspect of this:
>
> 'find the base of the specified changeset and rebase all descendants'
>
> which is under 80 characters! We could also use 'branchpoint' (and maybe
> other keywords from revsets?) to clarify what 'base' means:
>
> 'find the base (branchpoint) of the specified changeset and rebase all
> descendants'
>
> A little longer, but just a thought.

My problem with "base" is that it doesn't have a clear definition. 
"Branchpoint" might be better - it gives a hint that it pretty much is 
the common ancestor (or one of multiple common ancestor heads?) ... but 
actually it is one of the children of the ancestor. "Tree around" is 
also very vague ... by design. +1 for anything that is better than that ;-)

/Mads
Augie Fackler - Oct. 8, 2014, 6:57 p.m.
On Wed, Oct 8, 2014 at 2:28 PM, Sean Farley
<sean.michael.farley@gmail.com> wrote:
>
> Mads Kiilerich writes:
>
>> On 10/08/2014 07:43 PM, Augie Fackler wrote:
>>> # HG changeset patch
>>> # User Augie Fackler <raf@durin42.com>
>>> # Date 1412026314 14400
>>> #      Mon Sep 29 17:31:54 2014 -0400
>>> # Node ID 205bd6e5778956a22663ec1d81542c6bcdd40522
>>> # Parent  a1eb21f5caea4366310e32aa85248791d5bbfa0c
>>> rebase: clarify --source and --base documentation so it's less ambiguous
>>>
>>> I've been burned by these descriptions before, and others have as
>>> well. Hopefully this is more universally understandable.
>>>
>>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>>> --- a/hgext/rebase.py
>>> +++ b/hgext/rebase.py
>>> @@ -50,10 +50,10 @@
>>>
>>>   @command('rebase',
>>>       [('s', 'source', '',
>>> -     _('rebase from the specified changeset'), _('REV')),
>>> +     _('rebase the specified changeset and its descendants'), _('REV')),
>>
>> +1
>
> +1
>
>>>       ('b', 'base', '',
>>> -     _('rebase the tree around the specified changeset without '
>>> -       'ancestors of dest'),
>>> +     _('rebase specified changeset and all ancestors which are not already'
>>> +       ' ancestors of the destination'),
>>
>> I agree it is more clear, but it also leaves essential facts out so it
>> is more clearly incorrect.
>
> For once, I actually might agree with Mads ;-) Though, I'm not a
> strongly opinionated as him on this phrasing. We could emphasize the
> 'finding' aspect of this:
>
> 'find the base of the specified changeset and rebase all descendants'
>
> which is under 80 characters! We could also use 'branchpoint' (and maybe
> other keywords from revsets?) to clarify what 'base' means:
>
> 'find the base (branchpoint) of the specified changeset and rebase all
> descendants'
>
> A little longer, but just a thought.


mpm figured something out in irc, and is now working on some examples
in a titanpad.
Matt Mackall - Oct. 8, 2014, 7:15 p.m.
On Wed, 2014-10-08 at 20:09 +0200, Mads Kiilerich wrote:
> On 10/08/2014 07:43 PM, Augie Fackler wrote:
> > # HG changeset patch
> > # User Augie Fackler <raf@durin42.com>
> > # Date 1412026314 14400
> > #      Mon Sep 29 17:31:54 2014 -0400
> > # Node ID 205bd6e5778956a22663ec1d81542c6bcdd40522
> > # Parent  a1eb21f5caea4366310e32aa85248791d5bbfa0c
> > rebase: clarify --source and --base documentation so it's less ambiguous
> >
> > I've been burned by these descriptions before, and others have as
> > well. Hopefully this is more universally understandable.
> >
> > diff --git a/hgext/rebase.py b/hgext/rebase.py
> > --- a/hgext/rebase.py
> > +++ b/hgext/rebase.py
> > @@ -50,10 +50,10 @@
> >   
> >   @command('rebase',
> >       [('s', 'source', '',
> > -     _('rebase from the specified changeset'), _('REV')),
> > +     _('rebase the specified changeset and its descendants'), _('REV')),
> 
> +1

I've manually patched this half.

> >       ('b', 'base', '',
> > -     _('rebase the tree around the specified changeset without '
> > -       'ancestors of dest'),
> > +     _('rebase specified changeset and all ancestors which are not already'
> > +       ' ancestors of the destination'),

> I agree it is more clear, but it also leaves essential facts out so it 
> is more clearly incorrect.

On IRC, we discussed:

'rebase everything from branching point of specified changeset'

..which I also committed separately.

And I've also added a section of examples.
Pierre-Yves David - Oct. 8, 2014, 9:59 p.m.
On 10/08/2014 10:43 AM, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <raf@durin42.com>
> # Date 1412026314 14400
> #      Mon Sep 29 17:31:54 2014 -0400
> # Node ID 205bd6e5778956a22663ec1d81542c6bcdd40522
> # Parent  a1eb21f5caea4366310e32aa85248791d5bbfa0c
> rebase: clarify --source and --base documentation so it's less ambiguous

On the rebase documentation topic. The --rev option is waiting to be 
documented for 3 years now.

>
> I've been burned by these descriptions before, and others have as
> well. Hopefully this is more universally understandable.
>
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -50,10 +50,10 @@
>
>   @command('rebase',
>       [('s', 'source', '',
> -     _('rebase from the specified changeset'), _('REV')),
> +     _('rebase the specified changeset and its descendants'), _('REV')),
>       ('b', 'base', '',
> -     _('rebase the tree around the specified changeset without '
> -       'ancestors of dest'),
> +     _('rebase specified changeset and all ancestors which are not already'
> +       ' ancestors of the destination'),
>        _('REV')),
>       ('r', 'rev', [],
>        _('rebase these revisions'),
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Matt Mackall - Oct. 9, 2014, 12:30 a.m.
On Wed, 2014-10-08 at 14:59 -0700, Pierre-Yves David wrote:
> 
> On 10/08/2014 10:43 AM, Augie Fackler wrote:
> > # HG changeset patch
> > # User Augie Fackler <raf@durin42.com>
> > # Date 1412026314 14400
> > #      Mon Sep 29 17:31:54 2014 -0400
> > # Node ID 205bd6e5778956a22663ec1d81542c6bcdd40522
> > # Parent  a1eb21f5caea4366310e32aa85248791d5bbfa0c
> > rebase: clarify --source and --base documentation so it's less ambiguous
> 
> On the rebase documentation topic. The --rev option is waiting to be 
> documented for 3 years now.

> >       ('r', 'rev', [],
> >        _('rebase these revisions'),

You'll need to be more specific about what you think the problem is,
because it's invisible to me.
Pierre-Yves David - Oct. 9, 2014, 12:47 a.m.
On 10/08/2014 05:30 PM, Matt Mackall wrote:
> On Wed, 2014-10-08 at 14:59 -0700, Pierre-Yves David wrote:
>>
>> On 10/08/2014 10:43 AM, Augie Fackler wrote:
>>> # HG changeset patch
>>> # User Augie Fackler <raf@durin42.com>
>>> # Date 1412026314 14400
>>> #      Mon Sep 29 17:31:54 2014 -0400
>>> # Node ID 205bd6e5778956a22663ec1d81542c6bcdd40522
>>> # Parent  a1eb21f5caea4366310e32aa85248791d5bbfa0c
>>> rebase: clarify --source and --base documentation so it's less ambiguous
>>
>> On the rebase documentation topic. The --rev option is waiting to be
>> documented for 3 years now.
>
>>>        ('r', 'rev', [],
>>>         _('rebase these revisions'),
>
> You'll need to be more specific about what you think the problem is,
> because it's invisible to me.

It lack mention in the long description and example

(also could be 'rebase exactly these revisions'
Martin Geisler - Oct. 9, 2014, 7:47 a.m.
Mads Kiilerich <mads@kiilerich.com> writes:

> My gut feeling was that the brief description had to be less than 80
> characters ... but in this case we have good use for extra characters.

The help texts for command line flags are wrapped as necessary so you
can go wild here and include as much text as you need :-)

Keeping the explanation brief and to the point still make sense, but
From a help-layout-engine point of view, we can handle long strings.

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -50,10 +50,10 @@ 
 
 @command('rebase',
     [('s', 'source', '',
-     _('rebase from the specified changeset'), _('REV')),
+     _('rebase the specified changeset and its descendants'), _('REV')),
     ('b', 'base', '',
-     _('rebase the tree around the specified changeset without '
-       'ancestors of dest'),
+     _('rebase specified changeset and all ancestors which are not already'
+       ' ancestors of the destination'),
      _('REV')),
     ('r', 'rev', [],
      _('rebase these revisions'),