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

[Patrick Steinhardt <ps@pks.im>]

On Sun, Nov 09, 2025 at 10:22:54AM +0000, ZheNing Hu via GitGitGadget wrote:
> From: ZheNing Hu <adlternative@gmail.com>
> 
> Add --committer option to git-commit, allowing users to override the
> committer identity similar to how --author works. This provides a more
> convenient alternative to setting GIT_COMMITTER_* environment variables.

Yeah, I can see how that's useful.

> diff --git a/Documentation/git-commit.adoc b/Documentation/git-commit.adoc
> index 54c207ad45..a015c8328e 100644
> --- a/Documentation/git-commit.adoc
> +++ b/Documentation/git-commit.adoc
> @@ -12,7 +12,7 @@ git commit [-a | --interactive | --patch] [-s] [-v] [-u[<mode>]] [--amend]
>  	   [--dry-run] [(-c | -C | --squash) <commit> | --fixup [(amend|reword):]<commit>]
>  	   [-F <file> | -m <msg>] [--reset-author] [--allow-empty]
>  	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
> -	   [--date=<date>] [--cleanup=<mode>] [--[no-]status]
> +	   [--date=<date>] [--committer=<committer>] [--cleanup=<mode>] [--[no-]status]
>  	   [-i | -o] [--pathspec-from-file=<file> [--pathspec-file-nul]]
>  	   [(--trailer <token>[(=|:)<value>])...] [-S[<keyid>]]
>  	   [--] [<pathspec>...]

Nit: I'd move `--committer` before `--date` so that it comes directly
after `--author`.

> @@ -181,6 +181,13 @@ See linkgit:git-rebase[1] for details.
>  `--date=<date>`::
>  	Override the author date used in the commit.
>  
> +`--committer=<committer>`::
> +	Override the committer for the commit. Specify an explicit committer using the
> +	standard `A U Thor <committer@example.com>` format. Otherwise _<committer>_
> +	is assumed to be a pattern and is used to search for an existing
> +	commit by that author (i.e. `git rev-list --all -i --author=<committer>`);
> +	the commit author is then copied from the first such commit found.

This matches the description of `--author`.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 0243f17d53..88e77cbaab 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -690,6 +691,48 @@ static void determine_author_info(struct strbuf *author_ident)
>  	free(date);
>  }
>  
> +static void determine_committer_info(struct strbuf *committer_ident)
> +{
> +	char *name, *email, *date;
> +	struct ident_split committer;
> +
> +	name = xstrdup_or_null(getenv("GIT_COMMITTER_NAME"));
> +	email = xstrdup_or_null(getenv("GIT_COMMITTER_EMAIL"));
> +	date = xstrdup_or_null(getenv("GIT_COMMITTER_DATE"));
> +
> +	if (force_committer) {
> +		struct ident_split ident;
> +
> +		if (split_ident_line(&ident, force_committer, strlen(force_committer)) < 0)
> +			die(_("malformed --committer parameter"));
> +		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));
> +
> +		if (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_addch(&date_buf, ' ');
> +			strbuf_add(&date_buf, ident.tz_begin, ident.tz_end - ident.tz_begin);
> +			set_ident_var(&date, strbuf_detach(&date_buf, NULL));
> +		}
> +	}
> +
> +	if (force_date) {
> +		struct strbuf date_buf = STRBUF_INIT;
> +		if (parse_force_date(force_date, &date_buf))
> +			die(_("invalid date format: %s"), force_date);
> +		set_ident_var(&date, strbuf_detach(&date_buf, NULL));
> +	}
> +
> +	strbuf_addstr(committer_ident, fmt_ident(name, email, WANT_COMMITTER_IDENT, date,
> +				IDENT_STRICT));
> +	assert_split_ident(&committer, committer_ident);
> +	free(name);
> +	free(email);
> +	free(date);
> +}
> +
>  static int author_date_is_interesting(void)
>  {
>  	return author_message || force_date;

A lot of the infra in this new function is shared with
`determine_author_info()`. It would be great if we could refactor it so
that the common parts are shared given that this all is quite
non-trivial.

Maybe we could have something like `determine_identity()` that contains
the common bits between both functions? It might ultimately not really
be worth it, but at least the functionality in the `force_committer`
condition feels like it should be pulled out.

> @@ -1321,6 +1364,9 @@ static int parse_and_validate_options(int argc, const char *argv[],
>  	if (force_author && renew_authorship)
>  		die(_("options '%s' and '%s' cannot be used together"), "--reset-author", "--author");
>  
> +	if (force_committer && !strchr(force_committer, '>'))
> +		force_committer = find_author_by_nickname(force_committer);
> +
>  	if (logfile || have_option_m || use_message)
>  		use_editor = 0;
>  

Is it the right thing to search by author here? Shouldn't we rather be
searching by committer?

> @@ -1930,8 +1978,13 @@ int cmd_commit(int argc,
>  		append_merge_tag_headers(parents, &tail);
>  	}
>  
> +	if (force_committer) {
> +		determine_committer_info(&committer_ident);
> +	}
> +

Nit: we tend to not use braces around single-line bodies.

Thanks!

Patrick