Hi Elijah,
On Tue, 30 Nov 2021, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
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
>
