grep: Fix various bugs in recursive tree handling

[jhb]

Summary

The -OpS options were effectively ignored due to a collection of
bugs in the use of fts(3):

• fts_open(3) requires one of FTS_PHYSICAL or FTS_LOGICAL to be specified, but in the -O case, only FTS_COMFOLLOW was given. Fix this to use FTS_COMFOLLOW | FTS_PHYSICAL.

• The switch on the entry type returned by fts_read() did not check for symbolic links, so symbolic links fell into the default case and were always passed to procfile() even when -p was given. Fix this by adding cases in the switch statement to explicitly ignore FTS_SL.

• FTS_NOSTAT was passed to fts_open(), so fts_open() couldn't detect symbolic links when FTS_PHYSICAL was passed, instead both regular files and symbolic links were returned as FTS_NSOK entries. Fix by only using FTS_NOSTAT with FTS_LOGICAL.

While here, fix a few other nits:

• Treat FTS_NS as an error like FTS_DNR and FTS_ERR.

• Just ignore FTS_DP. The logic to skip descending into skipped directories is only relevant when a directory is first visited, not after the directory has been visited.

• Use warnc instead of warnx + strerror.

PR: 280676

[jhb]

usr.bin/grep/util.c

156

Possibly FTS_NS should be treated as an error here?

163–164

I don't understand why we use this logic for both FTS_D and FTS_DP.

174

I originally considered skipping FTS_SLNONE here, but I'm not sure if we want an error in that case?

185

I would really be happier if we explicitly listed cases here instead of using a default case that does real stuff.

[kevans]

usr.bin/grep/util.c

163–164

imo because it's harmless and, as you noted below, the default case does real things that we probably don't want to do for post-order.

174

Apple has a patch here that skips processing FTS_SLNONE unless it's listed on the command line: https://github.com/apple-oss-distributions/text_cmds/blob/main/grep/util.c#L206-L217

I kind of think that should've been FALLTHROUGH to an error case, but the general idea seems OK. That might also be reasonable for FTS_NS, too.

[jhb]

usr.bin/grep/util.c

145

Apple does set FTS_NOSTAT in this case since it doesn't need to detect symlinks.

163–164

I think it's confusing to read though. That is, can't we just do a simple break for FTS_DP?

[kevans]

usr.bin/grep/util.c

163–164

Yes, and no objection from me

[jhb]

usr.bin/grep/util.c

174

So I tested some cases here and I don't think we need Apple's extra handling for FTS_SLNONE. It seems to already DTRT with the current code:

I added a top-level 'blah' symlink that was broken and a 'bar/nested' symlink that was broken and this was the result:

> /usr/obj/usr/home/john/work/freebsd/main/amd64.amd64/usr.bin/grep/grep -rp blah foo bar baz blah
bar/foo.txt:blah
> /usr/obj/usr/home/john/work/freebsd/main/amd64.amd64/usr.bin/grep/grep -rO blah foo bar baz blah
foo:blah
bar/foo.txt:blah
grep: blah: No such file or directory
> /usr/obj/usr/home/john/work/freebsd/main/amd64.amd64/usr.bin/grep/grep -rS blah foo bar baz blah
foo:blah
bar/foo.txt:blah
baz/bar.txt:blah
grep: baz/nested: No such file or directory
grep: blah: No such file or directory

So -p didn't follow any of them (so didn't report any errors), -O didn't follow the nested link (so didn't report an error for the nested case), and -S followed both of them.

[jhb]

Various further fixes

[jhb]

Ping @kevans