diff mbox series

[v2,3/3] rebase --continue: remove .git/MERGE_MSG

Message ID 028c9dfc460b6c00bf481017a07a2a6d37780a76.1628775729.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit e5ee33e8551fa934862b8cbe50693f4841e7dace
Headers show
Series rebase --continue: remove .git/MERGE_MSG | expand

Commit Message

Phillip Wood Aug. 12, 2021, 1:42 p.m. UTC 
From: Phillip Wood <phillip.wood@dunelm.org.uk>

If the user skips the final commit by removing all the changes from
the index and worktree with 'git restore' (or read-tree) and then runs
'git rebase --continue' .git/MERGE_MSG is left behind. This will seed
the commit message the next time the user commits which is not what we
want to happen.

Reported-by: Victor Gambier <vgambier@excilys.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                |  3 +++
 t/t3403-rebase-skip.sh     |  8 ++++++++
 t/t3418-rebase-continue.sh | 10 ++++++++++
 3 files changed, 21 insertions(+)

Comments

Junio C Hamano Aug. 13, 2021, 11:01 p.m. UTC | #1 
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> If the user skips the final commit by removing all the changes from
> the index and worktree with 'git restore' (or read-tree) and then runs
> 'git rebase --continue' .git/MERGE_MSG is left behind. This will seed
> the commit message the next time the user commits which is not what we
> want to happen.

I just remembered that "git rebase --skip" option exists.  Would it
have the same issue if used at the last step?


[Footnote]

I am not saying that it is an error to use "git restore HEAD . &&
git rebase --continue" when you'd usually use "git rebase --skip".

Nuking the difference the working tree files and the index has
relative to HEAD and telling the machinery to continue gives the
signal that the "conflict resolution" happened to have resulted in
an empty change, which should yield the same resulting history as
"git rebase --skip" would, because the resulting empty change should
be dropped (unless --empty=keep is in effect, that is).
Phillip Wood Aug. 14, 2021, 8:01 p.m. UTC | #2 
On 14/08/2021 00:01, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> If the user skips the final commit by removing all the changes from
>> the index and worktree with 'git restore' (or read-tree) and then runs
>> 'git rebase --continue' .git/MERGE_MSG is left behind. This will seed
>> the commit message the next time the user commits which is not what we
>> want to happen.
> 
> I just remembered that "git rebase --skip" option exists.  Would it
> have the same issue if used at the last step?

--skip calls rerere_clear() which unlinks .git/MERGE_MSG. This patch 
adds a test for --skip as well as --continue.

Best Wishes

Phillip

> 
> [Footnote]
> 
> I am not saying that it is an error to use "git restore HEAD . &&
> git rebase --continue" when you'd usually use "git rebase --skip".
> 
> Nuking the difference the working tree files and the index has
> relative to HEAD and telling the machinery to continue gives the
> signal that the "conflict resolution" happened to have resulted in
> an empty change, which should yield the same resulting history as
> "git rebase --skip" would, because the resulting empty change should
> be dropped (unless --empty=keep is in effect, that is).
>
diff mbox series

Patch

diff --git a/sequencer.c b/sequencer.c
index 7f07cd00f3f..52c7b461179 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4716,6 +4716,9 @@  static int commit_staged_changes(struct repository *r,
 		    refs_delete_ref(get_main_ref_store(r), "",
 				    "CHERRY_PICK_HEAD", NULL, 0))
 			return error(_("could not remove CHERRY_PICK_HEAD"));
+		if (unlink(git_path_merge_msg(r)) && errno != ENOENT)
+			return error_errno(_("could not remove '%s'"),
+					   git_path_merge_msg(r));
 		if (!final_fixup)
 			return 0;
 	}
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index a44e68d0ffb..f6e48644978 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -20,6 +20,7 @@  test_expect_success setup '
 	git add hello &&
 	git commit -m "hello" &&
 	git branch skip-reference &&
+	git tag hello &&
 
 	echo world >> hello &&
 	git commit -a -m "hello world" &&
@@ -96,6 +97,13 @@  test_expect_success 'moved back to branch correctly' '
 
 test_debug 'gitk --all & sleep 1'
 
+test_expect_success 'skipping final pick removes .git/MERGE_MSG' '
+	test_must_fail git rebase --onto hello reverted-goodbye^ \
+		reverted-goodbye &&
+	git rebase --skip &&
+	test_path_is_missing .git/MERGE_MSG
+'
+
 test_expect_success 'correct advice upon picking empty commit' '
 	test_when_finished "git rebase --abort" &&
 	test_must_fail git rebase -i --onto goodbye \
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index bda5e5db802..738fbae9b29 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -31,6 +31,16 @@  test_expect_success 'merge based rebase --continue with works with touched file'
 	git rebase --continue
 '
 
+test_expect_success 'merge based rebase --continue removes .git/MERGE_MSG' '
+	git checkout -f --detach topic &&
+
+	test_must_fail git rebase --onto main HEAD^ &&
+	git read-tree --reset -u HEAD &&
+	test_path_is_file .git/MERGE_MSG &&
+	git rebase --continue &&
+	test_path_is_missing .git/MERGE_MSG
+'
+
 test_expect_success 'apply based rebase --continue works with touched file' '
 	rm -fr .git/rebase-* &&
 	git reset --hard &&