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

[Glen Choo <chooglen@google.com>]

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

> 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.

> 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.

> 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.

> +	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.