Skip to contentSkip to conversation

rebase --continue: remove .git/MERGE_MSG

PATCH Superseded Phillip Wood via GitGitGadget 5 participants 25 messages Aug 10, 2021 – Aug 17, 2021

lore
Share & tools
lore

Phillip Wood via GitGitGadget

author
#0 lore raw

cover rebase --continue: remove .git/MERGE_MSG

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

Signature · 1 line
gitgitgadget
author
#1 lore raw

PATCH 1/3 t3403: fix commit authorship

From: Phillip Wood <[email protected]>

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

Signed-off-by Phillip Wood <[email protected]>
---
 t/t3403-rebase-skip.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
t/t3403-rebase-skip.sh +2 -1
index e26762d0b29..6365c5af2f7 100755
@@ -36,7 +36,8 @@ test_expect_success setup '
 	test_tick &&
 	GIT_AUTHOR_NAME="Another Author" \
 		GIT_AUTHOR_EMAIL="[email protected]" \
-		git commit --amend --no-edit -m amended-goodbye &&
+		git commit --amend --no-edit -m amended-goodbye \
+			--reset-author &&
 	test_tick &&
 	git tag amended-goodbye &&
Signature · 1 line
gitgitgadget
#2 lore raw

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

Quoted 16 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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

Signed-off-by Phillip Wood <[email protected]>
---
 t/t3403-rebase-skip.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
t/t3403-rebase-skip.sh +0 -0
index e26762d0b29..6365c5af2f7 100755

t/t3403-rebase-skip.sh test_expect_success setup ' · line 40

Quoted 6 lines
t/t3403-rebase-skip.sh +2 -1
@@ -36,7 +36,8 @@ test_expect_success setup '
        test_tick &&
        GIT_AUTHOR_NAME="Another Author" \
                GIT_AUTHOR_EMAIL="[email protected]" \
-               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?

#3 lore raw

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

Quoted 22 lines · Elijah Newren
Quoted 20 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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

Signed-off-by Phillip Wood <[email protected]>
---
 t/t3403-rebase-skip.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
t/t3403-rebase-skip.sh +0 -1
index e26762d0b29..6365c5af2f7 100755
@@ -36,7 +36,8 @@ test_expect_success setup '
        test_tick &&
        GIT_AUTHOR_NAME="Another Author" \
                GIT_AUTHOR_EMAIL="[email protected]" \
-               git commit --amend --no-edit -m amended-goodbye &&
Quoted 8 lines

t/t3403-rebase-skip.sh test_expect_success setup ' · line 40

Quoted 2 lines
t/t3403-rebase-skip.sh +2 -0
+               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?

