Regression in e02ca72: git svn rebase is broken on Windows

Quoted 6 lines · Junio C Hamano <gitster <at> pobox.com> writes:

Quoted 3 lines · Tvangeste <i.4m.l33t <at> yandex.ru> writes:
Hi,
After bisecting this problem I ended up with the mentioned commit that

completely breaks git-svn for me on

Quoted 7 lines

Windows (mingw/msys version).
Quoted 6 lines
==========
#> git svn rebase
warning: unable to access '': Invalid argument
warning: unable to access '': Invalid argument
fatal: unable to access '../../../../w:/work/my/repo.git/.git/config':

Invalid argument

Quoted 7 lines

Quoted 7 lines
fatal: index file open failed: Invalid argument Cannot rebase: You have unstaged changes. Please commit or stash them.
rebase refs/remotes/trunk: command returned error: 1
==========
Please note that I use the official git repository as-is, this one (no

additional patches):

Quoted 12 lines

Quoted 11 lines
git://git.kernel.org/pub/scm/git/git.git
e02ca72f70ed8f0268a81f72cb3230c72e538e77 is the first bad commit commit e02ca72f70ed8f0268a81f72cb3230c72e538e77
Author: Jiang Xin
Date:   Tue Jun 25 23:53:43 2013 +0800
path.c: refactor relative_path(), not only strip prefix
Thanks,
--Tvangeste

Quoted 8 lines

The suspect commit and symptom look consistent. You started from a directory whose absolute path is "w:/work/..." and the updated code mistakenly thoguht that something that begins with "w" (not '/') is not an absolute, so added a series of ../ to make it relative, or something silly like that.
Jiang?

Indeed, this patch seems to change relative_path in a way that breaks git initialization, not just on Windows.

Previously, relative_path was always called with two absolute paths, and it only returned a relative path if the first was a subdir of the second (so a better name would probably have been 'relative_path_if_subdir'). The purpose was to improve performance by making GIT_DIR shorter if it was a subdir of GIT_WORK_TREE.

After this patch, relative_path always tries to return a relative path, even if both absolute paths are completely disjunct. This not only defeats the purpose (by making GIT_DIR longer, thus hurting performance), it is also not possible in general. POSIX explicitly allows for '//hostname' notation referring to network resources that are not explicitly mounted under '/'. I.e. given two absolute paths '//hostname1/a' and '//hostname2/b', there is no relative path from a to b or vice versa.

Additionally, GIT_DIR now may or may not have a trailing slash, which gives me a slightly uneasy feeling...

Jiang Xin

2016-06-15 22:58 UTC #11 lore raw

2013/9/11 Karsten Blees <[email protected]>:

Quoted 11 lines

Quoted 8 lines · Junio C Hamano <gitster <at> pobox.com> writes:
The suspect commit and symptom look consistent. You started from a directory whose absolute path is "w:/work/..." and the updated code mistakenly thoguht that something that begins with "w" (not '/') is not an absolute, so added a series of ../ to make it relative, or something silly like that.
Jiang?

Quoted 8 lines

Indeed, this patch seems to change relative_path in a way that breaks git initialization, not just on Windows.
Previously, relative_path was always called with two absolute paths, and it only returned a relative path if the first was a subdir of the second (so a better name would probably have been 'relative_path_if_subdir'). The purpose was to improve performance by making GIT_DIR shorter if it was a subdir of GIT_WORK_TREE.

Yes, it's what commit v1.5.6-1-g044bbbc says.

Quoted 3 lines

After this patch, relative_path always tries to return a relative path, even if both absolute paths are completely disjunct. This not only defeats the purpose (by making GIT_DIR longer, thus hurting performance), it is also not

Sometimes longer, sometimes shorter maybe.

Quoted 4 lines

possible in general. POSIX explicitly allows for '//hostname' notation referring to network resources that are not explicitly mounted under '/'. I.e. given two absolute paths '//hostname1/a' and '//hostname2/b', there is no relative path from a to b or vice versa.

Yes, path like "//hostname/path" can be used on Windows.
My hack "have_same_root" does not cover this case, so using a "simple_relative_path" function instead of "relative_path" in setup.c may be the better.

Signature · 1 line

Jiang Xin

Jiang Xin

2016-06-15 22:58 UTC #12 lore raw

PATCH 1/2 relative_path should honor dos_drive_prefix

