Re: [PATCH 3/3] parse-options: remove PARSE_OPT_NEGHELP

[Jeff King <peff@peff.net>]

On Mon, Feb 27, 2012 at 11:26:16PM +0100, René Scharfe wrote:

> >  OPT_REVERSE_BOOL(0, "no-index",&use_index,
> >              "finds in contents not managed by git"),
> 
> It's better than NEGHELP, but I find your use of two negations
> (REVERSE and "no-") confusing.  We don't need to invent new OPT_
> types for this, by the way, we can just do this:
> 
> 	OPT_NEGBIT(0, "no-index", &use_index,
> 	           "finds in contents not managed by git", 1),
> 
> It certainly shortens the patch.

Right. The point is that the code and the option want to look at the
conditional in two different ways. So you have to have negate it
_somewhere_. Either at point of use (your original patch), between the
option name and the variable name (what I wrote above), or using a
reversed help text (the original code before your patch).

I think we both agree that NEGHELP is the worst one. I have a slight
preference for doing the reversal at the point of the option definition,
if only because we know it happens at a single point, and it lets the
actual code (which is usually the trickier part) be more clear. But
beyond that, I think it is largely a matter of aesthetics.

> >I dunno. Given that there are only two uses of NEGHELP, and that they
> >don't come out too badly, I don't care _too_ much. But I have seen some
> >really tortured logic with double-negations like this, and I'm concerned
> >that a few months down the road somebody is going to want NEGHELP (or
> >something similar) in a case where it actually does really impact
> >readability.
> 
> I'm curious to see a case that can be solved better using NEGHELP,
> but we can always add it back if we find such a beast.  I'd much
> rather see it go until then because of it's non-obvious semantics.

Yeah, I should have spelled out my "or something similar" more. I'd
rather see something like NEGBIT above than NEGHELP; the latter is a bit
too subtle.

-Peff