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`.
sean [off-list ref] 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 <redacted>
---
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
On Sat, 21 Jan 2006 00:09:07 -0800
Junio C Hamano [off-list ref] 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.
[...]
quoted hunk
-'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" --