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é
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html