On Tue, Nov 02, 2021 at 08:10:44PM +0000, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
>
> 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?
> 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.
> +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
