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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 21, 2023, 21:05 UTC
Message-ID: <xmqqsfcthrpb.fsf@gitster.g>
In-Reply-To: <9356d14b09a468d8ef2884cd7d76e59ec5c16691.1682089075.git.gitgitgadget@gmail.com>

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

Show 5 quoted lines

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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.

It makes perfect sense to refuse blindly committing. But this makes me wonder what the procedure for the user to recover. In the simplest case, the untracked file may be expendable and the user would wish to easily redo the step after removing it? Like

	$ git rebase
	... stops with "merging will overwrite untracked 'foo'" ...
	$ git rebase --continue
	... refuses thanks to the fix in this step ...
	$ rm foo
	... now what?  "git rebase --redo"?  "git rebase --continue"?

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

Depending on the recovery procedure, this may or may not be a wise design decision, even though it may be the quickest way to implement it. If the recovery procedure involves redoing the failed step from scratch (i.e. "rm foo && git reset --hard && git rebase" would try to restart by replaying the failed step anew), then loss of author script file has no downside. If we want to salvage as much as what was done in the initial attempt, maybe not.

> When fast-forwarding it is not necessary to write the author script as

I'd prefer a comma before "it is not" here.

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

OK.

> 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

It is unclear to me what "This changes extends" refers to. Could you rephrase?

> author script whenever the commit is being amended.
>
> If we're not fast-forwarding then we must remove the author script if
> the pick fails.

The changes described in these three paragraphs are about efforts that were made needed only because the approach chosen to stop "continue" from continuing in this situation happens to be to remove the author script. If it were done differently (perhaps by adding another flag file that "continue" pays attention to), none of them may be necessary?

Show 7 quoted lines

> @@ -4141,6 +4140,7 @@ static int do_merge(struct repository *r,
>  	if (ret < 0) {
>  		error(_("could not even attempt to merge '%.*s'"),
>  		      merge_arg_len, arg);
> +		unlink(rebase_path_author_script());
>  		goto leave_merge;
>  	}

I agree that this is the right location to add new code to stop "continue" from moving forward. I do not know if the "unlink the author script" is the right choice of such a new code, though.

Coming back to my first point, perhaps adding an advice() after this error() would help end users who see this error to learn what to do to move forward?

Show 8 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 &&

Before the pre-context is an attempt to run "git rebase -i" to replay a commit that adds file6, stop the sequence by marking a step as "edit" to take control back. Then we create file6 manually in the working tree and make sure that "git rebase --continue" fails in the pre-context we can see here.

The recovery I asked about earlier is done with "rm file6" in this case.

> +	test_path_is_missing .git/rebase-merge/author-script &&

This is testing the implementation. The failed "continue" would have removed the file thanks to the change we saw earlier.

> +	echo changed >file1 &&
> +	git add file1 &&
> +	test_must_fail git rebase --continue 2>err &&

Then we make some edit, and try to "--continue". Why should this fail? Is it because the earlier "rebase --continue" that failed did not replay the original commit due to untracked file and the user needs to redo the step in its entirety before the working tree becomes ready to take any further changes?

> +	grep "error: you have staged changes in your working tree" err &&
> +	git reset --hard HEAD &&

And this "reset --hard" is another thing in the recovery procedure the user needs to take (the other one being the removal of file6 we have seen earlier). After that, "rebase --continue" will replay the step that was interrupted by the untracked file6 that was in the working tree. OK.

>  	git rebase --continue &&
>  	test_cmp_rev HEAD I
>  '

We of course do not want to become overly "helpful" and run "reset --hard" ourselves when we issue the "could not even attempt to merge" message, but when we step back and see what the user wanted to do, this is still not entirely satisfactory, is it?

My understanding of what the user wanted to do (and let's pretend that creation of file6 in the middle was merely for test writer's convenience and in the real scenario of what the user wanted to do, there was the file left untracked from the beginning before the rebase started) is to redo the A---F---I chain, making a bit of change to F when it is recreated, and then replay I on top of the result of tweaked F. But instead of allowing them to edit F by doing some modification to file1, we ended up forcing the user to discard the edit made to file1 with "reset --hard", and "continue" replayed I on top of the replayed F that "edit" did not have a chance to modify.

Of course, the user can now go back to A and replay F and I on top, essentially redoing the "rebase -i" with FAKE_LINES="edit 1 2" and this time, because untracked file6 is gone, it may work better and F may allow to be tweaked. But then it does not look any better than saying "git rebase --abort" before doing the final "continue", which will also allow the user to redo the whole thing from scratch again.

So, I dunno.