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

> "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^ &&