Re: git-grep: option parsing conflicts with prefix-dash searches

[Jeff King <peff@peff.net>]

On Fri, Feb 05, 2010 at 03:31:06PM -0800, Junio C Hamano wrote:

> Jan Engelhardt <jengelh@medozas.de> writes:
> 
> > Just about now I wanted to grep for accesses of a particular struct 
> > member. Needless to say that it was not a very amusing experience.
> > I would expect that (1) probably fails:
> >
> > (1)	$ git grep '->cnt' net/ipv4/netfilter/
> > 	error: unknown switch `>'
> >
> > So far so good, seems reasonable and matches what I would expect from 
> > most other userspace tools. So let's add -- to terminate the option 
> > list:
> 
> Also you can say "grep -e '->cnt'".  Not just "git grep" but regular grep
> understands this, too.

GNU grep understands "grep -- '->cnt'", so I find myself typing it a
lot. Even though "--" is used for revision and pathname separation, I
don't think there is a conflict in also using it to separate options
from patterns for the case that there is no "-e" at all. In other words:

  git grep -- foo

is not ambiguous. That "--" could not possibly be separating revisions
from pathnames because we have not yet seen any pattern, which is bogus.
In a case like:

  git grep -e pattern -- foo

I think it is clear that the "--" is separating pathnames, because we
already have a pathname. This matches standard grep, which will treat
the first non-option as a pattern only if we have no "-e" pattern.

So I think we can do just do this:

diff --git a/builtin-grep.c b/builtin-grep.c
index 0ef849c..46ffc1d 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -889,6 +889,16 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		/* die the same way as if we did it at the beginning */
 		setup_git_directory();
 
+	/*
+	 * skip a -- separator; we know it cannot be
+	 * separating revisions from pathnames if
+	 * we haven't even had any patterns yet
+	 */
+	if (argc > 0 && !opt.pattern_list && !strcmp(argv[0], "--")) {
+		argv++;
+		argc--;
+	}
+
 	/* First unrecognized non-option token */
 	if (argc > 0 && !opt.pattern_list) {
 		append_grep_pattern(&opt, argv[0], "command line", 0,

and make everybody happy. But I admit I haven't thought about it more
than 5 minutes or so, so perhaps there is a case I am missing that will
be ambiguous or confusing. The worst I could come up with is the
double-double-dash case:

  git grep -- pattern revision -- pathname

It is perhaps not as pretty as

  git grep -e pattern revision -- pathname

but I don't think it is ambiguous.

> > (2)	$ git grep -- '->cnt' net/ipv4/netfilter/
> > 	fatal: bad flag '->cnt' used after filename
> >
> > *bzzt*.
> 
> This indeed is bzzt, especially if you had a file called "./->cnt" in the
> work tree.  That would mean that you cannot tell the command to look for a
> pattern in the work tree.
> 
> But because you are not giving anything before "--", that "git grep" is
> not looking for anything.  Indeed, (2) is a user error.  If you try this:

That is not quite true. The way "git grep" is implemented now, it is
actually grepping for "--". parse_options stops at the "--", leaving it
in argv, and then we assume whatever is left by parse_options is a
pattern (since we saw no "-e"). But of course it is looking in the
'->cnt' pathspec (or revision!), which is bogus (and gets you "bad flag used
after filename").

But as you noted with:

> > What works is (3).
> >
> > (3)	$ git grep -- -- '->cnt' net/ipv4/netfilter/

...disambiguating the pathspec with the extra "--" gets it past option
parsing and looking for "--".

So actually my patch above is breaking somebody who truly wanted to grep
for "--" by doing

  git grep --

but that is sufficiently insane that I'm not too worried about it.

-Peff