Re: new module: relpath

[Eli Zaretskii]

> From: Akim Demaille <address@hidden>
> Date: Thu, 14 Jun 2012 18:02:35 +0200
> 
> In Bison, I consider computing relative paths between files
> (one concrete use is when the user creates the parser implementation
> file in --output=sub1/sub2/parser.c and the header file in
> --defines=sub3/sub4/parser.h, in which case parser.c should include
> "../../sub3/sub4/parser.h").
> 
> At least to experiment on the concept, and on a suggestion from
> Eric Blake weeks ago, I stole bits of the coreutils, and made them
> a gnulib module.

Perhaps the comments below, mainly related to portability to
MS-Windows (but not only that) will be helpful, even though a lot has
been said already.

> +  /* We already know path1[0] and path2[0] are '/'.  Special case
> +     '//', which is only present in a canonical name on platforms
> +     where it is distinct.  */

This should take into account the drive letter on MS-Windows.  (I
suggest to use FILE_SYSTEM_PREFIX from dosname.h for this.)  It should
also detect that the drive letters are different and return zero.
Finally, there's the issue of whether "d:/foo/bar" and "/foo/baz"
should return a non-zero value (these inputs are possible because
'relpath' does not require the arguments to be canonicalized; perhaps
it should, see below).

> +  if ((path1[1] == '/') != (path2[1] == '/'))

All the tests against a literal '/' should be replaced with IS_SLASH,
to be portable to MS-Windows.

> +  while (*path1 && *path2)
> +    {
> +      if (*path1 != *path2)

This comparison should be case-insensitive for MS-Windows.

> +/* Either output STR to stdout or
> +   if *PBUF is not NULL then append STR to *PBUF
> +   and update *PBUF to point to the end of the buffer
> +   and adjust *PLEN to reflect the remaining space.

The punctuation here could use some improvement.

Also, I suggest to name PBUF differently to reflect the fact that it
is a pointer to the end of the buffer, not to its beginning.

> +   Return TRUE on failure.  */

That is a strange semantics, IMO.  Why not return an int instead of a
bool, if you want an error indication that you can conveniently
accumulate with a bitwise OR?

> +  else
> +    {
> +      fputs (str, stdout);
> +    }

Style: these braces are unnecessary, I think.

> +/* Output the relative representation if possible.
> +   If BUF is non NULL, write to that buffer rather than to stdout.  */

This doesn't document most of the arguments and the return value.

Also, the "can_" part hints that the argument file names are expected
to be canonicalized, but the commentary doesn't say so.

> +  /* Skip the prefix common to --relative-to and path.  */

The references to --relative-to should be replaced (here and
elsewhere), as they no longer make sense in the gnulib context.

> +  /* Skip over extraneous '/'.  */
> +  if (*relto_suffix == '/')
> +    relto_suffix++;
> +  if (*fname_suffix == '/')
> +    fname_suffix++;

Why do you skip only over a single slash?  Can't there be an
arbitrarily long sequence of redundant slashes?

> +  if (buf_err)
> +    error (0, ENAMETOOLONG, "%s", _("generating relative path"));

This looks like some inside knowledge of what happens inside
buffer_or_output.  Why not set errno inside that function instead?
And I think the commentary to buffer_or_output should mention this.

> +/* Return FROM represented as relative to the dir of TARGET.
> +   The result is malloced.  */

This commentary doesn't say that the result can be NULL.

> +
> +char *
> +convert_abs_rel (const char *from, const char *target)
> +{
> +  char *realtarget = canonicalize_filename_mode (target, CAN_MISSING);
> +  char *realfrom = canonicalize_filename_mode (from, CAN_MISSING);

AFAICT, canonicalize_filename_mode can return NULL, but the rest of
the code doesn't cope with that.  In particular, ...

> +  /* Write to a PATH_MAX buffer.  */
> +  char *relative_from = xmalloc (PATH_MAX);
> +
> +  /* Get dirname to generate paths relative to.  */
> +  realtarget[dir_len (realtarget)] = '\0';

... wouldn't the last line segfault if realtarget is NULL?

HTH