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

From: Jiang Xin <worldhello.net@gmail.com>
Date: May 26, 2013, 00:21 UTC
Message-ID: <CANYiYbGiAdMtOwdAf1cgV74cJjaM1dABTE6LEe+LAWoEaUBSXw@mail.gmail.com>
In-Reply-To: <519C7CA1.2060402@alum.mit.edu>

2013/5/22 Michael Haggerty <mhagger@alum.mit.edu>:

> Sorry for coming late to the party.

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

Show 21 quoted lines

>
> On 05/22/2013 03:40 AM, Jiang Xin wrote:
>> 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 "./".

Show 23 quoted lines

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

Show 16 quoted lines

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

Show 7 quoted lines

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