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

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

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

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 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

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
---
 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

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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

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

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

[Elijah Newren]

On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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 <phillip.wood@dunelm.org.uk>
> ---
>  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.

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

>         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

[Elijah Newren]

On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Setting GIT_AUTHOR_* when committing with --amend will only change the
> author if we also pass --reset-author
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  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

[Elijah Newren]

On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> 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 1/3] t3403: fix commit authorship

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Setting GIT_AUTHOR_* when committing with --amend will only change the
>> author if we also pass --reset-author
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>  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?

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.

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

[Phillip Wood]

On 10/08/2021 17:58, Elijah Newren wrote:
> On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget
>>   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

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

[Phillip Wood]

On 10/08/2021 19:43, Junio C Hamano wrote:
> Elijah Newren <newren@gmail.com> writes:
> 
>> On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget
>> <gitgitgadget@gmail.com> wrote:
>>>
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> Setting GIT_AUTHOR_* when committing with --amend will only change the
>>> author if we also pass --reset-author
>>>
>>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>>> ---
>>>   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?
> 
> 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

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

[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 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 <phillip.wood@dunelm.org.uk>
      
 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 <phillip.wood@dunelm.org.uk>
      
     @@ 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

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
---
 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

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
---
 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

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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

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

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

[Elijah Newren]

On Thu, Aug 12, 2021 at 6:46 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> 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 <phillip.wood@dunelm.org.uk>
>
>  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 <phillip.wood@dunelm.org.uk>
>
>      @@ 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.

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> Thanks, this version looks good to me.

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

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

[Elijah Newren]

On Fri, Aug 13, 2021 at 8:31 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > 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 <newren@gmail.com>

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

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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


[Footnote]

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

Nuking the difference the working tree files and the index has
relative to HEAD and telling the machinery to continue gives the
signal that the "conflict resolution" happened to have resulted in
an empty change, which should yield the same resulting history as
"git rebase --skip" would, because the resulting empty change should
be dropped (unless --empty=keep is in effect, that is).

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

[Phillip Wood]

On 14/08/2021 00:01, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> If the user skips the final commit by removing all the changes from
>> the index and worktree with 'git restore' (or read-tree) and then runs
>> 'git rebase --continue' .git/MERGE_MSG is left behind. This will seed
>> the commit message the next time the user commits which is not what we
>> want to happen.
> 
> I just remembered that "git rebase --skip" option exists.  Would it
> have the same issue if used at the last step?

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

Best Wishes

Phillip

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

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

[Phillip Wood]

On 13/08/2021 18:21, Elijah Newren wrote:
> On Fri, Aug 13, 2021 at 8:31 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Elijah Newren <newren@gmail.com> writes:
>>
>>> 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 <newren@gmail.com>

Thanks

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

[Johannes Schindelin]

Hi Phillip,

On Thu, 12 Aug 2021, Phillip Wood wrote:

> On 10/08/2021 19:43, Junio C Hamano wrote:
> > Elijah Newren <newren@gmail.com> writes:
> >
> > > On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget
> > > <gitgitgadget@gmail.com> wrote:
> > > >
> > > > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> > > >
> > > > Setting GIT_AUTHOR_* when committing with --amend will only change the
> > > > author if we also pass --reset-author
> > > >
> > > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> > > > ---
> > > >   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?
> >
> > 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

Maybe you could even add a `test another.author@example.com = $(git show
-s --format=%ae HEAD)`?

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> > 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
>
> Maybe you could even add a `test another.author@example.com = $(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(+)

diff --git c/t/t3403-rebase-skip.sh w/t/t3403-rebase-skip.sh
index e26762d0b2..1405720767 100755
--- c/t/t3403-rebase-skip.sh
+++ w/t/t3403-rebase-skip.sh
@@ -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 <another.author@example.com>" >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" &&

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

[Phillip Wood]

On 15/08/2021 18:36, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>>>> 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
>>
>> Maybe you could even add a `test another.author@example.com = $(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

diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index e26762d0b2..c90e32817f 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -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="another.author@example.com" \
-               git commit --amend --no-edit -m amended-goodbye &&
+       another_author="Another Author <another.author@example.com>" &&
+       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
  '
  

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

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

> diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
> index e26762d0b2..c90e32817f 100755
> --- a/t/t3403-rebase-skip.sh
> +++ b/t/t3403-rebase-skip.sh
> @@ -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="another.author@example.com" \
> -               git commit --amend --no-edit -m amended-goodbye &&
> +       another_author="Another Author <another.author@example.com>" &&
> +       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
>  '
>  

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

[Phillip Wood]

On 16/08/2021 17:36, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> 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...

Okay lets leave the test as it is then

Thanks

Phillip