Changeset View
Standalone View
usr.bin/grep/zgrep.sh
Show First 20 Lines • Show All 69 Lines • ▼ Show 20 Lines | |||||
# skip all options and pass them on to grep taking care of options | # skip all options and pass them on to grep taking care of options | ||||
# with arguments, and if -e was supplied | # with arguments, and if -e was supplied | ||||
while [ $# -gt 0 -a ${endofopts} -eq 0 ] | while [ $# -gt 0 -a ${endofopts} -eq 0 ] | ||||
do | do | ||||
case $1 in | case $1 in | ||||
# from GNU grep-2.5.1 -- keep in sync! | # from GNU grep-2.5.1 -- keep in sync! | ||||
-[ABCDXdefm]) | --) | ||||
shift | |||||
endofopts=1 | |||||
;; | |||||
--*) | |||||
markj: Why is this case added? Is it related to the rest of the patch? | |||||
leresAuthorUnsubmitted Done Inline ActionsThe problem is when you add the wildcard to -*[ABCDXdefm] it can potentially match long flags. Examples are --ignore-case, --line-buffered, --with-filename, and --no-filename because they end with 'e' and 'd' but it could also happen with any --regexp=XXX, --include=XXX, or --exclude=XXX that end with a flag in the match class. The new version will include a test for this along with another test for a new problem; -e PATTERN followed by other flags; the break in the -e/-*e case needs to be a continue otherwise we stop processing flags prematurely. leres: The problem is when you add the wildcard to -*[ABCDXdefm] it can potentially match long flags. | |||||
markjUnsubmitted Not Done Inline ActionsI see, thanks. markj: I see, thanks. | |||||
grep_args="${grep_args} $1" | |||||
shift | |||||
;; | |||||
-*[ABCDXdefm]) | |||||
if [ $# -lt 2 ] | if [ $# -lt 2 ] | ||||
then | then | ||||
echo "${prg}: missing argument for $1 flag" >&2 | echo "${prg}: missing argument for $1 flag" >&2 | ||||
exit 1 | exit 1 | ||||
fi | fi | ||||
case $1 in | case $1 in | ||||
-e) | -*[ef]) | ||||
pattern="$2" | # -e: the pattern is the next argument | ||||
# -f: the pattern(s) is/are in a file | |||||
pattern="" | |||||
pattern_found=1 | pattern_found=1 | ||||
shift 2 | |||||
break | |||||
;; | ;; | ||||
*) | *) | ||||
;; | ;; | ||||
esac | esac | ||||
Not Done Inline ActionsWith this patch, zgrep now behaves differently with -e "". markj: With this patch, zgrep now behaves differently with -e "". | |||||
Done Inline ActionsNext version adds a test and solves this. leres: Next version adds a test and solves this. | |||||
grep_args="${grep_args} $1 $2" | grep_args="${grep_args} $1 $2" | ||||
shift 2 | shift 2 | ||||
;; | ;; | ||||
--) | |||||
shift | |||||
endofopts=1 | |||||
;; | |||||
-) | -) | ||||
hyphen=1 | hyphen=1 | ||||
shift | shift | ||||
;; | ;; | ||||
-h) | -h) | ||||
silent=1 | silent=1 | ||||
shift | shift | ||||
;; | ;; | ||||
Show All 29 Lines | exit 1 | ||||
fi | fi | ||||
fi | fi | ||||
ret=0 | ret=0 | ||||
# call grep ... | # call grep ... | ||||
if [ $# -lt 1 ] | if [ $# -lt 1 ] | ||||
then | then | ||||
# ... on stdin | # ... on stdin | ||||
if [ -n "${pattern}" ]; then | |||||
${cattool} ${catargs} - | ${grep} ${grep_args} -- "${pattern}" - || ret=$? | ${cattool} ${catargs} - | ${grep} ${grep_args} -- "${pattern}" - || ret=$? | ||||
else | else | ||||
${cattool} ${catargs} - | ${grep} ${grep_args} -- - || ret=$? | |||||
markjUnsubmitted Not Done Inline ActionsSo if -e is specified (this script doesn't appear to handle multiple uses of -e at all), we are now passing -e in grep_args, so we are losing quoting. As a result, with this patch the following no longer works: zgrep -e 'foo bar' *. markj: So if -e is specified (this script doesn't appear to handle multiple uses of -e at all), we are… | |||||
leresAuthorUnsubmitted Done Inline ActionsThe new version will include a test for this case. leres: The new version will include a test for this case. | |||||
leresAuthorUnsubmitted Done Inline ActionsHum... I missed your comment about multiple uses of -e; I did some testing I see that grep applies all -e patterns: # Works echo foobar > test grep -e foo -e xxx test foobar # No output zgrep -e foo -e xxx test So I should go back to my original idea of keep -e PATTERN in grep_args. leres: Hum... I missed your comment about multiple uses of -e; I did some testing I see that grep… | |||||
leresAuthorUnsubmitted Done Inline ActionsOk so fixing this is hard. I see several approaches to fix this. One is to use a temp file, put multiple -e patterns in the file and use -f tempfile. This is easy but yuck: temp file. The other involves rewriting the script to loop over $@ so we can look at the quoted versions of arguments: cat /var/tmp/y.sh #!/bin/sh for arg in "$@"; do echo "# ${arg}" done /var/tmp/y.sh foo \'\" '"foo bar"' "'foo bar'" # foo # '" # "foo bar" # 'foo bar' Executing the final pipeline with eval would let the shell strip quoting. I'm not inclined to work on this solution unless it is agreed to be a reasonable solution. It's worth noting that the grep(1) does not document what happens when you use more than one -e but clearly it handles it the same way it does mutiple patterns in a -f file. Here's yet another regression I just noticed: # Works echo foobar > test grep -efoo test foobar # Incorrect -H output zgrep -efoo test test:foobar leres: Ok so fixing this is hard. I see several approaches to fix this. One is to use a temp file, put… | |||||
markjUnsubmitted Not Done Inline ActionsPassing parameters through eval seems a bit dangerous. Without having thought about it carefully, can we guarantee that parameters don't get evaluated directly by the shell? In other words, can we do it in a way that ensures that zgrep.sh won't execute arbitrary commands? It looks like you are right about the other bug. (It is not a regression from your change, to be clear.) I don't think fixing these other issues should hold up this patch, once the edge case I pointed out above is fixed. markj: Passing parameters through eval seems a bit dangerous. Without having thought about it… | |||||
fi | |||||
else | |||||
# ... on all files given on the command line | # ... on all files given on the command line | ||||
if [ ${silent} -lt 1 -a $# -gt 1 ]; then | if [ ${silent} -lt 1 -a $# -gt 1 ]; then | ||||
grep_args="-H ${grep_args}" | grep_args="-H ${grep_args}" | ||||
fi | fi | ||||
for file; do | for file; do | ||||
if [ -n "${pattern}" ]; then | |||||
${cattool} ${catargs} -- "${file}" | | ${cattool} ${catargs} -- "${file}" | | ||||
${grep} --label="${file}" ${grep_args} -- "${pattern}" - || ret=$? | ${grep} --label="${file}" ${grep_args} -- "${pattern}" - || ret=$? | ||||
else | |||||
${cattool} ${catargs} -- "${file}" | | |||||
${grep} --label="${file}" ${grep_args} -- - || ret=$? | |||||
fi | |||||
done | done | ||||
fi | fi | ||||
exit ${ret} | exit ${ret} |
Why is this case added? Is it related to the rest of the patch?