2013/5/22 Michael Haggerty <mhagger@xxxxxxxxxxxx>:
> 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:
>> 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 "./".
>
>> 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@xxxxxxxxxxxx
> http://softwareswirl.blogspot.com/
--
蒋鑫
北京群英汇信息技术有限公司
邮件: worldhello.net@xxxxxxxxx
网址: http://www.ossxp.com/
博客: http://www.worldhello.net/
微博: http://weibo.com/gotgit/
电话: 18601196889
--
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