Re: new module: relpath

[Bruno Haible]

Hi Akim,

Akim Demaille wrote:
> 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").

I agree that
  1. this is general-purpose functionality that has its place in gnulib,
  2. when you start to copy code from coreutils to bison, it's time to
     move it to gnulib, to avoid duplicate maintenance.

About the module name 'relpath': The GNU standards, section "GNU Manuals",
say
   Please do not use the term "pathname" that is used in Unix
   documentation; use "file name" (two words) instead.  We use the term
   "path" only for search paths, which are lists of directory names.

Strictly speaking this is only about manuals, not about code. But the
more we use the term "file name" also in code, the less we are tempted
to misuse the term "path" in documentation. Gnulib already has modules
  concat-filename
  filename
  filenamecat
It would be good to choose a module name in this spirit.

> * lib/relpath.c: New.
> * lib/relpath.h: New.
> * modules/relpath: New.

When you contribute a gnulib module, it's also welcome to write a unit
test: 2 files tests/test-relpath.c and modules/relpath-tests.

> +License:
> +GPLv3+

Please write 'GPL' here. It is semantically equivalent to 'GPLv3+', but
gnulib-tool does not understand 'GPLv3+'.

> +++ b/modules/relpath

> +Depends-on:
> +canonicalize
> +dirname
> +error
> +pathmax
> +stdbool
> +xalloc
> +...
> +License:
> +GPLv3+

This module may produce error messages and be under GPL. Then the module
is not usable in shared libraries. If you want a module that is more widely
usable, consider an interface that returns error codes  or error strings,
remove the dependencies to 'error' and 'xalloc', and change the license
to 'LGPL'.

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

This comment is incomplete. It should tell what is the meaning of each
argument (CAN_FNAME, CAN_RELDIR, BUF if == NULL, LEN) and what is the
meaning of the return value ('bool' here: does true mean success or does
it mean failure?).

Also, I find it odd to have a function that produces either a string or
some output (on a fixed stream, that is not even passed as argument).
For a gnulib module it would be more appropriate to have only the string
case. Users who need to output it to stdout can do that after invoking
the function.

> +/* Return FROM represented as relative to the dir of TARGET.
> +   The result is malloced.  */
> +
> +char *
> +convert_abs_rel (const char *from, const char *target);

It is a bit strange here that if I want to compute a relative filename,
relative to a given directory, I will have to add "/." to this TARGET
directory. How about either
  - adding a second function that takes a directory as second argument
    (as opposed to a file in this directory), or
  - change the specification of convert_abs_rel in this way, outright?
    (Then many callers will have to perform a dirname() themselves.)

> +#include <config.h>
> +
> +#include <stdbool.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include <errno.h>
> +
> +#include "gettext.h"
> +#define _(msgid) gettext (msgid)
> +
> +#include "canonicalize.h"
> +#include "dirname.h"
> +#include "error.h"
> +#include "relpath.h"
> +#include "xalloc.h"

By convention, the specification header ("relpath.h" in this case) should
be included right after <config.h>. This helps verifying that the include
file is self-contained, i.e. does not need a prerequisite of <sys/types.h>
or similar.

Bruno