From: ZheNing Hu <adlternative@gmail.com>
Date: Sat, 15 Nov 2025 06:33:28 GMT
Subject: Re: [PATCH v3] commit: add --committer option
Message-ID: <CAOLTT8SfLGnov2ZT5s7fz+DiN0fW-VFjFDVzv0J5GuwevMX2Kw@mail.gmail.com>
In-Reply-To: <xmqqfrajqdv0.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> 于2025年11月13日周四 02:56写道：
>
> "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).
>

Since both `--author=<author>` and `--date=<date>` use the
"Override the *" phrasing,  `--committer` uses "Override the" just
to maintain consistency, though changing it to "Set the" would
also be acceptable.

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

Ok, will change.

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

Indeed, some specific logic should be moved to
determine_author_info()/determine_committer_info().

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

Ok.

> or something simple here?