[PATCH 0/3] rebase --continue: remove .git/MERGE_MSG

STALE1858d

Revision v1 of 2 in this series.

13 messages, 3 authors, 2021-08-13 · open the first message on its own page

[PATCH 0/3] rebase --continue: remove .git/MERGE_MSG

From: Phillip Wood via GitGitGadget <hidden>
Date: 2021-08-10 09:32:04

If the user removes all the changes from the worktree without running git
reset then it rebase --continue leaves behind .git/MERGE_MSG which will then
seed the message of the next commit. While working on this I noticed a
couple of fixups for the test files I was adding to, I've cc'd Elijah in
case my reasoning for patch 2 is off. Thanks to Victor for the bug report.

Phillip Wood (3):
  t3403: fix commit authorship
  rebase --apply: restore some tests
  rebase --continue: remove .git/MERGE_MSG

 sequencer.c                |  3 +++
 t/t3403-rebase-skip.sh     | 13 +++++++++++--
 t/t3418-rebase-continue.sh | 14 ++++++++++++--
 3 files changed, 26 insertions(+), 4 deletions(-)


base-commit: 2d755dfac9aadab25c3e025b849252b8c0a61465
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1013%2Fphillipwood%2Fwip%2Frebase-remove-merge-msg-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1013/phillipwood/wip/rebase-remove-merge-msg-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1013
-- 
gitgitgadget

[PATCH 1/3] t3403: fix commit authorship

From: Phillip Wood via GitGitGadget <hidden>
Date: 2021-08-10 09:32:05

From: Phillip Wood <redacted>

Setting GIT_AUTHOR_* when committing with --amend will only change the
author if we also pass --reset-author

Signed-off-by: Phillip Wood <redacted>
---
 t/t3403-rebase-skip.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index e26762d0b29..6365c5af2f7 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -36,7 +36,8 @@ test_expect_success setup '
 	test_tick &&
 	GIT_AUTHOR_NAME="Another Author" \
 		GIT_AUTHOR_EMAIL="another.author@example.com" \
-		git commit --amend --no-edit -m amended-goodbye &&
+		git commit --amend --no-edit -m amended-goodbye \
+			--reset-author &&
 	test_tick &&
 	git tag amended-goodbye &&
 
-- 
gitgitgadget

[PATCH 2/3] rebase --apply: restore some tests

From: Phillip Wood via GitGitGadget <hidden>
Date: 2021-08-10 09:32:16

From: Phillip Wood <redacted>

