bsdgrep(1): For -r, use the working directory if no directory is specified

[kevans]

Summary

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

Test Plan

Check that there are no regressions in the kyua tests.
Compare the output of grep -inr $pattern and grep -inr $pattern ., check that they match except for some path massaging.

[cem]

usr.bin/grep/grep.c

735

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)).

172

Another style(9) nit.

173–174

This indentation looks too deep (2 levels?).

[cem]

In D10108#212421, @kevans91_ksu.edu wrote:

I would have gone that route, but I didn't think it "ok" to remove const in this scenario. I would expect fts_open to not do any funky things to this, but I also don't know the motivation off-hand for the parameter not being declared const.

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).

[kevans]

In D10108#212446, @cem wrote:

In D10108#212421, @kevans91_ksu.edu wrote:

I would have gone that route, but I didn't think it "ok" to remove const in this scenario. I would expect fts_open to not do any funky things to this, but I also don't know the motivation off-hand for the parameter not being declared const.

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).

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

735

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
grep_calloc: 3
grep_realloc: 5
grep_strdup: 3

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.

[kevans]

• Remove cruft that snuck in when determining whether to exit with an examination of stdin
• Use '.' for working dir and simplify grep_tree

[kevans]

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.

[cem]

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.

[kevans]

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.

[kevans]

usr.bin/grep/util.c

133

...with suitable style(9) conformance

[cem]

I am imagining:

fts = fts_open((argv[0] == NULL) ? __DECONST(char **, tdir): argv, fts_flags, NULL);
if (fts == NULL) {
    ...

[kevans]

• Rename tdir => wd to accurately reflect contents and intention
• Split long statement out into two for clarity

[kevans]

Done, and I renamed tdir to wd to make more contextual sense.

[emaste]

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.

[cem]

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!

[kevans]

• Fix cast for wd

[kevans]

In D10108#212594, @emaste wrote:

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.

Whoops, sorry, I should have double-checked fts(3) after casting became necessary. =(

[kevans]

Sorry, missed this comment:

In D10108#212595, @cem wrote:

And: sorry for all the nitpicking :-). We do really appreciate your patience and appreciation for getting the details right.

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!

[emaste]

usr.bin/grep/util.c

132–134

style(9) nit > 80 cols here (and missing space before the : )

I'm not sure there's a clear rule for this case, but I would probably wrap it like so:

fts = fts_open((argv[0] == NULL) ? 
     __DECONST(char * const *, wd) : argv, fts_flags, NULL);

@cem, your opinion?

[cem]

@emaste That looks fine to me.

[kevans]

usr.bin/grep/util.c

132–134

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.

[kevans]

• style(9) nits for wrapping and spacing around operators

[kevans]

• Add test cases for this and r316473+r316484, mark them as expected timeouts/failures for now until bsdgrep becomes the system grep

[kevans]

@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

[ngie]

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

6

1. Please use separate lines for the tests.
2. Please use ATF_TESTS_SH+=

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.

[ngie]

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

1 ↗

(On Diff #27142)

Pretty sure # $FreeBSD$ should go below the license tort.

31 ↗

(On Diff #27142)

Ok, I sort of see why the timeout's there, but forcing it in the testcase is ugly...

[kevans]

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.

[kevans]

• Move test to local tests/ dir, move FreeBSD tag

[kevans]

• Space before copyright spiel

[kevans]

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?

[ngie]

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

2

chmod -x this file to mute this warning, please :).

[ngie]

In D10108#213206, @kevans91_ksu.edu wrote:

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?

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).

[kevans]

• Set -x on test
• Skip tests if functionality is not present in current version

[kevans]

In D10108#213210, @ngie wrote:

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).

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.

[kevans]

• Remove unintended redirect

[emaste]

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

31 ↗

(On Diff #27142)

what about grep -r foo </dev/null?

[kevans]

• Don't rely on implied working directory for the rgrep test, keep it clean

[kevans]

• 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

[ngie]

usr.bin/grep/tests/grep_freebsd_test.sh

32

The \ isn't needed.

43

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.

44

Same here, as above.

[kevans]

• Fix redundant newline escaping/wrapping, properly set require.progs

[kevans]

No hate here, I'd rather get it right. =)

[kevans]

• String escaping -- -x really isn't ideal

[emaste]

rS317050