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

[Junio C Hamano <gitster@pobox.com>]

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

> +	}
> +	strvec_pushl(&run_trailer.args, "--trailer", arg, NULL);
> +	return 0;
> +}