Re: [PATCH v2] rebase -i: add config to abbreviate command-names

[liam Beguin <liambeguin@gmail.com>]

Hi Johannes, 

On Tue, 2017-04-25 at 22:08 +0200, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Tue, 25 Apr 2017, Liam Beguin 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.
> > 
> > Signed-off-by: Liam Beguin <liambeguin@gmail.com>
> 
> Apart from either abbreviating commands after --edit-todo, or documenting
> explicitly that the new config option only concerns the initial todo list,
> there is another problem that just occurred to me: --exec.
> 
> When you call `git rebase -x "make DEVELOPER=1 -j15"`, the idea is to
> append an "exec make DEVELOPER=1 -j15" line after every pick line. The
> code in question looks like this:
> 
> add_exec_commands () {
>         {
>                 first=t
>                 while read -r insn rest
>                 do
>                         case $insn in
>                         pick)
>                                 test -n "$first" ||
>                                 printf "%s" "$cmd"
>                                 ;;
>                         esac
>                         printf "%s %s\n" "$insn" "$rest"
>                         first=
>                 done
>                 printf "%s" "$cmd"
>         } <"$1" >"$1.new" &&
>         mv "$1.new" "$1"
> }
> 
> Obviously, the git-rebase--interactive script expects at this point that
> the command is spelled out, so your patch needs to change the `pick)` case
> to `p|pick)`, I think.
> 
> In addition, since the rationale for the new option is to align the lines
> better, the `exec` would need to be replaced by `x`, and as multiple `-x`
> options are allowed, you would need something like this at the beginning
> of `add_exec_commands`, too:
> 
> 	# abbreviate `exec` if rebase.abbrevCmd is true
> 	test p != "$rebasecmd" ||
> 	cmd="$(echo "$cmd" | sed 's/^exec/x/')"
> 



> Also:
> 
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index 475e874d5155..8b1877f2df91 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -2614,6 +2614,25 @@ rebase.instructionFormat::
> >  	the instruction list during an interactive rebase.  The format will automatically
> >  	have the long commit hash prepended to the format.
> >  
> > +rebase.abbrevCmd::
> 
> It does not fail to amuse that the term "abbrevCmd" is abbreviated
> heavily itself. However, I would strongly suggest to avoid that. It would
> be much more pleasant to call the config option rebase.abbreviateCommands

I tried to use something similar to the rest of the options but I guess that
would be best.

> 
> > +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,
> 
> This is by no means your fault, but it is really horrible by how many
> different names Git's documentation refers to the todo script, nothing
> short of confusing. It is the todo script (which I called it initially,
> maybe not a good name, but it has the merit of the longest tradition at
> least), the todo list, the instruction sheet, the rebase script, the
> instruction list... etc
> 
> However, the thing is called "todo list" elsewhere in the same file,
> therefore lets try to avoid even more confusion and use that term instead
> of "instruction list" here.

thanks for pointing this out, I was not quite sure what to call this list.

> 
> > 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
> 
> A better name would be "pickcmd", as there are more rebase commands than
> just `pick` and what we want here is really only associated with one of
> those commands.

Wouldn't that make it confusing when the patch starts to handle other commands?
A common name across the script would limit further confusion.
I noticed that it is already called `action` in `rearrange_squash`.
would that do? (even though it has no reference to 'command')

> 
> Ciao,
> Johannes

Thanks for the detailed answer,
Liam