Michael Haggerty <mhagger@xxxxxxxxxxxx> 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".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html