Tvangeste found that the "relative_path" function could not work properly on Windows if "in" and "prefix" have dos driver prefix. ($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.

Signed-off-by Jiang Xin <[email protected]>

---
 path.c                | 20 ++++++++++++++++++++
 t/t0060-path-utils.sh |  4 ++++
 2 files changed, 24 insertions(+)

path.c +20 -0

index 7f3324a..ffcdea1 100644
@@ -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 && !strncasecmp(path1, path2, 1)) ||
+	       (!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]))

t/t0060-path-utils.sh +4 -0

index 76c7792..c3c3b33 100755
@@ -208,6 +208,10 @@ 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	/x/y		a/b
+relative_path /a/b	x/y		/a/b	POSIX
+relative_path d:/a/b	D:/a/c		../b	MINGW
+relative_path C:/a/b	D:/a/c		C:/a/b	MINGW
 relative_path a/b	"<empty>"	a/b
 relative_path a/b 	"<null>"	a/b
 relative_path "<empty>"	/a/b		./

Signature · 1 line

1.8.3.rc2.14.g5ac1b82

Jiang Xin

2016-06-15 22:58 UTC #13 lore raw

PATCH 2/2 Use simpler relative_path when set_git_dir

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 to simple_relative_path, and call it in setup.c.

Signed-off-by Jiang Xin <[email protected]>

---
 cache.h |  1 +
 path.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
 setup.c |  5 +----
 3 files changed, 47 insertions(+), 4 deletions(-)

cache.h +1 -0

index 8e42256..ab17f1d 100644
@@ -738,6 +738,7 @@ 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 *relative_path(const char *in, const char *prefix, struct strbuf *sb);
+const char *simple_relative_path(const char *in, const char *prefix);
 int normalize_path_copy(char *dst, const char *src);
 int longest_ancestor_length(const char *path, struct string_list *prefixes);
 char *strip_path_suffix(const char *path, const char *suffix);

path.c +45 -0

51 lines not on this page · raw message

setup.c +1 -4

index 0d9ea62..f4be6ff 100644
@@ -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(simple_relative_path(git_dir, work_tree));
 	initialized = 1;
-
-	strbuf_release(&sb);
 }
 
 static int check_repository_format_gently(const char *gitdir, int *nongit_ok)

Signature · 1 line

1.8.3.rc2.14.g5ac1b82

Johannes Sixt

2016-06-15 22:58 UTC #14 lore raw

Re: [PATCH 2/2] Use simpler relative_path when set_git_dir

Am 12.09.2013 11:12, schrieb Jiang Xin:

Quoted 37 lines

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 to simple_relative_path, and call it in setup.c.
Suggested-by Karsten Blees <[email protected]>
Signed-off-by Jiang Xin <[email protected]>
---
 cache.h |  1 +
 path.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
 setup.c |  5 +----
 3 files changed, 47 insertions(+), 4 deletions(-)
cache.h +1 -0
index 8e42256..ab17f1d 100644
@@ -738,6 +738,7 @@ 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 *relative_path(const char *in, const char *prefix, struct strbuf *sb);
+const char *simple_relative_path(const char *in, const char *prefix);
 int normalize_path_copy(char *dst, const char *src);
 int longest_ancestor_length(const char *path, struct string_list *prefixes);
 char *strip_path_suffix(const char *path, const char *suffix);
path.c +0 -0
index ffcdea1..0f0c92f 100644

