findutils-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: raf
Subject: Re: [PATCH] find: doc: Fix -prune SCM example and really make it efficient
Date: Wed, 26 Oct 2022 11:13:48 +1100

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]