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

[Phillip Wood <phillip.wood123@gmail.com>]

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

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


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

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