From: liam Beguin <liambeguin@gmail.com>
Date: Thu, 27 Apr 2017 01:20:51 GMT
Subject: Re: [PATCH v2] rebase -i: add config to abbreviate command-names
Message-ID: <1493256051.29673.35.camel@gmail.com>
In-Reply-To: <CACBZZX49HZbQ32TTM8V3hSHwEJxkewV3VwfJO75u+D-9RmsOtw@mail.gmail.com>

Hi Ævar,

On Wed, 2017-04-26 at 17:24 +0200, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Apr 25, 2017 at 6:43 AM, Liam Beguin <liambeguin@gmail.com> wrote:
> > Add the 'rebase.abbrevCmd' boolean config option to allow `git rebase -i`
> > to abbreviate the command-names in the instruction list.
> > 
> > This means that `git rebase -i` would print:
> >     p deadbee The oneline of this commit
> >     ...
> > 
> > instead of:
> >     pick deadbee The oneline of this commit
> >     ...
> > 
> > Using a single character command-name allows the lines to remain
> > aligned, making the whole set more readable.
> 
> Aside from the existing comments about the commit message from others,
> you should be noting that we *already* have these abbreviations for
> all the todo list options, and we note this in append_todo_help.
> 
> 
> > +rebase.abbrevCmd::
> > +       If set to true, `git rebase -i` will abbreviate the command-names in the
> > +       instruction list. This means that instead of looking like this,
> > +
> > [...]
> > +rebase.abbrevCmd::
> > +       If set to true, `git rebase -i` will abbreviate the command-names in the
> > +       instruction list. This means that instead of looking like this,
> > [...]
> 
> Better to split this out into a new *.txt file and use the include::*
> facility (grep for it) rather than copy/pasting this entirely across
> two files.
> 

Thanks for pointing this out, I'll update the documentation

> >  OPTIONS
> >  -------
> >  --onto <newbase>::
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index 2c9c0165b5ab..9f3e82b79615 100644
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -1210,6 +1210,10 @@ else
> >         revisions=$onto...$orig_head
> >         shortrevisions=$shorthead
> >  fi
> > +
> > +rebasecmd=pick
> > +test "$(git config --bool --get rebase.abbrevCmd)" = true && rebasecmd=p
> 
> Rather than hardcoding "p" here maybe it would be worthhwile to make
> that into a variable used both here and in append_todo_help, maybe
> not...
> 

I'm not sure I understand, do you mean that the option should also affect the
message added by append_todo_help ?

> >  format=$(git config --get rebase.instructionFormat)
> >  # the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse
> >  git rev-list $merges_option --format="%m%H ${format:-%s}" \
> > @@ -1228,7 +1232,7 @@ do
> > 
> >         if test t != "$preserve_merges"
> >         then
> > -               printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo"
> > +               printf '%s\n' "${comment_out}${rebasecmd} $sha1 $rest" >>"$todo"
> >         else
> >                 if test -z "$rebase_root"
> >                 then
> > @@ -1246,7 +1250,7 @@ do
> >                 if test f = "$preserve"
> >                 then
> >                         touch "$rewritten"/$sha1
> > -                       printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo"
> > +                       printf '%s\n' "${comment_out}${rebasecmd} $sha1 $rest" >>"$todo"
> >                 fi
> >         fi
> >  done
> 
> I haven't tried applying & running this patch, but it seems you
> definitely missed the case where --autosquash will add fixup or
> squash, that should be f or s with your patch, but you didn't change
> that code. See the rearrange_squash function.
> 
> Ditto for turning "exec" into "e" with --exec.
> 

I noticed this yesterday, I'll add both cases the next iteration.

> But if the motivation for this entire thing is to make sure the
> commands are aligned this doesn't fix that, because the sha1s can be
> of different lengths. So as others have pointed out maybe this entire
> thing should be dropped & replaced with some bool command to align the
> todo list, maybe turning that on by default.
> 
> Unless the real unstated reason is to make this easier to edit in vim
> or something, in which case this approach seems reasonable.

Keeping things aligned was the first motivation but the fact that it also
makes changing the action faster is also nice to have. I didn't think it
would help justify the patch.
The SHA1s not having the same length can easily be 'fixed' by setting a
higher value for 'core.abbrev'. 

Thanks, 
Liam