Page MenuHomeFreeBSD
Differential D10607

bsdgrep(1): Don't output matches with -c, -l flag, and -L flags
ClosedPublic
Actions

Authored by kevans on May 5 2017, 3:42 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Dec 18, 1:41 PM
Unknown Object (File)
Fri, Dec 6, 1:06 AM
Unknown Object (File)
Fri, Dec 6, 1:06 AM
Unknown Object (File)
Fri, Dec 6, 1:06 AM
Unknown Object (File)
Wed, Dec 4, 10:21 AM
Unknown Object (File)
Wed, Dec 4, 10:21 AM
Unknown Object (File)
Wed, Dec 4, 10:21 AM
Unknown Object (File)
Wed, Dec 4, 10:20 AM
View All Files
Subscribers
markj

Details

Reviewers
emaste
cem
ngie
Commits
rS317842: bsdgrep: don't ouptut matches with -c, -l, -L
Summary

Refactoring done in rS317703 broke -c, -l, and -L flags implying suppression of
match printing. Fortunately, this is just a matter of not doing any printing
of the resulting matches and context printing was not broken in this refactoring.

Add some regression tests since this area may still see further refactoring, include
different context flags as well even though they were not broken in this case.

Reported by: markj@
PR: 219077

Test Plan

Ensure no regressions in Kyua tests, test build libsysdecode with bsdgrep
installed as /usr/bin/grep as in the reported PR.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9081
Build 9495: arc lint + arc unit

Event Timeline

kevans created this revision.May 5 2017, 3:42 AM
Herald added a reviewer: ngie. · View Herald TranscriptMay 5 2017, 3:42 AM
Harbormaster completed remote builds in B9079: Diff 28047.May 5 2017, 3:42 AM
kevans updated this revision to Diff 28048.May 5 2017, 3:44 AM
  • Remove changes snuck into the revision that did not appear in the diff I did beforehand...
Harbormaster completed remote builds in B9080: Diff 28048.May 5 2017, 3:44 AM
cem accepted this revision.May 5 2017, 3:44 AM

LGTM minus what I guess is an accidental change to bnxt.

sys/dev/bnxt/bnxt.h
81 ↗(On Diff #28047)

Unrelated change?

This revision is now accepted and ready to land.May 5 2017, 3:44 AM
kevans added a comment.May 5 2017, 3:48 AM
In D10607#220010, @cem wrote:

LGTM minus what I guess is an accidental change to bnxt.

Yeah, I always diff it against 'master' (git) before submission, but this didn't show up so I'm guessing I should be diffing against...something else. It's slightly confusing, though, because this is the only machine that I actually manipulate this repo and its remotes on.

cem added a comment.May 5 2017, 4:00 AM
In D10607#220014, @kevans91_ksu.edu wrote:

Yeah, I always diff it against 'master' (git) before submission, but this didn't show up so I'm guessing I should be diffing against...something else. It's slightly confusing, though, because this is the only machine that I actually manipulate this repo and its remotes on.

How do you create the review? I tend to use arc diff --allow-untracked --create HEAD^, so I'd guess I'd diff HEAD against HEAD^. If you're creating a review against master instead of HEAD^, I think diffing against it is correct. I don't know :-).

kevans updated this revision to Diff 28049.May 5 2017, 4:06 AM
kevans edited edge metadata.
  • Further inspection showed problem a little more widespread, having broken -c, -l, and -L. Add appropriate tests for all of them, making sure that context still doesn't break them for some reason (except for -L, since it's explicitly non-match)
  • Use a 'printmatch' bool, similar to 'doctx,' so that we're not checking all of the flags every iteration in some cases and to be consistent with context printing
This revision now requires review to proceed.May 5 2017, 4:06 AM
Harbormaster completed remote builds in B9081: Diff 28049.May 5 2017, 4:06 AM
kevans retitled this revision from bsdgrep(1): Don't output matches with -l flag, only filenames to bsdgrep(1): Don't output matches with -c, -l flag, and -L flags.May 5 2017, 4:19 AM
kevans edited the summary of this revision. (Show Details)
kevans updated this revision to Diff 28050.May 5 2017, 4:25 AM
  • Go final step further, simplify doctx checking to whether we're printing matches and context flags are used
ngie added inline comments.May 5 2017, 4:35 AM
usr.bin/grep/util.c
244

Shouldn't pc.binary be (pc.binary && binbehave != BINFILE_TEXT) (is a binary file and -a or --binary-files=text is specified)? If so, this probably deserves its own testcase.

kevans added a comment.May 5 2017, 4:38 AM

I actually don't bother specifying diff target, just arc diff --create and let it default. I should probably be more specific to it so that I don't get surprises like this. =)

ngie added inline comments.May 5 2017, 4:39 AM
usr.bin/grep/util.c
244

It looks like you incorporated some of my recommendations while I was typing this up, but I'm still curious about BINFILE_BIN vs BINFILE_TEXT.

Also, if you could add some more testcases for -I/--binary-files behavior w.r.t. these flags, that would be awesome :).

kevans added inline comments.May 5 2017, 4:54 AM
usr.bin/grep/util.c
244

Indeed- I tossed around != BINFILE_TEXT and == BINFILE_BIN, and determined that == BINFILE_BIN might be a little bit better. BINFILE_SKIP and BINFILE_TEXT are both harmless since the former breaks out before any printing and the latter is, well, what it is. I think that we're less likely to add another BINFILE_* value that gets treated as BINFILE_BIN than we are to add another BINFILE_* flag that allows safe behavior here.

re: binary file test cases, I wish to do that in a separate patch if that's alright. Our test coverage for all of the binary file behaviors is currently at 0%, aside from context2 that forces what would otherwise be detected as a 'binary' file to be detected as text through -z/--null-data. I don't entirely count that as testing binary behavior, though, so 0% is a final number.

I think it makes sense to create a single binary test that checks -a, -I, -U, and --binary-files= with the couple of different combinations that could feasibly break things.

ngie accepted this revision.May 5 2017, 5:15 AM
ngie added inline comments.
usr.bin/grep/util.c
244

Sounds good to me -- thanks for the reply!

This revision is now accepted and ready to land.May 5 2017, 5:15 AM
Closed by commit rS317842: bsdgrep: don't ouptut matches with -c, -l, -L (authored by emaste). · Explain WhyMay 5 2017, 5:35 PM
This revision was automatically updated to reflect the committed changes.
kevans mentioned this in D10620: bsdgrep(1): Add more extensive testing for the different binary flags.May 5 2017, 11:33 PM

Revision Contents
Changeset List

PathSize
contrib/
netbsd-tests/
usr.bin/
grep/
29 lines
usr.bin/
grep/
8 lines

Diff 28049

View Options

contrib/netbsd-tests/usr.bin/grep/t_grep.sh

Loading...
View Options

usr.bin/grep/util.c

Loading...