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@xxxxxxxxx> 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