rebase --continue: remove .git/MERGE_MSG

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/t3418-rebase-continue.sh +0 -1

 test_rerere_autoupdate -m
 GIT_SEQUENCE_EDITOR=: && export GIT_SEQUENCE_EDITOR
 test_rerere_autoupdate -i
--

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

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.

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

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?

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

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.

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

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

--
gitgitgadget

Thanks, this version looks good to me.

Quoted 1 line · Elijah Newren

Thanks, this version looks good to me.

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

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.

Quoted 2 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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?

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

Quoted 5 lines · Junio C Hamano

Quoted 1 line · Elijah Newren

Thanks, this version looks good to me.

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

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 1 line

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

I'll expand the commit message to make that clear

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

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
     '

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 +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
 '

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