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 <liambeg...@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
