{"thread":{"id":"56832","subject":"[PATCH] rebase -i: fix rewording with --committer-date-is-author-date","startedAt":"2021-11-02T20:10:49Z","lastAt":"2021-11-04T06:27:36Z","messageCount":10,"participants":["Phillip Wood via GitGitGadget","Eric Sunshine","Phillip Wood","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"440348","messageId":"pull.1123.git.git.1635883844710.gitgitgadget@gmail.com","threadId":"56832","inReplyTo":null,"subject":"[PATCH] rebase -i: fix rewording with --committer-date-is-author-date","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-02T20:10:44Z","receivedAt":"2021-11-02T20:10:49Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nbaf8ec8d3a (rebase -r: don't write .git/MERGE_MSG when\nfast-forwarding, 2021-08-20) stopped reading the author script in\nrun_git_commit() when rewording a commit. This is normally safe\nbecause \"git commit --amend\" preserves the authorship. However if the\nuser passes \"--committer-date-is-author-date\" then we need to read the\nauthor date from the author script when rewording. Fix this regression\nby tightening the check for when it is safe to skip reading the author\nscript.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    rebase -i: fix rewording with --committer-date-is-author-date\n    \n    This regression was introduced in the current cycle and is present in\n    v2.34.0-rc0, v2.33.1 and maint\n    \n    Thanks to Jonas for reporting it and Peff for bisecting\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1123%2Fphillipwood%2Fwip%2Frebase-committer-date-is-author-date-fix-reword-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1123/phillipwood/wip/rebase-committer-date-is-author-date-fix-reword-v1\nPull-Request: https://github.com/git/git/pull/1123\n\n sequencer.c                    |  4 +++-\n t/t3436-rebase-more-options.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex cd2aabf1f76..ea96837cde3 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -997,7 +997,9 @@ static int run_git_commit(const char *defmsg,\n \n \tcmd.git_cmd = 1;\n \n-\tif (is_rebase_i(opts) && !(!defmsg && (flags & AMEND_MSG)) &&\n+\tif (is_rebase_i(opts) &&\n+\t    ((opts->committer_date_is_author_date && !opts->ignore_date) ||\n+\t     !(!defmsg && (flags & AMEND_MSG))) &&\n \t    read_env_script(&cmd.env_array)) {\n \t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n \ndiff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\nindex 4d106642ba7..94671d3c465 100755\n--- a/t/t3436-rebase-more-options.sh\n+++ b/t/t3436-rebase-more-options.sh\n@@ -82,6 +82,20 @@ test_expect_success '--committer-date-is-author-date works with merge backend' '\n \ttest_ctime_is_atime -1\n '\n \n+test_expect_success '--committer-date-is-author-date works when rewording' '\n+\tGIT_AUTHOR_DATE=\"@1234 +0300\" git commit --amend --reset-author &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_COMMIT_MESSAGE=edited \\\n+\t\t\tFAKE_LINES=\"reword 1\" \\\n+\t\t\tgit rebase -i --committer-date-is-author-date HEAD^\n+\t) &&\n+\ttest_write_lines edited \"\" >expect &&\n+\tgit log --format=\"%B\" -1 >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_ctime_is_atime -1\n+'\n+\n test_expect_success '--committer-date-is-author-date works with rebase -r' '\n \tgit checkout side &&\n \tGIT_AUTHOR_DATE=\"@1234 +0300\" git merge --no-ff commit3 &&\n@@ -155,6 +169,21 @@ test_expect_success '--reset-author-date with --committer-date-is-author-date wo\n \ttest_atime_is_ignored -2\n '\n \n+test_expect_success 'reset-author-date with --committer-date-is-author-date works when rewording' '\n+\tGIT_AUTHOR_DATE=\"@1234 +0300\" git commit --amend --reset-author &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_COMMIT_MESSAGE=edited \\\n+\t\t\tFAKE_LINES=\"reword 1\" \\\n+\t\t\tgit rebase -i --committer-date-is-author-date \\\n+\t\t\t\t--reset-author-date HEAD^\n+\t) &&\n+\ttest_write_lines edited \"\" >expect &&\n+\tgit log --format=\"%B\" -1 >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_atime_is_ignored -1\n+'\n+\n test_expect_success '--reset-author-date --committer-date-is-author-date works when forking merge' '\n \tGIT_SEQUENCE_EDITOR=\"echo \\\"merge -C $(git rev-parse HEAD) commit3\\\">\" \\\n \t\tPATH=\"./test-bin:$PATH\" git rebase -i --strategy=test \\\n\nbase-commit: 0cddd84c9f3e9c3d793ec93034ef679335f35e49\n-- \ngitgitgadget\n"},{"id":"440351","messageId":"CAPig+cSWDJ-zMv4zgU4oWcqXmaaCiH+03wSzQg2O=YRHthf55A@mail.gmail.com","threadId":"56832","inReplyTo":"pull.1123.git.git.1635883844710.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase -i: fix rewording with --committer-date-is-author-date","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-11-02T21:05:13Z","receivedAt":"2021-11-02T21:05:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 2, 2021 at 4:10 PM Phillip Wood via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> baf8ec8d3a (rebase -r: don't write .git/MERGE_MSG when\n> fast-forwarding, 2021-08-20) stopped reading the author script in\n> run_git_commit() when rewording a commit. This is normally safe\n> because \"git commit --amend\" preserves the authorship. However if the\n> user passes \"--committer-date-is-author-date\" then we need to read the\n> author date from the author script when rewording. Fix this regression\n> by tightening the check for when it is safe to skip reading the author\n> script.\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nShould this have a:\n\n    Reported-by: Jonas Kittner <jonas.kittner@ruhr-uni-bochum.de>\n\n?\n"},{"id":"440352","messageId":"349c1832-41b2-863d-254c-777e1915fea8@gmail.com","threadId":"56832","inReplyTo":"CAPig+cSWDJ-zMv4zgU4oWcqXmaaCiH+03wSzQg2O=YRHthf55A@mail.gmail.com","subject":"Re: [PATCH] rebase -i: fix rewording with --committer-date-is-author-date","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-11-02T21:29:27Z","receivedAt":"2021-11-02T21:29:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Eric\n\nOn 02/11/2021 21:05, Eric Sunshine wrote:\n> On Tue, Nov 2, 2021 at 4:10 PM Phillip Wood via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>> baf8ec8d3a (rebase -r: don't write .git/MERGE_MSG when\n>> fast-forwarding, 2021-08-20) stopped reading the author script in\n>> run_git_commit() when rewording a commit. This is normally safe\n>> because \"git commit --amend\" preserves the authorship. However if the\n>> user passes \"--committer-date-is-author-date\" then we need to read the\n>> author date from the author script when rewording. Fix this regression\n>> by tightening the check for when it is safe to skip reading the author\n>> script.\n>>\n>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> \n> Should this have a:\n> \n>      Reported-by: Jonas Kittner <jonas.kittner@ruhr-uni-bochum.de>\n\nYes it should and does locally but I forgot to push the updated version.\n\nThanks\n\nPhillip\n\n> ?\n> \n\n"},{"id":"440353","messageId":"pull.1123.v2.git.git.1635888651183.gitgitgadget@gmail.com","threadId":"56832","inReplyTo":"pull.1123.git.git.1635883844710.gitgitgadget@gmail.com","subject":"[PATCH v2] rebase -i: fix rewording with --committer-date-is-author-date","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-02T21:30:51Z","receivedAt":"2021-11-02T21:30:58Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nbaf8ec8d3a (rebase -r: don't write .git/MERGE_MSG when\nfast-forwarding, 2021-08-20) stopped reading the author script in\nrun_git_commit() when rewording a commit. This is normally safe\nbecause \"git commit --amend\" preserves the authorship. However if the\nuser passes \"--committer-date-is-author-date\" then we need to read the\nauthor date from the author script when rewording. Fix this regression\nby tightening the check for when it is safe to skip reading the author\nscript.\n\nReported-by: Jonas Kittner <jonas.kittner@ruhr-uni-bochum.de>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    rebase -i: fix rewording with --committer-date-is-author-date\n    \n    This regression was introduced in the current cycle and is present in\n    v2.34.0-rc0, v2.33.1 and maint\n    \n    Thanks to Jonas for reporting it and Peff for bisecting\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1123%2Fphillipwood%2Fwip%2Frebase-committer-date-is-author-date-fix-reword-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1123/phillipwood/wip/rebase-committer-date-is-author-date-fix-reword-v2\nPull-Request: https://github.com/git/git/pull/1123\n\nRange-diff vs v1:\n\n 1:  fb88c58d2fb ! 1:  6565c02ba89 rebase -i: fix rewording with --committer-date-is-author-date\n     @@ Commit message\n          by tightening the check for when it is safe to skip reading the author\n          script.\n      \n     +    Reported-by: Jonas Kittner <jonas.kittner@ruhr-uni-bochum.de>\n          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n      \n       ## sequencer.c ##\n\n\n sequencer.c                    |  4 +++-\n t/t3436-rebase-more-options.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex cd2aabf1f76..ea96837cde3 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -997,7 +997,9 @@ static int run_git_commit(const char *defmsg,\n \n \tcmd.git_cmd = 1;\n \n-\tif (is_rebase_i(opts) && !(!defmsg && (flags & AMEND_MSG)) &&\n+\tif (is_rebase_i(opts) &&\n+\t    ((opts->committer_date_is_author_date && !opts->ignore_date) ||\n+\t     !(!defmsg && (flags & AMEND_MSG))) &&\n \t    read_env_script(&cmd.env_array)) {\n \t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n \ndiff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\nindex 4d106642ba7..94671d3c465 100755\n--- a/t/t3436-rebase-more-options.sh\n+++ b/t/t3436-rebase-more-options.sh\n@@ -82,6 +82,20 @@ test_expect_success '--committer-date-is-author-date works with merge backend' '\n \ttest_ctime_is_atime -1\n '\n \n+test_expect_success '--committer-date-is-author-date works when rewording' '\n+\tGIT_AUTHOR_DATE=\"@1234 +0300\" git commit --amend --reset-author &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_COMMIT_MESSAGE=edited \\\n+\t\t\tFAKE_LINES=\"reword 1\" \\\n+\t\t\tgit rebase -i --committer-date-is-author-date HEAD^\n+\t) &&\n+\ttest_write_lines edited \"\" >expect &&\n+\tgit log --format=\"%B\" -1 >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_ctime_is_atime -1\n+'\n+\n test_expect_success '--committer-date-is-author-date works with rebase -r' '\n \tgit checkout side &&\n \tGIT_AUTHOR_DATE=\"@1234 +0300\" git merge --no-ff commit3 &&\n@@ -155,6 +169,21 @@ test_expect_success '--reset-author-date with --committer-date-is-author-date wo\n \ttest_atime_is_ignored -2\n '\n \n+test_expect_success 'reset-author-date with --committer-date-is-author-date works when rewording' '\n+\tGIT_AUTHOR_DATE=\"@1234 +0300\" git commit --amend --reset-author &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_COMMIT_MESSAGE=edited \\\n+\t\t\tFAKE_LINES=\"reword 1\" \\\n+\t\t\tgit rebase -i --committer-date-is-author-date \\\n+\t\t\t\t--reset-author-date HEAD^\n+\t) &&\n+\ttest_write_lines edited \"\" >expect &&\n+\tgit log --format=\"%B\" -1 >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_atime_is_ignored -1\n+'\n+\n test_expect_success '--reset-author-date --committer-date-is-author-date works when forking merge' '\n \tGIT_SEQUENCE_EDITOR=\"echo \\\"merge -C $(git rev-parse HEAD) commit3\\\">\" \\\n \t\tPATH=\"./test-bin:$PATH\" git rebase -i --strategy=test \\\n\nbase-commit: 0cddd84c9f3e9c3d793ec93034ef679335f35e49\n-- \ngitgitgadget\n"},{"id":"440355","messageId":"YYG8aq85UmMMVW4l@coredump.intra.peff.net","threadId":"56832","inReplyTo":"pull.1123.git.git.1635883844710.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase -i: fix rewording with --committer-date-is-author-date","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-02T22:32:10Z","receivedAt":"2021-11-02T22:32:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 02, 2021 at 08:10:44PM +0000, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n> \n> baf8ec8d3a (rebase -r: don't write .git/MERGE_MSG when\n> fast-forwarding, 2021-08-20) stopped reading the author script in\n> run_git_commit() when rewording a commit. This is normally safe\n> because \"git commit --amend\" preserves the authorship. However if the\n> user passes \"--committer-date-is-author-date\" then we need to read the\n> author date from the author script when rewording. Fix this regression\n> by tightening the check for when it is safe to skip reading the author\n> script.\n\nThat description makes sense, and the patch matches. Not being that\nfamiliar with this area, my biggest question would be: are there are\nother cases that would need the same treatment? And is there a way we\ncan make it easier to avoid forgetting such a case in the future?\n\n> diff --git a/sequencer.c b/sequencer.c\n> index cd2aabf1f76..ea96837cde3 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -997,7 +997,9 @@ static int run_git_commit(const char *defmsg,\n>  \n>  \tcmd.git_cmd = 1;\n>  \n> -\tif (is_rebase_i(opts) && !(!defmsg && (flags & AMEND_MSG)) &&\n> +\tif (is_rebase_i(opts) &&\n> +\t    ((opts->committer_date_is_author_date && !opts->ignore_date) ||\n> +\t     !(!defmsg && (flags & AMEND_MSG))) &&\n>  \t    read_env_script(&cmd.env_array)) {\n>  \t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n\nThis conditional is getting pretty complicated. I wonder if a helper\nlike:\n\n  if (is_rebase_i(opts) && !needs_env_script(...))\n\nmight help, but I guess it needs a funky array of inputs (defmsg, flags,\nand opts). So maybe it is just making things worse.\n\n> +test_expect_success '--committer-date-is-author-date works when rewording' '\n> +\tGIT_AUTHOR_DATE=\"@1234 +0300\" git commit --amend --reset-author &&\n> +\t(\n> +\t\tset_fake_editor &&\n> +\t\tFAKE_COMMIT_MESSAGE=edited \\\n> +\t\t\tFAKE_LINES=\"reword 1\" \\\n> +\t\t\tgit rebase -i --committer-date-is-author-date HEAD^\n> +\t) &&\n> +\ttest_write_lines edited \"\" >expect &&\n> +\tgit log --format=\"%B\" -1 >actual &&\n> +\ttest_cmp expect actual &&\n> +\ttest_ctime_is_atime -1\n> +'\n\nThis test make sense (I had to look up what \"-1\" means for\ntest_ctime_is_atime; it's passed to git-log to decide which commits to\nlook at).\n\n> +test_expect_success 'reset-author-date with --committer-date-is-author-date works when rewording' '\n> +\tGIT_AUTHOR_DATE=\"@1234 +0300\" git commit --amend --reset-author &&\n> +\t(\n> +\t\tset_fake_editor &&\n> +\t\tFAKE_COMMIT_MESSAGE=edited \\\n> +\t\t\tFAKE_LINES=\"reword 1\" \\\n> +\t\t\tgit rebase -i --committer-date-is-author-date \\\n> +\t\t\t\t--reset-author-date HEAD^\n> +\t) &&\n> +\ttest_write_lines edited \"\" >expect &&\n> +\tgit log --format=\"%B\" -1 >actual &&\n> +\ttest_cmp expect actual &&\n> +\ttest_atime_is_ignored -1\n> +'\n\nAnd this one I guess is covering the --ignore-date cut-out in the code?\nI think it would pass even without it, as that is just noting a case\nwhere we _don't_ need to call read_env_script(). But I don't know if\nthere is any user-visible effect of accidentally calling it when we\ndon't need to (my impression is that it's just a performance thing).\n\n-Peff\n"},{"id":"440366","messageId":"101071b2-0b7d-5ee8-ca81-171e08a1ffdf@gmail.com","threadId":"56832","inReplyTo":"YYG8aq85UmMMVW4l@coredump.intra.peff.net","subject":"Re: [PATCH] rebase -i: fix rewording with --committer-date-is-author-date","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-11-03T11:23:28Z","receivedAt":"2021-11-03T11:23:34Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nOn 02/11/2021 22:32, Jeff King wrote:\n> On Tue, Nov 02, 2021 at 08:10:44PM +0000, Phillip Wood via GitGitGadget wrote:\n> \n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> baf8ec8d3a (rebase -r: don't write .git/MERGE_MSG when\n>> fast-forwarding, 2021-08-20) stopped reading the author script in\n>> run_git_commit() when rewording a commit. This is normally safe\n>> because \"git commit --amend\" preserves the authorship. However if the\n>> user passes \"--committer-date-is-author-date\" then we need to read the\n>> author date from the author script when rewording. Fix this regression\n>> by tightening the check for when it is safe to skip reading the author\n>> script.\n> \n> That description makes sense, and the patch matches. Not being that\n> familiar with this area, my biggest question would be: are there are\n> other cases that would need the same treatment? And is there a way we\n> can make it easier to avoid forgetting such a case in the future?\n\nI don't think there are any other cases (but then I thought that when I \nwrote the buggy patch...). The only time we change the authorship is if \nthe user passes --committer-date-is-author-date or --reset-author-date. \nI agree it would be good to have a way to avoid this problem in the \nfuture but I haven't come up with an easy way to do that. One \npossibility would be to go back to always reading the author script. \nThat would mean revisiting the changes to do_merge() in baf8ec8d3a so \nthat it always writes the author script and .git/MERGE_MSG but removes \nthem when fast-forwarding (the problem that baf8ec8d3a tried to solve \nwas a left over .git/MERGE_MSG when do_merge() fast-forwarded) I don't \nwant to do that in the rc window though.\n\n>> diff --git a/sequencer.c b/sequencer.c\n>> index cd2aabf1f76..ea96837cde3 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -997,7 +997,9 @@ static int run_git_commit(const char *defmsg,\n>>   \n>>   \tcmd.git_cmd = 1;\n>>   \n>> -\tif (is_rebase_i(opts) && !(!defmsg && (flags & AMEND_MSG)) &&\n>> +\tif (is_rebase_i(opts) &&\n>> +\t    ((opts->committer_date_is_author_date && !opts->ignore_date) ||\n>> +\t     !(!defmsg && (flags & AMEND_MSG))) &&\n>>   \t    read_env_script(&cmd.env_array)) {\n>>   \t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n> \n> This conditional is getting pretty complicated. I wonder if a helper\n> like:\n> \n>    if (is_rebase_i(opts) && !needs_env_script(...))\n> \n> might help, but I guess it needs a funky array of inputs (defmsg, flags,\n> and opts). So maybe it is just making things worse.\n\nAs you say it needs a lot of inputs so I'm not sure how much a function \nwould help. I did consider changing it to\n\n         if (is_rebase_i(opts) &&\n\t    ((opts->committer_date_is_author_date && !opts->ignore_date) ||\n              defmsg || !(flags & AMEND_MSG)))\n\nbut as we're in the rc phase I decided to leave the existing condition \nalone.\n\n\n>> +test_expect_success '--committer-date-is-author-date works when rewording' '\n>> +\tGIT_AUTHOR_DATE=\"@1234 +0300\" git commit --amend --reset-author &&\n>> +\t(\n>> +\t\tset_fake_editor &&\n>> +\t\tFAKE_COMMIT_MESSAGE=edited \\\n>> +\t\t\tFAKE_LINES=\"reword 1\" \\\n>> +\t\t\tgit rebase -i --committer-date-is-author-date HEAD^\n>> +\t) &&\n>> +\ttest_write_lines edited \"\" >expect &&\n>> +\tgit log --format=\"%B\" -1 >actual &&\n>> +\ttest_cmp expect actual &&\n>> +\ttest_ctime_is_atime -1\n>> +'\n> \n> This test make sense (I had to look up what \"-1\" means for\n> test_ctime_is_atime; it's passed to git-log to decide which commits to\n> look at).\n\nYeah I had to check what the -1 was for when writing the test, maybe we \nshould change the helper to add the '-' for us once 2.34.0 is out.\n\n>> +test_expect_success 'reset-author-date with --committer-date-is-author-date works when rewording' '\n>> +\tGIT_AUTHOR_DATE=\"@1234 +0300\" git commit --amend --reset-author &&\n>> +\t(\n>> +\t\tset_fake_editor &&\n>> +\t\tFAKE_COMMIT_MESSAGE=edited \\\n>> +\t\t\tFAKE_LINES=\"reword 1\" \\\n>> +\t\t\tgit rebase -i --committer-date-is-author-date \\\n>> +\t\t\t\t--reset-author-date HEAD^\n>> +\t) &&\n>> +\ttest_write_lines edited \"\" >expect &&\n>> +\tgit log --format=\"%B\" -1 >actual &&\n>> +\ttest_cmp expect actual &&\n>> +\ttest_atime_is_ignored -1\n>> +'\n> \n> And this one I guess is covering the --ignore-date cut-out in the code?\n\nYes\n\n> I think it would pass even without it, as that is just noting a case\n> where we _don't_ need to call read_env_script().\n\nThat's right.\n\n> But I don't know if\n> there is any user-visible effect of accidentally calling it when we\n> don't need to (my impression is that it's just a performance thing).\n\nThere should not be any user-visible effects from reading the author \nscript when --ignore-date is given but we don't need to read it in that \ncase so I opted not to.\n\nThanks for your comments, are you happy for this to go in as is or \nshould I look at simplifying the conditional?\n\nPhillip\n\n\n> -Peff\n> \n\n"},{"id":"440370","messageId":"YYJ1rA8F22SSBeNS@coredump.intra.peff.net","threadId":"56832","inReplyTo":"101071b2-0b7d-5ee8-ca81-171e08a1ffdf@gmail.com","subject":"Re: [PATCH] rebase -i: fix rewording with --committer-date-is-author-date","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-03T11:42:36Z","receivedAt":"2021-11-03T11:43:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 03, 2021 at 11:23:28AM +0000, Phillip Wood wrote:\n\n> > That description makes sense, and the patch matches. Not being that\n> > familiar with this area, my biggest question would be: are there are\n> > other cases that would need the same treatment? And is there a way we\n> > can make it easier to avoid forgetting such a case in the future?\n> \n> I don't think there are any other cases (but then I thought that when I\n> wrote the buggy patch...). The only time we change the authorship is if the\n> user passes --committer-date-is-author-date or --reset-author-date. I agree\n> it would be good to have a way to avoid this problem in the future but I\n> haven't come up with an easy way to do that. One possibility would be to go\n> back to always reading the author script. That would mean revisiting the\n> changes to do_merge() in baf8ec8d3a so that it always writes the author\n> script and .git/MERGE_MSG but removes them when fast-forwarding (the problem\n> that baf8ec8d3a tried to solve was a left over .git/MERGE_MSG when\n> do_merge() fast-forwarded) I don't want to do that in the rc window though.\n\nI suspected the answers to my questions were \"I hope so\" and \"not\nreally\", which I think matches what you wrote. :)\n\nIf there isn't an easy way to make it more future-proof, then I'm\ncontent that you've given it some thought and didn't find any other\ncases. We can proceed from here with this fix, and be on the lookout for\nany other cases that people report (on the plus side, the BUG() made it\nquite obvious that there was a problem, rather than a subtle behavior\nchange).\n\n> Thanks for your comments, are you happy for this to go in as is or should I\n> look at simplifying the conditional?\n\nI'm happy enough with it. I don't know what the plan is for the -rc\nperiod, though. AFAICT the bug is in v2.33.1, so it's not technically a\nv2.34-rc problem. It could wait for the next maint release.\n\n-Peff\n"},{"id":"440390","messageId":"xmqqk0hp6sno.fsf@gitster.g","threadId":"56832","inReplyTo":"YYJ1rA8F22SSBeNS@coredump.intra.peff.net","subject":"Re: [PATCH] rebase -i: fix rewording with --committer-date-is-author-date","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-03T17:44:11Z","receivedAt":"2021-11-03T17:44:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I'm happy enough with it. I don't know what the plan is for the -rc\n> period, though. AFAICT the bug is in v2.33.1, so it's not technically a\n> v2.34-rc problem. It could wait for the next maint release.\n\nHmph, if it was in v2.33.0 then it is not, but if it was introduced\nbetween v2.33.0 and v2.33.1, then it is a problem for the current\ncycle, no?\n"},{"id":"440422","messageId":"YYM/bm7cHpJeJoGT@coredump.intra.peff.net","threadId":"56832","inReplyTo":"xmqqk0hp6sno.fsf@gitster.g","subject":"Re: [PATCH] rebase -i: fix rewording with --committer-date-is-author-date","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-04T02:03:26Z","receivedAt":"2021-11-04T02:03:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 03, 2021 at 10:44:11AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I'm happy enough with it. I don't know what the plan is for the -rc\n> > period, though. AFAICT the bug is in v2.33.1, so it's not technically a\n> > v2.34-rc problem. It could wait for the next maint release.\n> \n> Hmph, if it was in v2.33.0 then it is not, but if it was introduced\n> between v2.33.0 and v2.33.1, then it is a problem for the current\n> cycle, no?\n\nI guess it depends how you consider the cycle. It's in v2.33.1 but not\nv2.33.0, so yes, anybody going from v2.33.0 to v2.34.0 will see it as a\nregression. I don't have any problem with fixing it for v2.34.0, and I\nthink the fix Phillip provided is minimally invasive. I just didn't\nconsider it quite the same as a bug that had never been in a released\nversion at all (and I know you are often hesitant to throw too much into\nthe -rc part of the cycle).\n\n-Peff\n"},{"id":"440430","messageId":"xmqq4k8s4er3.fsf@gitster.g","threadId":"56832","inReplyTo":"YYM/bm7cHpJeJoGT@coredump.intra.peff.net","subject":"Re: [PATCH] rebase -i: fix rewording with --committer-date-is-author-date","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-04T06:27:28Z","receivedAt":"2021-11-04T06:27:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... I just didn't\n> consider it quite the same as a bug that had never been in a released\n> version at all (and I know you are often hesitant to throw too much into\n> the -rc part of the cycle).\n\nAh, I see.\n\n2.33.1 was 2.33.0 plus what was queued on 'master' back then for\n2.34.0, and more importantly, we do not issue the maintenance track\nvery often these days (except for occasional security patches).\n\nTo put it differently, I didn't have to issue 2.33.1. There was\nnothing urgent in there, and it was just that the cycle toward 2.34\nwas unusually busy and there were too many accumulated fixes on the\n'master' front that were 'maint' material.  And for that reason, in\nmy mind, 2.33.1 cannot be treated quite the same way as feature\nreleases X.YY.0, when I say \"The bug was already present in that\nreleased version, so fixing it is not all that urgent\".\n"}]}