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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Elijah

On 30/11/2021 03:58, 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.

Thanks for fixing this, the patch looks good and the commit message is 
excellent. I did see Ævar's concerns in another thread but I think this 
is the least surprising approach - if we're not actually rebasing 
anything it seems odd to run the exec command.

Best Wishes

Phillip

> Reported-by: Nikita Bobko <nikitabobko@gmail.com>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>      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
>