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

2013/5/22 Michael Haggerty [off-list ref]:

Sorry for coming late to the party.

I am on a business travel, and respond late also. ;-)

On 05/22/2013 03:40 AM, Jiang Xin wrote:

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?

The reason for introducing patch 02/15 is that we don't want to reinvent
the wheel. Patch 06/15 (git-clean: refactor git-clean into two phases)
needs to save relative_path of each git-clean candidate file/directory in
del_list, but the public method in path.c (i.e. relative_path) is not
powerful, and static method in quote.c (i.e. path_relative) can note be
used directly. One way is to enhanced relative_path in path.c, like this
patch.

Since we combine the two methods (relative_path in path.c and
path_relative in quote.c), the new relative_path must be compatible
with the original two methods.

relative_path in path.c
=======================

relative_path is called in one place:

        if (getenv(GIT_WORK_TREE_ENVIRONMENT))
                setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1);

        set_git_dir(relative_path(git_dir, work_tree));
        initialized = 1;

and set_git_dir only set the environment GIT_DIR_ENVIRONMENT
like this:

        int set_git_dir(const char *path)
        {
                if (setenv(GIT_DIR_ENVIRONMENT, path, 1))
                        return error("Could not set GIT_DIR to '%s'", path);
                setup_git_env();
                return 0;
        }

So the only restraint for relative_path is that the return value can
not be blank. If the abs and base arguments for relative_path are
the same, the return value should be "." ("./" is also OK), then
set the envionment GIT_DIR_ENVIRONMENT to "." (or "./").

path_relative in quote.c
========================

We can not simply move "path_relative" in quote.c to "relative_path"
in path.c directly. It is because:

* The arguments for "relative_path" are from user input. So must
   validate (remove duplicate slash) before use. But "path_relative"
   does not check duplicate slash in arguments.

* "path_relative" will return blank string, if abs and base are the same.

Also I noticed that "quote_path_relative" of quote.c (which calls
path_relative) will transform the blank string from path_relative to
"./" (not ".")

        char *quote_path_relative(const char *in, int len,
        ...
                const char *rel = path_relative(in, len, &sb, prefix, -1);
                ...
                if (!out->len)
                        strbuf_addstr(out, "./");

That's why the "path_relative" in path.c refactor the output of "." into "./".

quoted

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.

Yes, it will be nice to update the comments.

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

See Junio's reply.

Michael

--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

-- 
蒋鑫

北京群英汇信息技术有限公司
邮件: worldhello.net@gmail.com
网址: http://www.ossxp.com/
博客: http://www.worldhello.net/
微博: http://weibo.com/gotgit/
电话: 18601196889