git-grep documentation

git-grep documentation

[sean]

It looks as though git-grep.sh was coded to allow git-ls-files
options to be passed after a '--' marker.   However, this marker will 
never be seen by the script unless the user identifies the <pattern>
by preceding it with an '-e'.   

If it's not seen the '--' ends up getting passed to git-ls-files and any
options are mistakenly interpreted as paths.  For instance, the following
two commands will do different things:

$ git-grep -e NAME -- --others Documentation/git-grep.txt
$ git-grep    NAME -- --others Documentation/git-grep.txt


Either the comment in the git-grep.sh script should be changed to
say that only paths are acceptable after the '--' or fixed up to 
handle this case a bit better.   The documentation patch below is 
only applicable if the script is fixed first.

Sean


diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 2bfd8ed..c2c6ff3 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -8,7 +8,7 @@ git-grep - print lines matching a patter
 
 SYNOPSIS
 --------
-'git-grep' [<option>...] <pattern> [<path>...]
+'git-grep' [<option>...] [-e] <pattern> [-- [<ls option>...]] [<path>...]
 
 DESCRIPTION
 -----------
@@ -20,12 +20,21 @@ OPTIONS
 -------
 <option>...::
 	Either an option to pass to `grep` or `git-ls-files`.
-	Some `grep` options, such as `-C` and `-m`, that take
-	parameters are known to `git-grep`.
+
+	The specific `git-ls-files` options that may be supplied are:
+	`--cached`, `--deleted`, `--others`, `--killed`, `--ignored`, 
+	`--exclude=*`, `--exclude-from=*`, or `--exclude-per-directory=*`.
+	
+	All other options will be passed to `grep`.
 
 <pattern>::
 	The pattern to look for.
 
+<ls option>...::
+	Additional options for `git-ls-files` which must be preceded
+	by the '--' marker.   See `git-ls-files` for a list of 
+	available options.
+
 <path>...::
 	Optional paths to limit the set of files to be searched;
 	passed to `git-ls-files`.

Re: git-grep documentation

[Junio C Hamano]

sean <seanlkml@sympatico.ca> writes:

> It looks as though git-grep.sh was coded to allow git-ls-files
> options to be passed after a '--' marker...

I'd prefer to keep the options before path parameters if only
for the sanity's sake.  How about this instead?

-- >8 --
[PATCH] git-grep: clarification on parameters.

We forgot to make sure that there is no more than one pattern
parameter.  Also when looking for files in a directory called
'--others', it passed that path limiter without preceding the
end-of-options marker '--' to underlying git-ls-files, which
misunderstood it as one of its options instead.

	$ git grep --others -e Meta/Make Meta
	$ git grep -o -e Meta/Make Meta
	$ git grep -o Meta/Make Meta

look for a string "Meta/Make" from untracked files in Meta/
directory.

	$ git grep Meta/Make --others

looks for the same string from tracked files in ./--others
directory.

On the other hand,

	$ git grep -e Meta/Make --others

does not have a freestanding pattern, so everybody is parameter
and there is no path specifier.  It looks for the string in all
the untracked files without any path limiter.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 Documentation/git-grep.txt |   25 ++++++++++++++++++++++---
 git-grep.sh                |   27 +++++++++++++++++++--------
 2 files changed, 41 insertions(+), 11 deletions(-)

b11f9315a792d65a0113e10e478e740ce4f8be73
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 2bfd8ed..55d3bed 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -8,7 +8,7 @@ git-grep - print lines matching a patter
 
 SYNOPSIS
 --------
-'git-grep' [<option>...] <pattern> [<path>...]
+'git-grep' [<option>...] [-e] <pattern> [<path>...]
 
 DESCRIPTION
 -----------
@@ -18,13 +18,32 @@ containing a match to the given pattern.
 
 OPTIONS
 -------
+`--`::
+	Signals the end of options; the rest of the parameters
+	are <path> limiters.
+
 <option>...::
 	Either an option to pass to `grep` or `git-ls-files`.
 	Some `grep` options, such as `-C` and `-m`, that take
