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

Michael Haggerty [off-list ref] writes:

quoted

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?

quoted

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".