Re: [PATCH] sequencer: avoid adding exec commands for non-commit creating commands

[Taylor Blau <me@ttaylorr.com>]

On Tue, Nov 30, 2021 at 03:58:39AM +0000, Elijah Newren via GitGitGadget wrote:
> diff --git a/sequencer.c b/sequencer.c
> index ea96837cde3..aa790f0bba8 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5496,7 +5496,7 @@ static void todo_list_add_exec_commands(struct todo_list *todo_list,
>  	}
>
>  	/* insert or append final <commands> */
> -	if (insert || nr == todo_list->nr) {
> +	if (insert) {

Looks good. My worry after first reading this is that we wouldn't insert
an `--autosquash` rebase that ends in a fixup, e.g.:

    git commit -m foo
    git commit --fixup HEAD
    git rebase -i --autosquash -x true HEAD~2

But we're OK there, since we set insert to 1 when we see the first pick,
and leave it because we never saw another fixup. Then we'll still have
fixup as 1 when we exit the loop, and we correctly insert an exec line
at the end of the fixup chain.

So I think that having "|| nr == todo_list->nr" part of the conditional
was broken to begin with.

As far as I can tell, this behavior of always sticking an 'exec' line at
the end of the todo list has existed since the inception of the `-x`
option back in c214538416 (rebase -i: teach "--exec <cmd>", 2012-06-12).
See the unconditional `printf "%s" "$cmd"` at the end of the sub-shell
within `add_exec_commands()` from that commit.

But this is broken according to the docs, and I think that your fix and
test coverage are sensible. Thanks!

Thanks,
Taylor