{"thread":{"id":"56246","subject":"[PATCH 0/3] rebase --continue: remove .git/MERGE_MSG","startedAt":"2021-08-10T09:32:04Z","lastAt":"2021-08-17T10:06:16Z","messageCount":25,"participants":["Phillip Wood via GitGitGadget","Elijah Newren","Junio C Hamano","Phillip Wood","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"432356","messageId":"pull.1013.git.1628587917.gitgitgadget@gmail.com","threadId":"56246","inReplyTo":null,"subject":"[PATCH 0/3] rebase --continue: remove .git/MERGE_MSG","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-10T09:31:54Z","receivedAt":"2021-08-10T09:32:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"If the user removes all the changes from the worktree without running git\nreset then it rebase --continue leaves behind .git/MERGE_MSG which will then\nseed the message of the next commit. While working on this I noticed a\ncouple of fixups for the test files I was adding to, I've cc'd Elijah in\ncase my reasoning for patch 2 is off. Thanks to Victor for the bug report.\n\nPhillip Wood (3):\n  t3403: fix commit authorship\n  rebase --apply: restore some tests\n  rebase --continue: remove .git/MERGE_MSG\n\n sequencer.c                |  3 +++\n t/t3403-rebase-skip.sh     | 13 +++++++++++--\n t/t3418-rebase-continue.sh | 14 ++++++++++++--\n 3 files changed, 26 insertions(+), 4 deletions(-)\n\n\nbase-commit: 2d755dfac9aadab25c3e025b849252b8c0a61465\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1013%2Fphillipwood%2Fwip%2Frebase-remove-merge-msg-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1013/phillipwood/wip/rebase-remove-merge-msg-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1013\n-- \ngitgitgadget\n"},{"id":"432357","messageId":"7559781ca92096f89a6dcbfeeaa4d2b4d7b98094.1628587917.git.gitgitgadget@gmail.com","threadId":"56246","inReplyTo":"pull.1013.git.1628587917.gitgitgadget@gmail.com","subject":"[PATCH 1/3] t3403: fix commit authorship","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-10T09:31:55Z","receivedAt":"2021-08-10T09:32:05Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nSetting GIT_AUTHOR_* when committing with --amend will only change the\nauthor if we also pass --reset-author\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n t/t3403-rebase-skip.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\nindex e26762d0b29..6365c5af2f7 100755\n--- a/t/t3403-rebase-skip.sh\n+++ b/t/t3403-rebase-skip.sh\n@@ -36,7 +36,8 @@ test_expect_success setup '\n \ttest_tick &&\n \tGIT_AUTHOR_NAME=\"Another Author\" \\\n \t\tGIT_AUTHOR_EMAIL=\"another.author@example.com\" \\\n-\t\tgit commit --amend --no-edit -m amended-goodbye &&\n+\t\tgit commit --amend --no-edit -m amended-goodbye \\\n+\t\t\t--reset-author &&\n \ttest_tick &&\n \tgit tag amended-goodbye &&\n \n-- \ngitgitgadget\n\n"},{"id":"432358","messageId":"f7722dab21dafb73e64754b1286913eaa826f865.1628587917.git.gitgitgadget@gmail.com","threadId":"56246","inReplyTo":"pull.1013.git.1628587917.gitgitgadget@gmail.com","subject":"[PATCH 2/3] rebase --apply: restore some tests","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-10T09:31:56Z","receivedAt":"2021-08-10T09:32:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\n980b482d28 (\"rebase tests: mark tests specific to the am-backend with\n--am\", 2020-02-15) sought to prepare tests testing the \"apply\" backend\nin preparation for 2ac0d6273f (\"rebase: change the default backend\nfrom \"am\" to \"merge\"\", 2020-02-15). However some tests seem to have\nbeen missed leading to us testing the \"merge\" backend twice. This\npatch fixes some cases that I noticed while adding tests to these\nfiles, I have not audited all the other rebase test files.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n t/t3403-rebase-skip.sh     | 2 +-\n t/t3418-rebase-continue.sh | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\nindex 6365c5af2f7..a44e68d0ffb 100755\n--- a/t/t3403-rebase-skip.sh\n+++ b/t/t3403-rebase-skip.sh\n@@ -52,7 +52,7 @@ test_expect_success setup '\n \t'\n \n test_expect_success 'rebase with git am -3 (default)' '\n-\ttest_must_fail git rebase main\n+\ttest_must_fail git rebase --apply main\n '\n \n test_expect_success 'rebase --skip can not be used with other options' '\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex f4c2ee02bc9..e4cb8be0418 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -36,7 +36,7 @@ test_expect_success 'non-interactive rebase --continue works with touched file'\n \tgit reset --hard &&\n \tgit checkout main &&\n \n-\ttest_must_fail git rebase --onto main main topic &&\n+\ttest_must_fail git rebase --apply --onto main main topic &&\n \techo \"Resolved\" >F2 &&\n \tgit add F2 &&\n \ttest-tool chmtime =-60 F1 &&\n@@ -254,7 +254,7 @@ test_rerere_autoupdate () {\n \t'\n }\n \n-test_rerere_autoupdate\n+test_rerere_autoupdate --apply\n test_rerere_autoupdate -m\n GIT_SEQUENCE_EDITOR=: && export GIT_SEQUENCE_EDITOR\n test_rerere_autoupdate -i\n-- \ngitgitgadget\n\n"},{"id":"432359","messageId":"6a63b657f1313ea6d4141c37a7c24bca691ce107.1628587917.git.gitgitgadget@gmail.com","threadId":"56246","inReplyTo":"pull.1013.git.1628587917.gitgitgadget@gmail.com","subject":"[PATCH 3/3] rebase --continue: remove .git/MERGE_MSG","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-10T09:31:57Z","receivedAt":"2021-08-10T09:32:21Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIf the user skips the final commit by removing all the changes from\nthe index and worktree with 'git restore' (or read-tree) and then runs\n'git rebase --continue' .git/MERGE_MSG is left behind. This will seed\nthe commit message the next time the user commits which is not what we\nwant to happen.\n\nReported-by: Victor Gambier <vgambier@excilys.com>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c                |  3 +++\n t/t3403-rebase-skip.sh     |  8 ++++++++\n t/t3418-rebase-continue.sh | 10 ++++++++++\n 3 files changed, 21 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7f07cd00f3f..52c7b461179 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4716,6 +4716,9 @@ static int commit_staged_changes(struct repository *r,\n \t\t    refs_delete_ref(get_main_ref_store(r), \"\",\n \t\t\t\t    \"CHERRY_PICK_HEAD\", NULL, 0))\n \t\t\treturn error(_(\"could not remove CHERRY_PICK_HEAD\"));\n+\t\tif (unlink(git_path_merge_msg(r)) && errno != ENOENT)\n+\t\t\treturn error_errno(_(\"could not remove '%s'\"),\n+\t\t\t\t\t   git_path_merge_msg(r));\n \t\tif (!final_fixup)\n \t\t\treturn 0;\n \t}\ndiff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\nindex a44e68d0ffb..f6e48644978 100755\n--- a/t/t3403-rebase-skip.sh\n+++ b/t/t3403-rebase-skip.sh\n@@ -20,6 +20,7 @@ test_expect_success setup '\n \tgit add hello &&\n \tgit commit -m \"hello\" &&\n \tgit branch skip-reference &&\n+\tgit tag hello &&\n \n \techo world >> hello &&\n \tgit commit -a -m \"hello world\" &&\n@@ -96,6 +97,13 @@ test_expect_success 'moved back to branch correctly' '\n \n test_debug 'gitk --all & sleep 1'\n \n+test_expect_success 'skipping final pick removes .git/MERGE_MSG' '\n+\ttest_must_fail git rebase --onto hello reverted-goodbye^ \\\n+\t\treverted-goodbye &&\n+\tgit rebase --skip &&\n+\ttest_path_is_missing .git/MERGE_MSG\n+'\n+\n test_expect_success 'correct advice upon picking empty commit' '\n \ttest_when_finished \"git rebase --abort\" &&\n \ttest_must_fail git rebase -i --onto goodbye \\\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex e4cb8be0418..f23996c9be0 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -31,6 +31,16 @@ test_expect_success 'interactive rebase --continue works with touched file' '\n \tgit rebase --continue\n '\n \n+test_expect_success 'rebase --continue removes .git/MERGE_MSG' '\n+\tgit checkout -f --detach topic &&\n+\n+\ttest_must_fail git rebase --onto main HEAD^ &&\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_path_is_file .git/MERGE_MSG &&\n+\tgit rebase --continue &&\n+\ttest_path_is_missing .git/MERGE_MSG\n+'\n+\n test_expect_success 'non-interactive rebase --continue works with touched file' '\n \trm -fr .git/rebase-* &&\n \tgit reset --hard &&\n-- \ngitgitgadget\n"},{"id":"432384","messageId":"CABPp-BFJD77wkroL5NFsH++a-yn0h6s6KM=Xfr2Ad_PfPQrUuw@mail.gmail.com","threadId":"56246","inReplyTo":"f7722dab21dafb73e64754b1286913eaa826f865.1628587917.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] rebase --apply: restore some tests","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-08-10T16:58:09Z","receivedAt":"2021-08-10T16:58:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> 980b482d28 (\"rebase tests: mark tests specific to the am-backend with\n> --am\", 2020-02-15) sought to prepare tests testing the \"apply\" backend\n> in preparation for 2ac0d6273f (\"rebase: change the default backend\n> from \"am\" to \"merge\"\", 2020-02-15). However some tests seem to have\n> been missed leading to us testing the \"merge\" backend twice. This\n> patch fixes some cases that I noticed while adding tests to these\n> files, I have not audited all the other rebase test files.\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  t/t3403-rebase-skip.sh     | 2 +-\n>  t/t3418-rebase-continue.sh | 4 ++--\n>  2 files changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\n> index 6365c5af2f7..a44e68d0ffb 100755\n> --- a/t/t3403-rebase-skip.sh\n> +++ b/t/t3403-rebase-skip.sh\n> @@ -52,7 +52,7 @@ test_expect_success setup '\n>         '\n>\n>  test_expect_success 'rebase with git am -3 (default)' '\n> -       test_must_fail git rebase main\n> +       test_must_fail git rebase --apply main\n\nLooks good.\n\n>  '\n>\n>  test_expect_success 'rebase --skip can not be used with other options' '\n> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\n> index f4c2ee02bc9..e4cb8be0418 100755\n> --- a/t/t3418-rebase-continue.sh\n> +++ b/t/t3418-rebase-continue.sh\n> @@ -36,7 +36,7 @@ test_expect_success 'non-interactive rebase --continue works with touched file'\n>         git reset --hard &&\n>         git checkout main &&\n>\n> -       test_must_fail git rebase --onto main main topic &&\n> +       test_must_fail git rebase --apply --onto main main topic &&\n\nI think the point here is that you're noticing that \"git rebase -i\"\nand \"git rebase\" are both built on the same rebase backend (the merge\none) and thus that testing interactive vs. non-interactive isn't much\nof a test.  The real test we are interested in is merge-backend vs.\napply-backend.  Your code change here is the necessary one to do that,\nbut it least the test descriptions still talking about interactive vs.\nnon-interactive even though that's not what we're concentrating on\nanymore.\n\nI'd say we'd at least want to change the description for this test,\n\"non-interactive rebase ---continue works with...\" => \"rebase\n--continue with the apply backend works with...\", but that we'd\nprobably want to change the description of the test before it and\nmaybe even just use rebase --merge rather than rebase -i.\n\n>         echo \"Resolved\" >F2 &&\n>         git add F2 &&\n>         test-tool chmtime =-60 F1 &&\n> @@ -254,7 +254,7 @@ test_rerere_autoupdate () {\n>         '\n>  }\n>\n> -test_rerere_autoupdate\n> +test_rerere_autoupdate --apply\n\nLooks good.\n\n>  test_rerere_autoupdate -m\n>  GIT_SEQUENCE_EDITOR=: && export GIT_SEQUENCE_EDITOR\n>  test_rerere_autoupdate -i\n> --\n> gitgitgadget\n"},{"id":"432385","messageId":"CABPp-BFFTLgtrs_m9Gp7tSRwpBnAEoArhMZhhJcuzqNS_g8sUw@mail.gmail.com","threadId":"56246","inReplyTo":"7559781ca92096f89a6dcbfeeaa4d2b4d7b98094.1628587917.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] t3403: fix commit authorship","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-08-10T17:01:45Z","receivedAt":"2021-08-10T17:02:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> Setting GIT_AUTHOR_* when committing with --amend will only change the\n> author if we also pass --reset-author\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  t/t3403-rebase-skip.sh | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\n> index e26762d0b29..6365c5af2f7 100755\n> --- a/t/t3403-rebase-skip.sh\n> +++ b/t/t3403-rebase-skip.sh\n> @@ -36,7 +36,8 @@ test_expect_success setup '\n>         test_tick &&\n>         GIT_AUTHOR_NAME=\"Another Author\" \\\n>                 GIT_AUTHOR_EMAIL=\"another.author@example.com\" \\\n> -               git commit --amend --no-edit -m amended-goodbye &&\n> +               git commit --amend --no-edit -m amended-goodbye \\\n> +                       --reset-author &&\n\nMakes sense...but doesn't the fact that this test worked either way\nsuggest that the specifying of a special author name/email was totally\nsuperfluous and could just be removed?  If there really was a reason\nfor specifying a different name/email, then is the test faulty for not\nchecking for it somewhere?\n"},{"id":"432386","messageId":"CABPp-BHh5nEjhDFewCwJgNtTCPpODN7dAAKGHYz91PyFwT4rdg@mail.gmail.com","threadId":"56246","inReplyTo":"pull.1013.git.1628587917.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] rebase --continue: remove .git/MERGE_MSG","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-08-10T17:03:38Z","receivedAt":"2021-08-10T17:03:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> If the user removes all the changes from the worktree without running git\n> reset then it rebase --continue leaves behind .git/MERGE_MSG which will then\n\ns/then it/then/\n\n> seed the message of the next commit. While working on this I noticed a\n> couple of fixups for the test files I was adding to, I've cc'd Elijah in\n> case my reasoning for patch 2 is off. Thanks to Victor for the bug report.\n\nI had a couple small comments on the first two patches.  Otherwise,\nthe whole series looks good to me.\n"},{"id":"432401","messageId":"xmqq5ywddtsn.fsf@gitster.g","threadId":"56246","inReplyTo":"CABPp-BFFTLgtrs_m9Gp7tSRwpBnAEoArhMZhhJcuzqNS_g8sUw@mail.gmail.com","subject":"Re: [PATCH 1/3] t3403: fix commit authorship","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-10T18:43:52Z","receivedAt":"2021-08-10T18:43:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>>\n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> Setting GIT_AUTHOR_* when committing with --amend will only change the\n>> author if we also pass --reset-author\n>>\n>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>> ---\n>>  t/t3403-rebase-skip.sh | 3 ++-\n>>  1 file changed, 2 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\n>> index e26762d0b29..6365c5af2f7 100755\n>> --- a/t/t3403-rebase-skip.sh\n>> +++ b/t/t3403-rebase-skip.sh\n>> @@ -36,7 +36,8 @@ test_expect_success setup '\n>>         test_tick &&\n>>         GIT_AUTHOR_NAME=\"Another Author\" \\\n>>                 GIT_AUTHOR_EMAIL=\"another.author@example.com\" \\\n>> -               git commit --amend --no-edit -m amended-goodbye &&\n>> +               git commit --amend --no-edit -m amended-goodbye \\\n>> +                       --reset-author &&\n>\n> Makes sense...but doesn't the fact that this test worked either way\n> suggest that the specifying of a special author name/email was totally\n> superfluous and could just be removed?  If there really was a reason\n> for specifying a different name/email, then is the test faulty for not\n> checking for it somewhere?\n\nGood point.  The commit tagged with amended-goodbye is later used in\nsome tests that ensure the author ident does not change across a\nrebase.  If this commit gets created without authorship customized\n(i.e. before Phillip's fix), we would not catch a possible breakage\nto make rebase discard the original authorship information.\n\nBut with this fix, we now can catch such a breakage.\n\nThanks.\n"},{"id":"432575","messageId":"dc7b7943-a0c2-241f-63ed-417378b30224@gmail.com","threadId":"56246","inReplyTo":"CABPp-BFJD77wkroL5NFsH++a-yn0h6s6KM=Xfr2Ad_PfPQrUuw@mail.gmail.com","subject":"Re: [PATCH 2/3] rebase --apply: restore some tests","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-12T10:03:55Z","receivedAt":"2021-08-12T10:04:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 10/08/2021 17:58, Elijah Newren wrote:\n> On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget\n>>   test_expect_success 'rebase --skip can not be used with other options' '\n>> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\n>> index f4c2ee02bc9..e4cb8be0418 100755\n>> --- a/t/t3418-rebase-continue.sh\n>> +++ b/t/t3418-rebase-continue.sh\n>> @@ -36,7 +36,7 @@ test_expect_success 'non-interactive rebase --continue works with touched file'\n>>          git reset --hard &&\n>>          git checkout main &&\n>>\n>> -       test_must_fail git rebase --onto main main topic &&\n>> +       test_must_fail git rebase --apply --onto main main topic &&\n> \n> I think the point here is that you're noticing that \"git rebase -i\"\n> and \"git rebase\" are both built on the same rebase backend (the merge\n> one) and thus that testing interactive vs. non-interactive isn't much\n> of a test.\n\nExactly\n\n>  The real test we are interested in is merge-backend vs.\n> apply-backend.  Your code change here is the necessary one to do that,\n> but it least the test descriptions still talking about interactive vs.\n> non-interactive even though that's not what we're concentrating on\n> anymore.\n> \n> I'd say we'd at least want to change the description for this test,\n> \"non-interactive rebase ---continue works with...\" => \"rebase\n> --continue with the apply backend works with...\", but that we'd\n> probably want to change the description of the test before it and\n> maybe even just use rebase --merge rather than rebase -i.\n\nRewording the test descriptions is a good suggestion\n\nThanks\n\nPhillip\n\n> \n>>          echo \"Resolved\" >F2 &&\n>>          git add F2 &&\n>>          test-tool chmtime =-60 F1 &&\n>> @@ -254,7 +254,7 @@ test_rerere_autoupdate () {\n>>          '\n>>   }\n>>\n>> -test_rerere_autoupdate\n>> +test_rerere_autoupdate --apply\n> \n> Looks good.\n> \n>>   test_rerere_autoupdate -m\n>>   GIT_SEQUENCE_EDITOR=: && export GIT_SEQUENCE_EDITOR\n>>   test_rerere_autoupdate -i\n>> --\n>> gitgitgadget\n\n"},{"id":"432576","messageId":"3b438e42-488c-e4ab-d1fe-3d0992ef0619@gmail.com","threadId":"56246","inReplyTo":"xmqq5ywddtsn.fsf@gitster.g","subject":"Re: [PATCH 1/3] t3403: fix commit authorship","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-12T10:04:50Z","receivedAt":"2021-08-12T10:04:58Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 10/08/2021 19:43, Junio C Hamano wrote:\n> Elijah Newren <newren@gmail.com> writes:\n> \n>> On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget\n>> <gitgitgadget@gmail.com> wrote:\n>>>\n>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>>\n>>> Setting GIT_AUTHOR_* when committing with --amend will only change the\n>>> author if we also pass --reset-author\n>>>\n>>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>> ---\n>>>   t/t3403-rebase-skip.sh | 3 ++-\n>>>   1 file changed, 2 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\n>>> index e26762d0b29..6365c5af2f7 100755\n>>> --- a/t/t3403-rebase-skip.sh\n>>> +++ b/t/t3403-rebase-skip.sh\n>>> @@ -36,7 +36,8 @@ test_expect_success setup '\n>>>          test_tick &&\n>>>          GIT_AUTHOR_NAME=\"Another Author\" \\\n>>>                  GIT_AUTHOR_EMAIL=\"another.author@example.com\" \\\n>>> -               git commit --amend --no-edit -m amended-goodbye &&\n>>> +               git commit --amend --no-edit -m amended-goodbye \\\n>>> +                       --reset-author &&\n>>\n>> Makes sense...but doesn't the fact that this test worked either way\n>> suggest that the specifying of a special author name/email was totally\n>> superfluous and could just be removed?  If there really was a reason\n>> for specifying a different name/email, then is the test faulty for not\n>> checking for it somewhere?\n> \n> Good point.  The commit tagged with amended-goodbye is later used in\n> some tests that ensure the author ident does not change across a\n> rebase.  If this commit gets created without authorship customized\n> (i.e. before Phillip's fix), we would not catch a possible breakage\n> to make rebase discard the original authorship information.\n> \n> But with this fix, we now can catch such a breakage.\n\nI'll expand the commit message to make that clear\n\nThanks\n\nPhillip\n"},{"id":"432579","messageId":"pull.1013.v2.git.1628775729.gitgitgadget@gmail.com","threadId":"56246","inReplyTo":"pull.1013.git.1628587917.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] rebase --continue: remove .git/MERGE_MSG","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-12T13:42:06Z","receivedAt":"2021-08-12T13:42:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Thanks for the comments on V1. I have expanded the commit message for the\nfirst patch to give a bit more context and reworded a couple of test\ndescriptions in patch 2 as suggested by Elijah. Patch 3 is unchanged.\n\nV1 Cover Letter:\n\nIf the user removes all the changes from the worktree without running git\nreset then rebase --continue leaves behind .git/MERGE_MSG which will then\nseed the message of the next commit. While working on this I noticed a\ncouple of fixups for the test files I was adding to, I've cc'd Elijah in\ncase my reasoning for patch 2 is off. Thanks to Victor for the bug report.\n\ncc: Victor Gambier vgambier@excilys.com cc: Elijah Newren newren@gmail.com\ncc: Johannes Schindelin Johannes.Schindelin@gmx.de\n\nPhillip Wood (3):\n  t3403: fix commit authorship\n  rebase --apply: restore some tests\n  rebase --continue: remove .git/MERGE_MSG\n\n sequencer.c                |  3 +++\n t/t3403-rebase-skip.sh     | 13 +++++++++++--\n t/t3418-rebase-continue.sh | 18 ++++++++++++++----\n 3 files changed, 28 insertions(+), 6 deletions(-)\n\n\nbase-commit: 2d755dfac9aadab25c3e025b849252b8c0a61465\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1013%2Fphillipwood%2Fwip%2Frebase-remove-merge-msg-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1013/phillipwood/wip/rebase-remove-merge-msg-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1013\n\nRange-diff vs v1:\n\n 1:  7559781ca92 ! 1:  8755dfa9d04 t3403: fix commit authorship\n     @@ Commit message\n          t3403: fix commit authorship\n      \n          Setting GIT_AUTHOR_* when committing with --amend will only change the\n     -    author if we also pass --reset-author\n     +    author if we also pass --reset-author.  This commit is used in some\n     +    tests that ensure the author ident does not change when rebasing.\n     +    Creating this commit without changing the authorship meant that the\n     +    test would not catch regressions that caused rebase to discard the\n     +    original authorship information.\n      \n          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n      \n 2:  f7722dab21d ! 2:  84fe5823b4f rebase --apply: restore some tests\n     @@ Commit message\n          from \"am\" to \"merge\"\", 2020-02-15). However some tests seem to have\n          been missed leading to us testing the \"merge\" backend twice. This\n          patch fixes some cases that I noticed while adding tests to these\n     -    files, I have not audited all the other rebase test files.\n     +    files, I have not audited all the other rebase test files. I've\n     +    reworded a couple of the test descriptions to make it clear which\n     +    backend they are testing.\n      \n          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n      \n     @@ t/t3403-rebase-skip.sh: test_expect_success setup '\n       test_expect_success 'rebase --skip can not be used with other options' '\n      \n       ## t/t3418-rebase-continue.sh ##\n     -@@ t/t3418-rebase-continue.sh: test_expect_success 'non-interactive rebase --continue works with touched file'\n     +@@ t/t3418-rebase-continue.sh: test_expect_success 'setup' '\n     + \tgit checkout main\n     + '\n     + \n     +-test_expect_success 'interactive rebase --continue works with touched file' '\n     ++test_expect_success 'merge based rebase --continue with works with touched file' '\n     + \trm -fr .git/rebase-* &&\n     + \tgit reset --hard &&\n     + \tgit checkout main &&\n     +@@ t/t3418-rebase-continue.sh: test_expect_success 'interactive rebase --continue works with touched file' '\n     + \tgit rebase --continue\n     + '\n     + \n     +-test_expect_success 'non-interactive rebase --continue works with touched file' '\n     ++test_expect_success 'apply based rebase --continue works with touched file' '\n     + \trm -fr .git/rebase-* &&\n       \tgit reset --hard &&\n       \tgit checkout main &&\n       \n 3:  6a63b657f13 ! 3:  028c9dfc460 rebase --continue: remove .git/MERGE_MSG\n     @@ t/t3403-rebase-skip.sh: test_expect_success 'moved back to branch correctly' '\n       \ttest_must_fail git rebase -i --onto goodbye \\\n      \n       ## t/t3418-rebase-continue.sh ##\n     -@@ t/t3418-rebase-continue.sh: test_expect_success 'interactive rebase --continue works with touched file' '\n     +@@ t/t3418-rebase-continue.sh: test_expect_success 'merge based rebase --continue with works with touched file'\n       \tgit rebase --continue\n       '\n       \n     -+test_expect_success 'rebase --continue removes .git/MERGE_MSG' '\n     ++test_expect_success 'merge based rebase --continue removes .git/MERGE_MSG' '\n      +\tgit checkout -f --detach topic &&\n      +\n      +\ttest_must_fail git rebase --onto main HEAD^ &&\n     @@ t/t3418-rebase-continue.sh: test_expect_success 'interactive rebase --continue w\n      +\ttest_path_is_missing .git/MERGE_MSG\n      +'\n      +\n     - test_expect_success 'non-interactive rebase --continue works with touched file' '\n     + test_expect_success 'apply based rebase --continue works with touched file' '\n       \trm -fr .git/rebase-* &&\n       \tgit reset --hard &&\n\n-- \ngitgitgadget\n"},{"id":"432580","messageId":"8755dfa9d0415c3addbae7acf56e354f55a667ff.1628775729.git.gitgitgadget@gmail.com","threadId":"56246","inReplyTo":"pull.1013.v2.git.1628775729.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] t3403: fix commit authorship","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-12T13:42:07Z","receivedAt":"2021-08-12T13:42:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nSetting GIT_AUTHOR_* when committing with --amend will only change the\nauthor if we also pass --reset-author.  This commit is used in some\ntests that ensure the author ident does not change when rebasing.\nCreating this commit without changing the authorship meant that the\ntest would not catch regressions that caused rebase to discard the\noriginal authorship information.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n t/t3403-rebase-skip.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\nindex e26762d0b29..6365c5af2f7 100755\n--- a/t/t3403-rebase-skip.sh\n+++ b/t/t3403-rebase-skip.sh\n@@ -36,7 +36,8 @@ test_expect_success setup '\n \ttest_tick &&\n \tGIT_AUTHOR_NAME=\"Another Author\" \\\n \t\tGIT_AUTHOR_EMAIL=\"another.author@example.com\" \\\n-\t\tgit commit --amend --no-edit -m amended-goodbye &&\n+\t\tgit commit --amend --no-edit -m amended-goodbye \\\n+\t\t\t--reset-author &&\n \ttest_tick &&\n \tgit tag amended-goodbye &&\n \n-- \ngitgitgadget\n\n"},{"id":"432581","messageId":"84fe5823b4fa2572c2e0080e746131a4ec148ad3.1628775729.git.gitgitgadget@gmail.com","threadId":"56246","inReplyTo":"pull.1013.v2.git.1628775729.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] rebase --apply: restore some tests","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-12T13:42:08Z","receivedAt":"2021-08-12T13:42:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\n980b482d28 (\"rebase tests: mark tests specific to the am-backend with\n--am\", 2020-02-15) sought to prepare tests testing the \"apply\" backend\nin preparation for 2ac0d6273f (\"rebase: change the default backend\nfrom \"am\" to \"merge\"\", 2020-02-15). However some tests seem to have\nbeen missed leading to us testing the \"merge\" backend twice. This\npatch fixes some cases that I noticed while adding tests to these\nfiles, I have not audited all the other rebase test files. I've\nreworded a couple of the test descriptions to make it clear which\nbackend they are testing.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n t/t3403-rebase-skip.sh     | 2 +-\n t/t3418-rebase-continue.sh | 8 ++++----\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\nindex 6365c5af2f7..a44e68d0ffb 100755\n--- a/t/t3403-rebase-skip.sh\n+++ b/t/t3403-rebase-skip.sh\n@@ -52,7 +52,7 @@ test_expect_success setup '\n \t'\n \n test_expect_success 'rebase with git am -3 (default)' '\n-\ttest_must_fail git rebase main\n+\ttest_must_fail git rebase --apply main\n '\n \n test_expect_success 'rebase --skip can not be used with other options' '\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex f4c2ee02bc9..bda5e5db802 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -21,7 +21,7 @@ test_expect_success 'setup' '\n \tgit checkout main\n '\n \n-test_expect_success 'interactive rebase --continue works with touched file' '\n+test_expect_success 'merge based rebase --continue with works with touched file' '\n \trm -fr .git/rebase-* &&\n \tgit reset --hard &&\n \tgit checkout main &&\n@@ -31,12 +31,12 @@ test_expect_success 'interactive rebase --continue works with touched file' '\n \tgit rebase --continue\n '\n \n-test_expect_success 'non-interactive rebase --continue works with touched file' '\n+test_expect_success 'apply based rebase --continue works with touched file' '\n \trm -fr .git/rebase-* &&\n \tgit reset --hard &&\n \tgit checkout main &&\n \n-\ttest_must_fail git rebase --onto main main topic &&\n+\ttest_must_fail git rebase --apply --onto main main topic &&\n \techo \"Resolved\" >F2 &&\n \tgit add F2 &&\n \ttest-tool chmtime =-60 F1 &&\n@@ -254,7 +254,7 @@ test_rerere_autoupdate () {\n \t'\n }\n \n-test_rerere_autoupdate\n+test_rerere_autoupdate --apply\n test_rerere_autoupdate -m\n GIT_SEQUENCE_EDITOR=: && export GIT_SEQUENCE_EDITOR\n test_rerere_autoupdate -i\n-- \ngitgitgadget\n\n"},{"id":"432582","messageId":"028c9dfc460b6c00bf481017a07a2a6d37780a76.1628775729.git.gitgitgadget@gmail.com","threadId":"56246","inReplyTo":"pull.1013.v2.git.1628775729.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] rebase --continue: remove .git/MERGE_MSG","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-12T13:42:09Z","receivedAt":"2021-08-12T13:42:18Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIf the user skips the final commit by removing all the changes from\nthe index and worktree with 'git restore' (or read-tree) and then runs\n'git rebase --continue' .git/MERGE_MSG is left behind. This will seed\nthe commit message the next time the user commits which is not what we\nwant to happen.\n\nReported-by: Victor Gambier <vgambier@excilys.com>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c                |  3 +++\n t/t3403-rebase-skip.sh     |  8 ++++++++\n t/t3418-rebase-continue.sh | 10 ++++++++++\n 3 files changed, 21 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7f07cd00f3f..52c7b461179 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4716,6 +4716,9 @@ static int commit_staged_changes(struct repository *r,\n \t\t    refs_delete_ref(get_main_ref_store(r), \"\",\n \t\t\t\t    \"CHERRY_PICK_HEAD\", NULL, 0))\n \t\t\treturn error(_(\"could not remove CHERRY_PICK_HEAD\"));\n+\t\tif (unlink(git_path_merge_msg(r)) && errno != ENOENT)\n+\t\t\treturn error_errno(_(\"could not remove '%s'\"),\n+\t\t\t\t\t   git_path_merge_msg(r));\n \t\tif (!final_fixup)\n \t\t\treturn 0;\n \t}\ndiff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\nindex a44e68d0ffb..f6e48644978 100755\n--- a/t/t3403-rebase-skip.sh\n+++ b/t/t3403-rebase-skip.sh\n@@ -20,6 +20,7 @@ test_expect_success setup '\n \tgit add hello &&\n \tgit commit -m \"hello\" &&\n \tgit branch skip-reference &&\n+\tgit tag hello &&\n \n \techo world >> hello &&\n \tgit commit -a -m \"hello world\" &&\n@@ -96,6 +97,13 @@ test_expect_success 'moved back to branch correctly' '\n \n test_debug 'gitk --all & sleep 1'\n \n+test_expect_success 'skipping final pick removes .git/MERGE_MSG' '\n+\ttest_must_fail git rebase --onto hello reverted-goodbye^ \\\n+\t\treverted-goodbye &&\n+\tgit rebase --skip &&\n+\ttest_path_is_missing .git/MERGE_MSG\n+'\n+\n test_expect_success 'correct advice upon picking empty commit' '\n \ttest_when_finished \"git rebase --abort\" &&\n \ttest_must_fail git rebase -i --onto goodbye \\\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex bda5e5db802..738fbae9b29 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -31,6 +31,16 @@ test_expect_success 'merge based rebase --continue with works with touched file'\n \tgit rebase --continue\n '\n \n+test_expect_success 'merge based rebase --continue removes .git/MERGE_MSG' '\n+\tgit checkout -f --detach topic &&\n+\n+\ttest_must_fail git rebase --onto main HEAD^ &&\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_path_is_file .git/MERGE_MSG &&\n+\tgit rebase --continue &&\n+\ttest_path_is_missing .git/MERGE_MSG\n+'\n+\n test_expect_success 'apply based rebase --continue works with touched file' '\n \trm -fr .git/rebase-* &&\n \tgit reset --hard &&\n-- \ngitgitgadget\n"},{"id":"432619","messageId":"CABPp-BGHmCCeY7j9hVDGZPuLU8gR-ka3iEzUjK0wvkSUSYdNxA@mail.gmail.com","threadId":"56246","inReplyTo":"pull.1013.v2.git.1628775729.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] rebase --continue: remove .git/MERGE_MSG","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-08-13T00:46:22Z","receivedAt":"2021-08-13T00:46:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Aug 12, 2021 at 6:46 AM Phillip Wood via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> Thanks for the comments on V1. I have expanded the commit message for the\n> first patch to give a bit more context and reworded a couple of test\n> descriptions in patch 2 as suggested by Elijah. Patch 3 is unchanged.\n>\n> V1 Cover Letter:\n>\n> If the user removes all the changes from the worktree without running git\n> reset then rebase --continue leaves behind .git/MERGE_MSG which will then\n> seed the message of the next commit. While working on this I noticed a\n> couple of fixups for the test files I was adding to, I've cc'd Elijah in\n> case my reasoning for patch 2 is off. Thanks to Victor for the bug report.\n>\n> cc: Victor Gambier vgambier@excilys.com cc: Elijah Newren newren@gmail.com\n> cc: Johannes Schindelin Johannes.Schindelin@gmx.de\n>\n> Phillip Wood (3):\n>   t3403: fix commit authorship\n>   rebase --apply: restore some tests\n>   rebase --continue: remove .git/MERGE_MSG\n>\n>  sequencer.c                |  3 +++\n>  t/t3403-rebase-skip.sh     | 13 +++++++++++--\n>  t/t3418-rebase-continue.sh | 18 ++++++++++++++----\n>  3 files changed, 28 insertions(+), 6 deletions(-)\n>\n>\n> base-commit: 2d755dfac9aadab25c3e025b849252b8c0a61465\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1013%2Fphillipwood%2Fwip%2Frebase-remove-merge-msg-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1013/phillipwood/wip/rebase-remove-merge-msg-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1013\n>\n> Range-diff vs v1:\n>\n>  1:  7559781ca92 ! 1:  8755dfa9d04 t3403: fix commit authorship\n>      @@ Commit message\n>           t3403: fix commit authorship\n>\n>           Setting GIT_AUTHOR_* when committing with --amend will only change the\n>      -    author if we also pass --reset-author\n>      +    author if we also pass --reset-author.  This commit is used in some\n>      +    tests that ensure the author ident does not change when rebasing.\n>      +    Creating this commit without changing the authorship meant that the\n>      +    test would not catch regressions that caused rebase to discard the\n>      +    original authorship information.\n>\n>           Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n>  2:  f7722dab21d ! 2:  84fe5823b4f rebase --apply: restore some tests\n>      @@ Commit message\n>           from \"am\" to \"merge\"\", 2020-02-15). However some tests seem to have\n>           been missed leading to us testing the \"merge\" backend twice. This\n>           patch fixes some cases that I noticed while adding tests to these\n>      -    files, I have not audited all the other rebase test files.\n>      +    files, I have not audited all the other rebase test files. I've\n>      +    reworded a couple of the test descriptions to make it clear which\n>      +    backend they are testing.\n>\n>           Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n>      @@ t/t3403-rebase-skip.sh: test_expect_success setup '\n>        test_expect_success 'rebase --skip can not be used with other options' '\n>\n>        ## t/t3418-rebase-continue.sh ##\n>      -@@ t/t3418-rebase-continue.sh: test_expect_success 'non-interactive rebase --continue works with touched file'\n>      +@@ t/t3418-rebase-continue.sh: test_expect_success 'setup' '\n>      +  git checkout main\n>      + '\n>      +\n>      +-test_expect_success 'interactive rebase --continue works with touched file' '\n>      ++test_expect_success 'merge based rebase --continue with works with touched file' '\n>      +  rm -fr .git/rebase-* &&\n>      +  git reset --hard &&\n>      +  git checkout main &&\n>      +@@ t/t3418-rebase-continue.sh: test_expect_success 'interactive rebase --continue works with touched file' '\n>      +  git rebase --continue\n>      + '\n>      +\n>      +-test_expect_success 'non-interactive rebase --continue works with touched file' '\n>      ++test_expect_success 'apply based rebase --continue works with touched file' '\n>      +  rm -fr .git/rebase-* &&\n>         git reset --hard &&\n>         git checkout main &&\n>\n>  3:  6a63b657f13 ! 3:  028c9dfc460 rebase --continue: remove .git/MERGE_MSG\n>      @@ t/t3403-rebase-skip.sh: test_expect_success 'moved back to branch correctly' '\n>         test_must_fail git rebase -i --onto goodbye \\\n>\n>        ## t/t3418-rebase-continue.sh ##\n>      -@@ t/t3418-rebase-continue.sh: test_expect_success 'interactive rebase --continue works with touched file' '\n>      +@@ t/t3418-rebase-continue.sh: test_expect_success 'merge based rebase --continue with works with touched file'\n>         git rebase --continue\n>        '\n>\n>      -+test_expect_success 'rebase --continue removes .git/MERGE_MSG' '\n>      ++test_expect_success 'merge based rebase --continue removes .git/MERGE_MSG' '\n>       + git checkout -f --detach topic &&\n>       +\n>       + test_must_fail git rebase --onto main HEAD^ &&\n>      @@ t/t3418-rebase-continue.sh: test_expect_success 'interactive rebase --continue w\n>       + test_path_is_missing .git/MERGE_MSG\n>       +'\n>       +\n>      - test_expect_success 'non-interactive rebase --continue works with touched file' '\n>      + test_expect_success 'apply based rebase --continue works with touched file' '\n>         rm -fr .git/rebase-* &&\n>         git reset --hard &&\n>\n> --\n> gitgitgadget\n\nThanks, this version looks good to me.\n"},{"id":"432678","messageId":"xmqqim09xswt.fsf@gitster.g","threadId":"56246","inReplyTo":"CABPp-BGHmCCeY7j9hVDGZPuLU8gR-ka3iEzUjK0wvkSUSYdNxA@mail.gmail.com","subject":"Re: [PATCH v2 0/3] rebase --continue: remove .git/MERGE_MSG","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-13T15:31:46Z","receivedAt":"2021-08-13T15:31:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> Thanks, this version looks good to me.\n\nThanks, both.  Elijah, can I take that as your Reviewed-by?\n"},{"id":"432691","messageId":"CABPp-BHi2GG8zmFEHHhMOUjkmmUfmj+ODT+KYUAXrXcgypRVYA@mail.gmail.com","threadId":"56246","inReplyTo":"xmqqim09xswt.fsf@gitster.g","subject":"Re: [PATCH v2 0/3] rebase --continue: remove .git/MERGE_MSG","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-08-13T17:21:31Z","receivedAt":"2021-08-13T17:21:45Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Aug 13, 2021 at 8:31 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Elijah Newren <newren@gmail.com> writes:\n>\n> > Thanks, this version looks good to me.\n>\n> Thanks, both.  Elijah, can I take that as your Reviewed-by?\n\nI typically do a bit more checking when adding a Reviewed-by and had\njust a short time available yesterday.  But I've done the extra\nchecking now, so yes:\n\nReviewed-by: Elijah Newren <newren@gmail.com>\n"},{"id":"432720","messageId":"xmqqa6lluexv.fsf@gitster.g","threadId":"56246","inReplyTo":"028c9dfc460b6c00bf481017a07a2a6d37780a76.1628775729.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] rebase --continue: remove .git/MERGE_MSG","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-13T23:01:48Z","receivedAt":"2021-08-13T23:01:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> If the user skips the final commit by removing all the changes from\n> the index and worktree with 'git restore' (or read-tree) and then runs\n> 'git rebase --continue' .git/MERGE_MSG is left behind. This will seed\n> the commit message the next time the user commits which is not what we\n> want to happen.\n\nI just remembered that \"git rebase --skip\" option exists.  Would it\nhave the same issue if used at the last step?\n\n\n[Footnote]\n\nI am not saying that it is an error to use \"git restore HEAD . &&\ngit rebase --continue\" when you'd usually use \"git rebase --skip\".\n\nNuking the difference the working tree files and the index has\nrelative to HEAD and telling the machinery to continue gives the\nsignal that the \"conflict resolution\" happened to have resulted in\nan empty change, which should yield the same resulting history as\n\"git rebase --skip\" would, because the resulting empty change should\nbe dropped (unless --empty=keep is in effect, that is).\n\n"},{"id":"432743","messageId":"549ebc1e-2b3d-786b-6467-40fb521ea0cc@gmail.com","threadId":"56246","inReplyTo":"xmqqa6lluexv.fsf@gitster.g","subject":"Re: [PATCH v2 3/3] rebase --continue: remove .git/MERGE_MSG","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-14T20:01:50Z","receivedAt":"2021-08-14T20:01:57Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 14/08/2021 00:01, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> If the user skips the final commit by removing all the changes from\n>> the index and worktree with 'git restore' (or read-tree) and then runs\n>> 'git rebase --continue' .git/MERGE_MSG is left behind. This will seed\n>> the commit message the next time the user commits which is not what we\n>> want to happen.\n> \n> I just remembered that \"git rebase --skip\" option exists.  Would it\n> have the same issue if used at the last step?\n\n--skip calls rerere_clear() which unlinks .git/MERGE_MSG. This patch \nadds a test for --skip as well as --continue.\n\nBest Wishes\n\nPhillip\n\n> \n> [Footnote]\n> \n> I am not saying that it is an error to use \"git restore HEAD . &&\n> git rebase --continue\" when you'd usually use \"git rebase --skip\".\n> \n> Nuking the difference the working tree files and the index has\n> relative to HEAD and telling the machinery to continue gives the\n> signal that the \"conflict resolution\" happened to have resulted in\n> an empty change, which should yield the same resulting history as\n> \"git rebase --skip\" would, because the resulting empty change should\n> be dropped (unless --empty=keep is in effect, that is).\n> \n"},{"id":"432744","messageId":"d9408804-864d-22e2-abef-d8b95198cab1@gmail.com","threadId":"56246","inReplyTo":"CABPp-BHi2GG8zmFEHHhMOUjkmmUfmj+ODT+KYUAXrXcgypRVYA@mail.gmail.com","subject":"Re: [PATCH v2 0/3] rebase --continue: remove .git/MERGE_MSG","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-14T20:02:54Z","receivedAt":"2021-08-14T20:02:59Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 13/08/2021 18:21, Elijah Newren wrote:\n> On Fri, Aug 13, 2021 at 8:31 AM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Elijah Newren <newren@gmail.com> writes:\n>>\n>>> Thanks, this version looks good to me.\n>>\n>> Thanks, both.  Elijah, can I take that as your Reviewed-by?\n> \n> I typically do a bit more checking when adding a Reviewed-by and had\n> just a short time available yesterday.  But I've done the extra\n> checking now, so yes:\n> \n> Reviewed-by: Elijah Newren <newren@gmail.com>\n\nThanks\n\n"},{"id":"432750","messageId":"nycvar.QRO.7.76.6.2108142350420.59@tvgsbejvaqbjf.bet","threadId":"56246","inReplyTo":"3b438e42-488c-e4ab-d1fe-3d0992ef0619@gmail.com","subject":"Re: [PATCH 1/3] t3403: fix commit authorship","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-08-14T21:53:06Z","receivedAt":"2021-08-14T21:56:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Thu, 12 Aug 2021, Phillip Wood wrote:\n\n> On 10/08/2021 19:43, Junio C Hamano wrote:\n> > Elijah Newren <newren@gmail.com> writes:\n> >\n> > > On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget\n> > > <gitgitgadget@gmail.com> wrote:\n> > > >\n> > > > From: Phillip Wood <phillip.wood@dunelm.org.uk>\n> > > >\n> > > > Setting GIT_AUTHOR_* when committing with --amend will only change the\n> > > > author if we also pass --reset-author\n> > > >\n> > > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> > > > ---\n> > > >   t/t3403-rebase-skip.sh | 3 ++-\n> > > >   1 file changed, 2 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\n> > > > index e26762d0b29..6365c5af2f7 100755\n> > > > --- a/t/t3403-rebase-skip.sh\n> > > > +++ b/t/t3403-rebase-skip.sh\n> > > > @@ -36,7 +36,8 @@ test_expect_success setup '\n> > > >          test_tick &&\n> > > >          GIT_AUTHOR_NAME=\"Another Author\" \\\n> > > >                  GIT_AUTHOR_EMAIL=\"another.author@example.com\" \\\n> > > > -               git commit --amend --no-edit -m amended-goodbye &&\n> > > > +               git commit --amend --no-edit -m amended-goodbye \\\n> > > > +                       --reset-author &&\n> > >\n> > > Makes sense...but doesn't the fact that this test worked either way\n> > > suggest that the specifying of a special author name/email was totally\n> > > superfluous and could just be removed?  If there really was a reason\n> > > for specifying a different name/email, then is the test faulty for not\n> > > checking for it somewhere?\n> >\n> > Good point.  The commit tagged with amended-goodbye is later used in\n> > some tests that ensure the author ident does not change across a\n> > rebase.  If this commit gets created without authorship customized\n> > (i.e. before Phillip's fix), we would not catch a possible breakage\n> > to make rebase discard the original authorship information.\n> >\n> > But with this fix, we now can catch such a breakage.\n>\n> I'll expand the commit message to make that clear\n\nMaybe you could even add a `test another.author@example.com = $(git show\n-s --format=%ae HEAD)`?\n\nCiao,\nDscho\n"},{"id":"432767","messageId":"xmqqsfzasj80.fsf@gitster.g","threadId":"56246","inReplyTo":"nycvar.QRO.7.76.6.2108142350420.59@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/3] t3403: fix commit authorship","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-15T17:36:47Z","receivedAt":"2021-08-15T17:36:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> > Good point.  The commit tagged with amended-goodbye is later used in\n>> > some tests that ensure the author ident does not change across a\n>> > rebase.  If this commit gets created without authorship customized\n>> > (i.e. before Phillip's fix), we would not catch a possible breakage\n>> > to make rebase discard the original authorship information.\n>> >\n>> > But with this fix, we now can catch such a breakage.\n>>\n>> I'll expand the commit message to make that clear\n>\n> Maybe you could even add a `test another.author@example.com = $(git show\n> -s --format=%ae HEAD)`?\n\nThe version I have from Phillip has updated log message already, but\nnot with such a regression prevention.\n\nThe test that the patch under discussion corrects does this:\n\n    test_expect_success 'correct authorship when committing empty pick' '\n        test_when_finished \"git rebase --abort\" &&\n        test_must_fail git rebase -i --onto goodbye \\\n                amended-goodbye^ amended-goodbye &&\n        git commit --allow-empty &&\n        git log --pretty=format:\"%an <%ae>%n%ad%B\" -1 amended-goodbye >expect &&\n        git log --pretty=format:\"%an <%ae>%n%ad%B\" -1 HEAD >actual &&\n        test_cmp expect actual\n    '\n\nto ensure that the authorship is the same between the original\n(i.e. amended-goodbye) and the rebased (i.e. HEAD), with the\nexpectation that a bug may lose the authorship and instead use the\ndefault one used in the test suite.  What this test truly cares is\nnot that amended-goodbye was authored by another.author, but it was\nnot written by the default author.\n\nWe could test both, like the attached patch, for completeness.  The\nfirst half makes sure amended-goodbye (the original) was written by\nthe another.author, and the other one makes sure that author is not\nthe one we use to prepare commits for the tests by default.\n\nI do not think the latter is actually a good idea (\"As long as the\ncommand produces a result different from THIS, any random garbage is\naccepted\" does not make a good test), so perhaps the first half\nwould be good enough.\n\n t/t3403-rebase-skip.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git c/t/t3403-rebase-skip.sh w/t/t3403-rebase-skip.sh\nindex e26762d0b2..1405720767 100755\n--- c/t/t3403-rebase-skip.sh\n+++ w/t/t3403-rebase-skip.sh\n@@ -40,6 +40,14 @@ test_expect_success setup '\n \ttest_tick &&\n \tgit tag amended-goodbye &&\n \n+\t# Make sure the authorship info is different from the default one\n+\techo \"Another Author <another.author@example.com>\" >expect &&\n+\tgit log --pretty=format:\"%an <%ae>\" -1 amended-goodbye >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit log --pretty=format:\"%an <%ae>\" -1 goodbye >unexpect &&\n+\t! test_cmp unexpect actual &&\n+\n \tgit checkout -f skip-reference &&\n \techo moo > hello &&\n \tgit commit -a -m \"we should skip this\" &&\n"},{"id":"432770","messageId":"043edd52-c666-0939-bf6d-51b0d225157e@gmail.com","threadId":"56246","inReplyTo":"xmqqsfzasj80.fsf@gitster.g","subject":"Re: [PATCH 1/3] t3403: fix commit authorship","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-15T20:04:36Z","receivedAt":"2021-08-15T20:15:59Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 15/08/2021 18:36, Junio C Hamano wrote:\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n>>>> Good point.  The commit tagged with amended-goodbye is later used in\n>>>> some tests that ensure the author ident does not change across a\n>>>> rebase.  If this commit gets created without authorship customized\n>>>> (i.e. before Phillip's fix), we would not catch a possible breakage\n>>>> to make rebase discard the original authorship information.\n>>>>\n>>>> But with this fix, we now can catch such a breakage.\n>>>\n>>> I'll expand the commit message to make that clear\n>>\n>> Maybe you could even add a `test another.author@example.com = $(git show\n>> -s --format=%ae HEAD)`?\n> \n> The version I have from Phillip has updated log message already, but\n> not with such a regression prevention.\n> \n> The test that the patch under discussion corrects does this:\n> \n>      test_expect_success 'correct authorship when committing empty pick' '\n>          test_when_finished \"git rebase --abort\" &&\n>          test_must_fail git rebase -i --onto goodbye \\\n>                  amended-goodbye^ amended-goodbye &&\n>          git commit --allow-empty &&\n>          git log --pretty=format:\"%an <%ae>%n%ad%B\" -1 amended-goodbye >expect &&\n>          git log --pretty=format:\"%an <%ae>%n%ad%B\" -1 HEAD >actual &&\n>          test_cmp expect actual\n>      '\n\nI wonder if it would be better to hard code the author in this test rather\nthan rather than relying on git log like this\n\ndiff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\nindex e26762d0b2..c90e32817f 100755\n--- a/t/t3403-rebase-skip.sh\n+++ b/t/t3403-rebase-skip.sh\n@@ -34,9 +34,10 @@ test_expect_success setup '\n         git tag reverted-goodbye &&\n         git checkout goodbye &&\n         test_tick &&\n-       GIT_AUTHOR_NAME=\"Another Author\" \\\n-               GIT_AUTHOR_EMAIL=\"another.author@example.com\" \\\n-               git commit --amend --no-edit -m amended-goodbye &&\n+       another_author=\"Another Author <another.author@example.com>\" &&\n+       git commit --amend --no-edit -m amended-goodbye \\\n+               --author=\"$another_author\" --date=\"$GIT_AUTHOR_DATE\" &&\n+       another_author=\"$another_author $GIT_AUTHOR_DATE\" &&\n         test_tick &&\n         git tag amended-goodbye &&\n  \n@@ -110,8 +111,10 @@ test_expect_success 'correct authorship when committing empty pick' '\n         test_must_fail git rebase -i --onto goodbye \\\n                 amended-goodbye^ amended-goodbye &&\n         git commit --allow-empty &&\n-       git log --pretty=format:\"%an <%ae>%n%ad%B\" -1 amended-goodbye >expect &&\n-       git log --pretty=format:\"%an <%ae>%n%ad%B\" -1 HEAD >actual &&\n+       git log --pretty=format:\"$another_author%n%B\" -1 amended-goodbye \\\n+                >expect &&\n+       git log --date=raw --pretty=format:\"%an <%ae> %ad%n%B\" -1 HEAD \\\n+               >actual &&\n         test_cmp expect actual\n  '\n  \n"},{"id":"432811","messageId":"xmqq8s11s5wk.fsf@gitster.g","threadId":"56246","inReplyTo":"043edd52-c666-0939-bf6d-51b0d225157e@gmail.com","subject":"Re: [PATCH 1/3] t3403: fix commit authorship","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-16T16:36:43Z","receivedAt":"2021-08-16T16:36:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> I wonder if it would be better to hard code the author in this test rather\n> than rather than relying on git log like this\n\nTo be quite honest, I am pretty happy with the tests as posted, and\nthe choice among various ways we are discussing depends on what the\nlikely mode of breakage we expect.\n\nThe breakage we are expecting to catch in the second hunk of your\npatch is somehow \"rebase -i\" fails to keep the authorship of\namended-goodbye and ends up making the commit at HEAD under\ndifferent authorship.\n\n - A likely source of a different authorship information that would\n   be recorded, when such a bug gets introduced, is from the\n   environment (i.e. GIT_AUTHOR_NAME etc. that test-lib.sh sets up).\n   This can happen by a new bug in the test added before the second\n   hunk of your patch we see below, and with or without this patch,\n   such a bug in the test will not be caught.\n\n - Or amended-goodbye may by a test bug have been recorded under a\n   wrong authorship information to begin with, and if it were done\n   as the default author we use in our tests (i.e. the bug you fixed\n   in your patch that started this thread).  If we reintroduced such\n   a bug, the second hunk of this patch will help.\n\n - Or the variable $another_author gets clobbered in a future change\n   between the two hunks of this patch, and the check in the second\n   hunk would be broken.\n\nOverall, I do not think any of the above breakage is so likely to\nhappen, and that is why I am happy with the tests as posted.  The\nnecessity for a plain shell variable to retain its value for such a\nlong haul across tests the patch below introduces may be making the\ntest more brittle than safer.\n\nSo...\n\n> diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\n> index e26762d0b2..c90e32817f 100755\n> --- a/t/t3403-rebase-skip.sh\n> +++ b/t/t3403-rebase-skip.sh\n> @@ -34,9 +34,10 @@ test_expect_success setup '\n>         git tag reverted-goodbye &&\n>         git checkout goodbye &&\n>         test_tick &&\n> -       GIT_AUTHOR_NAME=\"Another Author\" \\\n> -               GIT_AUTHOR_EMAIL=\"another.author@example.com\" \\\n> -               git commit --amend --no-edit -m amended-goodbye &&\n> +       another_author=\"Another Author <another.author@example.com>\" &&\n> +       git commit --amend --no-edit -m amended-goodbye \\\n> +               --author=\"$another_author\" --date=\"$GIT_AUTHOR_DATE\" &&\n> +       another_author=\"$another_author $GIT_AUTHOR_DATE\" &&\n>         test_tick &&\n>         git tag amended-goodbye &&\n>  @@ -110,8 +111,10 @@ test_expect_success 'correct authorship when\n> committing empty pick' '\n>         test_must_fail git rebase -i --onto goodbye \\\n>                 amended-goodbye^ amended-goodbye &&\n>         git commit --allow-empty &&\n> -       git log --pretty=format:\"%an <%ae>%n%ad%B\" -1 amended-goodbye >expect &&\n> -       git log --pretty=format:\"%an <%ae>%n%ad%B\" -1 HEAD >actual &&\n> +       git log --pretty=format:\"$another_author%n%B\" -1 amended-goodbye \\\n> +                >expect &&\n> +       git log --date=raw --pretty=format:\"%an <%ae> %ad%n%B\" -1 HEAD \\\n> +               >actual &&\n>         test_cmp expect actual\n>  '\n>  \n"},{"id":"432932","messageId":"9444dcd0-4733-aed1-0b94-6aa6337552dd@gmail.com","threadId":"56246","inReplyTo":"xmqq8s11s5wk.fsf@gitster.g","subject":"Re: [PATCH 1/3] t3403: fix commit authorship","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-17T10:05:01Z","receivedAt":"2021-08-17T10:06:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 16/08/2021 17:36, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> \n>> I wonder if it would be better to hard code the author in this test rather\n>> than rather than relying on git log like this\n> \n> To be quite honest, I am pretty happy with the tests as posted, and\n> the choice among various ways we are discussing depends on what the\n> likely mode of breakage we expect.\n> \n> The breakage we are expecting to catch in the second hunk of your\n> patch is somehow \"rebase -i\" fails to keep the authorship of\n> amended-goodbye and ends up making the commit at HEAD under\n> different authorship.\n> \n>   - A likely source of a different authorship information that would\n>     be recorded, when such a bug gets introduced, is from the\n>     environment (i.e. GIT_AUTHOR_NAME etc. that test-lib.sh sets up).\n>     This can happen by a new bug in the test added before the second\n>     hunk of your patch we see below, and with or without this patch,\n>     such a bug in the test will not be caught.\n> \n>   - Or amended-goodbye may by a test bug have been recorded under a\n>     wrong authorship information to begin with, and if it were done\n>     as the default author we use in our tests (i.e. the bug you fixed\n>     in your patch that started this thread).  If we reintroduced such\n>     a bug, the second hunk of this patch will help.\n> \n>   - Or the variable $another_author gets clobbered in a future change\n>     between the two hunks of this patch, and the check in the second\n>     hunk would be broken.\n> \n> Overall, I do not think any of the above breakage is so likely to\n> happen, and that is why I am happy with the tests as posted.  The\n> necessity for a plain shell variable to retain its value for such a\n> long haul across tests the patch below introduces may be making the\n> test more brittle than safer.\n> \n> So...\n\nOkay lets leave the test as it is then\n\nThanks\n\nPhillip\n"}]}