Re: [PATCH v3] commit: add --committer option

[Junio C Hamano <gitster@pobox.com>]

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +`--committer=<committer>`::
> +	Override the committer for the commit. Specify an explicit committer using the

Isn't "set" or "use" more appropirate verb to use here?

We already take the committer identity from multiple plases, like
user.{name,email}, or GIT_COMMITTER_{NAME,EMAIL} configuration, and
with the patch we also take it from a command line option, with the
usual precedence order (i.e., command line trumps environment which
trumps configuration).

> -static void determine_author_info(struct strbuf *author_ident)
> +static void determine_identity(struct strbuf *ident_str, int is_author)

"is_author" does not sound grammatical for this case; if you are
giving an ident of an unknown kind to this function and supplying
another parameter to let it know which kind, "is_author" may make
sense, but not here.

As you will convert it into WANT_{AUTHOR,COMMITTER}_IDENT before
using anyway, why not let the caller use the "enum want_ident" to
tell this function what to do?

>  {
>  	char *name, *email, *date;
> -	struct ident_split author;
> -
> -	name = xstrdup_or_null(getenv("GIT_AUTHOR_NAME"));
> -	email = xstrdup_or_null(getenv("GIT_AUTHOR_EMAIL"));
> -	date = xstrdup_or_null(getenv("GIT_AUTHOR_DATE"));
> -
> -	if (author_message) {
> -		struct ident_split ident;
> +	struct ident_split ident;
> +	const char *env_name = is_author ? "GIT_AUTHOR_NAME" : "GIT_COMMITTER_NAME";
> +	const char *env_email = is_author ? "GIT_AUTHOR_EMAIL" : "GIT_COMMITTER_EMAIL";
> +	const char *env_date = is_author ? "GIT_AUTHOR_DATE" : "GIT_COMMITTER_DATE";
> +	const char *force_ident = is_author ? force_author : force_committer;
> +	const char *param_name = is_author ? "--author" : "--committer";
> +	int ident_flag = is_author ? WANT_AUTHOR_IDENT : WANT_COMMITTER_IDENT;
> +
> +	name = xstrdup_or_null(getenv(env_name));
> +	email = xstrdup_or_null(getenv(env_email));
> +	date = xstrdup_or_null(getenv(env_date));
> +
> +	if (is_author && author_message) {
> +		struct ident_split msg_ident;
>  		size_t len;
>  		const char *a;
>  
>  		a = find_commit_header(author_message_buffer, "author", &len);
>  		if (!a)
>  			die(_("commit '%s' lacks author header"), author_message);
> -		if (split_ident_line(&ident, a, len) < 0)
> +		if (split_ident_line(&msg_ident, a, len) < 0)
>  			die(_("commit '%s' has malformed author line"), author_message);
>  
> -		set_ident_var(&name, xmemdupz(ident.name_begin, ident.name_end - ident.name_begin));
> -		set_ident_var(&email, xmemdupz(ident.mail_begin, ident.mail_end - ident.mail_begin));
> +		set_ident_var(&name, xmemdupz(msg_ident.name_begin, msg_ident.name_end - msg_ident.name_begin));
> +		set_ident_var(&email, xmemdupz(msg_ident.mail_begin, msg_ident.mail_end - msg_ident.mail_begin));
>  
> -		if (ident.date_begin) {
> +		if (msg_ident.date_begin) {
>  			struct strbuf date_buf = STRBUF_INIT;
>  			strbuf_addch(&date_buf, '@');
> -			strbuf_add(&date_buf, ident.date_begin, ident.date_end - ident.date_begin);
> +			strbuf_add(&date_buf, msg_ident.date_begin, msg_ident.date_end - msg_ident.date_begin);
>  			strbuf_addch(&date_buf, ' ');
> -			strbuf_add(&date_buf, ident.tz_begin, ident.tz_end - ident.tz_begin);
> +			strbuf_add(&date_buf, msg_ident.tz_begin, msg_ident.tz_end - msg_ident.tz_begin);
>  			set_ident_var(&date, strbuf_detach(&date_buf, NULL));
>  		}
>  	}

The helper tries to be generic between both kinds of ident, but we
still need conditional that says "this part of the function is only
when we are looking for author", which is rather unsatisfactory.

Also why do we need this much patch noise, only because you renamed
one variable?  I wonder if it would make it cleaner to move the body
of this if() {} statement into a separate helper function, leaving
only

	if (whose_ident == WANT_AUTHOR_IDENT)
		set_author_from_message(&name, &email, &date);

or something simple here?