Date: Thu, 5 Aug 2004 11:07:13 +1000 From: Tim Robbins <tjr@freebsd.org> To: Alexey Dokuchaev <danfe@nsu.ru>, Garance A Drosehn <gad@FreeBSD.ORG>, src-committers@FreeBSD.ORG, cvs-src@FreeBSD.ORG, cvs-all@FreeBSD.ORG Subject: ftw [Re: cvs commit: src/usr.bin/patch - Imported sources] Message-ID: <20040805010713.GA40216@cat.robbins.dropbear.id.au> In-Reply-To: <20040804210524.GA8512@VARK.homeunix.com> References: <200408012045.i71KjtFX087582@repoman.freebsd.org> <20040802034509.GB81089@regency.nsu.ru> <20040802042750.GA24962@cat.robbins.dropbear.id.au> <20040804210524.GA8512@VARK.homeunix.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040805010713.GA40216>