Re: [PATCH 3/3] parse-options: remove PARSE_OPT_NEGHELP
- From
Jeff King <peff@peff.net>
- Date
- Feb 27, 2012, 18:25 UTC
- Message-ID
- <20120227182504.GA1600@sigill.intra.peff.net>
- In-Reply-To
- <4F49336C.3000303@lsrfire.ath.cx>
On Sat, Feb 25, 2012 at 08:15:56PM +0100, René Scharfe wrote:
Show 31 quoted lines
> 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