Re: bugs in dirname module - gnulib portion

[Paul Eggert]

Eric Blake <address@hidden> writes:

> -       /* If the file name ends in slash, use the trailing slash as
> -          the basename if no non-slashes have been found.  */
> +       /* If the file name ends in slash, but no non-slashes have
> +          been found, then return the empty string.  */
>         if (! *p)
>           {
>             if (ISSLASH (*base))
> -             base = p - 1;
> +             base = p;
>             break;
>           }

That is confusing.  Wouldn't this be a lot simpler, as a replacement
for the entire main loop in last_component?

  for (p = base; +p; p++)
    if (ISSLASH (*p))
      base = p + 1;


+  /* Collapse a sequence of trailing slashes into one.  */
+  size_t length = base_len (base);

This assumes C99 declaration-after-statement.  Let's keep the code
portable to C89.


> +  /* On systems with drive letters, `a/b:c' must return `./b:c' rather
> +     than `b:c' to avoid confusion with a drive letter.  On systems
> +     with pure POSIX semantics, this is not an issue.  */
> +  if (FILE_SYSTEM_PREFIX_LEN (base))
> +    {
> +      char *p = xmalloc (length + 3);
> +      p[0] = '.';
> +      p[1] = DIRECTORY_SEPARATOR;
> +      memcpy (p + 2, base, length);
> +      p[length + 2] = '\0';
> +      return p;
> +    }

This mishandles 'a/b:c/', no?  The trailing slash gets dropped.

Also, what's the point of DIRECTORY_SEPARATOR?  Can't we just use '/'?

> +  if (prefix_len && ! FILE_SYSTEM_DRIVE_PREFIX_IS_ABSOLUTE

This would be easier to read this way:

  if (FILE_SYSTEM_DRIVE_PREFIX_IS_RELATIVE && prefix_len

so that it's more obvious that the code is optimized away on normal systems.

> +  /* Be careful of drive prefixes, where that matters.  */
> +  if (0 < prefix_length)
> +    return (prefix_length
> +         + (! FILE_SYSTEM_DRIVE_PREFIX_IS_ABSOLUTE
> +            && ISSLASH (file[prefix_length])));
> +
> +  /* Don't strip the only slash from "/", or from "//" where that matters.  
> */
> +  return (ISSLASH (file[0])
> +       + (DOUBLE_SLASH_IS_DISTINCT_ROOT && ISSLASH (file[0])
> +          && ISSLASH (file[1]) && ! ISSLASH (file[2])));
>  }

Hmm, a bit hard to read, for the same reason as above.  Also, it tests
ISSLASH (file[0]) more than once.  How about something like this
instead, right after initializing prefix_length?  Or perhaps you can
come up with something better.

  prefix_length +=
    (prefix_length != 0
     ? FILE_SYSTEM_DRIVE_PREFIX_IS_RELATIVE && ISSLASH (file[prefix_length])
     : ! ISSLASH (file[0])
     : 0
     ? (DOUBLE_SLASH_IS_DISTINCT_ROOT
        && ISSLASH (file[1]) && ! ISSLASH (file[2]))
     ? 2
     : 1);