Re: [PATCH v2] ln: add the --relative option

[Jim Meyering]

Pádraig Brady wrote:
> Attached is a mergeable version.
> I'd be inclined to include this in the
> imminent release given the unlikely impact
> on existing code?

Thanks.  That is reasonable.
I'll make another snapshot once you've pushed this.

> * src/relpath.c: Refactored from realpath.c and adjusted
> to support returning the relative path rather than just outputting.
> * src/relpath.h: Export the relpath function.
> * src/Makefile.am: Reference the refactored relpath module.
> * po/POTFILES.in: Likewise.
> * src/ln.c (usage): Mention the new option.
> (do_link): Call the relative conversion is specified.

s/is/if/ ?

> (convert_abs_rel): Perform the relative conversion
> using the refactored relpath module.
> * src/realpath.c: Adjust to the refactored relpath module.
> * tests/ln/relative: Add a new test.
> * tests/Makefile.am: Reference the new test.
> * doc/coreutils.texi: Document the new feature.
> * NEWS: Mention the new feature.
> ---
>  NEWS               |    3 +
>  doc/coreutils.texi |    8 +++
>  po/POTFILES.in     |    1 +
>  src/Makefile.am    |    2 +
>  src/ln.c           |   56 +++++++++++++++++++++-
>  src/realpath.c     |   96 ++-----------------------------------
>  src/relpath.c      |  134 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/relpath.h      |   25 ++++++++++
>  tests/Makefile.am  |    1 +
>  tests/ln/relative  |   32 ++++++++++++
>  10 files changed, 265 insertions(+), 93 deletions(-)
>  create mode 100644 src/relpath.c
>  create mode 100644 src/relpath.h
>  create mode 100755 tests/ln/relative
>
> diff --git a/NEWS b/NEWS
> index 5b53eb8..0fe75e5 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -25,6 +25,9 @@ GNU coreutils NEWS                                    -*- 
> outline -*-
>    dd now accepts the conv=sparse flag to attempt to create sparse
>    output, by seeking rather than writing to the output file.
>
> +  ln now accepts the --relative option, to generate relative
> +  symbolic links to a target, irrespective of how the target is specified.

s/relative/a relative/ ? (and s/links/link/)
so that it doesn't sound like ln can create more than one link per target?

>    split now accepts an optional "from" argument to --numeric-suffixes,
>    which changes the start number from the default of 0.
>
> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> index 10be715..a8ed050 100644
> --- a/doc/coreutils.texi
> +++ b/doc/coreutils.texi
> @@ -9389,6 +9389,14 @@ symbolic link with identical contents; since symbolic 
> link contents
>  cannot be edited, any file name resolution performed through either
>  link will be the same as if a hard link had been created.
>
> +@item -r
> +@itemx --relative
> +@opindex -r
> +@opindex --relative
> +Make symbolic links relative to the link location.
> +@xref{realpath invocation}, which gives greater control
> +over relative path generation.

Adding an example or two would help describe how it works.

>  @item -s
>  @itemx --symbolic
>  @opindex -s
...
> diff --git a/src/ln.c b/src/ln.c
...
> +/* Return FROM represented as relative to the dir of TARGET.
> +   The result is malloced.  */
> +
> +static 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);
> +
> +  /* Write to a PATH_MAX buffer, guarding against very large values.  */
> +  size_t nalloc = MIN (PATH_MAX, 32 * 1024);

Isn't this imposing an arbitrary limitation?
Even if it's a limitation we want to avoid, that needn't happen now.

> +  char *relative_from = xmalloc (nalloc);
> +
> +  /* Get dirname to generate paths relative to.  */
> +  realtarget[dir_len (realtarget)] = '\0';
> +
> +  if (!relpath (realfrom, realtarget, relative_from, nalloc))
> +    {
> +      free (relative_from);
> +      relative_from = NULL;
> +    }
> +
> +  free (realtarget);
> +  free (realfrom);
> +
> +  if (relative_from)
> +    return relative_from;
> +  else
> +    return xstrdup (from);

Let's replace those four lines with one:

    return relative_from ? relative_from : xstrdup (from);

