Re: [RFC/PATCH] grep --no-index: allow to grep without git exclusions

Taken in total isolation, this patch does allow a use case where we did
not allow, but when considered in a larger picture of what "grep" is used
for and how different use cases the command should support, a few random
thoughts come to mind:

 - Like "diff --no-index", "grep --no-index" is about a directory that is
   not managed by git at all with random collection of files. Do we even
   want to be using any "exclude" in such a use case? Wouldn't it actually
   be a bug that we pay attention to standard-exlcludes in the current
   code, as .gitignore files scattered in such a directory should _not_
   mean anything, as it is not a git working tree at all?

 - Even in a git managed directory, you _could_ use "grep --no-index" to
   find hits from both tracked and untracked files. In this particular use
   case, it makes some sense to pay attention to "exclude", as that would
   catch what _could_ be committed, and paths that would be excluded won't
   be part of that set (unless you use "add -f"). But wouldn't that use
   case better be covered by a switch that is different from --no-index
   (which means "These are not managed by git at all")? It is still about
   files in a git working tree, it is just that the user wants us to pay
   attention also to untracked files, e.g. "grep --untracked-too"?

So I think the patch identified a good problem to solve, but it might be a
wrong solution that encourages a use of a wrong option (i.e. --no-index)
only because we do not have the right one (i.e. "I am in the working tree,
but I want untracked ones also considered.").

What do people think if we did this a bit differently?

 - Since 3081623 (grep --no-index: allow use of "git grep" outside a git
   repository, 2010-01-15) and 59332d1 (Resurrect "git grep --no-index",
   2010-02-06), "grep --no-index" incorrectly paid attention to the
   exclude patterns. We shouldn't have, and we'd fix that bug.

 - It might be useful to be able to "git grep" both tracked and untracked
   (i.e. new files you may want to "git add") paths, but there is no good
   way to do so. Introduce a new option --untracked-too (or more suitable
   name --- I am bad at naming and not married to this one) to allow
   this. This mode always takes "exclude" into account.

Opinions?

Regarding the patch:

+	/* --no-exclude-standard needs --no-index */
+	if (use_index && !exclude_standard)
+		die(_("--no-exclude-standard does not make sense without --no-index."));

For that matter,

    $ git grep --no-exclude-standard --exclude-standard --cached -e foo

should be an error, no?

2011/7/21 Bert Wesarg [off-list ref]:

It should be. But I think that unveils one of the shortcomings of the
(any) option parser: You wont get notified when an option was given,
regardless of its value. To handle the above I would have to use
OPTION_CALLBACK to set an addition flag exc_given (like it is done in
git-ls-files) and test against this. The same problem is possible for
a number option, if you want to know whether the option was actually
given on the command line, one need to invent an invalid value (which
isn't always possible) and use this as the initializer or use
OPTION_CALLBACK again.

Here is a proof-of-concept patch (probably whitespace damaged) to make
it more clear what I meant:

--- >8 ---

Subject: [PoC/RFC/PATCH] parse-option: flag the presence of an option regardless
 of its value

Signed-off-by: Bert Wesarg <redacted>

---
 parse-options.c |   15 +++++++++++++--
 parse-options.h |   20 ++++++++++++++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 73bd28a..df0f483 100644
--- a/parse-options.c
+++ b/parse-options.c

@@ -156,7 +156,10 @@ static int parse_short_opt(struct parse_opt_ctx_t

*p, const struct option *optio
 	for (; options->type != OPTION_END; options++) {
 		if (options->short_name == *p->opt) {
 			p->opt = p->opt[1] ? p->opt + 1 : NULL;
-			return get_value(p, options, OPT_SHORT);
+			if (options->given) {
+				*options->given = 1;
+				return get_value(p, options, OPT_SHORT);
+			}
 		}

 		/*

@@ -171,6 +174,9 @@ static int parse_short_opt(struct parse_opt_ctx_t

*p, const struct option *optio
 		char *arg;
 		int rc;

+		if (options->given)
+			*options->given = 1;
+
 		while (isdigit(p->opt[len]))
 			len++;
 		arg = xmemdupz(p->opt, len);

@@ -254,6 +260,8 @@ is_abbreviated:
 				continue;
 			p->opt = rest + 1;
 		}
+		if (options->given)
+			*options->given = 1;
 		return get_value(p, options, flags);
 	}

@@ -276,8 +284,11 @@ static int parse_nodash_opt(struct

parse_opt_ctx_t *p, const char *arg,
 	for (; options->type != OPTION_END; options++) {
 		if (!(options->flags & PARSE_OPT_NODASH))
 			continue;
-		if (options->short_name == arg[0] && arg[1] == '\0')
+		if (options->short_name == arg[0] && arg[1] == '\0') {
+			if (options->given)
+				*options->given = 1;
 			return get_value(p, options, OPT_SHORT);
+		}
 	}
 	return -2;
 }

diff --git a/parse-options.h b/parse-options.h
index d1b12fe..7fe3d49 100644
--- a/parse-options.h
+++ b/parse-options.h

@@ -100,6 +100,10 @@ typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
  *   OPTION_{BIT,SET_INT,SET_PTR} store the {mask,integer,pointer} to put in
  *   the value when met.
  *   CALLBACKS can use it like they want.
+ *
+ * `given`::
+ *   Pointer to a flag variable which is set to 1 if this option was given.
+ *   The flag should be 0 initialized, to make sense.
  */
 struct option {
 	enum parse_opt_type type;

@@ -112,6 +116,7 @@ struct option {
 	int flags;
 	parse_opt_cb *callback;
 	intptr_t defval;
+	int *given;
 };

 #define OPT_END()                   { OPTION_END }

@@ -146,6 +151,21 @@ struct option {
 	{ OPTION_CALLBACK, (s), (l), (v), "when", (h), PARSE_OPT_OPTARG, \
 		parse_opt_color_flag_cb, (intptr_t)"always" }

+/* same as above but with given flag pointer */
+#define OPT_BIT_GIVEN(s, l, v, h, b, g) \
+	{ OPTION_BIT,     (s), (l), (v), NULL, (h), PARSE_OPT_NOARG, NULL, (b), (g) }
+#define OPT_NEGBIT_GIVEN(s, l, v, h, b, g) \
+	{ OPTION_NEGBIT,  (s), (l), (v), NULL, (h), PARSE_OPT_NOARG, NULL, (b), (g) }
+#define OPT_BOOLEAN_GIVEN(s, l, v, h, g) \
+	{ OPTION_BOOLEAN, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG, NULL, 0, (g) }
+#define OPT_SET_INT_GIVEN(s, l, v, h, i, g) \
+	{ OPTION_SET_INT, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG, NULL, (i), (g) }
+#define OPT_SET_PTR_GIVEN(s, l, v, h, p, g) \
+	{ OPTION_SET_PTR, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG, NULL, (p), (g) }
+#define OPT_INTEGER_GIVEN(s, l, v, h, g) \
+	{ OPTION_INTEGER, (s), (l), (v), "n", (h), 0, NULL, 0, (g) }
+#define OPT_STRING_GIVEN(s, l, v, a, h, g) \
+	{ OPTION_STRING,  (s), (l), (v), (a), (h), 0, NULL, 0, (g) }

 /* parse_options() will filter out the processed options and leave the
  * non-option arguments in argv[].

-- 
1.7.6.588.g8d735

On Thu, Jul 21, 2011 at 3:57 AM, Junio C Hamano [off-list ref] wrote:

 - It might be useful to be able to "git grep" both tracked and untracked
  (i.e. new files you may want to "git add") paths, but there is no good
  way to do so. Introduce a new option --untracked-too (or more suitable
  name --- I am bad at naming and not married to this one) to allow
  this. This mode always takes "exclude" into account.

--ls-files ([c|d|o|i|s|u|k|m])+? This opens door for grepping only
untracked files, for instant.
-- 
Duy

On Wed, Jul 20, 2011 at 22:57, Junio C Hamano [off-list ref] wrote:

Taken in total isolation, this patch does allow a use case where we did
not allow, but when considered in a larger picture of what "grep" is used
for and how different use cases the command should support, a few random
thoughts come to mind:

 - Like "diff --no-index", "grep --no-index" is about a directory that is
  not managed by git at all with random collection of files. Do we even
  want to be using any "exclude" in such a use case? Wouldn't it actually
  be a bug that we pay attention to standard-exlcludes in the current
  code, as .gitignore files scattered in such a directory should _not_
  mean anything, as it is not a git working tree at all?

 - Even in a git managed directory, you _could_ use "grep --no-index" to
  find hits from both tracked and untracked files. In this particular use
  case, it makes some sense to pay attention to "exclude", as that would
  catch what _could_ be committed, and paths that would be excluded won't
  be part of that set (unless you use "add -f"). But wouldn't that use
  case better be covered by a switch that is different from --no-index
  (which means "These are not managed by git at all")? It is still about
  files in a git working tree, it is just that the user wants us to pay
  attention also to untracked files, e.g. "grep --untracked-too"?

So I think the patch identified a good problem to solve, but it might be a
wrong solution that encourages a use of a wrong option (i.e. --no-index)
only because we do not have the right one (i.e. "I am in the working tree,
but I want untracked ones also considered.").

What do people think if we did this a bit differently?

 - Since 3081623 (grep --no-index: allow use of "git grep" outside a git
  repository, 2010-01-15) and 59332d1 (Resurrect "git grep --no-index",
  2010-02-06), "grep --no-index" incorrectly paid attention to the
  exclude patterns. We shouldn't have, and we'd fix that bug.

I considered this when I noticed this, but feared the
backward-incompatibility. If you call it a bug, than we can change for
the better.

 - It might be useful to be able to "git grep" both tracked and untracked
  (i.e. new files you may want to "git add") paths, but there is no good
  way to do so. Introduce a new option --untracked-too (or more suitable
  name --- I am bad at naming and not married to this one) to allow
  this. This mode always takes "exclude" into account.

My proposal would be to name it --include-untracked.

Opinions?

Regarding the patch:

quoted

+     /* --no-exclude-standard needs --no-index */
+     if (use_index && !exclude_standard)
+             die(_("--no-exclude-standard does not make sense without --no-index."));

For that matter,

   $ git grep --no-exclude-standard --exclude-standard --cached -e foo

should be an error, no?

It should be. But I think that unveils one of the shortcomings of the
(any) option parser: You wont get notified when an option was given,
regardless of its value. To handle the above I would have to use
OPTION_CALLBACK to set an addition flag exc_given (like it is done in
git-ls-files) and test against this. The same problem is possible for
a number option, if you want to know whether the option was actually
given on the command line, one need to invent an invalid value (which
isn't always possible) and use this as the initializer or use
OPTION_CALLBACK again.

Bert