diff mbox series

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

Message ID e2a758eb4a5df0fab189b3dd235a1651a0c10342.1690903412.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series rebase -i: impove handling of failed commands | expand

Commit Message

Phillip Wood Aug. 1, 2023, 3:23 p.m. UTC 
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. As the path is
now used in two different places rebase_path_patch() is added and used
to obtain the path for the patch.

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(-)

Comments

Junio C Hamano Aug. 1, 2023, 5:23 p.m. UTC | #1 
"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^ &&
Phillip Wood Aug. 1, 2023, 6:47 p.m. UTC | #2 
On 01/08/2023 18:23, Junio C Hamano wrote:
> "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.

There is no intended change in the observable behavior in the previous 
patch.

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

Good point - the patch file is only ever created by "rebase -i". I'll 
add the assertion you suggest below.

Best Wishes

Phillip

> 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^ &&
diff mbox series

Patch

diff --git a/sequencer.c b/sequencer.c
index de66bda9d5b..70b0a7023b0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -138,6 +138,11 @@  static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend")
  * the commit object name of the corresponding patch.
  */
 static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")
+/*
+ * When we stop for the user to resolve conflicts this file contains
+ * the patch of the commit that is being picked.
+ */
+static GIT_PATH_FUNC(rebase_path_patch, "rebase-merge/patch")
 /*
  * For the post-rewrite hook, we make a list of rewritten commits and
  * their new sha1s.  The rewritten-pending list keeps the sha1s of
@@ -3507,7 +3512,6 @@  static int make_patch(struct repository *r,
 		return -1;
 	res |= write_rebase_head(&commit->object.oid);
 
-	strbuf_addf(&buf, "%s/patch", get_dir(opts));
 	memset(&log_tree_opt, 0, sizeof(log_tree_opt));
 	repo_init_revisions(r, &log_tree_opt, NULL);
 	log_tree_opt.abbrev = 0;
@@ -3515,15 +3519,15 @@  static int make_patch(struct repository *r,
 	log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
 	log_tree_opt.disable_stdin = 1;
 	log_tree_opt.no_commit_id = 1;
-	log_tree_opt.diffopt.file = fopen(buf.buf, "w");
+	log_tree_opt.diffopt.file = fopen(rebase_path_patch(), "w");
 	log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
 	if (!log_tree_opt.diffopt.file)
-		res |= error_errno(_("could not open '%s'"), buf.buf);
+		res |= error_errno(_("could not open '%s'"),
+				   rebase_path_patch());
 	else {
 		res |= log_tree_commit(&log_tree_opt, commit);
 		fclose(log_tree_opt.diffopt.file);
 	}
-	strbuf_reset(&buf);
 
 	strbuf_addf(&buf, "%s/message", get_dir(opts));
 	if (!file_exists(buf.buf)) {
@@ -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;
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^ &&