Re: [PATCH v13 02/15] path.c: refactor relative_path(), not only strip prefix
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 22, 2013, 16:23 UTC
- Message-ID
- <7v38tfkmpm.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <519C7CA1.2060402@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 19 quoted lines
>> 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?
Show 29 quoted lines
>> 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".