-	parameters are known to `git-grep`.
+	parameters are known to `git-grep`.  Among options
+	applicable to git-ls-files`, `--others` and
+	`--exclude=*` (and other variants of exclusion) may be
+	of interest.  Only `-o` is recognized as an option to
+	`git-ls-files` in the short form (e.g. `-d` and `-m` are
+	given to `grep`, not to `git-ls-files` as synonym
+	for `--deleted` and `--modifed`), so you need to spell
+	out `git-ls-files` options in longer form
+	e.g. `--deleted`.
 
 <pattern>::
-	The pattern to look for.
+	The pattern to look for.  The first non option is taken
+	as the pattern; if your pattern begins with a dash, use
+	`-e <pattern>`.  When a pattern is found without `-e`, it
+	also terminates the option processing and the rest of
+	the parameters are used as the `<path>...`, and you do
+	not specifically add `--` to protect the path limiter
+	that happens to begin with a dash from being mistaken as
+	an option.
 
 <path>...::
 	Optional paths to limit the set of files to be searched;
diff --git a/git-grep.sh b/git-grep.sh
index 2ed8e95..23b1e03 100755
--- a/git-grep.sh
+++ b/git-grep.sh
@@ -3,22 +3,32 @@
 # Copyright (c) Linus Torvalds, 2005
 #
 
-USAGE='<option>... <pattern> <path>...'
+USAGE='[<option>...] [-e] <pattern> [<path>...]'
 SUBDIRECTORY_OK='Yes'
 . git-sh-setup
 
+got_pattern () {
+	if [ -z "$no_more_patterns" ]
+	then
+		pattern="$1" no_more_patterns=yes
+	else
+		die "git-grep: do not specify more than one pattern"
+	fi
+}
+
+no_more_patterns=
 pattern=
 flags=()
 git_flags=()
 while : ; do
 	case "$1" in
-	--cached|--deleted|--others|--killed|\
-	--ignored|--exclude=*|\
+	-o|--cached|--deleted|--others|--killed|\
+	--ignored|--modified|--exclude=*|\
 	--exclude-from=*|\--exclude-per-directory=*)
 		git_flags=("${git_flags[@]}" "$1")
 		;;
 	-e)
-		pattern="$2"
+		got_pattern "$2"
 		shift
 		;;
 	-A|-B|-C|-D|-d|-f|-m)
@@ -34,8 +44,9 @@ while : ; do
 		flags=("${flags[@]}" "$1")
 		;;
 	*)
-		if [ -z "$pattern" ]; then
-			pattern="$1"
+		if [ -z "$no_more_patterns" ]
+		then
+			got_pattern "$1"
 			shift
 		fi
 		break
@@ -46,5 +57,5 @@ done
 [ "$pattern" ] || {
 	usage
 }
-git-ls-files -z "${git_flags[@]}" "$@" |
-	xargs -0 grep "${flags[@]}" -e "$pattern"
+git-ls-files -z "${git_flags[@]}" -- "$@" |
+	xargs -0 grep "${flags[@]}" -e "$pattern" --
-- 
1.1.4.g5c4a-dirty

Re: git-grep documentation

[sean]

On Sat, 21 Jan 2006 00:09:07 -0800
Junio C Hamano <junkio@cox.net> wrote:

> I'd prefer to keep the options before path parameters if only
> for the sanity's sake.  How about this instead

Hi Junio,

I like your patch and it fixes the "-e" problem on the command
line nicely.   Some comments below though...



> [PATCH] git-grep: clarification on parameters.
> 
> We forgot to make sure that there is no more than one pattern
> parameter.  Also when looking for files in a directory called
> '--others', it passed that path limiter without preceding the
> end-of-options marker '--' to underlying git-ls-files, which
> misunderstood it as one of its options instead.

Actually I misinterpreted the intent of git-grep as wanting to
allow additional git-ls-files options to be given after the '--'
marker on its command line.   This misconception arose because
of the following comment in git-grep.sh :

        --)
               # The rest are git-ls-files paths (or flags)

So your new patch should also fix that comment to remove the 
"(or flags)" portion.

[...]

> -'git-grep' [<option>...] <pattern> [<path>...]
> +'git-grep' [<option>...] [-e] <pattern> [<path>...]
>
>  DESCRIPTION
>  -----------
> @@ -18,13 +18,32 @@ containing a match to the given pattern.
>  
>  OPTIONS
>  -------
> +`--`::
> +	Signals the end of options; the rest of the parameters
> +	are <path> limiters.
> +

Since you comment on the -- marker here I think it should
also appear in the command line above:

'git-grep' [<option>...] [-e] <pattern> [--] [<path>...]


>  <option>...::
>  	Either an option to pass to `grep` or `git-ls-files`.
>  	Some `grep` options, such as `-C` and `-m`, that take
> -	parameters are known to `git-grep`.
> +	parameters are known to `git-grep`.  Among options
> +	applicable to git-ls-files`, `--others` and
> +	`--exclude=*` (and other variants of exclusion) may be
> +	of interest.  Only `-o` is recognized as an option to
> +	`git-ls-files` in the short form (e.g. `-d` and `-m` are
> +	given to `grep`, not to `git-ls-files` as synonym
> +	for `--deleted` and `--modifed`), so you need to spell
> +	out `git-ls-files` options in longer form
> +	e.g. `--deleted`.


I would argue that this is too verbose and strays into the internal
implementation too much.   For example a user doesn't care that
git-grep.sh needs to specially handle "grep" parameters that happen
to take a single parameter (like -C and -m).

