Re: [PATCH v3 2/7] rebase -i: remove patch file after conflict resolution

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

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

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When a rebase stops for the user to resolve conflicts it writes a patch
> for the conflicting commit to .git/rebase-merge/patch. This file has
> been written since the introduction of "git-rebase-interactive.sh" in
> 1b1dce4bae7 (Teach rebase an interactive mode, 2007-06-25). I assume the
> idea was to enable the user inspect the conflicting commit in the same
> way as they could for the patch based rebase. This file should be
> deleted when the rebase continues as if the rebase stops for a failed
> "exec" command or a "break" command it is confusing to the user if there
> is a stale patch lying around from an unrelated command.

Unlike the previous step, this describes the change in end-user
observable behaviour *and* asserts that it is an intended change.

Very good.

> As the path is
> now used in two different places rebase_path_patch() is added and used
> to obtain the path for the patch.

The get_dir() function gives different paths, between "rebase-merge"
(for "rebase -i") and "sequencer" (for everything else), and that is
the parent directory of "/patch" output make_patch() uses.

error_with_patch() is the only caller of make_patch(), and
error_with_patch() is called by

 error_failed_squash() - called from pick_commits()
 pick_commits() - when TODO_EDIT stops the sequence, or
		  a non fix-up insn failed when is_rebase_i(), or
		  a merge insn failed, or
		  a reschedule happened.

Are we sure that it is the right thing to do to hardcode
"rebase-merge/patch"?  Unless "rebase -i" is the only thing that
calls pick_commits() and reaches error_with_patch() to cause
make_patch() to be called, this changes the behaviour for cases the
tests added by this patch do not cover, doesn't it?

I would feel safer if this change is accompanied by something like

diff --git i/sequencer.c w/sequencer.c
index cc9821ece2..a5ec8538fa 100644
--- i/sequencer.c
+++ w/sequencer.c
@@ -3502,6 +3502,9 @@ static int make_patch(struct repository *r,
 	char hex[GIT_MAX_HEXSZ + 1];
 	int res = 0;
 
+	if (!is_rebase_i(opts))
+		BUG("make-patch should only be triggered during rebase -i");
+
 	oid_to_hex_r(hex, &commit->object.oid);
 	if (write_message(hex, strlen(hex), rebase_path_stopped_sha(), 1) < 0)
 		return -1;

to make sure that future changes to "git cherry-pick A..B" that
makes it easier to edit .git/sequencer/todo and tweak "pick" into
"edit" (aka "git cherry-pick -i") would not happen unless the author
of such a change considerts its ramifications first.

Alternatively, we could still introduce a handy path function, but
call it sequencer_path_patch() that does get_dir() + "/patch", i.e.
return different paths honoring is_rebase_i(), to make sure we will
behave the same way as before.  That might be safer.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  sequencer.c                | 13 +++++++++----
>  t/t3418-rebase-continue.sh | 18 ++++++++++++++++++
>  2 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index de66bda9d5b..70b0a7023b0 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4659,6 +4663,7 @@ static int pick_commits(struct repository *r,
>  	unlink(rebase_path_message());
>  	unlink(rebase_path_stopped_sha());
>  	unlink(rebase_path_amend());
> +	unlink(rebase_path_patch());
>  
>  	while (todo_list->current < todo_list->nr) {
>  		struct todo_item *item = todo_list->items + todo_list->current;

Other hunks are about "get_dir() + /patch" -> "rebase_path_patch()",
but this hunk is about the intended behaviour change.  We clear the
leftover patch file from the previous round before we enter the loop
to process new insn from the list, which makes sense.

> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index 2d0789e554b..261e7cd754c 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -244,6 +244,24 @@ test_expect_success 'the todo command "break" works' '
>  	test_path_is_file execed
>  '
>  
> +test_expect_success 'patch file is removed before break command' '
> +	test_when_finished "git rebase --abort" &&
> +	cat >todo <<-\EOF &&
> +	pick commit-new-file-F2-on-topic-branch
> +	break
> +	EOF
> +
> +	(
> +		set_replace_editor todo &&
> +		test_must_fail git rebase -i --onto commit-new-file-F2 HEAD
> +	) &&
> +	test_path_is_file .git/rebase-merge/patch &&
> +	echo 22>F2 &&
> +	git add F2 &&
> +	git rebase --continue &&
> +	test_path_is_missing .git/rebase-merge/patch
> +'
> +
>  test_expect_success '--reschedule-failed-exec' '
>  	test_when_finished "git rebase --abort" &&
>  	test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&