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

[Junio C Hamano <gitster@pobox.com>]

Michael Haggerty <mhagger@alum.mit.edu> writes:

>> 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?

That is a good point.  At least adding / at the end of "." seems
unneeded, given that the output in some cases have no trailing
slash, forcing a caller who wanted to get a directory to append a
trailing path components to it to check if it needs to add one
before doing so.  Always adding a slash / to the output may sound
consistent, but it is not quite; e.g. "/a/c based on /a/b is ../c"
case may be referring to a non directory /a/c and ensuring a
trailing slash to produce ../c/ is actively wrong.

"The caller knows" rule might work (I am thinking aloud, without
looking at existing callers to see what would break, only to see if
a consistent and simple-to-explain rule is possible).

When the caller asks to turn /a/b relative to /a/b (or /a/b/,
/a//b/./), then we do not know (or we do not want to know) if the
caller means it to be a directory with the intention of appending
something after it, so just return ".".  When the caller asks to
turn /a/b/ relative to the same base, return "./" with the trailing
slash.  Remember if the "abs path" side had a trailing slash,
normalize both input (turning "/./" into "/" and "foo/bar/../" into
foo/", squashing multiple slashes into one, etc.) and then strip the
trailing slash from them, do the usual "comparison and replacement
of leading path components with series of ../" and then append a
slash if the original had one, or something?

>> 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?

This comes from the original in quote.c, I think.  The caller
supplies a strbuf as a scratchpad area and releasing it is the
caller's responsibility.  If the function does not need any
scratchpad area (i.e. the answer is a substring of the input), the
result may point into the abs.

So the calling convention is:

	struct strbuf scratch = STRBUF_INIT;
	const char *result = relative(abs, base, &scratch);
        use(result);
        strbuf_release(&scratch);

and the lifetime rule is "consume it before either abs or scratch
is changed".