Re: [PATCH v13 02/15] path.c: refactor relative_path(), not only strip prefix

[Michael Haggerty <mhagger@alum.mit.edu>]

Sorry for coming late to the party.

On 05/22/2013 03:40 AM, Jiang Xin wrote:
> Original design of relative_path() is simple, just strip the prefix
> (*base) from the absolute path (*abs). In most cases, we need a real
> relative path, such as: ../foo, ../../bar. That's why there is another
> reimplementation (path_relative()) in quote.c.
> 
> Refactor relative_path() in path.c to return real relative path, so
> that user can reuse this function without reimplement his/her own.
> I will use this method for interactive git-clean later. Some of the
> implementations are borrowed from path_relative() in quote.c.
> 
> Different results for relative_path() before and after this refactor:
> 
>     abs path  base path  relative (original)  relative (refactor)
>     ========  =========  ===================  ===================
>     /a/b/c/   /a/b       c/                   c/
>     /a/b//c/  //a///b/   c/                   c/
>     /a/b      /a/b       .                    ./
>     /a/b/     /a/b       .                    ./
>     /a        /a/b/      /a                   ../
>     /         /a/b/      /                    ../../
>     /a/c      /a/b/      /a/c                 ../c
>     /a/b      (empty)    /a/b                 /a/b
>     /a/b      (null)     /a/b                 /a/b
>     (empty)   /a/b       (empty)              ./
>     (null)    (empty)    (null)               ./
>     (null)    /a/b       (segfault)           ./

The old and new versions both seem to be (differently) inconsistent
about when the output has a trailing slash.  What is the rule?

> diff --git a/path.c b/path.c
> index 04ff..0174d 100644
> --- a/path.c
> +++ b/path.c
> @@ -441,42 +441,104 @@ int adjust_shared_perm(const char *path)
>  	return 0;
>  }
>  
> -const char *relative_path(const char *abs, const char *base)
> +/*
> + * Give path as relative to prefix.
> + *
> + * The strbuf may or may not be used, so do not assume it contains the
> + * returned path.
> + */
> +const char *relative_path(const char *abs, const char *base,
> +			  struct strbuf *sb)

Thanks for adding documentation.  But I think it could be improved:

* The comment refers to "path" and "prefix" but the function parameters
are "abs" and "base".  I suggest making them agree.

* Who owns the memory pointed to by the return value?

* The comment says that "the strbuf may or may not be used".  So why is
it part of the interface?  (I suppose it is because the strbuf might be
given ownership of the returned memory if it has to be allocated.)
Would it be more straightforward to *always* return the result in the
strbuf?

* Please document when the return value contains a trailing slash and
also that superfluous internal slashes are (apparently) normalized away.

* Leading double-slashes have a special meaning on some operating
systems.  The old and new versions of this function both seem to ignore
differences between initial slashes.  Perhaps somebody who knows the
rules better could say whether this is an issue but I guess the problem
would rarely be encountered in practice.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/