Re: [PATCH 2/3] getcwd-lgpl: new module

[Jim Meyering]

Eric Blake wrote:
> For programs that aren't worried about being invoked from an
> arbitrarily long current working directory (perhaps because it
> always does chdir to a sane location first), the getcwd module
> is overkill, given that all modern portability have a getcwd
> that works on short paths.

Looks fine, modulo grammar nits, but you
might want to wait to hear from Paul.

You left out a word there:

  s/ portability/ portability targets/

> * modules/getcwd-lgpl: New module.
> * doc/posix-functions/getcwd.texi (getcwd): Document it.
> * MODULES.html.sh (lacking POSIX:2008): Likewise.
> * m4/getcwd.m4 (gl_PREREQ_GETCWD): Define extra witness.
> (gl_FUNC_GETCWD_LGPL): New macro.
> * lib/getcwd.c (rpl_getcwd): Provide alternate LGPL
> implementation.
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  ChangeLog                       |    9 ++
>  MODULES.html.sh                 |    1 +
>  doc/posix-functions/getcwd.texi |   13 ++-
>  lib/getcwd.c                    |  208 
> +++++++++++++++++++++++++--------------
>  m4/getcwd.m4                    |   30 +++++-
>  modules/getcwd-lgpl             |   25 +++++
>  6 files changed, 205 insertions(+), 81 deletions(-)
>  create mode 100644 modules/getcwd-lgpl
>
> diff --git a/ChangeLog b/ChangeLog
> index f113c64..1105b88 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,14 @@
>  2011-04-25  Eric Blake  <address@hidden>
>
> +     getcwd-lgpl: new module
> +     * modules/getcwd-lgpl: New module.
> +     * doc/posix-functions/getcwd.texi (getcwd): Document it.
> +     * MODULES.html.sh (lacking POSIX:2008): Likewise.
> +     * m4/getcwd.m4 (gl_PREREQ_GETCWD): Define extra witness.
> +     (gl_FUNC_GETCWD_LGPL): New macro.
> +     * lib/getcwd.c (rpl_getcwd): Provide alternate LGPL
> +     implementation.
> +
>       getcwd: consolidate m4 files
>       * m4/getcwd-abort-bug.m4, m4/getcwd-path-max.m4: Delete, moving
>       contents...
> diff --git a/MODULES.html.sh b/MODULES.html.sh
...
> diff --git a/doc/posix-functions/getcwd.texi b/doc/posix-functions/getcwd.texi
...
> diff --git a/lib/getcwd.c b/lib/getcwd.c
> index e52af18..c221ecc 100644
> --- a/lib/getcwd.c
> +++ b/lib/getcwd.c
> @@ -20,73 +20,77 @@
>  #endif
>
>  #include <errno.h>
> -#include <sys/types.h>
> -#include <sys/stat.h>
>  #include <stdbool.h>
> -#include <stddef.h>
> +#include <string.h>
> +
> +#if USE_GPL_GETCWD

... <cpp indentation>...

> +#else
> +/* LGPL version - only guarantees that NULL can be used as first

s/only guarantees/guarantees only/

> +   argument.  */
> +
> +# undef getcwd
> +char *
> +rpl_getcwd (char *buf, size_t size)
> +{
> +  char *tmp = NULL;
> +  bool first = true;
> +
> +  if (buf)
> +    return getcwd (buf, size);
> +
> +  do
> +    {
> +      tmp = buf;
> +      if (first)
> +        {
> +          size = size ? size : 4096;
> +          first = false;
> +        }
> +      else
> +        {
> +          size <<= 1;
> +        }
> +      if ((buf = realloc (tmp, size)) == NULL)
> +        {
> +          free (tmp);
> +          errno = ENOMEM;
> +          return NULL;
> +        }
> +      tmp = getcwd (buf, size);
> +    }
> +  while (!tmp && errno == ERANGE);
> +
> +  if (!tmp)
> +    {
> +      int saved_errno = errno;
> +      free (buf);
> +      errno = saved_errno;
> +    }
> +  else
> +    {
> +      /* Trim to fit, if possible.  */
> +      tmp = realloc (buf, strlen (buf) + 1);
> +      if (!tmp)
> +        tmp = buf;
> +    }
> +  return tmp;
> +}
> +
>  #endif
> diff --git a/m4/getcwd.m4 b/m4/getcwd.m4
> index 5016a1b..79f5276 100644
> --- a/m4/getcwd.m4
> +++ b/m4/getcwd.m4
> @@ -6,7 +6,7 @@
>  # with or without modifications, as long as this notice is preserved.
>
>  # Written by Paul Eggert.
> -# serial 4
> +# serial 5
>
>  AC_DEFUN([gl_FUNC_GETCWD_NULL],
>    [
> @@ -333,6 +333,28 @@ main ()
>  ])
>

Normally I wouldn't worry about comments too much, but this is prescriptive,
and might be used by a potential client to decide whether to use
the lighter weight LGPL replacement or the full-blown one, so...

> +dnl Check just for getcwd behavior on NULL.  Assumes that either the
> +dnl system getcwd works, or that the code is okay with spurious

"works" is subject to interpretation ;-)

    s/works/is robust/

"the code" might make the reader think of "this code"

    s/code/calling code|caller/

> +dnl failures when run from super-deep hierarchies.

This is imprecise.  The depth is not as relevant as the length of the
name.  For me, "super deep" means at least 100,000 levels, and probably
more like millions, but I'm biased ;-)  Does a working directory with
only 10 or 20 components count as super deep?  With pathologically
long components, such a directory may have a name that is too long.

How about "when run from a working directory whose absolute name is
longer than 1023 bytes".