Re: [PATCH 1/2] relative_path should honor dos_drive_prefix

Jiang Xin [off-list ref] writes:

2013/9/13 Junio C Hamano [off-list ref]:

quoted

For systems that need POSIX escape hatch for Apollo Domain ;-), we
would need a bit more work.  When both path1 and path2 begin with a
double-dash, we would need to check if they match up to the next
slash, so that

 - //host1/usr/src and //host1/usr/lib share the same root and the
   former can be made to ../src relative to the latter;

 - //host1/usr/src and //host2/usr/lib are of separate roots.

or something.

But how could we know which platform supports network pathnames and
needs such implementation.

Near the end of

http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_12

is this:

    If a pathname begins with two successive <slash> characters, the
    first component following the leading <slash> characters may be
    interpreted in an implementation-defined manner, although more than
    two leading <slash> characters shall be treated as a single <slash>
    character.

Two points to note are

 (1) Only paths that begin with exactly two slashes are special.

 (2) As it is "implementation-defined", we are not even allowed to
     treat that //host1/usr/src and //host1/usr/lib as sharing "the
     same root", and make the former to ../src relative to the
     latter.

So in the strictest sense, we do not have to bother. As long as we
make sure we do not molest anything that begins with exactly two
slashes.

2013/9/13 Junio C Hamano [off-list ref]:

Jiang Xin [off-list ref] writes:

quoted

2013/9/13 Junio C Hamano [off-list ref]:

quoted

For systems that need POSIX escape hatch for Apollo Domain ;-), we
would need a bit more work.  When both path1 and path2 begin with a
double-dash, we would need to check if they match up to the next
slash, so that

 - //host1/usr/src and //host1/usr/lib share the same root and the
   former can be made to ../src relative to the latter;

 - //host1/usr/src and //host2/usr/lib are of separate roots.

or something.

But how could we know which platform supports network pathnames and
needs such implementation.

Near the end of

http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_12

is this:

    If a pathname begins with two successive <slash> characters, the
    first component following the leading <slash> characters may be
    interpreted in an implementation-defined manner, although more than
    two leading <slash> characters shall be treated as a single <slash>
    character.

Two points to note are

 (1) Only paths that begin with exactly two slashes are special.

 (2) As it is "implementation-defined", we are not even allowed to
     treat that //host1/usr/src and //host1/usr/lib as sharing "the
     same root", and make the former to ../src relative to the
     latter.

So in the strictest sense, we do not have to bother. As long as we
make sure we do not molest anything that begins with exactly two
slashes.

I have checked the behavior of UNC path on Windows (msysGit):

* I can cd to a UNC path:

    cd //server1/share1/path

* can cd to other share:

    cd ../../share2/path

* and can cd to other server's share:

    cd ../../../server2/share/path

That means relative_path(path1, path2) support UNC paths out of the box.
We only need to check both path1 and path2 are UNC paths, or both not.

So, funciton “have_same_root" will write like this:


+static int have_same_root(const char *path1, const char *path2)
+{
+       int is_abs1, is_abs2;
+
+       is_abs1 = is_absolute_path(path1);
+       is_abs2 = is_absolute_path(path2);
+       if (is_abs1 && is_abs2) {
+               if (is_unc_path(path1) ^ is_unc_path(path2))
+                       return 0;
+               return tolower(path1[0]) == tolower(path2[0]);
+       } else {
+               return !is_abs1 && !is_abs2;
+       }
+}


-- 
Jiang Xin

In test cases for relative_path, path with one leading character
(such as /a, /x) may be recogonized as "a:/" or "x:/" if there is
such DOS drive on MINGW platform. Use an umambigous leading path
"/foo" instead.

