Show 35 quoted lines
> On Fri, Oct 23, 2020 at 03:09:39AM -0400, Jeff King wrote:
>
>> Commit e8cbe2118a (am: stop exporting GIT_COMMITTER_DATE, 2020-08-17)
>> rewrote the code for setting the committer date to use fmt_ident(),
>> rather than setting an environment variable and letting commit_tree()
>> handle it. But it introduced two bugs:
>>
>> - we use the author email string instead of the committer email
>>
>> - when parsing the committer ident, we used the wrong variable to
>> compute the length of the email, resulting in it always being a
>> zero-length string
>
> By the way, I wondered why we needed to do this parsing at all. The
> patch below does this in a much simpler way. It's a little bit ugly, I
> think, because we have to call getenv() ourselves. But that's the way
> fmt_ident() has always worked. We could probably improve that now that
> it takes a whose_ident flag (before that, it had no idea if we wanted
> author or committer ident).
>
> This is on top of the fixes (but we'd perhaps just want to do those on
> 'maint' as the minimal fix).
>
> -- >8 --
> Subject: [PATCH 4/3] am, sequencer: stop parsing our own committer ident
>
> For the --committer-date-is-author-date option of git-am and git-rebase,
> we format the committer ident, then re-parse it to find the name and
> email, and then feed those back to fmt_ident().
>
> We can simplify this by handling it all at the time of the fmt_ident()
> call. We pass in the appropriate getenv() results, and if they're not
> present, then our WANT_COMMITTER_IDENT flag tells fmt_ident() to fill in
> the appropriate value from the config. Which is exactly what
> git_committer_ident() was doing under the hood.
Show 146 quoted lines
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/am.c | 19 +++----------------
> sequencer.c | 28 ++--------------------------
> sequencer.h | 2 --
> 3 files changed, 5 insertions(+), 44 deletions(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 4949535a7f..52206bc56b 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -98,8 +98,6 @@ struct am_state {
> char *author_name;
> char *author_email;
> char *author_date;
> - char *committer_name;
> - char *committer_email;
> char *msg;
> size_t msg_len;
>
> @@ -132,8 +130,6 @@ struct am_state {
> */
> static void am_state_init(struct am_state *state)
> {
> - const char *committer;
> - struct ident_split id;
> int gpgsign;
>
> memset(state, 0, sizeof(*state));
> @@ -154,14 +150,6 @@ static void am_state_init(struct am_state *state)
>
> if (!git_config_get_bool("commit.gpgsign", &gpgsign))
> state->sign_commit = gpgsign ? "" : NULL;
> -
> - committer = git_committer_info(IDENT_STRICT);
> - if (split_ident_line(&id, committer, strlen(committer)) < 0)
> - die(_("invalid committer: %s"), committer);
> - state->committer_name =
> - xmemdupz(id.name_begin, id.name_end - id.name_begin);
> - state->committer_email =
> - xmemdupz(id.mail_begin, id.mail_end - id.mail_begin);
> }
>
> /**
> @@ -173,8 +161,6 @@ static void am_state_release(struct am_state *state)
> free(state->author_name);
> free(state->author_email);
> free(state->author_date);
> - free(state->committer_name);
> - free(state->committer_email);
> free(state->msg);
> strvec_clear(&state->git_apply_opts);
> }
> @@ -1594,8 +1580,9 @@ static void do_commit(const struct am_state *state)
> IDENT_STRICT);
>
> if (state->committer_date_is_author_date)
> - committer = fmt_ident(state->committer_name,
> - state->committer_email, WANT_COMMITTER_IDENT,
> + committer = fmt_ident(getenv("GIT_COMMITTER_NAME"),
> + getenv("GIT_COMMITTER_EMAIL"),
> + WANT_COMMITTER_IDENT,
> state->ignore_date ? NULL
> : state->author_date,
> IDENT_STRICT);
> diff --git a/sequencer.c b/sequencer.c
> index d76cbded00..07321a7d95 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -314,8 +314,6 @@ int sequencer_remove_state(struct replay_opts *opts)
> }
> }
>
> - free(opts->committer_name);
> - free(opts->committer_email);
> free(opts->gpg_sign);
> free(opts->strategy);
> for (i = 0; i < opts->xopts_nr; i++)
> @@ -1460,8 +1458,8 @@ static int try_to_commit(struct repository *r,
> } else {
> reset_ident_date();
> }
> - committer = fmt_ident(opts->committer_name,
> - opts->committer_email,
> + committer = fmt_ident(getenv("GIT_COMMITTER_NAME"),
> + getenv("GIT_COMMITTER_EMAIL"),
> WANT_COMMITTER_IDENT,
> opts->ignore_date ? NULL : date.buf,
> IDENT_STRICT);
> @@ -4467,22 +4465,6 @@ static int commit_staged_changes(struct repository *r,
> return 0;
> }
>
> -static int init_committer(struct replay_opts *opts)
> -{
> - struct ident_split id;
> - const char *committer;
> -
> - committer = git_committer_info(IDENT_STRICT);
> - if (split_ident_line(&id, committer, strlen(committer)) < 0)
> - return error(_("invalid committer '%s'"), committer);
> - opts->committer_name =
> - xmemdupz(id.name_begin, id.name_end - id.name_begin);
> - opts->committer_email =
> - xmemdupz(id.mail_begin, id.mail_end - id.mail_begin);
> -
> - return 0;
> -}
> -
> int sequencer_continue(struct repository *r, struct replay_opts *opts)
> {
> struct todo_list todo_list = TODO_LIST_INIT;
> @@ -4494,9 +4476,6 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
> if (read_populate_opts(opts))
> return -1;
> if (is_rebase_i(opts)) {
> - if (opts->committer_date_is_author_date && init_committer(opts))
> - return -1;
> -
> if ((res = read_populate_todo(r, &todo_list, opts)))
> goto release_todo_list;
>
> @@ -5391,9 +5370,6 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>
> res = -1;
>
> - if (opts->committer_date_is_author_date && init_committer(opts))
> - goto cleanup;
> -
> if (checkout_onto(r, opts, onto_name, &oid, orig_head))
> goto cleanup;
>
> diff --git a/sequencer.h b/sequencer.h
> index b2a501e445..f925e349c5 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -50,8 +50,6 @@ struct replay_opts {
>
> int mainline;
>
> - char *committer_name;
> - char *committer_email;
> char *gpg_sign;
> enum commit_msg_cleanup_mode default_msg_cleanup;
> int explicit_cleanup;
>