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

[Jeff King <peff@peff.net>]

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)

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.

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"),

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

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.

-Peff