Instead it is more useful for them to be told _specificly_ which 
git-ls-files options  are available and that all others will be 
passed to grep.   Somthing like:

  Either an option to pass to `grep` or `git-ls-files`.

  The following are the specific `git-ls-files` options
  that may be given: `-o`, `--cached`, `--deleted`, `--others`,
  `--killed`, `--ignored`, `--modified`, `--exclude=*`,
  `--exclude-from=*`, and `--exclude-per-directory=*`.

  All other options will be passed to `grep`.

>  
>  <pattern>::
> -	The pattern to look for.
> +	The pattern to look for.  The first non option is taken
> +	as the pattern; if your pattern begins with a dash, use
> +	`-e <pattern>`.  When a pattern is found without `-e`, it
> +	also terminates the option processing and the rest of
> +	the parameters are used as the `<path>...`, and you do
> +	not specifically add `--` to protect the path limiter
> +	that happens to begin with a dash from being mistaken as
> +	an option.

And here I think only your first two sentences are needed and the
rest actually isn't.    Your patch fixes the problem case and 
there is no reason now to warn the user away from supplying the --
marker in addition to the "-e"; it'll work properly in either case.


That's it and the rest looked good.  In case you agree with anything
i've said here, find an amended version of your patch below.

Cheers,
Sean





diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 2bfd8ed..7fd675b 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -8,7 +8,7 @@ git-grep - print lines matching a patter
 
 SYNOPSIS
 --------
-'git-grep' [<option>...] <pattern> [<path>...]
+'git-grep' [<option>...] [-e] <pattern> [--] [<path>...]
 
 DESCRIPTION
 -----------
@@ -18,13 +18,24 @@ containing a match to the given pattern.
 
 OPTIONS
 -------
+`--`::
+	Signals the end of options; the rest of the parameters
+	are <path> limiters.
+
 <option>...::
 	Either an option to pass to `grep` or `git-ls-files`.
-	Some `grep` options, such as `-C` and `-m`, that take
-	parameters are known to `git-grep`.
+
+	The following are the specific `git-ls-files` options
+	that may be given: `-o`, `--cached`, `--deleted`, `--others`, 
+	`--killed`, `--ignored`, `--modified`, `--exclude=*`, 
+	`--exclude-from=*`, and `--exclude-per-directory=*`.
+
+	All other options will be passed to `grep`.
 
 <pattern>::
-	The pattern to look for.
+	The pattern to look for.  The first non option is taken
+	as the pattern; if your pattern begins with a dash, use
+	`-e <pattern>`.  
 
 <path>...::
 	Optional paths to limit the set of files to be searched;
diff --git a/git-grep.sh b/git-grep.sh
index 2ed8e95..d815c09 100755
--- a/git-grep.sh
+++ b/git-grep.sh
@@ -3,22 +3,32 @@
 # Copyright (c) Linus Torvalds, 2005
 #
 
-USAGE='<option>... <pattern> <path>...'
+USAGE='[<option>...] [-e] <pattern> [<path>...]'
 SUBDIRECTORY_OK='Yes'
 . git-sh-setup
 
+got_pattern () {
+	if [ -z "$no_more_patterns" ]
+	then
+		pattern="$1" no_more_patterns=yes
+	else
+		die "git-grep: do not specify more than one pattern"
+	fi
+}
+
+no_more_patterns=
 pattern=
 flags=()
 git_flags=()
 while : ; do
 	case "$1" in
-	--cached|--deleted|--others|--killed|\
-	--ignored|--exclude=*|\
+	-o|--cached|--deleted|--others|--killed|\
+	--ignored|--modified|--exclude=*|\
 	--exclude-from=*|\--exclude-per-directory=*)
 		git_flags=("${git_flags[@]}" "$1")
 		;;
 	-e)
-		pattern="$2"
+		got_pattern "$2"
 		shift
 		;;
 	-A|-B|-C|-D|-d|-f|-m)
@@ -26,7 +36,7 @@ while : ; do
 		shift
 		;;
 	--)
-		# The rest are git-ls-files paths (or flags)
+		# The rest are git-ls-files paths 
 		shift
 		break
 		;;
@@ -34,8 +44,9 @@ while : ; do
 		flags=("${flags[@]}" "$1")
 		;;
 	*)
-		if [ -z "$pattern" ]; then
-			pattern="$1"
+		if [ -z "$no_more_patterns" ]
+		then
+			got_pattern "$1"
 			shift
 		fi
 		break
@@ -46,5 +57,5 @@ done
 [ "$pattern" ] || {
 	usage
 }
-git-ls-files -z "${git_flags[@]}" "$@" |
-	xargs -0 grep "${flags[@]}" -e "$pattern"
+git-ls-files -z "${git_flags[@]}" -- "$@" |
+	xargs -0 grep "${flags[@]}" -e "$pattern" --