980b482d28 ("rebase tests: mark tests specific to the am-backend with
--am", 2020-02-15) sought to prepare tests testing the "apply" backend
in preparation for 2ac0d6273f ("rebase: change the default backend
from "am" to "merge"", 2020-02-15). However some tests seem to have
been missed leading to us testing the "merge" backend twice. This
patch fixes some cases that I noticed while adding tests to these
files, I have not audited all the other rebase test files.

Signed-off-by: Phillip Wood <redacted>
---
 t/t3403-rebase-skip.sh     | 2 +-
 t/t3418-rebase-continue.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index 6365c5af2f7..a44e68d0ffb 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -52,7 +52,7 @@ test_expect_success setup '
 	'
 
 test_expect_success 'rebase with git am -3 (default)' '
-	test_must_fail git rebase main
+	test_must_fail git rebase --apply main
 '
 
 test_expect_success 'rebase --skip can not be used with other options' '
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index f4c2ee02bc9..e4cb8be0418 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -36,7 +36,7 @@ test_expect_success 'non-interactive rebase --continue works with touched file'
 	git reset --hard &&
 	git checkout main &&
 
-	test_must_fail git rebase --onto main main topic &&
+	test_must_fail git rebase --apply --onto main main topic &&
 	echo "Resolved" >F2 &&
 	git add F2 &&
 	test-tool chmtime =-60 F1 &&
@@ -254,7 +254,7 @@ test_rerere_autoupdate () {
 	'
 }
 
-test_rerere_autoupdate
+test_rerere_autoupdate --apply
 test_rerere_autoupdate -m
 GIT_SEQUENCE_EDITOR=: && export GIT_SEQUENCE_EDITOR
 test_rerere_autoupdate -i
-- 
gitgitgadget

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

From: Phillip Wood via GitGitGadget <hidden>
Date: 2021-08-10 09:32:21

From: Phillip Wood <redacted>

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 <redacted>
Signed-off-by: Phillip Wood <redacted>
---
 sequencer.c                |  3 +++
 t/t3403-rebase-skip.sh     |  8 ++++++++
 t/t3418-rebase-continue.sh | 10 ++++++++++
 3 files changed, 21 insertions(+)
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 e4cb8be0418..f23996c9be0 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -31,6 +31,16 @@ test_expect_success 'interactive rebase --continue works with touched file' '
 	git rebase --continue
 '
 
+test_expect_success '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 'non-interactive rebase --continue works with touched file' '
 	rm -fr .git/rebase-* &&
 	git reset --hard &&
-- 
gitgitgadget

Re: [PATCH 2/3] rebase --apply: restore some tests

From: Elijah Newren <hidden>
Date: 2021-08-10 16:58:22

On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget
[off-list ref] wrote:
quoted hunk
From: Phillip Wood <redacted>

980b482d28 ("rebase tests: mark tests specific to the am-backend with
--am", 2020-02-15) sought to prepare tests testing the "apply" backend
in preparation for 2ac0d6273f ("rebase: change the default backend
from "am" to "merge"", 2020-02-15). However some tests seem to have
been missed leading to us testing the "merge" backend twice. This
patch fixes some cases that I noticed while adding tests to these
files, I have not audited all the other rebase test files.

Signed-off-by: Phillip Wood <redacted>
---
 t/t3403-rebase-skip.sh     | 2 +-
 t/t3418-rebase-continue.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index 6365c5af2f7..a44e68d0ffb 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -52,7 +52,7 @@ test_expect_success setup '
        '

 test_expect_success 'rebase with git am -3 (default)' '
-       test_must_fail git rebase main
+       test_must_fail git rebase --apply main
Looks good.
quoted hunk
 '

 test_expect_success 'rebase --skip can not be used with other options' '
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index f4c2ee02bc9..e4cb8be0418 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -36,7 +36,7 @@ test_expect_success 'non-interactive rebase --continue works with touched file'
        git reset --hard &&
        git checkout main &&

-       test_must_fail git rebase --onto main main topic &&
+       test_must_fail git rebase --apply --onto main main topic &&
I think the point here is that you're noticing that "git rebase -i"
and "git rebase" are both built on the same rebase backend (the merge
one) and thus that testing interactive vs. non-interactive isn't much
of a test.  The real test we are interested in is merge-backend vs.
apply-backend.  Your code change here is the necessary one to do that,
but it least the test descriptions still talking about interactive vs.
non-interactive even though that's not what we're concentrating on
anymore.

I'd say we'd at least want to change the description for this test,
"non-interactive rebase ---continue works with..." => "rebase
--continue with the apply backend works with...", but that we'd
probably want to change the description of the test before it and
maybe even just use rebase --merge rather than rebase -i.
quoted hunk
        echo "Resolved" >F2 &&
        git add F2 &&
        test-tool chmtime =-60 F1 &&
@@ -254,7 +254,7 @@ test_rerere_autoupdate () {
        '
 }

-test_rerere_autoupdate
+test_rerere_autoupdate --apply
Looks good.
 test_rerere_autoupdate -m
 GIT_SEQUENCE_EDITOR=: && export GIT_SEQUENCE_EDITOR
 test_rerere_autoupdate -i
--
gitgitgadget

Re: [PATCH 1/3] t3403: fix commit authorship

From: Elijah Newren <hidden>
Date: 2021-08-10 17:02:01

On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget
[off-list ref] wrote:
quoted hunk
From: Phillip Wood <redacted>

Setting GIT_AUTHOR_* when committing with --amend will only change the
author if we also pass --reset-author

Signed-off-by: Phillip Wood <redacted>
---
 t/t3403-rebase-skip.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index e26762d0b29..6365c5af2f7 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -36,7 +36,8 @@ test_expect_success setup '
        test_tick &&
        GIT_AUTHOR_NAME="Another Author" \
                GIT_AUTHOR_EMAIL="another.author@example.com" \
-               git commit --amend --no-edit -m amended-goodbye &&
+               git commit --amend --no-edit -m amended-goodbye \
+                       --reset-author &&
Makes sense...but doesn't the fact that this test worked either way
suggest that the specifying of a special author name/email was totally
superfluous and could just be removed?  If there really was a reason
for specifying a different name/email, then is the test faulty for not
checking for it somewhere?

Re: [PATCH 0/3] rebase --continue: remove .git/MERGE_MSG

From: Elijah Newren <hidden>
Date: 2021-08-10 17:03:52

On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget
[off-list ref] wrote:
If the user removes all the changes from the worktree without running git
reset then it rebase --continue leaves behind .git/MERGE_MSG which will then
s/then it/then/
seed the message of the next commit. While working on this I noticed a
couple of fixups for the test files I was adding to, I've cc'd Elijah in
case my reasoning for patch 2 is off. Thanks to Victor for the bug report.
I had a couple small comments on the first two patches.  Otherwise,
the whole series looks good to me.

Re: [PATCH 2/3] rebase --apply: restore some tests

From: Phillip Wood <hidden>
Date: 2021-08-12 10:04:13

On 10/08/2021 17:58, Elijah Newren wrote:
On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget
quoted
  test_expect_success 'rebase --skip can not be used with other options' '
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index f4c2ee02bc9..e4cb8be0418 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -36,7 +36,7 @@ test_expect_success 'non-interactive rebase --continue works with touched file'
         git reset --hard &&
         git checkout main &&

-       test_must_fail git rebase --onto main main topic &&
+       test_must_fail git rebase --apply --onto main main topic &&
I think the point here is that you're noticing that "git rebase -i"
and "git rebase" are both built on the same rebase backend (the merge
one) and thus that testing interactive vs. non-interactive isn't much
of a test.
Exactly
 The real test we are interested in is merge-backend vs.
apply-backend.  Your code change here is the necessary one to do that,
but it least the test descriptions still talking about interactive vs.
non-interactive even though that's not what we're concentrating on
anymore.

I'd say we'd at least want to change the description for this test,
"non-interactive rebase ---continue works with..." => "rebase
--continue with the apply backend works with...", but that we'd
probably want to change the description of the test before it and
maybe even just use rebase --merge rather than rebase -i.
Rewording the test descriptions is a good suggestion

Thanks

Phillip
quoted
         echo "Resolved" >F2 &&
         git add F2 &&
         test-tool chmtime =-60 F1 &&
@@ -254,7 +254,7 @@ test_rerere_autoupdate () {
         '
  }

-test_rerere_autoupdate
+test_rerere_autoupdate --apply
Looks good.
quoted
  test_rerere_autoupdate -m
  GIT_SEQUENCE_EDITOR=: && export GIT_SEQUENCE_EDITOR
  test_rerere_autoupdate -i
--
gitgitgadget

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

From: Phillip Wood via GitGitGadget <hidden>
Date: 2021-08-12 13:42:15

Thanks for the comments on V1. I have expanded the commit message for the
first patch to give a bit more context and reworded a couple of test
descriptions in patch 2 as suggested by Elijah. Patch 3 is unchanged.

V1 Cover Letter:

If the user removes all the changes from the worktree without running git
reset then rebase --continue leaves behind .git/MERGE_MSG which will then
seed the message of the next commit. While working on this I noticed a
couple of fixups for the test files I was adding to, I've cc'd Elijah in
case my reasoning for patch 2 is off. Thanks to Victor for the bug report.

cc: Victor Gambier vgambier@excilys.com cc: Elijah Newren newren@gmail.com
cc: Johannes Schindelin Johannes.Schindelin@gmx.de

Phillip Wood (3):
  t3403: fix commit authorship
  rebase --apply: restore some tests
  rebase --continue: remove .git/MERGE_MSG

 sequencer.c                |  3 +++
 t/t3403-rebase-skip.sh     | 13 +++++++++++--
 t/t3418-rebase-continue.sh | 18 ++++++++++++++----
 3 files changed, 28 insertions(+), 6 deletions(-)


base-commit: 2d755dfac9aadab25c3e025b849252b8c0a61465
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1013%2Fphillipwood%2Fwip%2Frebase-remove-merge-msg-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1013/phillipwood/wip/rebase-remove-merge-msg-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1013

Range-diff vs v1:

 1:  7559781ca92 ! 1:  8755dfa9d04 t3403: fix commit authorship
     @@ Commit message
          t3403: fix commit authorship
      
          Setting GIT_AUTHOR_* when committing with --amend will only change the
     -    author if we also pass --reset-author
     +    author if we also pass --reset-author.  This commit is used in some
     +    tests that ensure the author ident does not change when rebasing.
     +    Creating this commit without changing the authorship meant that the
     +    test would not catch regressions that caused rebase to discard the
     +    original authorship information.
      
          Signed-off-by: Phillip Wood [off-list ref]
      
 2:  f7722dab21d ! 2:  84fe5823b4f rebase --apply: restore some tests
     @@ Commit message
          from "am" to "merge"", 2020-02-15). However some tests seem to have
          been missed leading to us testing the "merge" backend twice. This
          patch fixes some cases that I noticed while adding tests to these
     -    files, I have not audited all the other rebase test files.
     +    files, I have not audited all the other rebase test files. I've
     +    reworded a couple of the test descriptions to make it clear which
     +    backend they are testing.
      
          Signed-off-by: Phillip Wood [off-list ref]
      
     @@ t/t3403-rebase-skip.sh: test_expect_success setup '
       test_expect_success 'rebase --skip can not be used with other options' '
      
       ## t/t3418-rebase-continue.sh ##
     -@@ t/t3418-rebase-continue.sh: test_expect_success 'non-interactive rebase --continue works with touched file'
     +@@ t/t3418-rebase-continue.sh: test_expect_success 'setup' '
     + 	git checkout main
     + '
     + 
     +-test_expect_success 'interactive rebase --continue works with touched file' '
     ++test_expect_success 'merge based rebase --continue with works with touched file' '
     + 	rm -fr .git/rebase-* &&
     + 	git reset --hard &&
     + 	git checkout main &&
     +@@ t/t3418-rebase-continue.sh: test_expect_success 'interactive rebase --continue works with touched file' '
     + 	git rebase --continue
     + '
     + 
     +-test_expect_success 'non-interactive rebase --continue works with touched file' '
     ++test_expect_success 'apply based rebase --continue works with touched file' '
     + 	rm -fr .git/rebase-* &&
       	git reset --hard &&
       	git checkout main &&
       
 3:  6a63b657f13 ! 3:  028c9dfc460 rebase --continue: remove .git/MERGE_MSG
     @@ t/t3403-rebase-skip.sh: test_expect_success 'moved back to branch correctly' '
       	test_must_fail git rebase -i --onto goodbye \
      
       ## t/t3418-rebase-continue.sh ##
     -@@ t/t3418-rebase-continue.sh: test_expect_success 'interactive rebase --continue works with touched file' '
     +@@ t/t3418-rebase-continue.sh: test_expect_success 'merge based rebase --continue with works with touched file'
       	git rebase --continue
       '
       
     -+test_expect_success 'rebase --continue removes .git/MERGE_MSG' '
     ++test_expect_success 'merge based rebase --continue removes .git/MERGE_MSG' '
      +	git checkout -f --detach topic &&
      +
      +	test_must_fail git rebase --onto main HEAD^ &&
     @@ t/t3418-rebase-continue.sh: test_expect_success 'interactive rebase --continue w
      +	test_path_is_missing .git/MERGE_MSG
      +'
      +
     - test_expect_success 'non-interactive rebase --continue works with touched file' '
     + test_expect_success 'apply based rebase --continue works with touched file' '
       	rm -fr .git/rebase-* &&
       	git reset --hard &&

-- 
gitgitgadget

[PATCH v2 1/3] t3403: fix commit authorship

From: Phillip Wood via GitGitGadget <hidden>
Date: 2021-08-12 13:42:16

From: Phillip Wood <redacted>

Setting GIT_AUTHOR_* when committing with --amend will only change the
author if we also pass --reset-author.  This commit is used in some
tests that ensure the author ident does not change when rebasing.
Creating this commit without changing the authorship meant that the
test would not catch regressions that caused rebase to discard the
original authorship information.

Signed-off-by: Phillip Wood <redacted>
---
 t/t3403-rebase-skip.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index e26762d0b29..6365c5af2f7 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -36,7 +36,8 @@ test_expect_success setup '
 	test_tick &&
 	GIT_AUTHOR_NAME="Another Author" \
 		GIT_AUTHOR_EMAIL="another.author@example.com" \
-		git commit --amend --no-edit -m amended-goodbye &&
+		git commit --amend --no-edit -m amended-goodbye \
+			--reset-author &&
 	test_tick &&
 	git tag amended-goodbye &&
 
-- 
gitgitgadget

[PATCH v2 2/3] rebase --apply: restore some tests

From: Phillip Wood via GitGitGadget <hidden>
Date: 2021-08-12 13:42:17

From: Phillip Wood <redacted>

980b482d28 ("rebase tests: mark tests specific to the am-backend with
--am", 2020-02-15) sought to prepare tests testing the "apply" backend
in preparation for 2ac0d6273f ("rebase: change the default backend
from "am" to "merge"", 2020-02-15). However some tests seem to have
been missed leading to us testing the "merge" backend twice. This
patch fixes some cases that I noticed while adding tests to these
files, I have not audited all the other rebase test files. I've
reworded a couple of the test descriptions to make it clear which
backend they are testing.

Signed-off-by: Phillip Wood <redacted>
---
 t/t3403-rebase-skip.sh     | 2 +-
 t/t3418-rebase-continue.sh | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index 6365c5af2f7..a44e68d0ffb 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -52,7 +52,7 @@ test_expect_success setup '
 	'
 
 test_expect_success 'rebase with git am -3 (default)' '
-	test_must_fail git rebase main
+	test_must_fail git rebase --apply main
 '
 
 test_expect_success 'rebase --skip can not be used with other options' '
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index f4c2ee02bc9..bda5e5db802 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -21,7 +21,7 @@ test_expect_success 'setup' '
 	git checkout main
 '
 
-test_expect_success 'interactive rebase --continue works with touched file' '
+test_expect_success 'merge based rebase --continue with works with touched file' '
 	rm -fr .git/rebase-* &&
 	git reset --hard &&
 	git checkout main &&
@@ -31,12 +31,12 @@ test_expect_success 'interactive rebase --continue works with touched file' '
 	git rebase --continue
 '
 
-test_expect_success 'non-interactive rebase --continue works with touched file' '
+test_expect_success 'apply based rebase --continue works with touched file' '
 	rm -fr .git/rebase-* &&
 	git reset --hard &&
 	git checkout main &&
 
-	test_must_fail git rebase --onto main main topic &&
+	test_must_fail git rebase --apply --onto main main topic &&
 	echo "Resolved" >F2 &&
 	git add F2 &&
 	test-tool chmtime =-60 F1 &&
@@ -254,7 +254,7 @@ test_rerere_autoupdate () {
 	'
 }
 
-test_rerere_autoupdate
+test_rerere_autoupdate --apply
 test_rerere_autoupdate -m
 GIT_SEQUENCE_EDITOR=: && export GIT_SEQUENCE_EDITOR
 test_rerere_autoupdate -i
-- 
gitgitgadget

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

From: Phillip Wood via GitGitGadget <hidden>
Date: 2021-08-12 13:42:18

From: Phillip Wood <redacted>

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 <redacted>
Signed-off-by: Phillip Wood <redacted>
---
 sequencer.c                |  3 +++
 t/t3403-rebase-skip.sh     |  8 ++++++++
 t/t3418-rebase-continue.sh | 10 ++++++++++
 3 files changed, 21 insertions(+)
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 &&
-- 
gitgitgadget

Re: [PATCH v2 0/3] rebase --continue: remove .git/MERGE_MSG

From: Elijah Newren <hidden>
Date: 2021-08-13 00:46:38

On Thu, Aug 12, 2021 at 6:46 AM Phillip Wood via GitGitGadget
[off-list ref] wrote:
Thanks for the comments on V1. I have expanded the commit message for the
first patch to give a bit more context and reworded a couple of test
descriptions in patch 2 as suggested by Elijah. Patch 3 is unchanged.

V1 Cover Letter:

If the user removes all the changes from the worktree without running git
reset then rebase --continue leaves behind .git/MERGE_MSG which will then
seed the message of the next commit. While working on this I noticed a
couple of fixups for the test files I was adding to, I've cc'd Elijah in
case my reasoning for patch 2 is off. Thanks to Victor for the bug report.

cc: Victor Gambier vgambier@excilys.com cc: Elijah Newren newren@gmail.com
cc: Johannes Schindelin Johannes.Schindelin@gmx.de

Phillip Wood (3):
  t3403: fix commit authorship
  rebase --apply: restore some tests
  rebase --continue: remove .git/MERGE_MSG

 sequencer.c                |  3 +++
 t/t3403-rebase-skip.sh     | 13 +++++++++++--
 t/t3418-rebase-continue.sh | 18 ++++++++++++++----
 3 files changed, 28 insertions(+), 6 deletions(-)


base-commit: 2d755dfac9aadab25c3e025b849252b8c0a61465
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1013%2Fphillipwood%2Fwip%2Frebase-remove-merge-msg-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1013/phillipwood/wip/rebase-remove-merge-msg-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1013

Range-diff vs v1:

 1:  7559781ca92 ! 1:  8755dfa9d04 t3403: fix commit authorship
     @@ Commit message
          t3403: fix commit authorship

          Setting GIT_AUTHOR_* when committing with --amend will only change the
     -    author if we also pass --reset-author
     +    author if we also pass --reset-author.  This commit is used in some
     +    tests that ensure the author ident does not change when rebasing.
     +    Creating this commit without changing the authorship meant that the
     +    test would not catch regressions that caused rebase to discard the
     +    original authorship information.

          Signed-off-by: Phillip Wood [off-list ref]

 2:  f7722dab21d ! 2:  84fe5823b4f rebase --apply: restore some tests
     @@ Commit message
          from "am" to "merge"", 2020-02-15). However some tests seem to have
          been missed leading to us testing the "merge" backend twice. This
          patch fixes some cases that I noticed while adding tests to these
     -    files, I have not audited all the other rebase test files.
     +    files, I have not audited all the other rebase test files. I've
     +    reworded a couple of the test descriptions to make it clear which
     +    backend they are testing.

          Signed-off-by: Phillip Wood [off-list ref]

     @@ t/t3403-rebase-skip.sh: test_expect_success setup '
       test_expect_success 'rebase --skip can not be used with other options' '

       ## t/t3418-rebase-continue.sh ##
     -@@ t/t3418-rebase-continue.sh: test_expect_success 'non-interactive rebase --continue works with touched file'
     +@@ t/t3418-rebase-continue.sh: test_expect_success 'setup' '
     +  git checkout main
     + '
     +
     +-test_expect_success 'interactive rebase --continue works with touched file' '
     ++test_expect_success 'merge based rebase --continue with works with touched file' '
     +  rm -fr .git/rebase-* &&
     +  git reset --hard &&
     +  git checkout main &&
     +@@ t/t3418-rebase-continue.sh: test_expect_success 'interactive rebase --continue works with touched file' '
     +  git rebase --continue
     + '
     +
     +-test_expect_success 'non-interactive rebase --continue works with touched file' '
     ++test_expect_success 'apply based rebase --continue works with touched file' '
     +  rm -fr .git/rebase-* &&
        git reset --hard &&
        git checkout main &&

 3:  6a63b657f13 ! 3:  028c9dfc460 rebase --continue: remove .git/MERGE_MSG
     @@ t/t3403-rebase-skip.sh: test_expect_success 'moved back to branch correctly' '
        test_must_fail git rebase -i --onto goodbye \

       ## t/t3418-rebase-continue.sh ##
     -@@ t/t3418-rebase-continue.sh: test_expect_success 'interactive rebase --continue works with touched file' '
     +@@ t/t3418-rebase-continue.sh: test_expect_success 'merge based rebase --continue with works with touched file'
        git rebase --continue
       '

     -+test_expect_success 'rebase --continue removes .git/MERGE_MSG' '
     ++test_expect_success 'merge based rebase --continue removes .git/MERGE_MSG' '
      + git checkout -f --detach topic &&
      +
      + test_must_fail git rebase --onto main HEAD^ &&
     @@ t/t3418-rebase-continue.sh: test_expect_success 'interactive rebase --continue w
      + test_path_is_missing .git/MERGE_MSG
      +'
      +
     - test_expect_success 'non-interactive rebase --continue works with touched file' '
     + test_expect_success 'apply based rebase --continue works with touched file' '
        rm -fr .git/rebase-* &&
        git reset --hard &&

--
gitgitgadget
Thanks, this version looks good to me.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help