Re: [PATCH] find: doc: Fix -prune SCM example and really make it efficient

[James Youngman]

The style "test X -o Y" is obsolescent in POSIX (citation:
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html).  The
POSIX standard recommends the use instead of test X || test Y.   Which is,
in effect, what we are doing in the existing code.

Supposing efficiency is an overriding concern we could use something like
this:

-exec sh -c 'test -d "$1"/.svn || test -d "$1"/.git || test -d "$1"/CVS'
fnord {} \;

The fnord there of course is assigned to $0.  The above would need careful
testing for space handling in particular.






On Wed, Oct 26, 2022 at 1:16 AM raf <raf@raf.org> wrote:

> On Wed, Oct 26, 2022 at 12:35:34AM +0200, Bernhard Voelker <
> mail@bernhard-voelker.de> wrote:
>
> > First of all: thanks for the patch.
> > A concrete code change is always a good basis for discussion. :-)
> >
> > On 10/23/22 01:54, raf wrote:
> > > Subject: [PATCH] find: doc: Fix -prune SCM example and really make it
> efficient
> >
> > IMO we shouldn't call this a "fix", because there was nothing wrong with
> the previous sample code.
> > See below.
>
> The term "fix" really only applied to the fact that the
> directories labelled as sample output where really the
> sample input directories.
>
> But the use of the word "efficient" was arguably
> incorrect when three test processes are being executed
> for every file as well as every directory.
>
> I consider these to both be documentation bugs, even if
> the second one isn't a functional bug.
>
> > > * find/find.1 - Fix explanation of -prune SCM example and make it
> efficient
> > > * doc/find.texi - Make -prune SCM example efficient
> > > ---
> > >   NEWS          |  5 +++++
> > >   doc/find.texi |  6 +++---
> > >   find/find.1   | 19 ++++++++++++++-----
> > >   3 files changed, 22 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/NEWS b/NEWS
> > > index dffca5aa..b3f2074c 100644
> > > --- a/NEWS
> > > +++ b/NEWS
> > > @@ -13,6 +13,11 @@ GNU findutils NEWS - User visible changes.      -*-
> outline -*- (allout)
> > >     findutils now builds again on systems with musl-libc.
> > >     This requires gettext-0.19.8.
> > > +** Documentation Changes
> > > +
> > > +  The find.1 manual and the Texinfo manual -prune SCM example has
> > > +  been corrected (manual) and made much more efficient (both) [#62259]
> >
> > again, the previous version was not incorrect.
> >
> > > +
> > >   * Noteworthy changes in release 4.9.0 (2022-02-22) [stable]
> > > diff --git a/doc/find.texi b/doc/find.texi
> > > index 1f295837..8c3972d4 100644
> > > --- a/doc/find.texi
> > > +++ b/doc/find.texi
> > > @@ -5139,9 +5139,9 @@ already found.
> > >   @smallexample
> > >   find repo/ \
> > > --exec test -d @{@}/.svn \; -or \
> > > --exec test -d @{@}/.git \; -or \
> > > --exec test -d @{@}/CVS \; -print -prune
> > > +    -type d \
> > > +    -exec test -d @{@}/.svn -o -d @{@}/.git -o -d @{@}/CVS \; \
> > > +    -print -prune
> > >   @end smallexample
> >
> > The purpose was more to get an idea about how pruning works rather than
> getting the most
> > efficient way for the example use case.  Of course, we could and should
> also give a direction
> > about an even more efficient way.
> > In that example, this works because the 'test' utility - most probably
> coming from coreutils
> > or a compatible implementation - allows to use the -o operator to do
> more checks in one
> > process invocation.
> >
> > What about guiding the user in the documentation by saying that the
> '-exec ... -or -exec ...'
> > is the basic way in find to run several checks against the current
> entry, and to give a hint
> > that in this particular case the `test` utility allows to reduce the
> number of execv()s by
> > using the -o operator of that tool?
>
> That sounds to me like too much explication for an
> example. And it's not related to the stated purpose of
> the example which is to demonstrate prune. The purpose
> is not to demonstrate -exec or -or. That's why I
> tbought it would be OK to remove the triple -exec test.
> It's not relevant to the example. Unless it is, and
> it's just not obvious that it's another (unstated)
> purpose of the example (which is fine, of course).
>
> If you prefer the triple test processes, that's fine,
> but I really think that at least the -type d should be
> added to the example, just so that the triple test
> processes are only executed when the candidate entry is
> a directory.
>
> > BTW: `make syntax-check` complains about the new syntax:
> >
> >   $ make syntax-check
> >   ...
> >   prohibit_test_minus_ao
> >   doc/find.texi:5143:    -exec test -d @{@}/.svn -o -d @{@}/.git -o -d
> @{@}/CVS \; \
> >   maint.mk: use "test C1 && test C2", not "test C1 -a C2"; use "test C1
> || test C2", not "test C1 -o C2"
> >   make: *** [maint.mk:1099: sc_prohibit_test_minus_ao] Error 1
>
> That looks to me like a false positive in make syntax-check.
> It's not possible to replace the -o with || in this context.
> It's assuming that the test command is being parsed by a shell,
> when it's actually being parsed by find.
>
> But if we leave the triple exec in place, this won't matter.
>
> I am surprised that I didn't spot that. I think I checked
> it after changing the manual entry but before changing the
> texi. Sorry about that.
>
> > It seems we have to exempt the texi file from that check.
> >
> > >   In this example, @command{test} is used to tell if we are currently
> > > diff --git a/find/find.1 b/find/find.1
> > > index 429aa2f0..f5fa4eee 100644
> > > --- a/find/find.1
> > > +++ b/find/find.1
> > > @@ -2494,15 +2494,14 @@ projects' roots:
> > >   .in +4m
> > >   .B $ find repo/ \e
> > >   .in +4m
> > > -.B \e( \-exec test \-d \(aq{}/.svn\(aq \e; \e
> > > -.B \-or \-exec test \-d \(aq{}/.git\(aq \e; \e
> > > -.B \-or \-exec test \-d \(aq{}/CVS\(aq \e; \e
> > > -.B \e) \-print \-prune
> > > +.B \-type d \e
> > > +.B \-exec test \-d \(aq{}/.svn\(aq \-o \-d \(aq{}/.git\(aq \-o \-d
> \(aq{}/CVS\(aq \e; \e
> > > +.B \-print \-prune
> > >   .in -4m
> > >   .in -4m
> > >   \&
> > >   .fi
> > > -Sample output:
> > > +Sample directories:
> >
> > ugg, yes, the output does not contain the SCM directories.  That was
> wrong.  Good catch!
> >
> > >   .nf
> > >   \&
> > >   .in +4m
> > > @@ -2513,6 +2512,16 @@ Sample output:
> > >   .B repo/project4/.git
> > >   .in
> > >   \&
> > > +Sample output:
> > > +.nf
> > > +\&
> > > +.in +4m
> > > +.B repo/project1
> > > +.B repo/gnu/project2
> > > +.B repo/gnu/project3
> > > +.B repo/project4
> > > +.in
> > > +\&
> > >   .fi
> > >   In this example,
> > >   .B \-prune
> >
> > Would you like to propose a v2?
>
> Sure. But what should it look like? It sounds like the
> following might be OK:
>
>   Changing "Sample output" to "Sample directories".
>   Adding a "Sample output" paragraph.
>   Adding -type d to the command.
>   Leave the triple exec test in place.
>
> Is that what you have in mind?
>
> > P.S. You've sent more patches already, and they're becoming more
> non-trivial.
> > This requires that we have a Copyright assignment of you and your
> employer in
> > place.  I think I mentioned this already last time, but this got out of
> my focus.
> > Would you like to proceed with the FSF copyright paperwork, please?
> > It's just that we can only accept trivial patches without an official
> Copyright
> > assignment to the FSF - this one would still be okay because of its size,
> > but it seems you want to contribute more ... which is great.
> > Of course, I can and will assist you in that regard - we're always in
> need of
> > more official contributors.
>
> Sure.
>
> > Thanks & have a nice day,
> > Berny
>
> cheers,
> raf
>
>
>