Teach bsdgrep(1) to use the working directory if no -r directory is specified. This not only feels a little more sane than the default of grepping stdin, but it also matches newer GNU grep behavior.
PR: 216307
Details
Details
Diff Detail
Diff Detail
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 8514 Build 8818: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| usr.bin/grep/grep.c | ||
|---|---|---|
| 731 | Help me understand this change :-). What's happening here? | |
| usr.bin/grep/util.c | ||
| 134 | style(9) (missing space) I'd suggest breaking this into two statements for clarity. | |
| 139 | I'd strongly suggest breaking this up into two statements for clarity. Also, don't use boolean checks for pointer NULLness: if (!ptr) should be if (ptr == NULL) (style(9)). | |
| 171 | Another style(9) nit. | |
| 172–173 | This indentation looks too deep (2 levels?). | |
Comment Actions
Possibly just for convenience of passing the non-const argv. Some compilers produce (IMO, spurious) warnings passing non-const double pointers to const double pointer parameters. Or likely just to match the type of argv; FTS is very very old code (~1980).
Comment Actions
Yeah, I suppose I don't see the harm either way, now that I think about it. Changes to this implementation that might break this usage are going to have to be carefully considered anyways, yeah?
| usr.bin/grep/grep.c | ||
|---|---|---|
| 731 | I think the latter part of this addition was some cruft that snuck in from something else I was playing with. I can't imagine, looking over this again, why this can't just be aargc == 0 && dirbehave != DIR_RECURSE, because the intention at this point is really just to let it get to grep_tree without specifying a file, then grep_tree handles the case where aargv does not contain a file. This will go away shortly. | |
| usr.bin/grep/util.c | ||
| 133 | Surprise!! It does the standard err() out if the jemalloc(3) equivalents return NULL. Here are the counts of these things being called: grep_malloc: 8 I vote in favor of ripping these out as well since they don't add anything other than error checking, which doesn't warrant a separate implementation here, and another layer to go through if jemalloc(3) (or not, in the case of strdup) usage needs to be audited beyond this context to any extent. That doesn't seem likely, but I assume it's always possible. | |
Comment Actions
- Remove cruft that snuck in when determining whether to exit with an examination of stdin
- Use '.' for working dir and simplify grep_tree
Comment Actions
Marking all of these as done, because grep_tree now uses the patch by @emaste, and I've simplified the weird if in grep.c. I'm still in favor of ripping out the grep_* layer to jemalloc(3) (and strdup), though, in a separate change.
| usr.bin/grep/util.c | ||
|---|---|---|
| 133 | I'm ok with keeping err(3) in case of memory exhaustion, actually. The alternative is either segfaulting or copying err() calls out into every malloc caller. This line should be broken into two statements, though. | |
| usr.bin/grep/util.c | ||
|---|---|---|
| 133 | Something like: if((argv[0] != NULL)
fts = fts_open(argv, fts_flags, NULL);
else
fts = fts_open(__DECONST(char **, tdir), fts_flags, NULL);
if(fts == NULL)
err(2, "fts_open");Yeah? Probably s/tdir/wdir/ for better naming. | |
| usr.bin/grep/util.c | ||
|---|---|---|
| 133 | ...with suitable style(9) conformance | |
Comment Actions
I am imagining:
fts = fts_open((argv[0] == NULL) ? __DECONST(char **, tdir): argv, fts_flags, NULL);
if (fts == NULL) {
...Comment Actions
- Rename tdir => wd to accurately reflect contents and intention
- Split long statement out into two for clarity
Comment Actions
Also I had a small error; the proper cast should be __DECONST(char * const *, ...) to match the fts_open signature. No need to cast away more constness than necessary.
Comment Actions
And: sorry for all the nitpicking :-). We do really appreciate your patience and appreciation for getting the details right.
LGTM modulo Ed's last nit about the DECONST type.
Thanks!
Comment Actions
Whoops, sorry, I should have double-checked fts(3) after casting became necessary. =(
Comment Actions
Sorry, missed this comment:
OTOH, you guys are putting up with all of the patches I'm flinging your way and helping me get things right, even for these small features that aren't strictly necessary and haven't been highly (or at all, from what I've found) requested. =)
LGTM modulo Ed's last nit about the DECONST type.
Done!
| usr.bin/grep/util.c | ||
|---|---|---|
| 132–133 | Ah, my eyeballs are getting worse at eyeing line lengths. =) FWIW- I would be inclined to agree with your wrap, given that it groups the three actual arguments passed to fts_open on the same line. Kind of arbitrary, but it reads a little better to me at least. | |
Comment Actions
- Add test cases for this and r316473+r316484, mark them as expected timeouts/failures for now until bsdgrep becomes the system grep
Comment Actions
@ngie -- FYI, I've added two new test cases here under grep_freebsd_test rather than the standard grep_test since they are purely features added to bsdgrep(1) rather than tests of wonky behavior that GNU grep should be expected to handle
| contrib/netbsd-tests/usr.bin/grep/t_grep_freebsd.sh | ||
|---|---|---|
| 1 ↗ | (On Diff #27142) | If a testcase isn't upstreamable, please move it to the test directory (usr.bin/grep/tests). |
| 31 ↗ | (On Diff #27142) | Why add a timeout? |
| 45 ↗ | (On Diff #27142) | Why add a timeout? |
| usr.bin/grep/tests/Makefile | ||
| 5 ↗ | (On Diff #27142) |
|
| usr.bin/grep/util.c | ||
| 133 | Why DECONST when you can just declare wd as char *? Removing the DECONST poisoning will probably allow you to fit this all on one line. | |
| contrib/netbsd-tests/usr.bin/grep/t_grep_freebsd.sh | ||
|---|---|---|
| 31 ↗ | (On Diff #27142) | Because otherwise this test will wait forever -- default "grep -r" behavior with GNU grep is to grep stdin, until this patch goes in. What do you recommend instead? timeout(1)? |
| 45 ↗ | (On Diff #27142) | This is the same reasoning as above, but we don't expect timeout here because the rgrep symlink isn't installed in the normal GNU grep case. If it does get installed and WITH_GNU_GREP, then it will exhibit the default behavior of grepping stdin with no hope of exiting on its own. |
| usr.bin/grep/util.c | ||
| 133 | Generally because I'm not familiar with how to coerce the definition of wd ({ ".", NULL }) to a char * in a way that isn't just as dirty, if not dirtier. | |
Comment Actions
The linter/harbormaster seems to strongly dislike the test script, but this seems to generally be acceptable in, say, sed/tests/sed2_test.sh -- safe to ignore?
| contrib/netbsd-tests/usr.bin/grep/t_grep_freebsd.sh | ||
|---|---|---|
| 31 ↗ | (On Diff #27142) | timeout would be awesome, but the testcase could be skipped if you're using gnu grep (grep --version -> atf_skip "doesn't work on GNU grep", or conditionalize the timeout on GNU grep :}..?). Alternatively, why not close /dev/stdin by doing < /dev/null? |
| usr.bin/grep/tests/grep_freebsd_test.sh | ||
| 1 ↗ | (On Diff #27158) | chmod -x this file to mute this warning, please :). |
Comment Actions
It's whining about an executable file being checked in without a shebang (which is generally bad, but in this case because they're tests and the shebang is added at build time), it's a harmless complaint.
As I noted before though, doing chmod -x on the file will fix this issue (the executable bit is set at install time).
Comment Actions
Yeah, seemed kind of arbitrary, but I did it so that linter stays happy. =)
How is this new version for compromise on testing? No timeout, skip if grep -r doesn't match anything in the test directory (it should match) and skip if the rgrep symlink isn't valid/usable.
| contrib/netbsd-tests/usr.bin/grep/t_grep_freebsd.sh | ||
|---|---|---|
| 31 ↗ | (On Diff #27142) | what about grep -r foo </dev/null? |
Comment Actions
- Apologies for the mistakes, hopefully this is the simplest/best version
- Simplify rgrep test even further to not use atf-check -x and make sure we're testing rgrep vs. equivalent grep -r
| usr.bin/grep/tests/grep_freebsd_test.sh | ||
|---|---|---|
| 31 ↗ | (On Diff #27159) | The \ isn't needed. |
| 42 ↗ | (On Diff #27159) | Ok, you're going to hate me, but this should be done like: rgrep_head()
{
atf_set "require.progs" "rgrep"
}This code checks $PATH for rgrep's existence/executability. |
| 43 ↗ | (On Diff #27159) | Same here, as above. |