From: Jeff King <peff@peff.net>
To: Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
Jonas Kittner <jonas.kittner@ruhr-uni-bochum.de>,
Junio C Hamano <gitster@pobox.com>,
Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH] rebase -i: fix rewording with --committer-date-is-author-date
Date: Tue, 2 Nov 2021 18:32:10 -0400 [thread overview]
Message-ID: <YYG8aq85UmMMVW4l@coredump.intra.peff.net> (raw)
In-Reply-To: <pull.1123.git.git.1635883844710.gitgitgadget@gmail.com>
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?
> 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
next prev parent reply other threads:[~2021-11-02 22:32 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-02 20:10 [PATCH] rebase -i: fix rewording with --committer-date-is-author-date Phillip Wood via GitGitGadget
2021-11-02 21:05 ` Eric Sunshine
2021-11-02 21:29 ` Phillip Wood
2021-11-02 21:30 ` [PATCH v2] " Phillip Wood via GitGitGadget
2021-11-02 22:32 ` Jeff King [this message]
2021-11-03 11:23 ` [PATCH] " Phillip Wood
2021-11-03 11:42 ` Jeff King
2021-11-03 17:44 ` Junio C Hamano
2021-11-04 2:03 ` Jeff King
2021-11-04 6:27 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YYG8aq85UmMMVW4l@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=jonas.kittner@ruhr-uni-bochum.de \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.