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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 12, 2025, 18:56 UTC
Message-ID: <xmqqfrajqdv0.fsf@gitster.g>
In-Reply-To: <pull.1997.v3.git.1762966535495.gitgitgadget@gmail.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?

Show 51 quoted lines

>  {
>  	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?