git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
René Scharfe <rene.scharfe@lsrfire.ath.cx>
Date
Feb 27, 2012, 22:26 UTC
Message-ID
<4F4C0308.2050804@lsrfire.ath.cx>
In-Reply-To
<20120227182504.GA1600@sigill.intra.peff.net>
Am 27.02.2012 19:25, schrieb Jeff King:
Show 33 quoted lines
> 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.

Show 12 quoted lines
> 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.

Show 6 quoted lines
> 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é
Previous: Junio C HamanoNext: Jeff King
Message 16 of 19 in “parse-options: no- symmetry”
  1. 0/3 parse-options: no- symmetryRené Scharfe, Feb 25, 2012
  2. 1/3 test-parse-options: convert to OPT_BOOL()René Scharfe, Feb 25, 2012
  3. 2/3 parse-options: allow positivation of options starting, with no-René Scharfe, Feb 25, 2012
  4. Junio C HamanoFeb 26, 2012
  5. Thomas RastFeb 27, 2012
  6. Junio C HamanoFeb 27, 2012
  7. René ScharfeFeb 27, 2012
  8. Junio C HamanoFeb 27, 2012
  9. 4/3 parse-options: disallow --no-no-sthRené Scharfe, Feb 28, 2012
  10. Junio C HamanoFeb 28, 2012
  11. René ScharfeFeb 29, 2012
  12. Junio C HamanoFeb 29, 2012
  13. 3/3 parse-options: remove PARSE_OPT_NEGHELPRené Scharfe, Feb 25, 2012
  14. Jeff KingFeb 27, 2012
  15. Junio C HamanoFeb 27, 2012
  16. René ScharfeFeb 27, 2012
  17. Jeff KingFeb 28, 2012
  18. 3/3 parse-options: remove PARSE_OPT_NEGHELPRené Scharfe, Feb 28, 2012
  19. Jeff KingFeb 28, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.