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

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 30, 2021, 05:13 UTC
Message-ID: <YaWy3+nXC/6oLb1A@nand.local>
In-Reply-To: <pull.1149.git.git.1638244719381.gitgitgadget@gmail.com>

On Tue, Nov 30, 2021 at 03:58:39AM +0000, Elijah Newren via GitGitGadget wrote:

Show 10 quoted lines

> 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