Re: [PATCH v5] Add LFS support for fts functions [BZ #11460]

[Mark Wielaard <mjw at redhat dot com>]

On Mon, 2015-11-02 at 19:02 -0500, Carlos O'Donell wrote:
> On 10/22/2015 07:34 AM, Mark Wielaard wrote:
>
> Please fix the various nits and wait until the end of the week before
> checking in. That should be enough time for others to review if they
> think something should need a change.

OK.

> > +* fts.h can now be used with -D_FILE_OFFSET_BITS=64.  With LFS the following
> > +  new symbols are used: fts64_children, fts64_close, fts64_open, fts64_read
> > +  and fts64_set.
> 
> Suggest:
> 
> * The fts.h header can now be used with ...

Changed.

> > diff --git a/io/fts.c b/io/fts.c
> > index f832676..32f60a6 100644
> > --- a/io/fts.c
> > +++ b/io/fts.c
> > @@ -1,3 +1,20 @@
> 
> Should have a one line description.

Added "File tree traversal functions."

> > +/* Copyright (C) 1992-2015 Free Software Foundation, Inc.
> 
> How did you determine 1992?

I assumed they got introduced with the other files when the io
subdirectory got added. But your detective work says otherwise:

> The oldest ChangeLog reference to file modifications which would
> require the new FSF header is 1994 (ChangeLog.4).
> 
> i.e.
> ~~~
> Tue Nov 15 01:39:36 1994  Roland McGrath  <roland@churchy.gnu.ai.mit.edu>
> ...
>         * io/fts.c (MAXPATHLEN): Define if not defined.
> ...
>         * io/fts.c, io/fts.h: New files, from 4.4 BSD code by Keith Bostic.
> ...
> ~~~
> 
> Therefore I think this should be 1994-2015.

Agreed, changed.

> > --- a/io/fts.h
> > +++ b/io/fts.h
> > @@ -1,3 +1,20 @@
> 
> Needs one line header.

Added "File tree traversal function declarations."

> > +/* Copyright (C) 1992-2015 Free Software Foundation, Inc.
> 
> Likeise, should IMO be 1994-2015.

Changed.

> > --- /dev/null
> > +++ b/io/fts64.c
> > @@ -0,0 +1,29 @@
> 
> Line describing thefile.

Added "File tree traversal functions LFS version."

> > +static int
> > +do_test (void)
> > +{
> > +  char *paths[2] = { fts_test_dir, NULL };
> > +  FTS *fts;
> > +  fts = fts_open (paths, FTS_LOGICAL, &compare_ents);
> > +  if (fts == NULL)
> > +    {
> > +      printf ("fts_open: %m\n");
> 
> Please prefix "FAIL:" so it's clear it's a failure.

Added. And for the other failing outputs too.

> Optionally print "PASS:" entries for passing a given sub-test or the
> whole test, such that it's clear from *.test-results that we actually
> did some useful work.

I added a single puts ("PASS") before the final return 0.

Thanks,

Mark