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

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

Jan Engelhardt [off-list ref] writes:

quoted

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.

quoted

(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:

quoted

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