Re: git-grep documentation

[Junio C Hamano]

sean <seanlkml@sympatico.ca> writes:

> So your new patch should also fix that comment to remove the 
> "(or flags)" portion.

Probably.

> Since you comment on the -- marker here I think it should
> also appear in the command line above:
>
> 'git-grep' [<option>...] [-e] <pattern> [--] [<path>...]

I've thought about this but it is not any more correct than what
we have now (both are technically incorrect).  If you do not use
an `-e` and let a non-option terminate the option processing,
double dashes are not removed, so you do not want it there.

> Instead it is more useful for them to be told _specificly_ which 
> git-ls-files options  are available and that all others will be 
> passed to grep.   Somthing like:

I like it.

> ...    Your patch fixes the problem case and 
> there is no reason now to warn the user away from supplying the --
> marker in addition to the "-e"; it'll work properly in either case.

Does it?  I think if you give -- without -e it will look for a
path that matches -- because we pass our own -- to ls-files.

> That's it and the rest looked good.  In case you agree with anything
> i've said here, find an amended version of your patch below.

Thanks.

When people make an improvement proposal, I'd often prefer to
see a patch that is on top of the patch being discussed, not a
replacement.

Re: git-grep documentation

[sean]

On Sat, 21 Jan 2006 11:06:27 -0800
Junio C Hamano <junkio@cox.net> wrote:

> I've thought about this but it is not any more correct than what
> we have now (both are technically incorrect).  If you do not use
> an `-e` and let a non-option terminate the option processing,
> double dashes are not removed, so you do not want it there.

I think this should be fixed rather than requiring the user
to remember such an obscure detail.   It's easy to fix git-grep
to deal with it instead (see below).

> Does it?  I think if you give -- without -e it will look for a
> path that matches -- because we pass our own -- to ls-files.
> 

You're right,  in the patch below I added a specific test to handle
this case so the documentation can be simplified and the user is
free to use any combination of -e and --. 

> When people make an improvement proposal, I'd often prefer to
> see a patch that is on top of the patch being discussed, not a
> replacement.
> 

Yes, I can see how that would be easier to review.   Below
is a patch on top of your original that now includes the tweak
mentioned above:


diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 55d3bed..7fd675b 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -8,7 +8,7 @@ git-grep - print lines matching a patter
 
 SYNOPSIS
 --------
-'git-grep' [<option>...] [-e] <pattern> [<path>...]
+'git-grep' [<option>...] [-e] <pattern> [--] [<path>...]
 
 DESCRIPTION
 -----------
@@ -24,26 +24,18 @@ OPTIONS
 
 <option>...::
 	Either an option to pass to `grep` or `git-ls-files`.
-	Some `grep` options, such as `-C` and `-m`, that take
-	parameters are known to `git-grep`.  Among options
-	applicable to git-ls-files`, `--others` and
-	`--exclude=*` (and other variants of exclusion) may be
-	of interest.  Only `-o` is recognized as an option to
-	`git-ls-files` in the short form (e.g. `-d` and `-m` are
-	given to `grep`, not to `git-ls-files` as synonym
-	for `--deleted` and `--modifed`), so you need to spell
-	out `git-ls-files` options in longer form
-	e.g. `--deleted`.
+
+	The following are the specific `git-ls-files` options
+	that may be given: `-o`, `--cached`, `--deleted`, `--others`, 
+	`--killed`, `--ignored`, `--modified`, `--exclude=*`, 
+	`--exclude-from=*`, and `--exclude-per-directory=*`.
+
+	All other options will be passed to `grep`.
 
 <pattern>::
 	The pattern to look for.  The first non option is taken
 	as the pattern; if your pattern begins with a dash, use
-	`-e <pattern>`.  When a pattern is found without `-e`, it
-	also terminates the option processing and the rest of
-	the parameters are used as the `<path>...`, and you do
-	not specifically add `--` to protect the path limiter
-	that happens to begin with a dash from being mistaken as
-	an option.
+	`-e <pattern>`.  
 
 <path>...::
 	Optional paths to limit the set of files to be searched;
diff --git a/git-grep.sh b/git-grep.sh
index 23b1e03..4f06093 100755
--- a/git-grep.sh
+++ b/git-grep.sh
@@ -36,7 +36,7 @@ while : ; do
 		shift
 		;;
 	--)
-		# The rest are git-ls-files paths (or flags)
+		# The rest are git-ls-files paths 
 		shift
 		break
 		;;
@@ -49,6 +49,7 @@ while : ; do
 			got_pattern "$1"
 			shift
 		fi
+		[ "$1" = -- ] && shift
 		break
 		;;
 	esac