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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 26, 2017, 15:24 UTC
Message-ID: <CACBZZX49HZbQ32TTM8V3hSHwEJxkewV3VwfJO75u+D-9RmsOtw@mail.gmail.com>
In-Reply-To: <20170425044320.17840-1-liambeguin@gmail.com>

On Tue, Apr 25, 2017 at 6:43 AM, Liam Beguin <liambeguin@gmail.com> wrote:

Show 13 quoted lines

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

Show 9 quoted lines

> +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.

Show 14 quoted lines

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

Show 21 quoted lines

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

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.