> +}
...
> diff --git a/src/realpath.c b/src/realpath.c
...
> diff --git a/src/relpath.c b/src/relpath.c
...
> +/* Return the length of the longest common prefix
> +   of canonical PATH1 and PATH2, ensuring only full path components
> +   are matched.  Return 0 on no match.  */
> +static int _GL_ATTRIBUTE_PURE
> +path_common_prefix (const char *path1, const char *path2)
> +{
> +  int i = 0;
> +  int ret = 0;
> +
> +  /* 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.  */
> +  if ((path1[1] == '/') != (path2[1] == '/'))
> +    return 0;
> +
> +  while (*path1 && *path2)
> +    {
> +      if (*path1 != *path2)
> +        break;
> +      if (*path1 == '/')
> +        ret = i + 1;
> +      path1++;
> +      path2++;
> +      i++;
> +    }
> +
> +  if (!*path1 && !*path2)
> +    ret = i;
> +  if (!*path1 && *path2 == '/')
> +    ret = i;
> +  if (!*path2 && *path1 == '/')
> +    ret = i;

It would be clearer and slightly more efficient to do this:

     if ((!*path1 && !*path2)
         || (!*path1 && *path2 == '/')
         || (!*path2 && *path1 == '/'))
       ret = i;

> +  return ret;
> +}
> +
> +/* 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.  */
> +static bool
> +buffer_or_output (const char* str, char **pbuf, size_t *plen)
> +{
> +  if (*pbuf)
> +    {
> +      size_t slen = strlen (str);
> +      if (slen >= *plen)
> +        return true;
> +      memcpy (*pbuf, str, slen + 1);
> +      *pbuf += slen;
> +      *plen -= slen;
> +    }
> +  else
> +    fputs (str, stdout);

Please put curly braces around a one-line else body
whenever there are braces around the corresponding "then" body.

> +  return false;
> +}
> +
> +/* Output the relative representation if possible.
> +   If BUF is non NULL, output is to that buffer rather than stdout.  */

Use active voice, not passive:

      If BUF is non NULL, write to that buffer rather than to stdout.  */

> +bool
> +relpath (const char *can_fname, const char *can_reldir, char *buf, size_t 
> len)
> +{
> +  bool buf_err = false;
> +
> +  /* Skip the prefix common to --relative-to and path.  */
> +  int common_index = path_common_prefix (can_reldir, can_fname);
> +  if (!common_index)
> +    return false;
> +
> +  const char *relto_suffix = can_reldir + common_index;
> +  const char *fname_suffix = can_fname + common_index;
> +
> +  /* skip over extraneous '/'.  */

s/s/S/

> +  if (*relto_suffix == '/')
> +    relto_suffix++;
> +  if (*fname_suffix == '/')
> +    fname_suffix++;
> +
> +  /* Replace remaining components of --relative-to with '..', to get
> +     to a common directory.  Then output the remainder of fname.  */
> +  if (*relto_suffix)
> +    {
> +      buf_err |= buffer_or_output ("..", &buf, &len);
> +      for (; *relto_suffix; ++relto_suffix)
> +        {
> +          if (*relto_suffix == '/')
> +            buf_err |= buffer_or_output ("/..", &buf, &len);
> +        }
> +
> +      if (*fname_suffix)
> +        {
> +          buf_err |= buffer_or_output ("/", &buf, &len);
> +          buf_err |= buffer_or_output (fname_suffix, &buf, &len);
> +        }
> +    }
> +  else
> +    {
> +      if (*fname_suffix)
> +        buf_err |= buffer_or_output (fname_suffix, &buf, &len);
> +      else
> +        buf_err |= buffer_or_output (".", &buf, &len);

avoid a little duplication:

         buf_err |= buffer_or_output (*fname_suffix ? fname_suffix : ".",
                                      &buf, &len);

> +    }
> +
> +  if (buf_err)
> +    error (0, ENAMETOOLONG, "%s", _("generating relative path"));
> +
> +  return !buf_err;
> +}
> diff --git a/src/relpath.h b/src/relpath.h