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@xxxxxxxxxxxx http://softwareswirl.blogspot.com/ -- 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