From: Phillip Wood Date: Fri, 23 Oct 2020 14:06:01 GMT Subject: Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident Message-ID: <9acc1d9f-7646-3ef9-b744-e47c4871873d@gmail.com> In-Reply-To: <20201023072630.GA2918369@coredump.intra.peff.net> Hi Peff On 23/10/2020 08:26, Jeff King wrote: > 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. Makes sense and it simplifies the code nicely Thanks Phillip > Signed-off-by: Jeff King > --- > 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; >