Re: [PATCH] rebase -i: fix rewording with --committer-date-is-author-date
From: Jeff King <hidden>
Date: 2021-11-02 22:32:12
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?
quoted hunk ↗ jump to hunk
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