Re: [PATCH v2 4/6] rebase --continue: refuse to commit after failed command

From: Glen Choo <chooglen@google.com>
Date: Jun 21, 2023, 20:35 UTC
Message-ID: <kl6lpm5or1tg.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <9356d14b09a468d8ef2884cd7d76e59ec5c16691.1682089075.git.gitgitgadget@gmail.com>

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 6 quoted lines

> If a commit cannot be picked because it would overwrite an untracked
> file then "git rebase --continue" should refuse to commit any staged
> changes as the commit was not picked. Do this by using the existing
> check for a missing author script in run_git_commit() which prevents
> "rebase --continue" from committing staged changes after failed exec
> commands.

For someone unfamiliar with "git rebase" code, I think it is easy enough to gather that "rebase --continue" will refuse to accept staged changes if the author script is missing, so we are reusing that mechanism to achieve our desired effect. It's not obvious whether this might have unintended consequences (Are we reusing something unrelated for an unintended purpose?) or what alternatives exist (Is sequencer.c so complex that there isn't another way to do this?). It would have been helpful for me to see how these considerations factored into your decision.

Show 9 quoted lines

> When fast-forwarding it is not necessary to write the author script as
> we're reusing an existing commit, not creating a new one. If a
> fast-forwarded commit is modified by an "edit" or "reword" command then
> the modification is committed with "git commit --amend" which reuses the
> author of the commit being amended so the author script is not needed.
> baf8ec8d3a (rebase -r: don't write .git/MERGE_MSG when fast-forwarding,
> 2021-08-20) changed run_git_commit() to allow a missing author script
> when rewording a commit. This changes extends that to allow a missing
> author script whenever the commit is being amended.

As I understand it, the author script can now be missing in other circumstances, so we have to adjust the rest of the machinery to handle that case? If so, this seems to suggest that there are some unintended consequences.

Show 9 quoted lines

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index ff0afad63e2..c1fe55dc2c1 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1288,6 +1288,12 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
>  	test_must_fail git rebase --continue &&
>  	test_cmp_rev HEAD F &&
>  	rm file6 &&
> +	test_path_is_missing .git/rebase-merge/author-script &&

Checking that the path is missing seems like testing implementation details. If so, I would prefer to remove this assertion here and elsewhere.

Show 5 quoted lines

> +	echo changed >file1 &&
> +	git add file1 &&
> +	test_must_fail git rebase --continue 2>err &&
> +	grep "error: you have staged changes in your working tree" err &&
> +	git reset --hard HEAD &&

This seems reasonable.