Re: [PATCH] various: disallow --no-no-OPT for --no-opt options

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 19, 2017, 02:40 UTC
Message-ID: <xmqqh91lce6y.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170419014143.56io56xn6mawy5xi@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 18 quoted lines

> On Wed, Apr 19, 2017 at 12:29:18AM +0200, René Scharfe wrote:
> ...
>> PARSE_OPT_NONEG should only be used for options where a negation doesn't
>> make sense, e.g. for the --stage option of checkout-index.
>
> I think we do strive to avoid "--no-no-foo", and instead have "--no-foo"
> and "--foo" to cover both sides.  So for example:
>
>> > -		OPT_BOOL(0, "no-add", &state->no_add,
>> > +		OPT_BOOL_NONEG(0, "no-add", &state->no_add,
>> >   			N_("ignore additions made by the patch")),
>
> This could be more like:
>
>   OPT_NEGBOOL(0, "add", &state->no_add, ...)
>
> where NEGBOOL would be smart enough to show "--no-add" in the help as
> the primary.

I very much appreciate that this topic to avoid --no-no-OPT nonsense, but just disabling --no-no-OPT without giving --OPT the meaning the user who would have used --no-no-OPT wanted does not sound like a good solution. Your NEGBOOL looks like a better approach.

> It might even be possible to detect the existing line and
> have parse-options automatically respect "--foo" when "--no-foo" is
> present.  But that may run afoul of callers that add both "--foo" and
> "--no-foo" manually.

True but wouldn't that something we would want to avoid anyway? That is, "git cmd [--OPT | --no-OPT | --no-no-OPT]" from the end user's point of view should be an error because it is unclear what difference there are between --OPT and --no-no-OPT. And we should be able to add a rule to parse_options_check() to catch such an error.

Having said that, I am not sure if we want to go the route of "existing line that begins with 'no-' behaves magical". For boolean, I suspect we may be get away with such a magic without confusing ourselves too much, though.

Thanks.