ftw [Re: cvs commit: src/usr.bin/patch - Imported sources]

[Tim Robbins]

On Wed, Aug 04, 2004 at 02:05:24PM -0700, David Schultz wrote:
> On Mon, Aug 02, 2004, Tim Robbins wrote:
> > On Mon, Aug 02, 2004 at 10:45:09AM +0700, Alexey Dokuchaev wrote:
> > > On Sun, Aug 01, 2004 at 08:45:55PM +0000, Garance A Drosehn wrote:
> > > > gad         2004-08-01 20:45:55 UTC
> > > > 
> > > >   FreeBSD src repository
> > > > 
> > > >   src/usr.bin/patch - Imported sources
> > > >   Update of /home/ncvs/src/usr.bin/patch
> > > >   In directory repoman.freebsd.org:/tmp/cvs-serv87568
> > > >   
> > > >   Log Message:
> > > >   Import of a BSD-licensed version of `patch', which will  eventually
> > > >   replace the version we currently have in src/gnu/usr.bin/patch/.
> > > >   Among other things, this version includes a --posix option for strict
> > > >   POSIX conformance.
> > > >   
> > > >   This version is the current source from OpenBSD as of today.  It is
> > > >   their 3.5-release, plus a few updates to patch.c and pch.c that they
> > > >   made about three weeks ago.
> > > 
> > > May I ask why you preferred OpenBSD's version over NetBSD's?  It was
> > > shown in the past that OpenBSD's way of doing thing is a bit rough on
> > > the edges sometimes (humanize_number(3) vs. fmt_scaled(3) and
> > > scan_scaled(3), ftw(3) and nftw(3), etc).
> > 
> > Actually, OpenBSD's ftw()/nftw() implementation is better than the one
> > we recently imported, in terms of both style and functionality; I wish we'd
> > gone with it instead. What we have in -CURRENT at the moment is incredibly
> > buggy for such a simple function. The droll, inane comments ("Because
> > errno is our friend") and style violations only make things worse.
> 
> I agree about the style.  What's wrong with the functionality?
> 
> I have no objection to switching to Todd's (much cleaner) [n]ftw()
> implementation, although I'm not aware of any non-stylistic
> problems with the current code.

It calls the user-specified function too many times. For example, it calls
it at least twice for every directory: once when fts_read() returns FTS_D,
once when it returns FTS_DP. It looks like this segment of code is missing
a 'continue':

            case FTS_D:
                if ((MODE_NFTW != mode) || !(flags & FTW_DEPTH)) {
                    ftw_flag = FTW_D;
                }
                break;

and should be this instead:
            case FTS_D:
                if ((MODE_NFTW != mode) || !(flags & FTW_DEPTH)) {
                    ftw_flag = FTW_D;
                } else
		    continue;
                break;

(and similarly for FTS_DP.) The default case should be 'continue', too.
It should also handle FTS_DC and FTS_DEFAULT.

OpenBSD's implementation gets almost everything right. The only problem
I know of is that they sometimes call the user-specified function twice
when a directory cannot be read. This seems to be non-trivial to fix,
and our current implementation also suffers from this problem.

I'd like it if we could switch to the OpenBSD code.


Tim