From: ZheNing Hu Date: Mon, 10 Nov 2025 14:17:07 GMT Subject: Re: [PATCH] commit: add --committer option Message-ID: In-Reply-To: Patrick Steinhardt 于2025年11月10日周一 17:24写道： > > On Sun, Nov 09, 2025 at 10:22:54AM +0000, ZheNing Hu via GitGitGadget wrote: > > From: ZheNing Hu > > > > 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. > I'm glad we're aligned on this. > > 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[]] [--amend] > > [--dry-run] [(-c | -C | --squash) | --fixup [(amend|reword):]] > > [-F | -m ] [--reset-author] [--allow-empty] > > [--allow-empty-message] [--no-verify] [-e] [--author=] > > - [--date=] [--cleanup=] [--[no-]status] > > + [--date=] [--committer=] [--cleanup=] [--[no-]status] > > [-i | -o] [--pathspec-from-file= [--pathspec-file-nul]] > > [(--trailer [(=|:)])...] [-S[]] > > [--] [...] > > Nit: I'd move `--committer` before `--date` so that it comes directly > after `--author`. > Agreed, will fix. > > @@ -181,6 +181,13 @@ See linkgit:git-rebase[1] for details. > > `--date=`:: > > Override the author date used in the commit. > > > > +`--committer=`:: > > + Override the committer for the commit. Specify an explicit committer using the > > + standard `A U Thor ` format. Otherwise __ > > + 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=`); > > + the commit author is then copied from the first such commit found. > > This matches the description of `--author`. > Agreed, I will fix it to use committer description. > > 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. > Good suggestion, I will refactor them to use `determine_identity()`, which should be more generic, and it even caught that I missed updating `GIT_COMMITTER_*`. > > @@ -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? > Yes, there should also be a `find_identity_by_nickname()` or `find_committer_by_nickname()` here. > > @@ -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. > Agree. > Thanks! > > Patrick