Page MenuHomeFreeBSD

zgrep: Allow multiple options '-e'
AbandonedPublic

Authored by martymac on Feb 6 2020, 10:17 PM.

Details

Reviewers
markj
bapt
kevans
Summary

The attached patch allows to pass several options -e as one would do with standard grep(1) command.

This review comes from the following PR :
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=238380

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martymac created this revision.Feb 6 2020, 10:17 PM
martymac edited the summary of this revision. (Show Details)Feb 6 2020, 10:19 PM
markj added a comment.Feb 11 2020, 6:42 PM

Could you upload a diff with full context?

usr.bin/grep/zgrep.sh
145

The indentation is off here.

martymac updated this revision to Diff 68164.EditedFeb 12 2020, 11:09 AM

Hi Mark,

Done! Thanks :)

markj added inline comments.Feb 12 2020, 11:08 PM
usr.bin/grep/zgrep.sh
92

I don't think this will work if the pattern contains any whitespace. The shell will tokenize $2 when it builds the new grep_args, so things like zgrep -e "foo bar" won't work anymore.

You can play some ugly games with eval to make it work I think. To handle -e, write

arg="\"$2\""

and add it to grep_args. Then, when constructing the grep command, use eval to expand the arguments. Maybe there is a better way but I don't really see it. (Rewrite in lua? :)

147

I think you could combine the two cases by converting a pattern passed as a positional argument to a -e parameter.

martymac added a comment.EditedFeb 14 2020, 11:01 AM

Hi Mark,

Thanks a lot for your feedback.

Good catch! Damn, you are right, my patch does not work when grepping patterns that include whitespaces :/ Using eval would work but is a no go for me as it would handle grep arguments in an insecure way : one would, for example, be able to fork a subshell by passing it as a grep pattern.

We could replace spaces in grep patterns with character classes such as :blank: to make the final grep pattern space-free (such as 'foo[[:blank:]]bar') but this is really ugly (and it includes the tab character). After several tries, I could not find a simple, elegant way to work around that problem. Maybe you're right, it would be better to leave the tool as-is and replace it "some day" (TM) by a re-written version, maybe in a language more fit for arguments manipulation ?

martymac abandoned this revision.Thu, Mar 12, 12:11 PM

Close for now as the suggested patch does not handle spaces in file names (see discussion).