path.c const char *relative_path(const char *in, const char *prefix, · line 561

Quoted 4 lines

path.c +1 -0

@@ -558,6 +558,51 @@ const char *relative_path(const char *in, const char *prefix,
 }
 
 /*
+ * A simpler implementation of relative_path

So we have a heavy-duty function relative_path(), but it is not capable of doing the "simple" operations that this function does?

There must be something wrong.

This function were easier to sell if it were named remove_optional_prefix() or something.

Quoted 43 lines

path.c +42 -0

+ *
+ * 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 *simple_relative_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;
+}
...

Torsten Bögershausen

2016-06-15 22:58 UTC #15 lore raw

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

Quoted 23 lines · Jiang Xin

Tvangeste found that the "relative_path" function could not work properly on Windows if "in" and "prefix" have dos driver prefix. ($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 <[email protected]>
Helped-by Johannes Sixt <[email protected]>
Signed-off-by Jiang Xin <[email protected]>
---
 path.c                | 20 ++++++++++++++++++++
 t/t0060-path-utils.sh |  4 ++++
 2 files changed, 24 insertions(+)
path.c +0 -0
index 7f3324a..ffcdea1 100644

path.c int adjust_shared_perm(const char *path) · line 450

Quoted 10 lines

path.c +7 -0

@@ -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 && !strncasecmp(path1, path2, 1)) ||

                                       ^^^^^^^^^^^

I wonder: should strncasecmp() be replaced with strncmp_icase() ?

See dir.c:
int strncmp_icase(const char *a, const char *b, size_t count)

{
	return ignore_case ? strncasecmp(a, b, count) : strncmp(a, b, count);
}

/Torsten

Quoted 39 lines

path.c +13 -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]))

t/t0060-path-utils.sh +4 -0

index 76c7792..c3c3b33 100755
@@ -208,6 +208,10 @@ 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	/x/y		a/b
+relative_path /a/b	x/y		/a/b	POSIX
+relative_path d:/a/b	D:/a/c		../b	MINGW
+relative_path C:/a/b	D:/a/c		C:/a/b	MINGW
 relative_path a/b	"<empty>"	a/b
 relative_path a/b 	"<null>"	a/b
 relative_path "<empty>"	/a/b		./

Johannes Sixt

2016-06-15 22:58 UTC #16 lore raw

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

Am 12.09.2013 16:13, schrieb Torsten Bögershausen:

Quoted 10 lines · Jiang Xin

path.c int adjust_shared_perm(const char *path) · line 450

Quoted 7 lines

path.c +7 -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 && !strncasecmp(path1, path2, 1)) ||

                                       ^^^^^^^^^^^

I wonder: should strncasecmp() be replaced with strncmp_icase() ?

I don't think so: On POSIX, it is irrelevant, because the call will only compare a slash to a slash. On Windows, it compares the drive letters (or a slash); it is *always* case-insensitive, even if the volume mounted is NTFS with case-sensitivity enabled and core.ignorecase is false.

Quoted 5 lines

See dir.c:
int strncmp_icase(const char *a, const char *b, size_t count)
{
	return ignore_case ? strncasecmp(a, b, count) : strncmp(a, b, count);
}

-- Hannes

Junio C Hamano

2016-06-15 22:58 UTC #17 lore raw

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

Quoted 12 lines · Johannes Sixt

Am 12.09.2013 16:13, schrieb Torsten Bögershausen:
Quoted 10 lines · Jiang Xin
path.c int adjust_shared_perm(const char *path) · line 450
Quoted 7 lines
path.c +7 -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 && !strncasecmp(path1, path2, 1)) ||
                                       ^^^^^^^^^^^
I wonder: should strncasecmp() be replaced with strncmp_icase() ?

Quoted 4 lines

I don't think so: On POSIX, it is irrelevant, because the call will only compare a slash to a slash. On Windows, it compares the drive letters (or a slash); it is *always* case-insensitive, even if the volume mounted is NTFS with case-sensitivity enabled and core.ignorecase is false.

Ah, you are right, of course. We could even do

	tolower(path1[0]) == tolower(path2[0])

which might be more explicit.

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.

Jiang Xin

2016-06-15 22:58 UTC #18 lore raw

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

2013/9/13 Junio C Hamano <[email protected]>:

Quoted 6 lines

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

Quoted 8 lines

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

Signature · 1 line

Jiang Xin

Torsten Bögershausen

2016-06-15 22:58 UTC #19 lore raw

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

Quoted 9 lines · Jiang Xin

2013/9/13 Junio C Hamano <[email protected]>:
Quoted 8 lines
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;

Quoted 7 lines

Quoted 5 lines
- //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.

Similar to the has_dos_drive_prefix:

For Windows/Mingw we do like this

mingw.h
#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')

And all other platforms defines has_dos_drive_prefix() to be 0 here git-compat-util.h
#ifndef has_dos_drive_prefix
#define has_dos_drive_prefix(path) 0
#endif

mingw.h:
#define has_unc_path_prefix(path) ((path)[0] == '/' && (path)[1] == '/') (Or may be)
#define has_unc_path_prefix(path) (is_dir_sep((path)[0]) && is_dir_sep((path)[1]))

Junio C Hamano

2016-06-15 22:58 UTC #20 lore raw

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

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

Quoted 10 lines · Jiang Xin

2013/9/13 Junio C Hamano <[email protected]>:
Quoted 9 lines
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;

Quoted 6 lines

Quoted 3 lines
- //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

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.

Jiang Xin

2016-06-15 22:58 UTC #21 lore raw

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

2013/9/13 Junio C Hamano <[email protected]>:

Quoted 35 lines

Quoted 10 lines · Jiang Xin
2013/9/13 Junio C Hamano <[email protected]>:
Quoted 9 lines
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;
Quoted 6 lines
Quoted 3 lines
- //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.

Quoted 9 lines

(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;
+ }
+}

Signature · 1 line

Jiang Xin

Jiang Xin

2016-06-15 22:58 UTC #22 lore raw

PATCH v3 1/3 test: use unambigous leading path (/foo) for mingw

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 <[email protected]>

---
 t/t0060-path-utils.sh | 56 +++++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

t/t0060-path-utils.sh +28 ~~-28~~

index 3a48de2..92976e0 100755
@@ -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

Signature · 1 line

1.8.4.460.g2f083d1

Jiang Xin

2016-06-15 22:58 UTC #23 lore raw

PATCH v3 2/3 relative_path should honor DOS and UNC paths

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.

Signed-off-by Jiang Xin <[email protected]>

---
 compat/mingw.h        |  9 +++++++++
 git-compat-util.h     |  4 ++++
 path.c                | 25 +++++++++++++++++++++++++
 t/t0060-path-utils.sh |  8 ++++++++
 4 files changed, 46 insertions(+)

compat/mingw.h +9 -0

index bd0a88b..06e9f49 100644
@@ -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;

git-compat-util.h +4 -0

index 9549de6..93c2206 100644
@@ -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

path.c +25 -0

index 9fd28bcd..544d10d 100644
@@ -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]))

t/t0060-path-utils.sh +8 -0

index 92976e0..830b6d5 100755
@@ -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	./

Signature · 1 line

1.8.4.460.g2f083d1

Junio C Hamano

2016-06-15 22:58 UTC #24 lore raw

Re: [PATCH v3 2/3] relative_path should honor DOS and UNC paths

compat/mingw.h int winansi_fprintf(FILE *stream, const char *format, ...) __attribute__((format · line 317

Quoted 12 lines · Jiang Xin

compat/mingw.h +4 -0

index bd0a88b..06e9f49 100644
@@ -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;

If path[1] == '\0', it would be !is_dir_sep() and we end up inspecting past the end of the string?

A message that is not in the archive (sent off-list or not yet received): [email protected]

Torsten Bögershausen

2016-06-15 22:58 UTC #26 lore raw

Re: [PATCH v3 2/3] relative_path should honor DOS and UNC paths

Quoted 9 lines · Torsten Bögershausen

Quoted 8 lines · Junio C Hamano
Quoted 6 lines · Jiang Xin
compat/mingw.h +0 -0
index bd0a88b..06e9f49 100644
@@ -311,6 +311,15 @@ int winansi_fprintf(FILE *stream, const char *format, ...) __attribute__((format
 

Quoted 9 lines

Quoted 9 lines
compat/mingw.h int winansi_fprintf(FILE *stream, const char *format, ...) __attribute__((format · line 317
Quoted 6 lines
compat/mingw.h +4 -0
 #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;
If path[1] == '\0', it would be !is_dir_sep() and we end up inspecting past the end of the string?

Yes
(If there was a previous mail, it was incomplete, sorry)

I think we want to catch "2 (back)slashes followed by a letter"
<http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247%28v=vs.85%29.aspx>

#define is_unc_path(path) ((is_dir_sep(path)[0]) && is_dir_sep((path)[1]) && isalpha((path[2])))

Then we need
#define is_relative_path(path) (((path)[0]) && !is_dir_sep((path)[1]))

And may be 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_relative_path(path2))
			return 0;
		if (!is_relative_path(path1) && is_unc_path(path2))
			return 0;
		return tolower(path1[0]) == tolower(path2[0]);
	} else {
		return !is_abs1 && !is_abs2;
	}
}

Could that work?

Jiang Xin

2016-06-15 22:58 UTC #27 lore raw

Re: [PATCH v3 2/3] relative_path should honor DOS and UNC paths

2013/9/18 Junio C Hamano <[email protected]>:

Quoted 8 lines

Quoted 6 lines · Jiang Xin

compat/mingw.h +0 -0

index bd0a88b..06e9f49 100644
@@ -311,6 +311,15 @@ int winansi_fprintf(FILE *stream, const char *format, ...) __attribute__((format

Quoted 6 lines

compat/mingw.h int winansi_fprintf(FILE *stream, const char *format, ...) __attribute__((format · line 317

Quoted 6 lines

compat/mingw.h +4 -0

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

A UNC path must start with two slashes, but not three or more slashes.

Quoted 3 lines

If path[1] == '\0', it would be !is_dir_sep() and we end up inspecting past the end of the string?

The funciton "is_unc_path" will return false (0), if path is "", "/", "//", "///three/slashes/", or "/usr/local". So the problem is ?

Signature · 1 line

Jiang Xin

Junio C Hamano

2016-06-15 22:58 UTC #28 lore raw

Re: [PATCH v3 2/3] relative_path should honor DOS and UNC paths

Quoted 9 lines · Jiang Xin

2013/9/18 Junio C Hamano <[email protected]>:
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+     if (!is_dir_sep(*path) || !is_dir_sep(*(path+1)) || is_dir_sep(*(path+2)))
+             return 0;
If path[1] == '\0', it would be !is_dir_sep() and we end up inspecting past the end of the string?
The funciton "is_unc_path" will return false (0), if path is "", "/", "//", "///three/slashes/", or "/usr/local". So the problem is ?

If path[1] == '\0' (e.g. path="/"), !is_dir_sep(path[1]) is true, not false (as I misread earlier), so we hit an early return and will not peek path[2]. So no problem. Sorry for the noise.

But I agree with J6t and Torsten in near-by thread that the simpler one that does not worry about // should be done as a separate patch and //, if we decide to do it, should build on top.

Thanks.

Jiang Xin

2016-06-15 22:58 UTC #29 lore raw

PATCH v3 3/3 Use simpler relative_path when set_git_dir

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.

Signed-off-by Jiang Xin <[email protected]>

---
 cache.h |  1 +
 path.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
 setup.c |  5 +----
 3 files changed, 47 insertions(+), 4 deletions(-)

cache.h +1 -0

index a47b9c0..73fa334 100644
@@ -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);

path.c +45 -0

51 lines not on this page · raw message

setup.c +1 -4

index f08dd64..dbf4138 100644
@@ -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)

Signature · 1 line

1.8.4.460.g2f083d1

Johannes Sixt

2016-06-15 22:58 UTC #30 lore raw

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

In reply to Johannes Sixt

Am 17.09.2013 10:24, schrieb Jiang Xin:

Quoted 8 lines

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:

Quoted 8 lines

    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

Torsten Bögershausen

2016-06-15 22:58 UTC #31 lore raw

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

Quoted 44 lines · Johannes Sixt

Am 17.09.2013 10:24, schrieb Jiang Xin:
Quoted 8 lines
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:
Quoted 8 lines
    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.

Quoted 6 lines

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

Jiang Xin

2016-06-15 22:58 UTC #32 lore raw

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

2013/9/18 Johannes Sixt <[email protected]>:

Quoted 27 lines

Am 17.09.2013 10:24, schrieb Jiang Xin:
Quoted 8 lines
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:
Quoted 8 lines
    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.

Quoted 8 lines

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

Quoted 6 lines

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.

Signature · 1 line

Jiang Xin

Jiang Xin

2016-06-15 22:58 UTC #33 lore raw

v4 cover relative path regression fix

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

Signature · 1 line

1.8.4.460.gbed9cb4

Jiang Xin

2016-06-15 22:58 UTC #34 lore raw

PATCH v4 1/3 test: use unambigous leading path (/foo) for mingw

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 <[email protected]>

---
 t/t0060-path-utils.sh | 56 +++++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

t/t0060-path-utils.sh +28 ~~-28~~

index 3a48de2..92976e0 100755
@@ -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

Signature · 1 line

1.8.4.460.gbed9cb4

Sebastian Schuberth

2016-06-15 22:58 UTC #35 lore raw

Re: [PATCH v4 1/3] test: use unambigous leading path (/foo) for mingw

Quoted 7 lines · 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 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".

t/t0060-path-utils.sh test_expect_success SYMLINKS 'real path works on symlinks' '

Quoted 3 lines

t/t0060-path-utils.sh +1 -1

-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

Signature · 1 line

Sebastian Schuberth

Jiang Xin

2016-06-15 22:59 UTC #36 lore raw

Re: [PATCH v4 1/3] test: use unambigous leading path (/foo) for mingw

2013/10/11 Sebastian Schuberth <[email protected]>:

Quoted 5 lines

Quoted 5 lines
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.

Quoted 7 lines

Quoted 2 lines
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 12 lines

t/t0060-path-utils.sh test_expect_success SYMLINKS 'real path works on symlinks' '
Quoted 4 lines
t/t0060-path-utils.sh +1 -1
-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/

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

Signature · 1 line

Jiang Xin

Jiang Xin

2016-06-15 22:59 UTC #37 lore raw

v5 cover relative path regression fix

In reply to Sebastian Schuberth

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

Signature · 1 line

1.8.4

Jiang Xin

2016-06-15 22:59 UTC #38 lore raw

PATCH v5 1/3 test: use unambigous leading path (/foo) for MSYS

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 <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 t/t0060-path-utils.sh | 56 +++++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

t/t0060-path-utils.sh +28 ~~-28~~

index 3a48de2..92976e0 100755
@@ -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

Signature · 1 line

1.8.4

Sebastian Schuberth

2016-06-15 22:59 UTC #39 lore raw

Re: [PATCH v5 1/3] test: use unambigous leading path (/foo) for MSYS

Quoted 5 lines · 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 MSYS platform. Use an umambigous leading path "/foo" instead.

Quoted 8 lines

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 <[email protected]>
Signed-off-by Junio C Hamano <[email protected]>
---
 t/t0060-path-utils.sh | 56 +++++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

Thanks, ack.

Signature · 1 line

Sebastian Schuberth

Eric Sunshine

2016-06-15 22:59 UTC #40 lore raw

Re: [PATCH v5 1/3] test: use unambigous leading path (/foo) for MSYS

Quoted 7 lines · 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 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/

Quoted 2 lines

Signed-off-by Jiang Xin <[email protected]>
Signed-off-by Junio C Hamano <[email protected]>

Jiang Xin

2016-06-15 22:59 UTC #41 lore raw

PATCH v5 2/3 relative_path should honor dos-drive-prefix

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.

Signed-off-by Jiang Xin <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 path.c                | 20 ++++++++++++++++++++
 t/t0060-path-utils.sh |  4 ++++
 2 files changed, 24 insertions(+)

path.c +20 -0

index 7f3324a..0c16dc5 100644
@@ -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]))

t/t0060-path-utils.sh +4 -0

index 92976e0..40dfa2d 100755
@@ -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	./

Signature · 1 line

1.8.4

Jiang Xin

2016-06-15 22:59 UTC #42 lore raw

PATCH v5 3/3 Use simpler relative_path when set_git_dir

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.

Signed-off-by Jiang Xin <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 cache.h |  1 +
 path.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
 setup.c |  5 +----
 3 files changed, 47 insertions(+), 4 deletions(-)

cache.h +1 -0

index 8e42256..94475bd 100644
@@ -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);

path.c +45 -0

51 lines not on this page · raw message

setup.c +1 -4

index 0d9ea62..dad39c1 100644
@@ -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)

Signature · 1 line

1.8.4

Jiang Xin

2016-06-15 22:58 UTC #43 lore raw

PATCH v4 2/3 relative_path should honor dos-driver-prefix

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.

Signed-off-by Jiang Xin <[email protected]>

---
 path.c                | 20 ++++++++++++++++++++
 t/t0060-path-utils.sh |  4 ++++
 2 files changed, 24 insertions(+)

path.c +20 -0

index 7f3324a..0c16dc5 100644
@@ -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]))

t/t0060-path-utils.sh +4 -0

index 92976e0..40dfa2d 100755
@@ -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	./

Signature · 1 line

1.8.4.460.gbed9cb4

Sebastian Schuberth

2016-06-15 22:58 UTC #44 lore raw

Re: [PATCH v4 2/3] relative_path should honor dos-driver-prefix

Quoted 3 lines · Jiang Xin

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/

Quoted 5 lines

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/

Signature · 1 line

Sebastian Schuberth

Jiang Xin

2016-06-15 22:58 UTC #45 lore raw

PATCH v4 3/3 Use simpler relative_path when set_git_dir

In reply to Torsten Bögershausen

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.

Signed-off-by Jiang Xin <[email protected]>

---
 cache.h |  1 +
 path.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
 setup.c |  5 +----
 3 files changed, 47 insertions(+), 4 deletions(-)

cache.h +1 -0

index 8e42256..94475bd 100644
@@ -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);

path.c +45 -0

51 lines not on this page · raw message

setup.c +1 -4

index 0d9ea62..dad39c1 100644
@@ -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)

Signature · 1 line

1.8.4.460.gbed9cb4

Junio C Hamano

2016-06-15 22:58 UTC #46 lore raw

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

Quoted 9 lines · Torsten Bögershausen

path.c int adjust_shared_perm(const char *path) · line 450

Quoted 7 lines

path.c +7 -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 && !strncasecmp(path1, path2, 1)) ||

                                       ^^^^^^^^^^^

I wonder: should strncasecmp() be replaced with strncmp_icase() ?

True.

Quoted 5 lines

See dir.c:
int strncmp_icase(const char *a, const char *b, size_t count)
{
	return ignore_case ? strncasecmp(a, b, count) : strncmp(a, b, count);
}

Karsten Blees

2016-06-15 22:58 UTC #47 lore raw

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

[1] http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xbd_chap04.html#tag_21_04_12

Am 12.09.2013 11:12, schrieb Jiang Xin:

Quoted 10 lines

Tvangeste found that the "relative_path" function could not work properly on Windows if "in" and "prefix" have dos driver prefix. ($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.

I still don't think that cd'ing through the root is a Good Thing for absolute paths, as it is not possible to do so in general.

POSIX says the meaning of '//' is implementation-defined [1].

Cygwin supports //hostname/share/directory/file. You cannot cd to the hostname (i.e. root would be //hostname/share).

On Windows, we have drive letters, UNC paths and namespaces:
C:\directory\file
\\hostname\share\directory\file (same as cygwin) \\?\C:\directory\file \\?\UNC\hostname\share\directory\file

I'm not sure about '//' support on other git platforms (the most likely candidate would probably be HP-UX, as HP bought Apollo/DomainOS, which supported '//hostname/directory/file back in 1981).

You could of course handle all these special cases. However, with the POSIX definition above, the only reliable and future-proof way to check if we can cd out of a path component of an _absolute_ path is to actually try it until we get an error. And we probably don't want to do actual IO in relative_path (which is pure string manipulation so far).

Tvangeste

author

2016-06-15 22:58 UTC #48 lore raw

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

Thank you, this fixes the problem with git svn rebase on Windows for me.

--Tvangeste

Quoted 74 lines · Jiang Xin

Tvangeste found that the "relative_path" function could not work properly on Windows if "in" and "prefix" have dos driver prefix. ($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.

Signed-off-by Jiang Xin <[email protected]>

---
 path.c                | 20 ++++++++++++++++++++
 t/t0060-path-utils.sh |  4 ++++
 2 files changed, 24 insertions(+)

path.c +20 -0

index 7f3324a..ffcdea1 100644
@@ -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 && !strncasecmp(path1, path2, 1)) ||
+              (!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]))

t/t0060-path-utils.sh +4 -1

index 76c7792..c3c3b33 100755
@@ -208,6 +208,10 @@ 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      /x/y            a/b
+relative_path /a/b     x/y             /a/b    POSIX
+relative_path d:/a/b   D:/a/c          ../b    MINGW
+relative_path C:/a/b   D:/a/c          C:/a/b  MINGW
 relative_path a/b      "<empty>"       a/b
 relative_path a/b      "<null>"        a/b
 relative_path "<empty>"        /a/b            ./
--

1.8.3.rc2.14.g5ac1b82

Jiang Xin

2016-06-15 22:58 UTC #49 lore raw

v2 cover fixes for relative_path

Updates since v1:

* New patch 1/3 on t0060, which use umambigous leading path (/foo).
* Call tolower instead of strncasecmp in patch 2/3.
* Rename simple_relative_path to remove_leading_path in patch 3/3.

Jiang Xin (3):
test: use unambigous leading path (/foo) for mingw 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(-)

Signature · 1 line

1.8.4.459.gd80d422

Jiang Xin

2016-06-15 22:58 UTC #50 lore raw

PATCH v2 1/3 test: use unambigous leading path (/foo) for mingw

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 doc drive on MINGW platform. Use an umambigous leading path "/foo" instead.

Signed-off-by Jiang Xin <[email protected]>

---
 t/t0060-path-utils.sh | 56 +++++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

t/t0060-path-utils.sh +28 ~~-28~~

index 3a48de2..82a6f21 100755
@@ -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

Signature · 1 line

1.8.4.459.gd80d422

Junio C Hamano

2016-06-15 22:58 UTC #51 lore raw

Re: [PATCH v2 1/3] test: use unambigous leading path (/foo) for mingw

Quoted 4 lines · 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 doc drive on MINGW platform. Use an umambigous leading path "/foo" instead.

"DOS drive", you mean?

Are they really spelled as /a or /x (not e.g. //a or something)?

Just double-checking.

Quoted 71 lines

Signed-off-by Jiang Xin <[email protected]>

---
 t/t0060-path-utils.sh | 56 +++++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

t/t0060-path-utils.sh +28 ~~-28~~

index 3a48de2..82a6f21 100755
@@ -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

Torsten Bögershausen

2016-06-15 22:58 UTC #52 lore raw

Re: [PATCH v2 1/3] test: use unambigous leading path (/foo) for mingw

Quoted 7 lines · Junio C Hamano

Quoted 4 lines · 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 doc drive on MINGW platform. Use an umambigous leading path "/foo" instead.

Quoted 5 lines

"DOS drive", you mean?
Are they really spelled as /a or /x (not e.g. //a or something)?
Just double-checking.

Yes,
there is a directoctory structure in / like this:

/usr
/bin
/lib
Then we have the drive letters mapped to single letters:
/c/Documents and Settings
/c/temp

As an alternative
c:/temp can be used
or the DOS style
"c:\temp"

And the // or "\\" is used for the UNC names (Universal Name Convention) //Servername/ShareName/Directory

/Torsten

Junio C Hamano

2016-06-15 22:58 UTC #53 lore raw

Re: [PATCH v2 1/3] test: use unambigous leading path (/foo) for mingw

Quoted 9 lines · Torsten Bögershausen

Yes,
there is a directoctory structure in / like this:
/usr
/bin
/lib
Then we have the drive letters mapped to single letters:
/c/Documents and Settings
/c/temp

Thanks.

Jiang Xin

2016-06-15 22:58 UTC #54 lore raw

PATCH v2 2/3 relative_path should honor dos_drive_prefix

Tvangeste found that the "relative_path" function could not work properly on Windows if "in" and "prefix" have dos driver prefix. ($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.

Signed-off-by Jiang Xin <[email protected]>

---
 path.c                | 20 ++++++++++++++++++++
 t/t0060-path-utils.sh |  4 ++++
 2 files changed, 24 insertions(+)

path.c +20 -0

index 9fd28bcd..65d376d 100644
@@ -434,6 +434,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.
  *

@@ -454,6 +464,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]))

t/t0060-path-utils.sh +4 -0

index 82a6f21..0187d11 100755
@@ -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	./

Signature · 1 line

1.8.4.459.gd80d422

Torsten Bögershausen

2016-06-15 22:58 UTC #55 lore raw

Re: [PATCH v2 2/3] relative_path should honor dos_drive_prefix

Quoted 26 lines · Jiang Xin

Tvangeste found that the "relative_path" function could not work properly on Windows if "in" and "prefix" have dos driver prefix. ($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 <[email protected]>
Helped-by Johannes Sixt <[email protected]>
Signed-off-by Jiang Xin <[email protected]>
---
 path.c                | 20 ++++++++++++++++++++
 t/t0060-path-utils.sh |  4 ++++
 2 files changed, 24 insertions(+)
path.c +0 -0
index 9fd28bcd..65d376d 100644
@@ -434,6 +434,16 @@ int adjust_shared_perm(const char *path)
 	return 0;
 }
 

path.c int adjust_shared_perm(const char *path) · line 446

Quoted 10 lines

path.c +10 -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);
+}
+

I think the name of the fuction is somewhat misleading, as we are not sure if they really have the same root.
And that is investigated further down.

may_have_same_root() could be a better name.

[snip]

Quoted 8 lines

path.c +0 -0

 	while (i < prefix_len && j < in_len && prefix[i] == in[j]) {
 		if (is_dir_sep(prefix[i])) {
 			while (is_dir_sep(prefix[i]))

t/t0060-path-utils.sh +0 -0

index 82a6f21..0187d11 100755

t/t0060-path-utils.sh relative_path foo/a/b/ foo/a/b ./ · line 216

Quoted 7 lines

t/t0060-path-utils.sh +4 -0

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

Side question:
What happens if we feed in a relative path with a dos drive? like "c:foo" which is different from c:/foo.

Jiang Xin

2016-06-15 22:58 UTC #56 lore raw

PATCH v2 3/3 Use simpler relative_path when set_git_dir