rebase -i: fix rewording with --committer-date-is-author-date
PATCH 1/1 rebase -i: fix rewording with --committer-date-is-author-date
From: Phillip Wood <[email protected]>
baf8ec8d3a (rebase -r: don't write .git/MERGE_MSG when fast-forwarding, 2021-08-20) stopped reading the author script in run_git_commit() when rewording a commit. This is normally safe because "git commit --amend" preserves the authorship. However if the user passes "--committer-date-is-author-date" then we need to read the author date from the author script when rewording. Fix this regression by tightening the check for when it is safe to skip reading the author script.
---
rebase -i: fix rewording with --committer-date-is-author-date
This regression was introduced in the current cycle and is present in v2.34.0-rc0, v2.33.1 and maint
Thanks to Jonas for reporting it and Peff for bisecting
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1123%2Fphillipwood%2Fwip%2Frebase-committer-date-is-author-date-fix-reword-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1123/phillipwood/wip/rebase-committer-date-is-author-date-fix-reword-v1
Pull-Request: https://github.com/git/git/pull/1123
sequencer.c | 4 +++- t/t3436-rebase-more-options.sh | 29 +++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-)
sequencer.c +3 -1
@@ -997,7 +997,9 @@ static int run_git_commit(const char *defmsg, cmd.git_cmd =1 ; -if (is_rebase_i (opts) && !(!defmsg && (flags &AMEND_MSG )) && +if (is_rebase_i (opts) && + ((opts->committer_date_is_author_date && !opts->ignore_date) || + !(!defmsg && (flags &AMEND_MSG ))) &&read_env_script (&cmd.env_array)) {const char *gpg_opt =gpg_sign_opt_quoted (opts);
t/t3436-rebase-more-options.sh +29 -0
@@ -82,6 +82,20 @@ test_expect_success '--committer-date-is-author-date works with merge backend' ' test_ctime_is_atime -1 ' +test_expect_success '--committer-date-is-author-date works when rewording' ' + GIT_AUTHOR_DATE="@1234 +0300" git commit --amend --reset-author && + ( + set_fake_editor && + FAKE_COMMIT_MESSAGE=edited \ + FAKE_LINES="reword 1" \ + git rebase -i --committer-date-is-author-date HEAD^ + ) && + test_write_lines edited "" >expect && + git log --format="%B" -1 >actual && + test_cmp expect actual && + test_ctime_is_atime -1 +' + test_expect_success '--committer-date-is-author-date works with rebase -r' ' git checkout side && GIT_AUTHOR_DATE="@1234 +0300" git merge --no-ff commit3 &&
@@ -155,6 +169,21 @@ test_expect_success '--reset-author-date with --committer-date-is-author-date wo test_atime_is_ignored -2 ' +test_expect_success 'reset-author-date with --committer-date-is-author-date works when rewording' ' + GIT_AUTHOR_DATE="@1234 +0300" git commit --amend --reset-author && + ( + set_fake_editor && + FAKE_COMMIT_MESSAGE=edited \ + FAKE_LINES="reword 1" \ + git rebase -i --committer-date-is-author-date \ + --reset-author-date HEAD^ + ) && + test_write_lines edited "" >expect && + git log --format="%B" -1 >actual && + test_cmp expect actual && + test_atime_is_ignored -1 +' + test_expect_success '--reset-author-date --committer-date-is-author-date works when forking merge' ' GIT_SEQUENCE_EDITOR="echo \"merge -C $(git rev-parse HEAD) commit3\">" \ PATH="./test-bin:$PATH" git rebase -i --strategy=test \
base-commit: 0cddd84c9f3e9c3d793ec93034ef679335f35e49
Signature · 1 line
gitgitgadget
Quoted 10 lines · Phillip Wood via GitGitGadget
baf8ec8d3a(rebase -r: don't write .git/MERGE_MSG when fast-forwarding, 2021-08-20) stopped reading the author script inrun_git_commit()when rewording a commit. This is normally safe because "git commit --amend" preserves the authorship. However if the user passes "--committer-date-is-author-date" then we need to read the author date from the author script when rewording. Fix this regression by tightening the check for when it is safe to skip reading the author script.Signed-off-by Phillip Wood <[email protected]>
Should this have a:
?
Hi Eric
Quoted 11 lines · Eric Sunshine
Quoted 9 lines · Phillip Wood via GitGitGadget
baf8ec8d3a(rebase -r: don't write .git/MERGE_MSG when fast-forwarding, 2021-08-20) stopped reading the author script inrun_git_commit()when rewording a commit. This is normally safe because "git commit --amend" preserves the authorship. However if the user passes "--committer-date-is-author-date" then we need to read the author date from the author script when rewording. Fix this regression by tightening the check for when it is safe to skip reading the author script.
Quoted 5 lines
Quoted 1 line
Signed-off-by Phillip Wood <[email protected]>Should this have a:
Reported-by Jonas Kittner <[email protected]>
Yes it should and does locally but I forgot to push the updated version.
Thanks
Phillip
Quoted 2 lines
?
PATCH v2 1/1 rebase -i: fix rewording with --committer-date-is-author-date
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
baf8ec8d3a (rebase -r: don't write .git/MERGE_MSG when fast-forwarding, 2021-08-20) stopped reading the author script in run_git_commit() when rewording a commit. This is normally safe because "git commit --amend" preserves the authorship. However if the user passes "--committer-date-is-author-date" then we need to read the author date from the author script when rewording. Fix this regression by tightening the check for when it is safe to skip reading the author script.
---
rebase -i: fix rewording with --committer-date-is-author-date
This regression was introduced in the current cycle and is present in v2.34.0-rc0, v2.33.1 and maint
Thanks to Jonas for reporting it and Peff for bisecting
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1123%2Fphillipwood%2Fwip%2Frebase-committer-date-is-author-date-fix-reword-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1123/phillipwood/wip/rebase-committer-date-is-author-date-fix-reword-v2
Pull-Request: https://github.com/git/git/pull/1123
Range-diff vs v1:
1: fb88c58d2fb ! 1: 6565c02ba89 rebase -i: fix rewording with --committer-date-is-author-date
@@ Commit message
by tightening the check for when it is safe to skip reading the author script.
+ Reported-by: Jonas Kittner <[email protected]>
## sequencer.c ##
sequencer.c | 4 +++- t/t3436-rebase-more-options.sh | 29 +++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-)
sequencer.c +3 -1
@@ -997,7 +997,9 @@ static int run_git_commit(const char *defmsg, cmd.git_cmd =1 ; -if (is_rebase_i (opts) && !(!defmsg && (flags &AMEND_MSG )) && +if (is_rebase_i (opts) && + ((opts->committer_date_is_author_date && !opts->ignore_date) || + !(!defmsg && (flags &AMEND_MSG ))) &&read_env_script (&cmd.env_array)) {const char *gpg_opt =gpg_sign_opt_quoted (opts);
t/t3436-rebase-more-options.sh +29 -0
@@ -82,6 +82,20 @@ test_expect_success '--committer-date-is-author-date works with merge backend' ' test_ctime_is_atime -1 ' +test_expect_success '--committer-date-is-author-date works when rewording' ' + GIT_AUTHOR_DATE="@1234 +0300" git commit --amend --reset-author && + ( + set_fake_editor && + FAKE_COMMIT_MESSAGE=edited \ + FAKE_LINES="reword 1" \ + git rebase -i --committer-date-is-author-date HEAD^ + ) && + test_write_lines edited "" >expect && + git log --format="%B" -1 >actual && + test_cmp expect actual && + test_ctime_is_atime -1 +' + test_expect_success '--committer-date-is-author-date works with rebase -r' ' git checkout side && GIT_AUTHOR_DATE="@1234 +0300" git merge --no-ff commit3 &&
@@ -155,6 +169,21 @@ test_expect_success '--reset-author-date with --committer-date-is-author-date wo test_atime_is_ignored -2 ' +test_expect_success 'reset-author-date with --committer-date-is-author-date works when rewording' ' + GIT_AUTHOR_DATE="@1234 +0300" git commit --amend --reset-author && + ( + set_fake_editor && + FAKE_COMMIT_MESSAGE=edited \ + FAKE_LINES="reword 1" \ + git rebase -i --committer-date-is-author-date \ + --reset-author-date HEAD^ + ) && + test_write_lines edited "" >expect && + git log --format="%B" -1 >actual && + test_cmp expect actual && + test_atime_is_ignored -1 +' + test_expect_success '--reset-author-date --committer-date-is-author-date works when forking merge' ' GIT_SEQUENCE_EDITOR="echo \"merge -C $(git rev-parse HEAD) commit3\">" \ PATH="./test-bin:$PATH" git rebase -i --strategy=test \
base-commit: 0cddd84c9f3e9c3d793ec93034ef679335f35e49
Signature · 1 line
gitgitgadget
Quoted 10 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
baf8ec8d3a(rebase -r: don't write .git/MERGE_MSG when fast-forwarding, 2021-08-20) stopped reading the author script inrun_git_commit()when rewording a commit. This is normally safe because "git commit --amend" preserves the authorship. However if the user passes "--committer-date-is-author-date" then we need to read the author date from the author script when rewording. Fix this regression by tightening the check for when it is safe to skip reading the author script.
That description makes sense, and the patch matches. Not being that familiar with this area, my biggest question would be: are there are other cases that would need the same treatment? And is there a way we can make it easier to avoid forgetting such a case in the future?
Quoted 5 lines
sequencer.c +0
-0
sequencer.c static int run_git_commit(const char *defmsg, · line 1004
Quoted 9 lines
sequencer.c +3
-1@@ -997,7 +997,9 @@ static int run_git_commit(const char *defmsg, cmd.git_cmd =1 ; -if (is_rebase_i (opts) && !(!defmsg && (flags &AMEND_MSG )) && +if (is_rebase_i (opts) && + ((opts->committer_date_is_author_date && !opts->ignore_date) || + !(!defmsg && (flags &AMEND_MSG ))) &&read_env_script (&cmd.env_array)) {const char *gpg_opt =gpg_sign_opt_quoted (opts);
This conditional is getting pretty complicated. I wonder if a helper like:
if (is_rebase_i(opts) && !needs_env_script(...))
might help, but I guess it needs a funky array of inputs (defmsg, flags, and opts). So maybe it is just making things worse.
sequencer.c static int run_git_commit(const char *defmsg,
Quoted 13 lines
sequencer.c +13
-0+test_expect_success '--committer-date-is-author-date works when rewording' ' + GIT_AUTHOR_DATE="@1234 +0300" git commit --amend --reset-author && + ( + set_fake_editor && + FAKE_COMMIT_MESSAGE=edited \ +FAKE_LINES ="reword 1" \ + git rebase -i --committer-date-is-author-date HEAD^ + ) && + test_write_lines edited "" >expect && + git log --format="%B" -1 >actual && + test_cmp expect actual && + test_ctime_is_atime -1 +'
This test make sense (I had to look up what "-1" means for test_ctime_is_atime; it's passed to git-log to decide which commits to look at).
Quoted 4 lines
sequencer.c +4
-0+test_expect_success 'reset-author-date with --committer-date-is-author-date works when rewording' ' + GIT_AUTHOR_DATE="@1234 +0300" git commit --amend --reset-author && + ( + set_fake_editor &&
sequencer.c
Quoted 10 lines
sequencer.c +10
-0+ FAKE_COMMIT_MESSAGE=edited \ +FAKE_LINES ="reword 1" \ + git rebase -i --committer-date-is-author-date \ + --reset-author-date HEAD^ + ) && + test_write_lines edited "" >expect && + git log --format="%B" -1 >actual && + test_cmp expect actual && + test_atime_is_ignored -1 +'
And this one I guess is covering the --ignore-date cut-out in the code? I think it would pass even without it, as that is just noting a case where we _don't_ need to call read_env_script(). But I don't know if there is any user-visible effect of accidentally calling it when we don't need to (my impression is that it's just a performance thing).
-Peff
Hi Peff
Quoted 13 lines · Jeff King
Quoted 10 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
baf8ec8d3a(rebase -r: don't write .git/MERGE_MSG when fast-forwarding, 2021-08-20) stopped reading the author script inrun_git_commit()when rewording a commit. This is normally safe because "git commit --amend" preserves the authorship. However if the user passes "--committer-date-is-author-date" then we need to read the author date from the author script when rewording. Fix this regression by tightening the check for when it is safe to skip reading the author script.
Quoted 4 lines
That description makes sense, and the patch matches. Not being that familiar with this area, my biggest question would be: are there are other cases that would need the same treatment? And is there a way we can make it easier to avoid forgetting such a case in the future?
I don't think there are any other cases (but then I thought that when I wrote the buggy patch...). The only time we change the authorship is if the user passes --committer-date-is-author-date or --reset-author-date. I agree it would be good to have a way to avoid this problem in the future but I haven't come up with an easy way to do that. One possibility would be to go back to always reading the author script. That would mean revisiting the changes to do_merge() in baf8ec8d3a so that it always writes the author script and .git/MERGE_MSG but removes them when fast-forwarding (the problem that baf8ec8d3a tried to solve was a left over .git/MERGE_MSG when do_merge() fast-forwarded) I don't want to do that in the rc window though.
Quoted 18 lines
Quoted 5 lines
sequencer.c +0
-0sequencer.c static int run_git_commit(const char *defmsg, · line 1004
Quoted 9 lines
sequencer.c +3
-1@@ -997,7 +997,9 @@ static int run_git_commit(const char *defmsg, cmd.git_cmd =1 ; -if (is_rebase_i (opts) && !(!defmsg && (flags &AMEND_MSG )) && +if (is_rebase_i (opts) && + ((opts->committer_date_is_author_date && !opts->ignore_date) || + !(!defmsg && (flags &AMEND_MSG ))) &&read_env_script (&cmd.env_array)) {const char *gpg_opt =gpg_sign_opt_quoted (opts);This conditional is getting pretty complicated. I wonder if a helper like:
Quoted 4 lines
if (is_rebase_i(opts) && !needs_env_script(...))might help, but I guess it needs a funky array of inputs (defmsg, flags, and opts). So maybe it is just making things worse.
As you say it needs a lot of inputs so I'm not sure how much a function would help. I did consider changing it to
if (is_rebase_i(opts) &&
((opts->committer_date_is_author_date && !opts->ignore_date) ||
defmsg || !(flags & AMEND_MSG)))but as we're in the rc phase I decided to leave the existing condition alone.
Quoted 9 lines
Quoted 9 lines
sequencer.c +9
-0+test_expect_success '--committer-date-is-author-date works when rewording' ' + GIT_AUTHOR_DATE="@1234 +0300" git commit --amend --reset-author && + ( + set_fake_editor && + FAKE_COMMIT_MESSAGE=edited \ +FAKE_LINES ="reword 1" \ + git rebase -i --committer-date-is-author-date HEAD^ + ) && + test_write_lines edited "" >expect &&
Quoted 8 lines
sequencer.c
Quoted 4 lines
sequencer.c +4
-0+ git log --format="%B" -1 >actual && + test_cmp expect actual && + test_ctime_is_atime -1 +'This test make sense (I had to look up what "-1" means for
test_ctime_is_atime; it's passed to git-log to decide which commits to look at).
Yeah I had to check what the -1 was for when writing the test, maybe we should change the helper to add the '-' for us once 2.34.0 is out.
Quoted 8 lines
Quoted 8 lines
sequencer.c +8
-0+test_expect_success 'reset-author-date with --committer-date-is-author-date works when rewording' ' + GIT_AUTHOR_DATE="@1234 +0300" git commit --amend --reset-author && + ( + set_fake_editor && + FAKE_COMMIT_MESSAGE=edited \ +FAKE_LINES ="reword 1" \ + git rebase -i --committer-date-is-author-date \ + --reset-author-date HEAD^
Quoted 8 lines
sequencer.c
Quoted 6 lines
sequencer.c +6
-0+ ) && + test_write_lines edited "" >expect && + git log --format="%B" -1 >actual && + test_cmp expect actual && + test_atime_is_ignored -1 +'And this one I guess is covering the --ignore-date cut-out in the code?
Yes
Quoted 2 lines
I think it would pass even without it, as that is just noting a case where we _don't_ need to call
read_env_script().
That's right.
Quoted 3 lines
But I don't know if
there is any user-visible effect of accidentally calling it when we don't need to (my impression is that it's just a performance thing).
There should not be any user-visible effects from reading the author script when --ignore-date is given but we don't need to read it in that case so I opted not to.
Thanks for your comments, are you happy for this to go in as is or should I look at simplifying the conditional?
Phillip
Quoted 2 lines
-Peff
Quoted 5 lines · Phillip Wood
Quoted 4 lines
That description makes sense, and the patch matches. Not being that familiar with this area, my biggest question would be: are there are other cases that would need the same treatment? And is there a way we can make it easier to avoid forgetting such a case in the future?
Quoted 10 lines
I don't think there are any other cases (but then I thought that when I wrote the buggy patch...). The only time we change the authorship is if the user passes --committer-date-is-author-date or --reset-author-date. I agree it would be good to have a way to avoid this problem in the future but I haven't come up with an easy way to do that. One possibility would be to go back to always reading the author script. That would mean revisiting the changes to
do_merge()inbaf8ec8d3aso that it always writes the author script and .git/MERGE_MSG but removes them when fast-forwarding (the problem thatbaf8ec8d3atried to solve was a left over .git/MERGE_MSG whendo_merge()fast-forwarded) I don't want to do that in the rc window though.
I suspected the answers to my questions were "I hope so" and "not really", which I think matches what you wrote. :)
If there isn't an easy way to make it more future-proof, then I'm content that you've given it some thought and didn't find any other cases. We can proceed from here with this fix, and be on the lookout for any other cases that people report (on the plus side, the BUG() made it quite obvious that there was a problem, rather than a subtle behavior change).
Quoted 2 lines
Thanks for your comments, are you happy for this to go in as is or should I look at simplifying the conditional?
I'm happy enough with it. I don't know what the plan is for the -rc period, though. AFAICT the bug is in v2.33.1, so it's not technically a v2.34-rc problem. It could wait for the next maint release.
-Peff
Quoted 3 lines · Jeff King
I'm happy enough with it. I don't know what the plan is for the -rc period, though. AFAICT the bug is in v2.33.1, so it's not technically a v2.34-rc problem. It could wait for the next maint release.
Hmph, if it was in v2.33.0 then it is not, but if it was introduced between v2.33.0 and v2.33.1, then it is a problem for the current cycle, no?
Quoted 9 lines · Junio C Hamano
Quoted 3 lines · Jeff King
I'm happy enough with it. I don't know what the plan is for the -rc period, though. AFAICT the bug is in v2.33.1, so it's not technically a v2.34-rc problem. It could wait for the next maint release.
Hmph, if it was in v2.33.0 then it is not, but if it was introduced between v2.33.0 and v2.33.1, then it is a problem for the current cycle, no?
I guess it depends how you consider the cycle. It's in v2.33.1 but not v2.33.0, so yes, anybody going from v2.33.0 to v2.34.0 will see it as a regression. I don't have any problem with fixing it for v2.34.0, and I think the fix Phillip provided is minimally invasive. I just didn't consider it quite the same as a bug that had never been in a released version at all (and I know you are often hesitant to throw too much into the -rc part of the cycle).
-Peff
Quoted 4 lines · Jeff King
... I just didn't
consider it quite the same as a bug that had never been in a released version at all (and I know you are often hesitant to throw too much into the -rc part of the cycle).
Ah, I see.
2.33.1 was 2.33.0 plus what was queued on 'master' back then for 2.34.0, and more importantly, we do not issue the maintenance track very often these days (except for occasional security patches).
To put it differently, I didn't have to issue 2.33.1. There was nothing urgent in there, and it was just that the cycle toward 2.34 was unusually busy and there were too many accumulated fixes on the 'master' front that were 'maint' material. And for that reason, in my mind, 2.33.1 cannot be treated quite the same way as feature releases X.YY.0, when I say "The bug was already present in that released version, so fixing it is not all that urgent".