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

["René Scharfe" <rene.scharfe@lsrfire.ath.cx>]

Am 27.02.2012 19:25, schrieb Jeff King:
> On Sat, Feb 25, 2012 at 08:15:56PM +0100, René Scharfe wrote:
>
>> diff --git a/builtin/grep.c b/builtin/grep.c
>> index e4ea900..b151467 100644
>> --- a/builtin/grep.c
>> +++ b/builtin/grep.c
>> @@ -671,7 +671,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>>   	struct string_list path_list = STRING_LIST_INIT_NODUP;
>>   	int i;
>>   	int dummy;
>> -	int use_index = 1;
>> +	int no_index = 0;
>>   	enum {
>>   		pattern_type_unspecified = 0,
>>   		pattern_type_bre,
>> @@ -684,9 +684,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>>   	struct option options[] = {
>>   		OPT_BOOLEAN(0, "cached",&cached,
>>   			"search in index instead of in the work tree"),
>> -		{ OPTION_BOOLEAN, 0, "index",&use_index, NULL,
>> -			"finds in contents not managed by git",
>> -			PARSE_OPT_NOARG | PARSE_OPT_NEGHELP },
>> +		OPT_BOOL(0, "no-index",&no_index,
>> +			 "finds in contents not managed by git"),
>>   		OPT_BOOLEAN(0, "untracked",&untracked,
>>   			"search in both tracked and untracked files"),
>>   		OPT_SET_INT(0, "exclude-standard",&opt_exclude,
>> @@ -851,7 +850,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>>   		break; /* nothing */
>>   	}
>>
>> -	if (use_index&&  !startup_info->have_repository)
>> +	if (!no_index&&  !startup_info->have_repository)

[Unrelated: The whitespace in the two lines above and before ampersands 
in general was damaged by Thunderbird.  First time I noticed.]

> Hmm. We usually try to avoid these sorts of double negations in the
> code, as they can often make the logic hard to read. In this case, it is
> not _so_ bad, because out of the 4 uses of use_index/no_index, only one
> is "!no_index", and it is in a relatively simple conditional.

The variable could be named "unmanaged", "external" or similar instead 
of "no_index".  The latter just matches the option name and thus was the 
obvious first choice to me.

> But I do feel like the original was slightly easier to read, and that
> getting rid of NEGHELP is restricting how the developer can express the
> options.
>
> I think your original motivation was that NEGHELP lead to confusion
> where the name of the option does not match its description. Would it be
> better to simply be explicit that an option is a reversed boolean (i.e.,
> what the user specifies on the command line and what is in the code are
> naturally opposites). Like:
>
>   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.

> Using NEGHELP, the "reverse" is between the option name and the
> description, which is very subtle. Here it is between the option name
> and the variable, which is hopefully a little more explicit (especially
> with the big REVERSE in the macro name).

We have precedence for OPT_NEGBIT in grep, although the double negations 
for -h and --full-name are required because both turn off bits that 
other options turn on, while for --no-index it wouldn't be strictly 
needed, as there is no option that overrules it except --index.

I don't care too much either way, though.  The changes from patch 2 (the 
no no-no one) are not restricted to OPT_BOOL.

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

René