From: Jiang Xin <worldhello.net@gmail.com>
To: Michael Haggerty <mhagger@alum.mit.edu>
Cc: Junio C Hamano <gitster@pobox.com>, Git List <git@vger.kernel.org>
Subject: Re: [PATCH v13 02/15] path.c: refactor relative_path(), not only strip prefix
Date: Sun, 26 May 2013 08:21:14 +0800 [thread overview]
Message-ID: <CANYiYbGiAdMtOwdAf1cgV74cJjaM1dABTE6LEe+LAWoEaUBSXw@mail.gmail.com> (raw)
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. ;-)
>
> 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@alum.mit.edu
> http://softwareswirl.blogspot.com/
--
蒋鑫
北京群英汇信息技术有限公司
邮件: worldhello.net@gmail.com
网址: http://www.ossxp.com/
博客: http://www.worldhello.net/
微博: http://weibo.com/gotgit/
电话: 18601196889
next prev parent reply other threads:[~2013-05-26 0:21 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-05-17 9:20 [PATCH v11 00/15] *** Interactive git-clean *** Jiang Xin
2013-05-17 9:20 ` [PATCH v11 01/15] path.c: refactor relative_path(), not only strip prefix Jiang Xin
2013-05-17 9:20 ` [PATCH v11 02/15] test: test relative_path through test-path-utils Jiang Xin
2013-05-17 9:20 ` [PATCH v11 03/15] quote.c: remove path_relative, use relative_path instead Jiang Xin
2013-05-17 9:20 ` [PATCH v11 04/15] Refactor quote_path_relative, remove unused params Jiang Xin
2013-05-17 9:20 ` [PATCH v11 05/15] Refactor write_name_quoted_relative, " Jiang Xin
2013-05-17 9:20 ` [PATCH v11 06/15] git-clean: refactor git-clean into two phases Jiang Xin
2013-05-17 9:20 ` [PATCH v11 07/15] git-clean: add support for -i/--interactive Jiang Xin
2013-05-17 9:20 ` [PATCH v11 08/15] git-clean: show items of del_list in columns Jiang Xin
2013-05-17 9:20 ` [PATCH v11 09/15] git-clean: add colors to interactive git-clean Jiang Xin
2013-05-17 9:20 ` [PATCH v11 10/15] git-clean: use a git-add-interactive compatible UI Jiang Xin
2013-05-17 9:20 ` [PATCH v11 11/15] git-clean: add filter by pattern interactive action Jiang Xin
2013-05-17 9:20 ` [PATCH v11 12/15] git-clean: add select by numbers " Jiang Xin
2013-05-17 9:20 ` [PATCH v11 13/15] git-clean: add ask each " Jiang Xin
2013-05-17 9:20 ` [PATCH v11 14/15] git-clean: add documentation for interactive git-clean Jiang Xin
2013-05-17 9:20 ` [PATCH v11 15/15] test: add t7301 for git-clean--interactive Jiang Xin
2013-05-18 3:18 ` [PATCH v12 00/15] Interactive git-clean Jiang Xin
2013-05-20 22:48 ` Junio C Hamano
2013-05-18 3:18 ` [PATCH v12 01/15] test: add test cases for relative_path Jiang Xin
2013-05-21 20:37 ` Junio C Hamano
2013-05-22 1:40 ` [PATCH v13 00/15] interactive git-clean Jiang Xin
2013-05-22 1:40 ` [PATCH v13 01/15] test: add test cases for relative_path Jiang Xin
2013-05-22 1:40 ` [PATCH v13 02/15] path.c: refactor relative_path(), not only strip prefix Jiang Xin
2013-05-22 8:06 ` Michael Haggerty
2013-05-22 16:23 ` Junio C Hamano
2013-05-26 0:21 ` Jiang Xin [this message]
2013-05-30 6:56 ` Jiang Xin
2013-05-22 1:40 ` [PATCH v13 03/15] quote.c: remove path_relative, use relative_path instead Jiang Xin
2013-05-22 1:40 ` [PATCH v13 04/15] Refactor quote_path_relative, remove unused params Jiang Xin
2013-05-22 1:40 ` [PATCH v13 05/15] Refactor write_name_quoted_relative, " Jiang Xin
2013-05-22 1:40 ` [PATCH v13 06/15] git-clean: refactor git-clean into two phases Jiang Xin
2013-05-22 1:40 ` [PATCH v13 07/15] git-clean: add support for -i/--interactive Jiang Xin
2013-05-22 1:40 ` [PATCH v13 08/15] git-clean: show items of del_list in columns Jiang Xin
2013-05-22 1:40 ` [PATCH v13 09/15] git-clean: add colors to interactive git-clean Jiang Xin
2013-05-22 1:40 ` [PATCH v13 10/15] git-clean: use a git-add-interactive compatible UI Jiang Xin
2013-05-22 1:40 ` [PATCH v13 11/15] git-clean: add filter by pattern interactive action Jiang Xin
2013-05-22 1:40 ` [PATCH v13 12/15] git-clean: add select by numbers " Jiang Xin
2013-05-22 1:40 ` [PATCH v13 13/15] git-clean: add ask each " Jiang Xin
2013-05-22 1:40 ` [PATCH v13 14/15] git-clean: add documentation for interactive git-clean Jiang Xin
2013-05-22 1:40 ` [PATCH v13 15/15] test: add t7301 for git-clean--interactive Jiang Xin
2013-05-18 3:18 ` [PATCH v12 02/15] path.c: refactor relative_path(), not only strip prefix Jiang Xin
2013-05-18 3:18 ` [PATCH v12 03/15] quote.c: remove path_relative, use relative_path instead Jiang Xin
2013-05-18 3:18 ` [PATCH v12 04/15] Refactor quote_path_relative, remove unused params Jiang Xin
2013-05-18 3:18 ` [PATCH v12 05/15] Refactor write_name_quoted_relative, " Jiang Xin
2013-05-18 3:18 ` [PATCH v12 06/15] git-clean: refactor git-clean into two phases Jiang Xin
2013-05-18 3:18 ` [PATCH v12 07/15] git-clean: add support for -i/--interactive Jiang Xin
2013-05-18 3:19 ` [PATCH v12 08/15] git-clean: show items of del_list in columns Jiang Xin
2013-05-18 3:19 ` [PATCH v12 09/15] git-clean: add colors to interactive git-clean Jiang Xin
2013-05-18 3:19 ` [PATCH v12 10/15] git-clean: use a git-add-interactive compatible UI Jiang Xin
2013-05-18 3:19 ` [PATCH v12 11/15] git-clean: add filter by pattern interactive action Jiang Xin
2013-05-18 3:19 ` [PATCH v12 12/15] git-clean: add select by numbers " Jiang Xin
2013-05-18 3:19 ` [PATCH v12 13/15] git-clean: add ask each " Jiang Xin
2013-05-18 3:19 ` [PATCH v12 14/15] git-clean: add documentation for interactive git-clean Jiang Xin
2013-05-18 3:19 ` [PATCH v12 15/15] test: add t7301 for git-clean--interactive Jiang Xin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CANYiYbGiAdMtOwdAf1cgV74cJjaM1dABTE6LEe+LAWoEaUBSXw@mail.gmail.com \
--to=worldhello.net@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=mhagger@alum.mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).