Also change two leading slashes (//) to three leading slashes (///),
otherwize it will be recognized as UNC path on MINGW platform.

Signed-off-by: Jiang Xin <redacted>
---
 t/t0060-path-utils.sh | 56 +++++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 3a48de2..92976e0 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh

@@ -190,33 +190,33 @@ test_expect_success SYMLINKS 'real path works on symlinks' '
 	test "$sym" = "$(test-path-utils real_path "$dir2/syml")"
 '
 
-relative_path /a/b/c/	/a/b/		c/
-relative_path /a/b/c/	/a/b		c/
-relative_path /a//b//c/	//a/b//		c/	POSIX
-relative_path /a/b	/a/b		./
-relative_path /a/b/	/a/b		./
-relative_path /a	/a/b		../
-relative_path /		/a/b/		../../
-relative_path /a/c	/a/b/		../c
-relative_path /a/c	/a/b		../c
-relative_path /x/y	/a/b/		../../x/y
-relative_path /a/b	"<empty>"	/a/b
-relative_path /a/b 	"<null>"	/a/b
-relative_path a/b/c/	a/b/		c/
-relative_path a/b/c/	a/b		c/
-relative_path a/b//c	a//b		c
-relative_path a/b/	a/b/		./
-relative_path a/b/	a/b		./
-relative_path a		a/b		../
-relative_path x/y	a/b		../../x/y
-relative_path a/c	a/b		../c
-relative_path a/b	"<empty>"	a/b
-relative_path a/b 	"<null>"	a/b
-relative_path "<empty>"	/a/b		./
-relative_path "<empty>"	"<empty>"	./
-relative_path "<empty>"	"<null>"	./
-relative_path "<null>"	"<empty>"	./
-relative_path "<null>"	"<null>"	./
-relative_path "<null>"	/a/b		./
+relative_path /foo/a/b/c/	/foo/a/b/	c/
+relative_path /foo/a/b/c/	/foo/a/b	c/
+relative_path /foo/a//b//c/	///foo/a/b//	c/		POSIX
+relative_path /foo/a/b		/foo/a/b	./
+relative_path /foo/a/b/		/foo/a/b	./
+relative_path /foo/a		/foo/a/b	../
+relative_path /			/foo/a/b/	../../../
+relative_path /foo/a/c		/foo/a/b/	../c
+relative_path /foo/a/c		/foo/a/b	../c
+relative_path /foo/x/y		/foo/a/b/	../../x/y
+relative_path /foo/a/b		"<empty>"	/foo/a/b
+relative_path /foo/a/b 		"<null>"	/foo/a/b
+relative_path foo/a/b/c/	foo/a/b/	c/
+relative_path foo/a/b/c/	foo/a/b		c/
+relative_path foo/a/b//c	foo/a//b	c
+relative_path foo/a/b/		foo/a/b/	./
+relative_path foo/a/b/		foo/a/b		./
+relative_path foo/a		foo/a/b		../
+relative_path foo/x/y		foo/a/b		../../x/y
+relative_path foo/a/c		foo/a/b		../c
+relative_path foo/a/b		"<empty>"	foo/a/b
+relative_path foo/a/b 		"<null>"	foo/a/b
+relative_path "<empty>"		/foo/a/b	./
+relative_path "<empty>"		"<empty>"	./
+relative_path "<empty>"		"<null>"	./
+relative_path "<null>"		"<empty>"	./
+relative_path "<null>"		"<null>"	./
+relative_path "<null>"		/foo/a/b	./
 
 test_done

-- 
1.8.4.460.g2f083d1

Tvangeste found that the "relative_path" function could not work
properly on Windows if "in" and "prefix" have DOS driver prefix
(such as "C:/windows"). And the "relative_path" function won't
work properly if either "in" or "prefix" is a UNC path (like
"//host/share"). ($gmane/234434)

E.g., When execute: test-path-utils relative_path "C:/a/b" "D:/x/y",
should return "C:/a/b", but returns "../../C:/a/b", which is wrong.

So make relative_path honor DOS and UNC paths, and add test cases
for it in t0060.

Reported-by: Tvangeste <redacted>
Helped-by: Johannes Sixt [off-list ref]
Signed-off-by: Jiang Xin <redacted>
---
 compat/mingw.h        |  9 +++++++++
 git-compat-util.h     |  4 ++++
 path.c                | 25 +++++++++++++++++++++++++
 t/t0060-path-utils.sh |  8 ++++++++
 4 files changed, 46 insertions(+)

diff --git a/compat/mingw.h b/compat/mingw.h
index bd0a88b..06e9f49 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h

@@ -311,6 +311,15 @@ int winansi_fprintf(FILE *stream, const char *format, ...) __attribute__((format
 
 #define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
 #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
+static inline int is_unc_path(const char *path)
+{
+	if (!is_dir_sep(*path) || !is_dir_sep(*(path+1)) || is_dir_sep(*(path+2)))
+		return 0;
+	for (path += 2; *path; path++)
+		if (is_dir_sep(*path))
+			return 1;
+	return 0;
+}
 static inline char *mingw_find_last_dir_sep(const char *path)
 {
 	char *ret = NULL;

diff --git a/git-compat-util.h b/git-compat-util.h
index 9549de6..93c2206 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -274,6 +274,10 @@ extern char *gitbasename(char *);
 #define is_dir_sep(c) ((c) == '/')
 #endif
 
+#ifndef is_unc_path
+#define is_unc_path(path) 0
+#endif
+
 #ifndef find_last_dir_sep
 #define find_last_dir_sep(path) strrchr(path, '/')
 #endif

diff --git a/path.c b/path.c
index 9fd28bcd..544d10d 100644
--- a/path.c
+++ b/path.c

@@ -434,6 +434,21 @@ int adjust_shared_perm(const char *path)
 	return 0;
 }
 
+static int have_same_root(const char *path1, const char *path2)
+{
+	int is_abs1, is_abs2;
+
+	is_abs1 = is_absolute_path(path1);
+	is_abs2 = is_absolute_path(path2);
+	if (is_abs1 && is_abs2) {
+		if (is_unc_path(path1) ^ is_unc_path(path2))
+			return 0;
+		return tolower(path1[0]) == tolower(path2[0]);
+	} else {
+		return !is_abs1 && !is_abs2;
+	}
+}
+
 /*
  * Give path as relative to prefix.
  *

@@ -454,6 +469,16 @@ const char *relative_path(const char *in, const char *prefix,
 	else if (!prefix_len)
 		return in;
 
+	if (have_same_root(in, prefix)) {
+		/* bypass dos_drive, for "c:" is identical to "C:" */
+		if (has_dos_drive_prefix(in)) {
+			i = 2;
+			j = 2;
+		}
+	} else {
+		return in;
+	}
+
 	while (i < prefix_len && j < in_len && prefix[i] == in[j]) {
 		if (is_dir_sep(prefix[i])) {
 			while (is_dir_sep(prefix[i]))

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 92976e0..830b6d5 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh

@@ -210,6 +210,14 @@ relative_path foo/a/b/		foo/a/b		./
 relative_path foo/a		foo/a/b		../
 relative_path foo/x/y		foo/a/b		../../x/y
 relative_path foo/a/c		foo/a/b		../c
+relative_path foo/a/b		/foo/x/y	foo/a/b
+relative_path /foo/a/b		foo/x/y		/foo/a/b
+relative_path d:/a/b		D:/a/c		../b		MINGW
+relative_path C:/a/b		D:/a/c		C:/a/b		MINGW
+relative_path //host1/a/b	//host1/a/c	../b
+relative_path //host1/a/b	//host2/a/c	../../../host1/a/b
+relative_path //host1/a/b	/foo/a/c	//host1/a/b	MINGW
+relative_path /foo/a/b		//host2/a/c	/foo/a/b	MINGW
 relative_path foo/a/b		"<empty>"	foo/a/b
 relative_path foo/a/b 		"<null>"	foo/a/b
 relative_path "<empty>"		/foo/a/b	./

-- 
1.8.4.460.g2f083d1

Using a relative_path as git_dir first appears in v1.5.6-1-g044bbbc.
It will make git_dir shorter only if git_dir is inside work_tree,
and this will increase performance. But my last refactor effort on
relative_path function (commit v1.8.3-rc2-12-ge02ca72) changed that.
Always use relative_path as git_dir may bring troubles like
$gmane/234434.

Because new relative_path is a combination of original relative_path
from path.c and original path_relative from quote.c, so in order to
restore the origin implementation, save the original relative_path
as remove_leading_path, and call it in setup.c.

Suggested-by: Karsten Blees <redacted>
Signed-off-by: Jiang Xin <redacted>
---
 cache.h |  1 +
 path.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
 setup.c |  5 +----
 3 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index a47b9c0..73fa334 100644
--- a/cache.h
+++ b/cache.h

@@ -747,6 +747,7 @@ int is_directory(const char *);
 const char *real_path(const char *path);
 const char *real_path_if_valid(const char *path);
 const char *absolute_path(const char *path);
+const char *remove_leading_path(const char *in, const char *prefix);
 const char *relative_path(const char *in, const char *prefix, struct strbuf *sb);
 int normalize_path_copy_len(char *dst, const char *src, int *prefix_len);
 int normalize_path_copy(char *dst, const char *src);

diff --git a/path.c b/path.c
index 544d10d..5ad3554 100644
--- a/path.c
+++ b/path.c

@@ -556,6 +556,51 @@ const char *relative_path(const char *in, const char *prefix,
 }
 
 /*
+ * A simpler implementation of relative_path
+ *
+ * Get relative path by removing "prefix" from "in". This function
+ * first appears in v1.5.6-1-g044bbbc, and makes git_dir shorter
+ * to increase performance when traversing the path to work_tree.
+ */
+const char *remove_leading_path(const char *in, const char *prefix)
+{
+	static char buf[PATH_MAX + 1];
+	int i = 0, j = 0;
+
+	if (!prefix || !prefix[0])
+		return in;
+	while (prefix[i]) {
+		if (is_dir_sep(prefix[i])) {
+			if (!is_dir_sep(in[j]))
+				return in;
+			while (is_dir_sep(prefix[i]))
+				i++;
+			while (is_dir_sep(in[j]))
+				j++;
+			continue;
+		} else if (in[j] != prefix[i]) {
+			return in;
+		}
+		i++;
+		j++;
+	}
+	if (
+	    /* "/foo" is a prefix of "/foo" */
+	    in[j] &&
+	    /* "/foo" is not a prefix of "/foobar" */
+	    !is_dir_sep(prefix[i-1]) && !is_dir_sep(in[j])
+	   )
+		return in;
+	while (is_dir_sep(in[j]))
+		j++;
+	if (!in[j])
+		strcpy(buf, ".");
+	else
+		strcpy(buf, in + j);
+	return buf;
+}
+
+/*
  * It is okay if dst == src, but they should not overlap otherwise.
  *
  * Performs the following normalizations on src, storing the result in dst:

diff --git a/setup.c b/setup.c
index f08dd64..dbf4138 100644
--- a/setup.c
+++ b/setup.c

@@ -227,7 +227,6 @@ int is_inside_work_tree(void)
 
 void setup_work_tree(void)
 {
-	struct strbuf sb = STRBUF_INIT;
 	const char *work_tree, *git_dir;
 	static int initialized = 0;
 

@@ -247,10 +246,8 @@ void setup_work_tree(void)
 	if (getenv(GIT_WORK_TREE_ENVIRONMENT))
 		setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1);
 
-	set_git_dir(relative_path(git_dir, work_tree, &sb));
+	set_git_dir(remove_leading_path(git_dir, work_tree));
 	initialized = 1;
-
-	strbuf_release(&sb);
 }
 
 static int check_repository_format_gently(const char *gitdir, int *nongit_ok)

-- 
1.8.4.460.g2f083d1

Am 17.09.2013 10:24, schrieb Jiang Xin:

I have checked the behavior of UNC path on Windows (msysGit):

* I can cd to a UNC path:

    cd //server1/share1/path

* can cd to other share:

    cd ../../share2/path

* and can cd to other server's share:

    cd ../../../server2/share/path

That means relative_path(path1, path2) support UNC paths out of the box.
We only need to check both path1 and path2 are UNC paths, or both not.

Your tests are flawed. You issued the commands in bash, which (or rather
MSYS) does everything for you that you need to make it work. But in
reality it does not, because the system cannot apply .. to //server/share:

$ git ls-remote //srv/public/../repos/his/setups.git
fatal: '//srv/public/../repos/his/setups.git' does not appear to be a
git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

even though the repository (and //srv/public, let me assure) exists:

$ git ls-remote //srv/repos/his/setups.git
bea489b0611a72c41f133343fdccbd3e2b9f80b5        HEAD
...

The situation does not change with your latest round (v3).

Please let me suggest not to scratch where there is no itch. ;) Your
round v2 was good enough.

If you really want to check UNC paths, then you must compare two path
components after the the double-slash, not just one.

Furthermore, you should audit all code that references
is_absolute_path(), relative_path(), normalize_path_copy(), and possibly
a few others whether the functions or call sites need improvement.
That's worth a separate patch.

-- Hannes

On 2013-09-17 21.32, Johannes Sixt wrote:

Am 17.09.2013 10:24, schrieb Jiang Xin:

quoted

I have checked the behavior of UNC path on Windows (msysGit):

* I can cd to a UNC path:

    cd //server1/share1/path

* can cd to other share:

    cd ../../share2/path

* and can cd to other server's share:

    cd ../../../server2/share/path

That means relative_path(path1, path2) support UNC paths out of the box.
We only need to check both path1 and path2 are UNC paths, or both not.

Your tests are flawed. You issued the commands in bash, which (or rather
MSYS) does everything for you that you need to make it work. But in
reality it does not, because the system cannot apply .. to //server/share:

$ git ls-remote //srv/public/../repos/his/setups.git
fatal: '//srv/public/../repos/his/setups.git' does not appear to be a
git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

even though the repository (and //srv/public, let me assure) exists:

$ git ls-remote //srv/repos/his/setups.git
bea489b0611a72c41f133343fdccbd3e2b9f80b5        HEAD
...

The situation does not change with your latest round (v3).

Please let me suggest not to scratch where there is no itch. ;) Your
round v2 was good enough.

If you really want to check UNC paths, then you must compare two path
components after the the double-slash, not just one.

Furthermore, you should audit all code that references
is_absolute_path(), relative_path(), normalize_path_copy(), and possibly
a few others whether the functions or call sites need improvement.
That's worth a separate patch.

-- Hannes

I tend to agree here.
The V2 patch fixed a regression.
This should be one commit on its own:
Documentation/SubmittingPatches:
(1) Make separate commits for logically separate changes.

Fixing a bug is a good thing, thanks for working on this,

The support for UNC paths is a new feature, and this deserves a seperate commit.
/Torsten

2013/9/18 Johannes Sixt [off-list ref]:

Am 17.09.2013 10:24, schrieb Jiang Xin:

quoted

I have checked the behavior of UNC path on Windows (msysGit):

* I can cd to a UNC path:

    cd //server1/share1/path

* can cd to other share:

    cd ../../share2/path

* and can cd to other server's share:

    cd ../../../server2/share/path

That means relative_path(path1, path2) support UNC paths out of the box.
We only need to check both path1 and path2 are UNC paths, or both not.

Your tests are flawed. You issued the commands in bash, which (or rather
MSYS) does everything for you that you need to make it work. But in
reality it does not, because the system cannot apply .. to //server/share:

$ git ls-remote //srv/public/../repos/his/setups.git
fatal: '//srv/public/../repos/his/setups.git' does not appear to be a
git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

even though the repository (and //srv/public, let me assure) exists:

$ git ls-remote //srv/repos/his/setups.git
bea489b0611a72c41f133343fdccbd3e2b9f80b5        HEAD
...

After see this link (provided by Torsten):

    <http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247%28v=vs.85%29.aspx>

I find the following commands could work:

    $ git ls-remote //srv/repos/his/setups.git
    $ git ls-remote //srv/repos/his/../his/setups.git
    $ git ls-remote //?/UNC/srv/repos/his/setups.git
    $ git ls-remote //?/UNC/srv/repos/../repos/his/setups.git

But no luck for this one:

    $ git ls-remote //srv/repos/../repos/his/setups.git

I trace it using gdb, and find it failed in "stat()/mingw_stat()" call
of function "enter_repo" in path.c. But I can not find out why
"git ls-remote //?/UNC/srv/repos/../repos/his/setups.git" could
work (success in shell, failed in gdb).

Please let me suggest not to scratch where there is no itch. ;) Your
round v2 was good enough.

If you really want to check UNC paths, then you must compare two path
components after the the double-slash, not just one.

I have already try this (honor two path components after //)
during the reroll for patch v3. But I am not satisfied with it,
and it seems like over-engineered: Rename "have_same_root"
to "get_common_root_prefix_width", and it return -1 for no
same_root found, otherwize return the length of root_prefix_width.

Since restored the default behavior of setup.c in commit ("Use
simpler relative_path when set_git_dir"), function "relative_path"
are only used in "quote.c" and "builtin/clean.c", and two paths
provided to "relative_path" are always (I can not find exception)
relative paths. So no itch exist I think.

-- 
Jiang Xin

Remove implementations on UNC names in patch v3.
So patch v4 is the same like v2, but with minor
update for commit logs.

Jiang Xin (3):
  test: use unambigous leading path (/foo) for mingw
  relative_path should honor dos-driver-prefix
  Use simpler relative_path when set_git_dir

 cache.h               |  1 +
 path.c                | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++
 setup.c               |  5 +---
 t/t0060-path-utils.sh | 60 +++++++++++++++++++++++++----------------------
 4 files changed, 99 insertions(+), 32 deletions(-)

-- 
1.8.4.460.gbed9cb4

Tvangeste found that the "relative_path" function could not work
properly on Windows if "in" and "prefix" have DOS driver prefix
(such as "C:/windows"). ($gmane/234434)

E.g., When execute: test-path-utils relative_path "C:/a/b" "D:/x/y",
should return "C:/a/b", but returns "../../C:/a/b", which is wrong.

So make relative_path honor dos-driver-prefix, and add test cases
for it in t0060.

Reported-by: Tvangeste <redacted>
Helped-by: Johannes Sixt [off-list ref]
Signed-off-by: Jiang Xin <redacted>
---
 path.c                | 20 ++++++++++++++++++++
 t/t0060-path-utils.sh |  4 ++++
 2 files changed, 24 insertions(+)

diff --git a/path.c b/path.c
index 7f3324a..0c16dc5 100644
--- a/path.c
+++ b/path.c

@@ -441,6 +441,16 @@ int adjust_shared_perm(const char *path)
 	return 0;
 }
 
+static int have_same_root(const char *path1, const char *path2)
+{
+	int is_abs1, is_abs2;
+
+	is_abs1 = is_absolute_path(path1);
+	is_abs2 = is_absolute_path(path2);
+	return (is_abs1 && is_abs2 && tolower(path1[0]) == tolower(path2[0])) ||
+	       (!is_abs1 && !is_abs2);
+}
+
 /*
  * Give path as relative to prefix.
  *

@@ -461,6 +471,16 @@ const char *relative_path(const char *in, const char *prefix,
 	else if (!prefix_len)
 		return in;
 
+	if (have_same_root(in, prefix)) {
+		/* bypass dos_drive, for "c:" is identical to "C:" */
+		if (has_dos_drive_prefix(in)) {
+			i = 2;
+			j = 2;
+		}
+	} else {
+		return in;
+	}
+
 	while (i < prefix_len && j < in_len && prefix[i] == in[j]) {
 		if (is_dir_sep(prefix[i])) {
 			while (is_dir_sep(prefix[i]))

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 92976e0..40dfa2d 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh

@@ -210,6 +210,10 @@ relative_path foo/a/b/		foo/a/b		./
 relative_path foo/a		foo/a/b		../
 relative_path foo/x/y		foo/a/b		../../x/y
 relative_path foo/a/c		foo/a/b		../c
+relative_path foo/a/b		/foo/x/y	foo/a/b
+relative_path /foo/a/b		foo/x/y		/foo/a/b
+relative_path d:/a/b		D:/a/c		../b		MINGW
+relative_path C:/a/b		D:/a/c		C:/a/b		MINGW
 relative_path foo/a/b		"<empty>"	foo/a/b
 relative_path foo/a/b 		"<null>"	foo/a/b
 relative_path "<empty>"		/foo/a/b	./

-- 
1.8.4.460.gbed9cb4

In test cases for relative_path, path with one leading character
(such as /a, /x) may be recogonized as "a:/" or "x:/" if there is
such DOS drive on MINGW platform. Use an umambigous leading path
"/foo" instead.

Also change two leading slashes (//) to three leading slashes (///),
otherwize it will be recognized as UNC name on MINGW platform.

Signed-off-by: Jiang Xin <redacted>
---
 t/t0060-path-utils.sh | 56 +++++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 3a48de2..92976e0 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh

@@ -190,33 +190,33 @@ test_expect_success SYMLINKS 'real path works on symlinks' '
 	test "$sym" = "$(test-path-utils real_path "$dir2/syml")"
 '
 
-relative_path /a/b/c/	/a/b/		c/
-relative_path /a/b/c/	/a/b		c/
-relative_path /a//b//c/	//a/b//		c/	POSIX
-relative_path /a/b	/a/b		./
-relative_path /a/b/	/a/b		./
-relative_path /a	/a/b		../
-relative_path /		/a/b/		../../
-relative_path /a/c	/a/b/		../c
-relative_path /a/c	/a/b		../c
-relative_path /x/y	/a/b/		../../x/y
-relative_path /a/b	"<empty>"	/a/b
-relative_path /a/b 	"<null>"	/a/b
-relative_path a/b/c/	a/b/		c/
-relative_path a/b/c/	a/b		c/
-relative_path a/b//c	a//b		c
-relative_path a/b/	a/b/		./
-relative_path a/b/	a/b		./
-relative_path a		a/b		../
-relative_path x/y	a/b		../../x/y
-relative_path a/c	a/b		../c
-relative_path a/b	"<empty>"	a/b
-relative_path a/b 	"<null>"	a/b
-relative_path "<empty>"	/a/b		./
-relative_path "<empty>"	"<empty>"	./
-relative_path "<empty>"	"<null>"	./
-relative_path "<null>"	"<empty>"	./
-relative_path "<null>"	"<null>"	./
-relative_path "<null>"	/a/b		./
+relative_path /foo/a/b/c/	/foo/a/b/	c/
+relative_path /foo/a/b/c/	/foo/a/b	c/
+relative_path /foo/a//b//c/	///foo/a/b//	c/		POSIX
+relative_path /foo/a/b		/foo/a/b	./
+relative_path /foo/a/b/		/foo/a/b	./
+relative_path /foo/a		/foo/a/b	../
+relative_path /			/foo/a/b/	../../../
+relative_path /foo/a/c		/foo/a/b/	../c
+relative_path /foo/a/c		/foo/a/b	../c
+relative_path /foo/x/y		/foo/a/b/	../../x/y
+relative_path /foo/a/b		"<empty>"	/foo/a/b
+relative_path /foo/a/b 		"<null>"	/foo/a/b
+relative_path foo/a/b/c/	foo/a/b/	c/
+relative_path foo/a/b/c/	foo/a/b		c/
+relative_path foo/a/b//c	foo/a//b	c
+relative_path foo/a/b/		foo/a/b/	./
+relative_path foo/a/b/		foo/a/b		./
+relative_path foo/a		foo/a/b		../
+relative_path foo/x/y		foo/a/b		../../x/y
+relative_path foo/a/c		foo/a/b		../c
+relative_path foo/a/b		"<empty>"	foo/a/b
+relative_path foo/a/b 		"<null>"	foo/a/b
+relative_path "<empty>"		/foo/a/b	./
+relative_path "<empty>"		"<empty>"	./
+relative_path "<empty>"		"<null>"	./
+relative_path "<null>"		"<empty>"	./
+relative_path "<null>"		"<null>"	./
+relative_path "<null>"		/foo/a/b	./
 
 test_done

-- 
1.8.4.460.gbed9cb4

Using a relative_path as git_dir first appears in v1.5.6-1-g044bbbc.
It will make git_dir shorter only if git_dir is inside work_tree,
and this will increase performance. But my last refactor effort on
relative_path function (commit v1.8.3-rc2-12-ge02ca72) changed that.
Always use relative_path as git_dir may bring troubles like
$gmane/234434.

Because new relative_path is a combination of original relative_path
from path.c and original path_relative from quote.c, so in order to
restore the origin implementation, save the original relative_path
as remove_leading_path, and call it in setup.c.

Suggested-by: Karsten Blees <redacted>
Signed-off-by: Jiang Xin <redacted>
---
 cache.h |  1 +
 path.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
 setup.c |  5 +----
 3 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index 8e42256..94475bd 100644
--- a/cache.h
+++ b/cache.h

@@ -737,6 +737,7 @@ int is_directory(const char *);
 const char *real_path(const char *path);
 const char *real_path_if_valid(const char *path);
 const char *absolute_path(const char *path);
+const char *remove_leading_path(const char *in, const char *prefix);
 const char *relative_path(const char *in, const char *prefix, struct strbuf *sb);
 int normalize_path_copy(char *dst, const char *src);
 int longest_ancestor_length(const char *path, struct string_list *prefixes);

diff --git a/path.c b/path.c
index 0c16dc5..fa62da5 100644
--- a/path.c
+++ b/path.c

@@ -558,6 +558,51 @@ const char *relative_path(const char *in, const char *prefix,
 }
 
 /*
+ * A simpler implementation of relative_path
+ *
+ * Get relative path by removing "prefix" from "in". This function
+ * first appears in v1.5.6-1-g044bbbc, and makes git_dir shorter
+ * to increase performance when traversing the path to work_tree.
+ */
+const char *remove_leading_path(const char *in, const char *prefix)
+{
+	static char buf[PATH_MAX + 1];
+	int i = 0, j = 0;
+
+	if (!prefix || !prefix[0])
+		return in;
+	while (prefix[i]) {
+		if (is_dir_sep(prefix[i])) {
+			if (!is_dir_sep(in[j]))
+				return in;
+			while (is_dir_sep(prefix[i]))
+				i++;
+			while (is_dir_sep(in[j]))
+				j++;
+			continue;
+		} else if (in[j] != prefix[i]) {
+			return in;
+		}
+		i++;
+		j++;
+	}
+	if (
+	    /* "/foo" is a prefix of "/foo" */
+	    in[j] &&
+	    /* "/foo" is not a prefix of "/foobar" */
+	    !is_dir_sep(prefix[i-1]) && !is_dir_sep(in[j])
+	   )
+		return in;
+	while (is_dir_sep(in[j]))
+		j++;
+	if (!in[j])
+		strcpy(buf, ".");
+	else
+		strcpy(buf, in + j);
+	return buf;
+}
+
+/*
  * It is okay if dst == src, but they should not overlap otherwise.
  *
  * Performs the following normalizations on src, storing the result in dst:

diff --git a/setup.c b/setup.c
index 0d9ea62..dad39c1 100644
--- a/setup.c
+++ b/setup.c

@@ -360,7 +360,6 @@ int is_inside_work_tree(void)
 
 void setup_work_tree(void)
 {
-	struct strbuf sb = STRBUF_INIT;
 	const char *work_tree, *git_dir;
 	static int initialized = 0;
 

@@ -380,10 +379,8 @@ void setup_work_tree(void)
 	if (getenv(GIT_WORK_TREE_ENVIRONMENT))
 		setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1);
 
-	set_git_dir(relative_path(git_dir, work_tree, &sb));
+	set_git_dir(remove_leading_path(git_dir, work_tree));
 	initialized = 1;
-
-	strbuf_release(&sb);
 }
 
 static int check_repository_format_gently(const char *gitdir, int *nongit_ok)

-- 
1.8.4.460.gbed9cb4

On 20.09.2013 04:38, Jiang Xin wrote:

In test cases for relative_path, path with one leading character
(such as /a, /x) may be recogonized as "a:/" or "x:/" if there is
such DOS drive on MINGW platform. Use an umambigous leading path
"/foo" instead.

 >
 > Also change two leading slashes (//) to three leading slashes (///),
 > otherwize it will be recognized as UNC name on MINGW platform.

Note that the path mangling comes from MSYS [1], not MinGW, so you 
should place "MINGW" with "MSYS" in several places. As a side-note, the 
official spelling is "MinGW", not "MINGW".

-relative_path /a/b/c/	/a/b/		c/

+relative_path /foo/a/b/c/	/foo/a/b/	c/

Wouldn't it have been more straight-forward to just replace "a" with 
"foo", "b" with "bar" and "c" with "baz" (or whatever)? So that the 
first line would say

relative_path /foo/bar/baz/	/foo/bar/		baz/

Thanks for the fix!

[1] http://www.mingw.org/wiki/Posix_path_conversion

-- 
Sebastian Schuberth

On 20.09.2013 04:38, Jiang Xin wrote:

Tvangeste found that the "relative_path" function could not work
properly on Windows if "in" and "prefix" have DOS driver prefix
(such as "C:/windows"). ($gmane/234434)

s/driver/drive/

E.g., When execute: test-path-utils relative_path "C:/a/b" "D:/x/y",
should return "C:/a/b", but returns "../../C:/a/b", which is wrong.

So make relative_path honor dos-driver-prefix, and add test cases
for it in t0060.

s/dos-driver-prefix/DOS drive prefix/

-- 
Sebastian Schuberth

2013/10/11 Sebastian Schuberth [off-list ref]:

quoted

In test cases for relative_path, path with one leading character
(such as /a, /x) may be recogonized as "a:/" or "x:/" if there is
such DOS drive on MINGW platform. Use an umambigous leading path
"/foo" instead.

Also change two leading slashes (//) to three leading slashes (///),
otherwize it will be recognized as UNC name on MINGW platform.

Note that the path mangling comes from MSYS [1], not MinGW, so you should
place "MINGW" with "MSYS" in several places. As a side-note, the official
spelling is "MinGW", not "MINGW".

I will make a reroll. s/MINGW/MSYS/i

quoted

-relative_path /a/b/c/  /a/b/           c/

quoted

+relative_path /foo/a/b/c/      /foo/a/b/       c/

Wouldn't it have been more straight-forward to just replace "a" with "foo",
"b" with "bar" and "c" with "baz" (or whatever)? So that the first line
would say

relative_path /foo/bar/baz/     /foo/bar/               baz/

These test cases have been used in some commit logs, such as
commit: v1.8.3-rc2-13-gad66df2. And for me (a non-English speaker)
a,b,c,x,y,z are more readable than bar, baz, qux, ...

-- 
Jiang Xin

Update since v4:

 * Update commit logs with the help from Sebastian Schuberth:

   s/MINGW/MSYS/i
   s/dos-driver-prefix/dos-drive-prefix/

Jiang Xin (3):
  test: use unambigous leading path (/foo) for MSYS
  relative_path should honor dos-drive-prefix
  Use simpler relative_path when set_git_dir

 cache.h               |  1 +
 path.c                | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++
 setup.c               |  5 +---
 t/t0060-path-utils.sh | 60 +++++++++++++++++++++++++----------------------
 4 files changed, 99 insertions(+), 32 deletions(-)

-- 
1.8.4

In test cases for relative_path, path with one leading character
(such as /a, /x) may be recogonized as "a:/" or "x:/" if there is
such DOS drive on MSYS platform. Use an umambigous leading path
"/foo" instead.

Also change two leading slashes (//) to three leading slashes (///),
otherwize it will be recognized as UNC name on MSYS platform.

Signed-off-by: Jiang Xin <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 t/t0060-path-utils.sh | 56 +++++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 3a48de2..92976e0 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh

@@ -190,33 +190,33 @@ test_expect_success SYMLINKS 'real path works on symlinks' '
 	test "$sym" = "$(test-path-utils real_path "$dir2/syml")"
 '
 
-relative_path /a/b/c/	/a/b/		c/
-relative_path /a/b/c/	/a/b		c/
-relative_path /a//b//c/	//a/b//		c/	POSIX
-relative_path /a/b	/a/b		./
-relative_path /a/b/	/a/b		./
-relative_path /a	/a/b		../
-relative_path /		/a/b/		../../
-relative_path /a/c	/a/b/		../c
-relative_path /a/c	/a/b		../c
-relative_path /x/y	/a/b/		../../x/y
-relative_path /a/b	"<empty>"	/a/b
-relative_path /a/b 	"<null>"	/a/b
-relative_path a/b/c/	a/b/		c/
-relative_path a/b/c/	a/b		c/
-relative_path a/b//c	a//b		c
-relative_path a/b/	a/b/		./
-relative_path a/b/	a/b		./
-relative_path a		a/b		../
-relative_path x/y	a/b		../../x/y
-relative_path a/c	a/b		../c
-relative_path a/b	"<empty>"	a/b
-relative_path a/b 	"<null>"	a/b
-relative_path "<empty>"	/a/b		./
-relative_path "<empty>"	"<empty>"	./
-relative_path "<empty>"	"<null>"	./
-relative_path "<null>"	"<empty>"	./
-relative_path "<null>"	"<null>"	./
-relative_path "<null>"	/a/b		./
+relative_path /foo/a/b/c/	/foo/a/b/	c/
+relative_path /foo/a/b/c/	/foo/a/b	c/
+relative_path /foo/a//b//c/	///foo/a/b//	c/		POSIX
+relative_path /foo/a/b		/foo/a/b	./
+relative_path /foo/a/b/		/foo/a/b	./
+relative_path /foo/a		/foo/a/b	../
+relative_path /			/foo/a/b/	../../../
+relative_path /foo/a/c		/foo/a/b/	../c
+relative_path /foo/a/c		/foo/a/b	../c
+relative_path /foo/x/y		/foo/a/b/	../../x/y
+relative_path /foo/a/b		"<empty>"	/foo/a/b
+relative_path /foo/a/b 		"<null>"	/foo/a/b
+relative_path foo/a/b/c/	foo/a/b/	c/
+relative_path foo/a/b/c/	foo/a/b		c/
+relative_path foo/a/b//c	foo/a//b	c
+relative_path foo/a/b/		foo/a/b/	./
+relative_path foo/a/b/		foo/a/b		./
+relative_path foo/a		foo/a/b		../
+relative_path foo/x/y		foo/a/b		../../x/y
+relative_path foo/a/c		foo/a/b		../c
+relative_path foo/a/b		"<empty>"	foo/a/b
+relative_path foo/a/b 		"<null>"	foo/a/b
+relative_path "<empty>"		/foo/a/b	./
+relative_path "<empty>"		"<empty>"	./
+relative_path "<empty>"		"<null>"	./
+relative_path "<null>"		"<empty>"	./
+relative_path "<null>"		"<null>"	./
+relative_path "<null>"		/foo/a/b	./
 
 test_done

-- 
1.8.4

Tvangeste found that the "relative_path" function could not work
properly on Windows if "in" and "prefix" have DOS drive prefix
(such as "C:/windows"). ($gmane/234434)

E.g., When execute: test-path-utils relative_path "C:/a/b" "D:/x/y",
should return "C:/a/b", but returns "../../C:/a/b", which is wrong.

So make relative_path honor DOS drive prefix, and add test cases
for it in t0060.

Reported-by: Tvangeste <redacted>
Helped-by: Johannes Sixt [off-list ref]
Signed-off-by: Jiang Xin <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 path.c                | 20 ++++++++++++++++++++
 t/t0060-path-utils.sh |  4 ++++
 2 files changed, 24 insertions(+)

diff --git a/path.c b/path.c
index 7f3324a..0c16dc5 100644
--- a/path.c
+++ b/path.c

@@ -441,6 +441,16 @@ int adjust_shared_perm(const char *path)
 	return 0;
 }
 
+static int have_same_root(const char *path1, const char *path2)
+{
+	int is_abs1, is_abs2;
+
+	is_abs1 = is_absolute_path(path1);
+	is_abs2 = is_absolute_path(path2);
+	return (is_abs1 && is_abs2 && tolower(path1[0]) == tolower(path2[0])) ||
+	       (!is_abs1 && !is_abs2);
+}
+
 /*
  * Give path as relative to prefix.
  *

@@ -461,6 +471,16 @@ const char *relative_path(const char *in, const char *prefix,
 	else if (!prefix_len)
 		return in;
 
+	if (have_same_root(in, prefix)) {
+		/* bypass dos_drive, for "c:" is identical to "C:" */
+		if (has_dos_drive_prefix(in)) {
+			i = 2;
+			j = 2;
+		}
+	} else {
+		return in;
+	}
+
 	while (i < prefix_len && j < in_len && prefix[i] == in[j]) {
 		if (is_dir_sep(prefix[i])) {
 			while (is_dir_sep(prefix[i]))

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 92976e0..40dfa2d 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh

@@ -210,6 +210,10 @@ relative_path foo/a/b/		foo/a/b		./
 relative_path foo/a		foo/a/b		../
 relative_path foo/x/y		foo/a/b		../../x/y
 relative_path foo/a/c		foo/a/b		../c
+relative_path foo/a/b		/foo/x/y	foo/a/b
+relative_path /foo/a/b		foo/x/y		/foo/a/b
+relative_path d:/a/b		D:/a/c		../b		MINGW
+relative_path C:/a/b		D:/a/c		C:/a/b		MINGW
 relative_path foo/a/b		"<empty>"	foo/a/b
 relative_path foo/a/b 		"<null>"	foo/a/b
 relative_path "<empty>"		/foo/a/b	./

-- 
1.8.4

Using a relative_path as git_dir first appears in v1.5.6-1-g044bbbc.
It will make git_dir shorter only if git_dir is inside work_tree,
and this will increase performance. But my last refactor effort on
relative_path function (commit v1.8.3-rc2-12-ge02ca72) changed that.
Always use relative_path as git_dir may bring troubles like
$gmane/234434.

Because new relative_path is a combination of original relative_path
from path.c and original path_relative from quote.c, so in order to
restore the origin implementation, save the original relative_path
as remove_leading_path, and call it in setup.c.

Suggested-by: Karsten Blees <redacted>
Signed-off-by: Jiang Xin <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 cache.h |  1 +
 path.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
 setup.c |  5 +----
 3 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index 8e42256..94475bd 100644
--- a/cache.h
+++ b/cache.h

@@ -737,6 +737,7 @@ int is_directory(const char *);
 const char *real_path(const char *path);
 const char *real_path_if_valid(const char *path);
 const char *absolute_path(const char *path);
+const char *remove_leading_path(const char *in, const char *prefix);
 const char *relative_path(const char *in, const char *prefix, struct strbuf *sb);
 int normalize_path_copy(char *dst, const char *src);
 int longest_ancestor_length(const char *path, struct string_list *prefixes);

diff --git a/path.c b/path.c
index 0c16dc5..fa62da5 100644
--- a/path.c
+++ b/path.c

@@ -558,6 +558,51 @@ const char *relative_path(const char *in, const char *prefix,
 }
 
 /*
+ * A simpler implementation of relative_path
+ *
+ * Get relative path by removing "prefix" from "in". This function
+ * first appears in v1.5.6-1-g044bbbc, and makes git_dir shorter
+ * to increase performance when traversing the path to work_tree.
+ */
+const char *remove_leading_path(const char *in, const char *prefix)
+{
+	static char buf[PATH_MAX + 1];
+	int i = 0, j = 0;
+
+	if (!prefix || !prefix[0])
+		return in;
+	while (prefix[i]) {
+		if (is_dir_sep(prefix[i])) {
+			if (!is_dir_sep(in[j]))
+				return in;
+			while (is_dir_sep(prefix[i]))
+				i++;
+			while (is_dir_sep(in[j]))
+				j++;
+			continue;
+		} else if (in[j] != prefix[i]) {
+			return in;
+		}
+		i++;
+		j++;
+	}
+	if (
+	    /* "/foo" is a prefix of "/foo" */
+	    in[j] &&
+	    /* "/foo" is not a prefix of "/foobar" */
+	    !is_dir_sep(prefix[i-1]) && !is_dir_sep(in[j])
+	   )
+		return in;
+	while (is_dir_sep(in[j]))
+		j++;
+	if (!in[j])
+		strcpy(buf, ".");
+	else
+		strcpy(buf, in + j);
+	return buf;
+}
+
+/*
  * It is okay if dst == src, but they should not overlap otherwise.
  *
  * Performs the following normalizations on src, storing the result in dst:

diff --git a/setup.c b/setup.c
index 0d9ea62..dad39c1 100644
--- a/setup.c
+++ b/setup.c

@@ -360,7 +360,6 @@ int is_inside_work_tree(void)
 
 void setup_work_tree(void)
 {
-	struct strbuf sb = STRBUF_INIT;
 	const char *work_tree, *git_dir;
 	static int initialized = 0;
 

@@ -380,10 +379,8 @@ void setup_work_tree(void)
 	if (getenv(GIT_WORK_TREE_ENVIRONMENT))
 		setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1);
 
-	set_git_dir(relative_path(git_dir, work_tree, &sb));
+	set_git_dir(remove_leading_path(git_dir, work_tree));
 	initialized = 1;
-
-	strbuf_release(&sb);
 }
 
 static int check_repository_format_gently(const char *gitdir, int *nongit_ok)

-- 
1.8.4

On Mon, Oct 14, 2013 at 4:29 AM, Jiang Xin [off-list ref] wrote:

In test cases for relative_path, path with one leading character
(such as /a, /x) may be recogonized as "a:/" or "x:/" if there is
such DOS drive on MSYS platform. Use an umambigous leading path
"/foo" instead.

Also change two leading slashes (//) to three leading slashes (///),
otherwize it will be recognized as UNC name on MSYS platform.

Signed-off-by: Jiang Xin <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 t/t0060-path-utils.sh | 56 +++++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

Thanks, ack.

-- 
Sebastian Schuberth

On Sun, Oct 13, 2013 at 10:29 PM, Jiang Xin [off-list ref] wrote:

In test cases for relative_path, path with one leading character
(such as /a, /x) may be recogonized as "a:/" or "x:/" if there is
such DOS drive on MSYS platform. Use an umambigous leading path
"/foo" instead.

Also change two leading slashes (//) to three leading slashes (///),
otherwize it will be recognized as UNC name on MSYS platform.

s/otherwize/otherwise/

Signed-off-by: Jiang Xin <redacted>
Signed-off-by: Junio C Hamano <redacted>