[PATCH] rebase -i: fix rewording with --committer-date-is-author-date

From: Phillip Wood <redacted>

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.

Signed-off-by: Phillip Wood <redacted>
---
    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(-)

diff --git a/sequencer.c b/sequencer.c
index cd2aabf1f76..ea96837cde3 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -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);
 

diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
index 4d106642ba7..94671d3c465 100755
--- a/t/t3436-rebase-more-options.sh
+++ b/t/t3436-rebase-more-options.sh

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

On Tue, Nov 2, 2021 at 4:10 PM Phillip Wood via GitGitGadget
[off-list ref] wrote:

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.

Signed-off-by: Phillip Wood <redacted>

Should this have a:

    Reported-by: Jonas Kittner [off-list ref]

?

Hi Eric

On 02/11/2021 21:05, Eric Sunshine wrote:

On Tue, Nov 2, 2021 at 4:10 PM Phillip Wood via GitGitGadget
[off-list ref] wrote:

quoted

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.

Signed-off-by: Phillip Wood <redacted>

Should this have a:

     Reported-by: Jonas Kittner [off-list ref]

Yes it should and does locally but I forgot to push the updated version.

Thanks

Phillip

?

From: Phillip Wood <redacted>

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.

Reported-by: Jonas Kittner <redacted>
Signed-off-by: Phillip Wood <redacted>
---
    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 [off-list ref]
          Signed-off-by: Phillip Wood [off-list ref]
      
       ## sequencer.c ##


 sequencer.c                    |  4 +++-
 t/t3436-rebase-more-options.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index cd2aabf1f76..ea96837cde3 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -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);
 

diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
index 4d106642ba7..94671d3c465 100755
--- a/t/t3436-rebase-more-options.sh
+++ b/t/t3436-rebase-more-options.sh

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

On Tue, Nov 02, 2021 at 08:10:44PM +0000, Phillip Wood via GitGitGadget wrote:

From: Phillip Wood <redacted>

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.

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?

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.

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

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

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

On 02/11/2021 22:32, Jeff King wrote:

On Tue, Nov 02, 2021 at 08:10:44PM +0000, Phillip Wood via GitGitGadget wrote:

quoted

From: Phillip Wood <redacted>

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.

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

diff --git a/sequencer.c b/sequencer.c
index cd2aabf1f76..ea96837cde3 100644
--- a/sequencer.c
+++ b/sequencer.c

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

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

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

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

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

And this one I guess is covering the --ignore-date cut-out in the code?

Yes

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.

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

-Peff

On Wed, Nov 03, 2021 at 11:23:28AM +0000, Phillip Wood wrote:

quoted

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.

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

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