Re: [PATCH v4] [GSOC] commit: add --trailer option
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 15, 2021, 04:38 UTC
- Message-ID
- <xmqqczw12fqg.fsf@gitster.g>
- In-Reply-To
- <pull.901.v4.git.1615737505834.gitgitgadget@gmail.com>
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
One thing I forgot to mention.
Show 5 quoted lines
> +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.
> + } > + strvec_pushl(&run_trailer.args, "--trailer", arg, NULL); > + return 0; > +}