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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Elijah,

On Tue, 30 Nov 2021, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> The `--exec <cmd>` is documented as
>
>     Append "exec <cmd>" after each line creating a commit in the final
>     history.
>     ...
>     If --autosquash is used, "exec" lines will not be appended for the
>     intermediate commits, and will only appear at the end of each
>     squash/fixup series.
>
> Unfortunately, it would also add exec commands after non-pick
> operations, such as 'no-op', which could be seen for example with
>     git rebase -i --exec true HEAD
>
> todo_list_add_exec_commands() intent was to insert exec commands after
> each logical pick, while trying to consider a chains of fixup and squash
> commits to be part of the pick before it.  So it would keep an 'insert'
> boolean tracking if it had seen a pick or merge, but not write the exec
> command until it saw the next non-fixup/squash command.  Since that
> would make it miss the final exec command, it had some code that would
> check whether it still needed to insert one at the end, but instead of a
> simple
>
>     if (insert)
>
> it had a
>
>     if (insert || <condition that is always true>)
>
> That's buggy; as per the docs, we should only add exec commands for
> lines that create commits, i.e. only if insert is true.  Fix the
> conditional.
>
> There was one testcase in the testsuite that we tweak for this change;
> it was introduced in 54fd3243da ("rebase -i: reread the todo list if
> `exec` touched it", 2017-04-26), and was merely testing that after an
> exec had fired that the todo list would be re-read.  The test at the
> time would have worked given any revision at all, though it would only
> work with 'HEAD' as a side-effect of this bug.  Since we're fixing this
> bug, choose something other than 'HEAD' for that test.
>
> Finally, add a testcase that verifies when we have no commits to pick,
> that we get no exec lines in the generated todo list.
>
> Reported-by: Nikita Bobko <nikitabobko@gmail.com>
> Signed-off-by: Elijah Newren <newren@gmail.com>

This patch gets my whole-hearted ACK!

Thank you,
Dscho

> ---
>     sequencer: avoid adding exec commands for non-commit creating commands
>
>     Original report over at
>     https://lore.kernel.org/git/YaVzufpKcC0t+q+L@nand.local/T/#m13fbd7b054c06ba1f98ae66e6d1b9fcc51bb875e
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1149%2Fnewren%2Frebase-exec-empty-bug-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1149/newren/rebase-exec-empty-bug-v1
> Pull-Request: https://github.com/git/git/pull/1149
>
>  sequencer.c                 | 2 +-
>  t/t3429-rebase-edit-todo.sh | 7 ++++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> 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) {
>  		ALLOC_GROW(items, nr + commands->nr, alloc);
>  		COPY_ARRAY(items + nr, base_items, commands->nr);
>  		nr += commands->nr;
> diff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh
> index 7024d49ae7b..abd66f36021 100755
> --- a/t/t3429-rebase-edit-todo.sh
> +++ b/t/t3429-rebase-edit-todo.sh
> @@ -13,10 +13,15 @@ test_expect_success 'setup' '
>
>  test_expect_success 'rebase exec modifies rebase-todo' '
>  	todo=.git/rebase-merge/git-rebase-todo &&
> -	git rebase HEAD -x "echo exec touch F >>$todo" &&
> +	git rebase HEAD~1 -x "echo exec touch F >>$todo" &&
>  	test -e F
>  '
>
> +test_expect_success 'rebase exec with an empty list does not exec anything' '
> +	git rebase HEAD -x "true" 2>output &&
> +	! grep "Executing: true" output
> +'
> +
>  test_expect_success 'loose object cache vs re-reading todo list' '
>  	GIT_REBASE_TODO=.git/rebase-merge/git-rebase-todo &&
>  	export GIT_REBASE_TODO &&
>
> base-commit: 35151cf0720460a897cde9b8039af364743240e7
> --
> gitgitgadget
>