Re: git-grep documentation
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...
Show 7 quoted lines
> [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.
[...]
Show 13 quoted lines
> -'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>...]
Show 13 quoted lines
> <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`.
Show 11 quoted lines
>
> <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" --
sean