Re: [PATCH v4] [GSOC] commit: add --trailer option

[ZheNing Hu <adlternative@gmail.com>]

Junio C Hamano <gitster@pobox.com> 于2021年3月15日周一 下午12:38写道：
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> One thing I forgot to mention.
>
> > +static int opt_pass_trailer(const struct option *opt, const char *arg, int unset)
> > +{
> > +     if (unset) {
> > +             strvec_clear(&run_trailer.args);
> > +             return -1;
>
> What input is this part of the code designed to handle, and what
> outcome does it want to show to the user?
>
> Does the test added in this patch cover this codepath, where unset
> is true?
>
> I think this codepath is reached when the command line says
> "--no-trailer", and I am guessing that by clearing the code wants to
> say "ignore the accumulated trailers and start accumulating afresh",
> i.e.
>
>     git commit --trailer='T1: V1' --trailer='T2: V2' \
>                 --no-trailer \
>                 --trailer='T3: V3'
>
> would want to add only the "T3: V3" trailer; which is a sensible
> design, but I do not think returning -1 would be compatible with
> that design.
>
> If on the other hand the code wants to say "--no-trailer is a
> command line error", then using PARSE_OPT_NONEG to let the
> parse-options API issue an error message and die would be more
> appropriate.  That is an often used pattern for an option that can
> appear on the command line only once without accumulating, which may
> be less appropriate for the "--trailer", though.
>

As you said, what I want to express is "--no-trailer is acommand line error".
`--no-trailer` may don't have any big benefits for users.
I will follow you suggections.
> > +     }
> > +     strvec_pushl(&run_trailer.args, "--trailer", arg, NULL);
> > +     return 0;
> > +}
>