Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident

[Phillip Wood <phillip.wood123@gmail.com>]

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 <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;
>