Re: New Feature Submission for GNU Make

[Eli Zaretskii]

> Date: Sun, 29 May 2011 21:45:38 -0700
> From: Ben Robinson <address@hidden>
> 
> I am submitting a patch which adds two new functions to GNU Make.

Thanks.  Please wait for Paul's word about inclusion of these in
Make.  What's below are a few comments based on a first impression
from the code.

> $(trimpath names...)
> For each file name in names, returns a name that does not contain any . or
> .. components, nor any repeated path separators (/).

The part about "." and ".." is inaccurate: the function removes any
redundant "." and "..", but it's not true that the value will never
include these, as your tests clearly show:

> ifneq ($(trimpath ../foo/),../foo)
>   $(error )
> endif

> $(relpath names...)
> For each file name in names, returns the relative path to the file.

This doesn't say relative to what.  From reading the code I understand
that it's relative to the starting directory (which could be different
from the current directory when a given recipe runs).  Why not use
$(CURDIR) instead?

>                                                                     This
> relative path does not contain any . or .. components, nor any repeated path
> separators (/).

Again, this is inaccurate: periods will still appear when they are
needed.

> These functions contain capabilities which are significantly different from
> the existing abspath and realpath, in that they do not convert what could be
> an extremely short relative path (e.g. ".") into a long absolute path.
>  relpath is in fact the inverse of abspath, and adding it would make the set
> of path conversion functions more complete.  In addition, trimpath can be
> applied to paths both absolute and relative, and eliminate needless
> characters which can improve readability and performance.

Any use case where adding these functions would be needed?
Completeness is not generally enough to add features.

A few comments on the code, mostly related to Windows portability:

> >   if (name[0] == '/') {
> >     ++fixed;
> >     abspath = 1;
> >   }

This is not the GNU style of using braces (here and everywhere else in
the patch).

> >   /* A Windows style absolute path */
> >   if (name[1] == ':' && name[2] == '/') {
> >     fixed += 3;
> >     abspath = 1;
> >   }

Please use the existing IS_ABSOLUTE and ROOT_LEN macros.

Also, I'm not sure the Windows parts should be compiled on Posix
platforms, since it is possible (although unlikely) to have a file or
directory there named "C:".

> >     /* Take only the first path-separator. */
> >     if (*start == '/') {
> >       ++start;
> >       *dest++ = '/';
> >     }

Please don't assume the directory separator is always '/', it could be
'\\' on DOS/Windows.  Please use IS_PATHSEP instead of literal
slashes.

> >     /* Skip sequence of multiple path-separators. */
> >     while (*start == '/') {
> >       ++start;
> >     }

This will defeat UNC "//foo/bar" or "\\\\foo\\bar" file names on
Windows.

> >   /* Strip the trailing separator if any. */
> >   if (dest > apath && dest[-1] == '/') {
> >     /* Unless name is an absolute path resulting in only '/' */
> >     if (!(name[0] == '/' && dest == apath + 1)) {
> >       --dest;

This does not consider the DOS/Windows case where an absolute file
name does not begin with a slash.

> >   /* If the resulting path is empty, return a '.' */
> >   if (dest == apath) {
> >     *dest++ = '.';
> >   }

Same here.

> >   /* A unix style absolute path */
> >   if (name[0] == '/') {
> >     abspath = 1;
> >     if (starting_directory[1] == ':' && starting_directory[2] == '/') {
> >       /* A Windows system should not get passed a unix style absolute path 
> > */
> >       return NULL;

What happens if starting_directory is a "//foo/bar" UNC?

> >   /* A Windows style absolute path */
> >   if (name[1] == ':' && name[2] == '/') {

Again, please use IS_ABSOLUTE.

> >     if (name[0] != starting_directory[0]) {
> >       /* Cannot convert a path on a different drive letter to relative */
> >       return NULL;

??? Why not? simply return the original name without any changes, it's
better than failing.

> >   if (strlen(tname) != 1 && tname[0] != '/') {
> >     strcat(tname, "/");
> >   }

This again works only on Unix.

> >   /* Skip common characters in both paths */
> >   while (*srcname == *srcdir && *srcname  != '\0' && *srcdir  != '\0') {
> >     ++srcname;
> >     ++srcdir;
> >   }
> >   /* Now rewind to last common / */
> >   while (*srcname != '/' && *srcdir != '/' && srcdir != starting_directory) 
> > {
> >     --srcname;
> >     --srcdir;
> >   }

What will happen here if the argument of relpath is exactly identical
to starting_directory?

Also, the test `srcdir != starting_directory' will not DTRT on
Windows, because the first character is not a slash.

> >   /* If the resulting path is empty, return a '.' */
> >   if (dest == apath) {
> >     *dest++ = '.';
> >   }

Not TRT on Windows.

Thanks again for working on this.