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
