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

[raf]

On Wed, Oct 26, 2022 at 03:20:09PM +0200, Bernhard Voelker 
 wrote:

> On 10/26/22 11:33, James Youngman wrote:
> > 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:

Efficiency is only a concern because the example is
described as being efficient, but the reason for that
is its use of -prune. Three sh processes per directory
would be fine, but it's currently doing three sh
processes for every file type, not just directories.
That seems to me to just be too wasteful. That's why I
want to insert "-type d" before the -exec. I'm not that
concerned with combining the three -execs into one. It
just seemed like a good idea at the time.

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

The quoted environment variables do protect against
shell meta-characters like spaces and quotes in names.

I think this would have been nicer:

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

BUT: I just tested this with a directory name
containing a space, and {} doesn't automatically
backslash-quote shell meta-characters when it's used as
part of a larger command line argument that might
result in multiple shell tokens. That's a surprise to
me. I'd consider it a bug, but changing it might not be
backwards-compatible (but it might be). Quoting the {}
isn't enough. Shells won't treat it the same as quoted
environment variables.

This also works and is shorter:

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

Would that be OK? Or is it too wierd to use $0 that way?

> Thanks for the reminder - I had forgotten about that.
> Personally I never use `test X -o Y` ... because of the ambiguity mentioned 
> there.
> 
> Still, I think the find manual should first use the triple -exec example as 
> today,
> and then mention that this special case can be tuned by using the above 
> '-exec sh -c ...'.
> 
> I believe documenting both ways is good because the tests for certain child 
> names is
> just a special case which can be done by the test utility - while other 
> real-life
> examples would need to perform other checks.
> 
> Have a nice day,
> Berny

OK. I'll submit another version of the patch that does that.

But which version of the single -exec command would be best:

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

  -exec sh -c 'test -d "$0"/.svn || test -d "$0"/.git || test -d "$0"/CVS' {} \;
  -exec sh -c '[ -d "$0"/.svn ] || [ -d "$0"/.git ] || [ -d "$0"/CVS ]' {} \;

I don't like including "fnord", because I feel many
readers won't immediately understand why it's there,
and so it should be accompanied by an explanation of
why it's there, but including such an explanation is
not desirable because it has nothing to do with the
point of the example which is to demonstrate -prune
as an efficiency measure, not to demonstrate sh -c.
But I'll use whichever version you prefer.

cheers,
raf

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

[Bernhard Voelker]

On 10/26/22 11:33, James Youngman wrote:
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.


Thanks for the reminder - I had forgotten about that.
Personally I never use `test X -o Y` ... because of the ambiguity mentioned 
there.

Still, I think the find manual should first use the triple -exec example as 
today,
and then mention that this special case can be tuned by using the above '-exec 
sh -c ...'.

I believe documenting both ways is good because the tests for certain child 
names is
just a special case which can be done by the test utility - while other 
real-life
examples would need to perform other checks.

Have a nice day,
Berny

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  wrote:

> On Wed, Oct 26, 2022 at 12:35:34AM +0200, Bernhard Voelker <
> m...@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 a

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

[raf]

On Wed, Oct 26, 2022 at 12:35:34AM +0200, Bernhard Voelker 
 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
>

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

[Bernhard Voelker]

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.


* 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?

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

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?

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.

Thanks & have a nice day,
Berny