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

[Junio C Hamano <gitster@pobox.com>]

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

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

> @@ -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?

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