Good point. The commit tagged with amended-goodbye is later used in some tests that ensure the author ident does not change across a rebase. If this commit gets created without authorship customized (i.e. before Phillip's fix), we would not catch a possible breakage to make rebase discard the original authorship information.

But with this fix, we now can catch such a breakage.

Thanks.

#4 lore raw

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

Quoted 33 lines · Junio C Hamano
Quoted 22 lines · Elijah Newren
Quoted 20 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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

Signed-off-by Phillip Wood <[email protected]>
---
  t/t3403-rebase-skip.sh | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
t/t3403-rebase-skip.sh +0 -1
index e26762d0b29..6365c5af2f7 100755
@@ -36,7 +36,8 @@ test_expect_success setup '
         test_tick &&
         GIT_AUTHOR_NAME="Another Author" \
                 GIT_AUTHOR_EMAIL="[email protected]" \
-               git commit --amend --no-edit -m amended-goodbye &&
Quoted 8 lines

t/t3403-rebase-skip.sh test_expect_success setup ' · line 40

Quoted 2 lines
t/t3403-rebase-skip.sh +2 -0
+               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?

Quoted 7 lines

Good point. The commit tagged with amended-goodbye is later used in some tests that ensure the author ident does not change across a rebase. If this commit gets created without authorship customized (i.e. before Phillip's fix), we would not catch a possible breakage to make rebase discard the original authorship information.

But with this fix, we now can catch such a breakage.

I'll expand the commit message to make that clear

Thanks

Phillip

#5 lore raw

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

Hi Phillip,

Quoted 40 lines · Phillip Wood
Quoted 39 lines · Junio C Hamano
Quoted 22 lines · Elijah Newren
Quoted 20 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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

Signed-off-by Phillip Wood <[email protected]>
---
  t/t3403-rebase-skip.sh | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
t/t3403-rebase-skip.sh +0 -1
index e26762d0b29..6365c5af2f7 100755
@@ -36,7 +36,8 @@ test_expect_success setup '
         test_tick &&
         GIT_AUTHOR_NAME="Another Author" \
                 GIT_AUTHOR_EMAIL="[email protected]" \
-               git commit --amend --no-edit -m amended-goodbye &&
Quoted 8 lines

t/t3403-rebase-skip.sh test_expect_success setup ' · line 40

Quoted 2 lines
t/t3403-rebase-skip.sh +2 -0
+               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?

Good point. The commit tagged with amended-goodbye is later used in some tests that ensure the author ident does not change across a rebase. If this commit gets created without authorship customized (i.e. before Phillip's fix), we would not catch a possible breakage to make rebase discard the original authorship information.

Quoted 3 lines
Quoted 1 line

But with this fix, we now can catch such a breakage.

I'll expand the commit message to make that clear

Maybe you could even add a test [email protected] = $(git show -s --format=%ae HEAD)?

Ciao,
Dscho

#6 lore raw

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

Quoted 6 lines · Johannes Schindelin
Quoted 6 lines
Quoted 6 lines

Good point. The commit tagged with amended-goodbye is later used in some tests that ensure the author ident does not change across a rebase. If this commit gets created without authorship customized (i.e. before Phillip's fix), we would not catch a possible breakage to make rebase discard the original authorship information.

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

But with this fix, we now can catch such a breakage.

I'll expand the commit message to make that clear

Maybe you could even add a test [email protected] = $(git show -s --format=%ae HEAD)?

The version I have from Phillip has updated log message already, but not with such a regression prevention.

The test that the patch under discussion corrects does this:

    test_expect_success 'correct authorship when committing empty pick' '
        test_when_finished "git rebase --abort" &&
        test_must_fail git rebase -i --onto goodbye \
                amended-goodbye^ amended-goodbye &&
        git commit --allow-empty &&
        git log --pretty=format:"%an <%ae>%n%ad%B" -1 amended-goodbye >expect &&
        git log --pretty=format:"%an <%ae>%n%ad%B" -1 HEAD >actual &&
        test_cmp expect actual
    '

to ensure that the authorship is the same between the original (i.e. amended-goodbye) and the rebased (i.e. HEAD), with the expectation that a bug may lose the authorship and instead use the default one used in the test suite. What this test truly cares is not that amended-goodbye was authored by another.author, but it was not written by the default author.

We could test both, like the attached patch, for completeness. The first half makes sure amended-goodbye (the original) was written by the another.author, and the other one makes sure that author is not the one we use to prepare commits for the tests by default.

I do not think the latter is actually a good idea ("As long as the command produces a result different from THIS, any random garbage is accepted" does not make a good test), so perhaps the first half would be good enough.

 t/t3403-rebase-skip.sh | 8 ++++++++
 1 file changed, 8 insertions(+)
t/t3403-rebase-skip.sh +8 -0
index e26762d0b2..1405720767 100755
@@ -40,6 +40,14 @@ test_expect_success setup '
 	test_tick &&
 	git tag amended-goodbye &&
 
+	# Make sure the authorship info is different from the default one
+	echo "Another Author <[email protected]>" >expect &&
+	git log --pretty=format:"%an <%ae>" -1 amended-goodbye >actual &&
+	test_cmp expect actual &&
+
+	git log --pretty=format:"%an <%ae>" -1 goodbye >unexpect &&
+	! test_cmp unexpect actual &&
+
 	git checkout -f skip-reference &&
 	echo moo > hello &&
 	git commit -a -m "we should skip this" &&
#7 lore raw

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

Quoted 20 lines · Junio C Hamano
Quoted 6 lines · Johannes Schindelin
Quoted 6 lines
Quoted 6 lines

Good point. The commit tagged with amended-goodbye is later used in some tests that ensure the author ident does not change across a rebase. If this commit gets created without authorship customized (i.e. before Phillip's fix), we would not catch a possible breakage to make rebase discard the original authorship information.

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

But with this fix, we now can catch such a breakage.

I'll expand the commit message to make that clear

Maybe you could even add a test [email protected] = $(git show -s --format=%ae HEAD)?

The version I have from Phillip has updated log message already, but not with such a regression prevention.

The test that the patch under discussion corrects does this:

Quoted 9 lines
     test_expect_success 'correct authorship when committing empty pick' '
         test_when_finished "git rebase --abort" &&
         test_must_fail git rebase -i --onto goodbye \
                 amended-goodbye^ amended-goodbye &&
         git commit --allow-empty &&
         git log --pretty=format:"%an <%ae>%n%ad%B" -1 amended-goodbye >expect &&
         git log --pretty=format:"%an <%ae>%n%ad%B" -1 HEAD >actual &&
         test_cmp expect actual
     '

I wonder if it would be better to hard code the author in this test rather than rather than relying on git log like this

t/t3403-rebase-skip.sh +8 -5
index e26762d0b2..c90e32817f 100755
@@ -34,9 +34,10 @@ test_expect_success setup '
        git tag reverted-goodbye &&
        git checkout goodbye &&
        test_tick &&
-       GIT_AUTHOR_NAME="Another Author" \
-               GIT_AUTHOR_EMAIL="[email protected]" \
-               git commit --amend --no-edit -m amended-goodbye &&
+       another_author="Another Author <[email protected]>" &&
+       git commit --amend --no-edit -m amended-goodbye \
+               --author="$another_author" --date="$GIT_AUTHOR_DATE" &&
+       another_author="$another_author $GIT_AUTHOR_DATE" &&
        test_tick &&
        git tag amended-goodbye &&
 

@@ -110,8 +111,10 @@ test_expect_success 'correct authorship when committing empty pick' '
        test_must_fail git rebase -i --onto goodbye \
                amended-goodbye^ amended-goodbye &&
        git commit --allow-empty &&
-       git log --pretty=format:"%an <%ae>%n%ad%B" -1 amended-goodbye >expect &&
-       git log --pretty=format:"%an <%ae>%n%ad%B" -1 HEAD >actual &&
+       git log --pretty=format:"$another_author%n%B" -1 amended-goodbye \
+                >expect &&
+       git log --date=raw --pretty=format:"%an <%ae> %ad%n%B" -1 HEAD \
+               >actual &&
        test_cmp expect actual
 '
#8 lore raw

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

Quoted 2 lines · Phillip Wood

I wonder if it would be better to hard code the author in this test rather than rather than relying on git log like this

To be quite honest, I am pretty happy with the tests as posted, and the choice among various ways we are discussing depends on what the likely mode of breakage we expect.

The breakage we are expecting to catch in the second hunk of your patch is somehow "rebase -i" fails to keep the authorship of amended-goodbye and ends up making the commit at HEAD under different authorship.

- A likely source of a different authorship information that would be recorded, when such a bug gets introduced, is from the environment (i.e. GIT_AUTHOR_NAME etc. that test-lib.sh sets up). This can happen by a new bug in the test added before the second hunk of your patch we see below, and with or without this patch, such a bug in the test will not be caught.

- Or amended-goodbye may by a test bug have been recorded under a wrong authorship information to begin with, and if it were done as the default author we use in our tests (i.e. the bug you fixed in your patch that started this thread). If we reintroduced such a bug, the second hunk of this patch will help.

- Or the variable $another_author gets clobbered in a future change between the two hunks of this patch, and the check in the second hunk would be broken.

Overall, I do not think any of the above breakage is so likely to happen, and that is why I am happy with the tests as posted. The necessity for a plain shell variable to retain its value for such a long haul across tests the patch below introduces may be making the test more brittle than safer.

So...

Quoted 31 lines
t/t3403-rebase-skip.sh +4 -3
index e26762d0b2..c90e32817f 100755
@@ -34,9 +34,10 @@ test_expect_success setup '
        git tag reverted-goodbye &&
        git checkout goodbye &&
        test_tick &&
-       GIT_AUTHOR_NAME="Another Author" \
-               GIT_AUTHOR_EMAIL="[email protected]" \
-               git commit --amend --no-edit -m amended-goodbye &&
+       another_author="Another Author <[email protected]>" &&
+       git commit --amend --no-edit -m amended-goodbye \
+               --author="$another_author" --date="$GIT_AUTHOR_DATE" &&
+       another_author="$another_author $GIT_AUTHOR_DATE" &&
        test_tick &&
        git tag amended-goodbye &&
 @@ -110,8 +111,10 @@ test_expect_success 'correct authorship when

committing empty pick' '

        test_must_fail git rebase -i --onto goodbye \
                amended-goodbye^ amended-goodbye &&
        git commit --allow-empty &&
t/t3403-rebase-skip.sh +4 -2
-       git log --pretty=format:"%an <%ae>%n%ad%B" -1 amended-goodbye >expect &&
-       git log --pretty=format:"%an <%ae>%n%ad%B" -1 HEAD >actual &&
+       git log --pretty=format:"$another_author%n%B" -1 amended-goodbye \
+                >expect &&
+       git log --date=raw --pretty=format:"%an <%ae> %ad%n%B" -1 HEAD \
+               >actual &&
        test_cmp expect actual
 '
#9 lore raw

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

Quoted 31 lines · Junio C Hamano
Quoted 2 lines · Phillip Wood

I wonder if it would be better to hard code the author in this test rather than rather than relying on git log like this

To be quite honest, I am pretty happy with the tests as posted, and the choice among various ways we are discussing depends on what the likely mode of breakage we expect.

The breakage we are expecting to catch in the second hunk of your patch is somehow "rebase -i" fails to keep the authorship of amended-goodbye and ends up making the commit at HEAD under different authorship.

- A likely source of a different authorship information that would be recorded, when such a bug gets introduced, is from the environment (i.e. GIT_AUTHOR_NAME etc. that test-lib.sh sets up). This can happen by a new bug in the test added before the second hunk of your patch we see below, and with or without this patch, such a bug in the test will not be caught.

- Or amended-goodbye may by a test bug have been recorded under a wrong authorship information to begin with, and if it were done as the default author we use in our tests (i.e. the bug you fixed in your patch that started this thread). If we reintroduced such a bug, the second hunk of this patch will help.

- Or the variable $another_author gets clobbered in a future change between the two hunks of this patch, and the check in the second hunk would be broken.

Quoted 7 lines

Overall, I do not think any of the above breakage is so likely to happen, and that is why I am happy with the tests as posted. The necessity for a plain shell variable to retain its value for such a long haul across tests the patch below introduces may be making the test more brittle than safer.

So...

Okay lets leave the test as it is then

Thanks

Phillip

author
#10 lore raw

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

In reply to Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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 <[email protected]>
---
 t/t3403-rebase-skip.sh     | 2 +-
 t/t3418-rebase-continue.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
t/t3403-rebase-skip.sh +1 -1
index 6365c5af2f7..a44e68d0ffb 100755
@@ -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' '
t/t3418-rebase-continue.sh +2 -2
index f4c2ee02bc9..e4cb8be0418 100755
@@ -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
Signature · 1 line
gitgitgadget
#11 lore raw

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

Quoted 22 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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 <[email protected]>
---
 t/t3403-rebase-skip.sh     | 2 +-
 t/t3418-rebase-continue.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
t/t3403-rebase-skip.sh +0 -0
index 6365c5af2f7..a44e68d0ffb 100755

t/t3403-rebase-skip.sh test_expect_success setup ' · line 55

Quoted 5 lines
t/t3403-rebase-skip.sh +1 -1
@@ -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.

t/t3418-rebase-continue.sh test_expect_success 'non-interactive rebase --continue works with touched file' · line 39

Quoted 13 lines
t/t3403-rebase-skip.sh +0 -0
 '
 
 test_expect_success 'rebase --skip can not be used with other options' '
t/t3418-rebase-continue.sh +1 -1
index f4c2ee02bc9..e4cb8be0418 100755
@@ -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.

t/t3418-rebase-continue.sh test_rerere_autoupdate () { · line 257

Quoted 9 lines
t/t3418-rebase-continue.sh +1 -1
        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 5 lines
t/t3418-rebase-continue.sh +0 -1
 test_rerere_autoupdate -m
 GIT_SEQUENCE_EDITOR=: && export GIT_SEQUENCE_EDITOR
 test_rerere_autoupdate -i
--

gitgitgadget

#12 lore raw

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

Quoted 10 lines · Elijah Newren

On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget

Quoted 9 lines

test_expect_success 'rebase --skip can not be used with other options' '

t/t3418-rebase-continue.sh +0 -0
index f4c2ee02bc9..e4cb8be0418 100755
@@ -36,7 +36,7 @@ test_expect_success 'non-interactive rebase --continue works with touched file'
         git reset --hard &&
         git checkout main &&
Quoted 7 lines

t/t3418-rebase-continue.sh test_expect_success 'non-interactive rebase --continue works with touched file' · line 39

Quoted 2 lines
t/t3418-rebase-continue.sh +1 -1
-       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

Quoted 11 lines

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 18 lines

t/t3418-rebase-continue.sh test_rerere_autoupdate () { · line 257

Quoted 10 lines
t/t3418-rebase-continue.sh +1 -1
         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 5 lines
t/t3418-rebase-continue.sh +0 -1
  test_rerere_autoupdate -m
  GIT_SEQUENCE_EDITOR=: && export GIT_SEQUENCE_EDITOR
  test_rerere_autoupdate -i
--

gitgitgadget

author
#13 lore raw

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

In reply to Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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 <[email protected]>
Signed-off-by Phillip Wood <[email protected]>
---
 sequencer.c                |  3 +++
 t/t3403-rebase-skip.sh     |  8 ++++++++
 t/t3418-rebase-continue.sh | 10 ++++++++++
 3 files changed, 21 insertions(+)
sequencer.c +3 -0
index 7f07cd00f3f..52c7b461179 100644
@@ -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;
 	}
t/t3403-rebase-skip.sh +8 -0
index a44e68d0ffb..f6e48644978 100755
@@ -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 \
t/t3418-rebase-continue.sh +10 -0
index e4cb8be0418..f23996c9be0 100755
@@ -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 &&
Signature · 1 line
gitgitgadget
#14 lore raw
In reply to Phillip Wood via GitGitGadget
Quoted 3 lines · Phillip Wood via GitGitGadget

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/

Quoted 3 lines

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.

author
#15 lore raw

v2 cover rebase --continue: remove .git/MERGE_MSG

In reply to Phillip Wood via GitGitGadget

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 [email protected] cc: Elijah Newren [email protected]
cc Johannes Schindelin [email protected]

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 <[email protected]>

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 <[email protected]>

@@ 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 &&
Signature · 1 line
gitgitgadget
author
#16 lore raw

PATCH v2 1/3 t3403: fix commit authorship

From: Phillip Wood <[email protected]>

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 <[email protected]>
---
 t/t3403-rebase-skip.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
t/t3403-rebase-skip.sh +2 -1
index e26762d0b29..6365c5af2f7 100755
@@ -36,7 +36,8 @@ test_expect_success setup '
 	test_tick &&
 	GIT_AUTHOR_NAME="Another Author" \
 		GIT_AUTHOR_EMAIL="[email protected]" \
-		git commit --amend --no-edit -m amended-goodbye &&
+		git commit --amend --no-edit -m amended-goodbye \
+			--reset-author &&
 	test_tick &&
 	git tag amended-goodbye &&
Signature · 1 line
gitgitgadget
author
#17 lore raw

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

In reply to Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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 <[email protected]>
---
 t/t3403-rebase-skip.sh     | 2 +-
 t/t3418-rebase-continue.sh | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)
t/t3403-rebase-skip.sh +1 -1
index 6365c5af2f7..a44e68d0ffb 100755
@@ -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' '
t/t3418-rebase-continue.sh +4 -4
index f4c2ee02bc9..bda5e5db802 100755
@@ -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
Signature · 1 line
gitgitgadget
author
#18 lore raw

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

In reply to Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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 <[email protected]>
Signed-off-by Phillip Wood <[email protected]>
---
 sequencer.c                |  3 +++
 t/t3403-rebase-skip.sh     |  8 ++++++++
 t/t3418-rebase-continue.sh | 10 ++++++++++
 3 files changed, 21 insertions(+)
sequencer.c +3 -0
index 7f07cd00f3f..52c7b461179 100644
@@ -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;
 	}
t/t3403-rebase-skip.sh +8 -0
index a44e68d0ffb..f6e48644978 100755
@@ -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 \
t/t3418-rebase-continue.sh +10 -0
index bda5e5db802..738fbae9b29 100755
@@ -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 &&
Signature · 1 line
gitgitgadget
#19 lore raw
Quoted 7 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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

#20 lore raw
Quoted 4 lines · Junio C Hamano
Quoted 2 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

Quoted 8 lines
Quoted 5 lines

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

Quoted 13 lines

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

#21 lore raw
In reply to Phillip Wood via GitGitGadget
Quoted 108 lines · Phillip Wood via GitGitGadget

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 [email protected] cc: Elijah Newren [email protected]
cc Johannes Schindelin [email protected]

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 <[email protected]>

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 <[email protected]>

@@ 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 &&
Quoted 2 lines

--
gitgitgadget

Thanks, this version looks good to me.

#22 lore raw
Quoted 1 line · Elijah Newren

Thanks, this version looks good to me.

Thanks, both. Elijah, can I take that as your Reviewed-by?

Reviewed-by
#23 lore raw
Quoted 6 lines · Junio C Hamano
Quoted 1 line · Elijah Newren

Thanks, this version looks good to me.

Thanks, both. Elijah, can I take that as your Reviewed-by?

I typically do a bit more checking when adding a Reviewed-by and had just a short time available yesterday. But I've done the extra checking now, so yes:

Reviewed-by Elijah Newren <[email protected]>
#24 lore raw
Quoted 6 lines · Elijah Newren
Quoted 5 lines · Junio C Hamano
Quoted 1 line · Elijah Newren

Thanks, this version looks good to me.

Quoted 7 lines
Quoted 1 line

Thanks, both. Elijah, can I take that as your Reviewed-by?

I typically do a bit more checking when adding a Reviewed-by and had just a short time available yesterday. But I've done the extra checking now, so yes:

Reviewed-by Elijah Newren <[email protected]>

Thanks