[PATCH] fix crash in path.c on Windows

[PATCH] fix crash in path.c on Windows

[René Scharfe]

Use PATH_SEP and is_absolute_path() instead of using Unix conventions
to enhance portability.  On Windows, the assert() fails almost always
without this change.

Also convert all backslashes in DOS-style paths to slashes, as that's
what git uses internally.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
The patch doesn't fix t1504 on Windows, but it allows one to at least
run this test without git crashing multiple times due to the failed
assertion.  The test case "first_of_two" still fails, due to the
weird translation applied to environment variables like
$GIT_CEILING_DIRECTORIES.  Is there a method to it?  Here some
experiments:

	set a=	getenv("a")
	======= ===========
	c	c
	/c	c:/
	c/	c/
	/c/	c:/
	c:c	c:c
	/c:c	c:c
	c:/c	c:/c
	/c:/c	c:/c
	c/:/c	c\;c:\
	/c:c/	c:c/
	/c/:c	/c/:c
	/c/:/c	c:\;c:\
	/c:/c/	c:/c/
	/c/:/c/	c:\;c:\

The test case can be convinced to pass by replacing "bar" with
"/bar", but I'm not sure that's the right fix.  Shouldn't we warn on
invalid paths in $GIT_CEILING_DIRECTORIES?

 path.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/path.c b/path.c
index a074aea..4cae7f6 100644
--- a/path.c
+++ b/path.c
@@ -387,7 +387,7 @@ int normalize_absolute_path(char *buf, const char *path)
 	assert(path);
 
 	while (*comp_start) {
-		assert(*comp_start == '/');
+		assert(is_absolute_path(comp_start));
 		while (*++comp_end && *comp_end != '/')
 			; /* nothing */
 		comp_len = comp_end - comp_start;
@@ -438,11 +438,20 @@ int longest_ancestor_length(const char *path, const char *prefix_list)
 		return -1;
 
 	for (colon = ceil = prefix_list; *colon; ceil = colon+1) {
-		for (colon = ceil; *colon && *colon != ':'; colon++);
+		for (colon = ceil; *colon && *colon != PATH_SEP; colon++);
 		len = colon - ceil;
 		if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))
 			continue;
 		strlcpy(buf, ceil, len+1);
+
+		if (has_dos_drive_prefix(buf)) {
+			char *p;
+			for (p = buf; *p; p++) {
+				if (*p == '\\')
+					*p = '/';
+			}
+		}
+
 		len = normalize_absolute_path(buf, buf);
 		/* Strip "trailing slashes" from "/". */
 		if (len == 1)
-- 
1.6.1.2

Re: [PATCH] fix crash in path.c on Windows

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> @@ -387,7 +387,7 @@ int normalize_absolute_path(char *buf, const char *path)
>  	assert(path);
>  
>  	while (*comp_start) {
> -		assert(*comp_start == '/');
> +		assert(is_absolute_path(comp_start));
>  		while (*++comp_end && *comp_end != '/')
>  			; /* nothing */
>  		comp_len = comp_end - comp_start;

This change does not make sense to me.  The assert is about the initial
iteration beginning at the "root" level, and at the same time previous
iteration ended at dir_sep.  On mingw you would probably need these two as
separate tests.  In other words, I would understand if the fix were like
this:

	if (it begins with dos_prefix) {
        	/* this is never true outside windows */
		copy the dos prefix out and advance comp_start as
        	necessary;
	}
        while (*comp_start) {
        	assert(is_dir_sep(*comp_start));
                while (*++comp_end && !is_dir_sep(*comp_end))
                	; /* nothing */
		...
	}

Re: [PATCH] fix crash in path.c on Windows

[Johannes Sixt]

René Scharfe schrieb:
> 	set a=	getenv("a")
> 	======= ===========
> 	c	c
> 	/c	c:/
> 	c/	c/
> 	/c/	c:/
> 	c:c	c:c
> 	/c:c	c:c
> 	c:/c	c:/c
> 	/c:/c	c:/c
> 	c/:/c	c\;c:\
> 	/c:c/	c:c/
> 	/c/:c	/c/:c
> 	/c/:/c	c:\;c:\
> 	/c:/c/	c:/c/
> 	/c/:/c/	c:\;c:\

Bash translates leading single-letter path components to drive prefix
notation if it invokes a non-MSYS program; and it also translates ':' to
';' if the value looks like a path list. Sometimes there is an ambiguity
and bash guesses wrong.

> @@ -387,7 +387,7 @@ int normalize_absolute_path(char *buf, const char *path)
>  	assert(path);
>  
>  	while (*comp_start) {
> -		assert(*comp_start == '/');
> +		assert(is_absolute_path(comp_start));
>  		while (*++comp_end && *comp_end != '/')
>  			; /* nothing */
>  		comp_len = comp_end - comp_start;

Junio has pointed out your thinko here. Furthermore, *all* uses of '/' in
this loop must be replaced by is_dir_sep().

But I would really appreciate if you could unify normalize_absolute_path()
with setup.c's sanitary_path_copy() because they do almost the same thing
(and the latter already works on Windows).

> @@ -438,11 +438,20 @@ int longest_ancestor_length(const char *path, const char *prefix_list)
>  		return -1;
>  
>  	for (colon = ceil = prefix_list; *colon; ceil = colon+1) {
> -		for (colon = ceil; *colon && *colon != ':'; colon++);
> +		for (colon = ceil; *colon && *colon != PATH_SEP; colon++);
>  		len = colon - ceil;
>  		if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))
>  			continue;
>  		strlcpy(buf, ceil, len+1);
> +
> +		if (has_dos_drive_prefix(buf)) {
> +			char *p;
> +			for (p = buf; *p; p++) {
> +				if (*p == '\\')
> +					*p = '/';
> +			}

IMNSHO this is a kind of normalization and, therefore, must be done by
normalize_absolute_path() (sanitary_path_copy() already does this).

> +		}
> +
>  		len = normalize_absolute_path(buf, buf);
>  		/* Strip "trailing slashes" from "/". */
>  		if (len == 1)

-- Hannes

Re: [PATCH] fix crash in path.c on Windows

[René Scharfe]

Johannes Sixt schrieb:
> René Scharfe schrieb:
>> 	set a=	getenv("a")
>> 	======= ===========
>> 	c	c
>> 	/c	c:/
>> 	c/	c/
>> 	/c/	c:/
>> 	c:c	c:c
>> 	/c:c	c:c
>> 	c:/c	c:/c
>> 	/c:/c	c:/c
>> 	c/:/c	c\;c:\
>> 	/c:c/	c:c/
>> 	/c/:c	/c/:c
>> 	/c/:/c	c:\;c:\
>> 	/c:/c/	c:/c/
>> 	/c/:/c/	c:\;c:\
> 
> Bash translates leading single-letter path components to drive prefix
> notation if it invokes a non-MSYS program; and it also translates ':' to
> ';' if the value looks like a path list. Sometimes there is an ambiguity
> and bash guesses wrong.

Sure, but what rules or heuristics does it follow?  Do we need to
post-process the results or can we simply change the test case in t1504?

>> @@ -387,7 +387,7 @@ int normalize_absolute_path(char *buf, const char *path)
>>  	assert(path);
>>  
>>  	while (*comp_start) {
>> -		assert(*comp_start == '/');
>> +		assert(is_absolute_path(comp_start));
>>  		while (*++comp_end && *comp_end != '/')
>>  			; /* nothing */
>>  		comp_len = comp_end - comp_start;
> 
> Junio has pointed out your thinko here. Furthermore, *all* uses of '/' in
> this loop must be replaced by is_dir_sep().

It's not a thinko because I didn't think there. :)  It's more a case of
mechanical refactoring resulting in confusing code.  If we inline
is_absolute_path() manually there, we get the following:

	assert(*comp_start == '/' || has_dos_drive_prefix(comp_start));

The loop works because DOS drive prefixes never contain slashes.

Is is_absolute_path() too forgiving on Windows, i.e. should it stop
classifying paths starting with a slash as absolute on that platform?

> But I would really appreciate if you could unify normalize_absolute_path()
> with setup.c's sanitary_path_copy() because they do almost the same thing
> (and the latter already works on Windows).

Good idea.

>> @@ -438,11 +438,20 @@ int longest_ancestor_length(const char *path, const char *prefix_list)
>>  		return -1;
>>  
>>  	for (colon = ceil = prefix_list; *colon; ceil = colon+1) {
>> -		for (colon = ceil; *colon && *colon != ':'; colon++);
>> +		for (colon = ceil; *colon && *colon != PATH_SEP; colon++);
>>  		len = colon - ceil;
>>  		if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))
>>  			continue;
>>  		strlcpy(buf, ceil, len+1);
>> +
>> +		if (has_dos_drive_prefix(buf)) {
>> +			char *p;
>> +			for (p = buf; *p; p++) {
>> +				if (*p == '\\')
>> +					*p = '/';
>> +			}
> 
> IMNSHO this is a kind of normalization and, therefore, must be done by
> normalize_absolute_path() (sanitary_path_copy() already does this).

Makes sense.

René

Re: [PATCH] fix crash in path.c on Windows

[Johannes Sixt]

René Scharfe schrieb:
> Johannes Sixt schrieb:
>> Bash translates leading single-letter path components to drive prefix
>> notation if it invokes a non-MSYS program; and it also translates ':' to
>> ';' if the value looks like a path list. Sometimes there is an ambiguity
>> and bash guesses wrong.
> 
> Sure, but what rules or heuristics does it follow?  Do we need to
> post-process the results or can we simply change the test case in t1504?

Mingw git assumes that it operates in a typical Windows environment -
drive-letter notation for absolute paths, and ';' separates paths in
PATH-like environment variables. If a test case breaks because bash
guesses wrongly, and does not transform the environment or command line
argument correctly, change the test case. (For example if /c:c was meant
as a two paths, then change it to /foo:bar, perhaps bash gets this right.)

Everything else, I don't know. Audit how sensible the test case is, and
whether it addresses a likely real-world case. If we don't care, skip it.

We try hard not to place Windows specific code outside of compat/.

> Is is_absolute_path() too forgiving on Windows, i.e. should it stop
> classifying paths starting with a slash as absolute on that platform?

No. /foo/bar is a valid "absolute" path Windows - even though it is
relative to the current drive. The point is that it is not relative to the
current directory.

-- Hannes

[PATCH] fix t1504 on Windows

[René Scharfe]

The test t1504 crashes on Windows due to a failed assertion in
normalize_absolute_path() because this function expects absolute paths to
start with a slash, while on Windows they can start with a drive letter
or a backslash.

As suggested by Johannes, fix it by using sanitary_path_copy() instead,
which can handle Windows-style paths just fine.  This patch just exports
it for usage; a later patch may move it from setup.c to path.c where it
fits better.

Secondly, use the portability macro PATH_SEP instead of expecting colons
to be used as path list delimiter.  On Windows, semicolons are used,
while colons separate drive letter and path.

Also change the test script to help bash recognize the path list variable
for test case "first_of_two" as needing path conversion.  git expects
Windows-style paths, which bash happily creates if we turn the second
path of the list into an absolute one.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 cache.h                 |    1 +
 path.c                  |   11 ++++++-----
 setup.c                 |    2 +-
 t/t1504-ceiling-dirs.sh |    2 +-
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/cache.h b/cache.h
index 45e713e..eeb774a 100644
--- a/cache.h
+++ b/cache.h
@@ -626,6 +626,7 @@ const char *make_nonrelative_path(const char *path);
 const char *make_relative_path(const char *abs, const char *base);
 int normalize_absolute_path(char *buf, const char *path);
 int longest_ancestor_length(const char *path, const char *prefix_list);
+int sanitary_path_copy(char *dst, const char *src);
 
 /* Read and unpack a sha1 file into memory, write memory to a sha1 file */
 extern int sha1_object_info(const unsigned char *, unsigned long *);
diff --git a/path.c b/path.c
index a074aea..ea1913a 100644
--- a/path.c
+++ b/path.c
@@ -438,15 +438,16 @@ int longest_ancestor_length(const char *path, const char *prefix_list)
 		return -1;
 
 	for (colon = ceil = prefix_list; *colon; ceil = colon+1) {
-		for (colon = ceil; *colon && *colon != ':'; colon++);
+		for (colon = ceil; *colon && *colon != PATH_SEP; colon++);
 		len = colon - ceil;
 		if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))
 			continue;
 		strlcpy(buf, ceil, len+1);
-		len = normalize_absolute_path(buf, buf);
-		/* Strip "trailing slashes" from "/". */
-		if (len == 1)
-			len = 0;
+		if (sanitary_path_copy(buf, buf) < 0)
+			continue;
+		len = strlen(buf);
+		if (buf[len - 1] == '/')
+			buf[--len] = '\0';
 
 		if (!strncmp(path, buf, len) &&
 		    path[len] == '/' &&
diff --git a/setup.c b/setup.c
index dfda532..4fe438c 100644
--- a/setup.c
+++ b/setup.c
@@ -4,7 +4,7 @@
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
 
-static int sanitary_path_copy(char *dst, const char *src)
+int sanitary_path_copy(char *dst, const char *src)
 {
 	char *dst0;
 
diff --git a/t/t1504-ceiling-dirs.sh b/t/t1504-ceiling-dirs.sh
index 91b704a..9c9c4c9 100755
--- a/t/t1504-ceiling-dirs.sh
+++ b/t/t1504-ceiling-dirs.sh
@@ -96,7 +96,7 @@ test_prefix subdir_ceil_at_subdi_slash "sub/dir/"
 GIT_CEILING_DIRECTORIES="foo:$TRASH_ROOT/sub"
 test_fail second_of_two
 
-GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub:bar"
+GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub:/bar"
 test_fail first_of_two
 
 GIT_CEILING_DIRECTORIES="foo:$TRASH_ROOT/sub:bar"
-- 
1.6.1.2

Re: [PATCH] fix crash in path.c on Windows

[Robin Rosenberg]

torsdag 05 februari 2009 17:48:34 skrev René Scharfe:
> Is is_absolute_path() too forgiving on Windows, i.e. should it stop
> classifying paths starting with a slash as absolute on that platform?

//server/share/file is an absolute path.

-- robin

Re: [PATCH] fix t1504 on Windows

[Johannes Sixt]

René Scharfe schrieb:
> The test t1504 crashes on Windows due to a failed assertion in
> normalize_absolute_path() because this function expects absolute paths to
> start with a slash, while on Windows they can start with a drive letter
> or a backslash.
...
> diff --git a/t/t1504-ceiling-dirs.sh b/t/t1504-ceiling-dirs.sh
> index 91b704a..9c9c4c9 100755
> --- a/t/t1504-ceiling-dirs.sh
> +++ b/t/t1504-ceiling-dirs.sh
> @@ -96,7 +96,7 @@ test_prefix subdir_ceil_at_subdi_slash "sub/dir/"
>  GIT_CEILING_DIRECTORIES="foo:$TRASH_ROOT/sub"
>  test_fail second_of_two
>  
> -GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub:bar"
> +GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub:/bar"
>  test_fail first_of_two
>  
>  GIT_CEILING_DIRECTORIES="foo:$TRASH_ROOT/sub:bar"

I also need this to complete this test:

diff --git a/t/t1504-ceiling-dirs.sh b/t/t1504-ceiling-dirs.sh
index 9c9c4c9..e377d48 100755
--- a/t/t1504-ceiling-dirs.sh
+++ b/t/t1504-ceiling-dirs.sh
@@ -93,13 +93,13 @@ GIT_CEILING_DIRECTORIES="$TRASH_ROOT/subdi"
 test_prefix subdir_ceil_at_subdi_slash "sub/dir/"


-GIT_CEILING_DIRECTORIES="foo:$TRASH_ROOT/sub"
+GIT_CEILING_DIRECTORIES="/foo:$TRASH_ROOT/sub"
 test_fail second_of_two

 GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub:/bar"
 test_fail first_of_two

-GIT_CEILING_DIRECTORIES="foo:$TRASH_ROOT/sub:bar"
+GIT_CEILING_DIRECTORIES="/foo:$TRASH_ROOT/sub:/bar"
 test_fail second_of_three



That said, I'm in the process of preparing a series that includes your
patch and that does the proper cleanup and code moving that you again
didn't do :-/ But it turns out that this is non-trivial because of bash's
(MSYS's) I-know-better-what-is-a-path-and-what-not behavior. It will take
some time...

-- Hannes

Re: [PATCH] fix t1504 on Windows

[Johannes Schindelin]

Hi,

On Fri, 6 Feb 2009, Johannes Sixt wrote:

> That said, I'm in the process of preparing a series that includes your 
> patch and that does the proper cleanup and code moving that you again 
> didn't do :-/ But it turns out that this is non-trivial because of 
> bash's (MSYS's) I-know-better-what-is-a-path-and-what-not behavior. It 
> will take some time...

Please be sure to test on msysGit, too, we have some important patches 
affecting the bash I-know-... behavior (IMHO improving things a lot).

(I seem to remember that you use your own MSys environment rather than the 
msysGit environment.)

Ciao,
Dscho

Re: [PATCH] fix t1504 on Windows

[Johannes Sixt]

Johannes Schindelin schrieb:
> Please be sure to test on msysGit, too, we have some important patches 
> affecting the bash I-know-... behavior (IMHO improving things a lot).

Sure, I'll do.

> (I seem to remember that you use your own MSys environment rather than the 
> msysGit environment.)

I switched to msysGit, the build environment, soon after you published it.
But I don't use rxvt or interactive bash.

-- Hannes

Re: [PATCH] fix t1504 on Windows

[Johannes Schindelin]

Hi,

On Fri, 6 Feb 2009, Johannes Sixt wrote:

> Johannes Schindelin schrieb:
> 
> > (I seem to remember that you use your own MSys environment rather than 
> > the msysGit environment.)
> 
> I switched to msysGit, the build environment, soon after you published 
> it. But I don't use rxvt or interactive bash.

Ah.

So the path mangling only hits you when you run a shell script, such as 
the tests?

Ciao,
Dscho

Re: [PATCH] fix t1504 on Windows

[Johannes Sixt]

Johannes Schindelin schrieb:
> So the path mangling only hits you when you run a shell script, such as 
> the tests?

Correct.

-- Hannes

Re: [PATCH] fix t1504 on Windows

[René Scharfe]

Johannes Sixt schrieb:
> I also need this to complete this test:
> 
> diff --git a/t/t1504-ceiling-dirs.sh b/t/t1504-ceiling-dirs.sh
> index 9c9c4c9..e377d48 100755
> --- a/t/t1504-ceiling-dirs.sh
> +++ b/t/t1504-ceiling-dirs.sh
> @@ -93,13 +93,13 @@ GIT_CEILING_DIRECTORIES="$TRASH_ROOT/subdi"
>  test_prefix subdir_ceil_at_subdi_slash "sub/dir/"
> 
> 
> -GIT_CEILING_DIRECTORIES="foo:$TRASH_ROOT/sub"
> +GIT_CEILING_DIRECTORIES="/foo:$TRASH_ROOT/sub"
>  test_fail second_of_two
> 
>  GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub:/bar"
>  test_fail first_of_two
> 
> -GIT_CEILING_DIRECTORIES="foo:$TRASH_ROOT/sub:bar"
> +GIT_CEILING_DIRECTORIES="/foo:$TRASH_ROOT/sub:/bar"
>  test_fail second_of_three

I don't, which makes me uneasy -- the tests shouldn't depend on details
in our setup. :-/

> That said, I'm in the process of preparing a series that includes your
> patch and that does the proper cleanup and code moving that you again
> didn't do :-/ But it turns out that this is non-trivial because of bash's
> (MSYS's) I-know-better-what-is-a-path-and-what-not behavior. It will take
> some time...

Glad to hear the first part, but what code moving do you mean?  Something
like the following?

-- snip! --
Remove the unused function normalize_absolute_path() and its tests.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 cache.h               |    1 -
 path.c                |   53 -------------------------------------------------
 t/t0060-path-utils.sh |   30 ---------------------------
 test-path-utils.c     |    7 ------
 4 files changed, 0 insertions(+), 91 deletions(-)

diff --git a/cache.h b/cache.h
index eeb774a..763bce7 100644
--- a/cache.h
+++ b/cache.h
@@ -624,7 +624,6 @@ int is_directory(const char *);
 const char *make_absolute_path(const char *path);
 const char *make_nonrelative_path(const char *path);
 const char *make_relative_path(const char *abs, const char *base);
-int normalize_absolute_path(char *buf, const char *path);
 int longest_ancestor_length(const char *path, const char *prefix_list);
 int sanitary_path_copy(char *dst, const char *src);
 
diff --git a/path.c b/path.c
index ea1913a..3fb9710 100644
--- a/path.c
+++ b/path.c
@@ -363,59 +363,6 @@ const char *make_relative_path(const char *abs, const char *base)
 }
 
 /*
- * path = absolute path
- * buf = buffer of at least max(2, strlen(path)+1) bytes
- * It is okay if buf == path, but they should not overlap otherwise.
- *
- * Performs the following normalizations on path, storing the result in buf:
- * - Removes trailing slashes.
- * - Removes empty components.
- * - Removes "." components.
- * - Removes ".." components, and the components the precede them.
- * "" and paths that contain only slashes are normalized to "/".
- * Returns the length of the output.
- *
- * Note that this function is purely textual.  It does not follow symlinks,
- * verify the existence of the path, or make any system calls.
- */
-int normalize_absolute_path(char *buf, const char *path)
-{
-	const char *comp_start = path, *comp_end = path;
-	char *dst = buf;
-	int comp_len;
-	assert(buf);
-	assert(path);
-
-	while (*comp_start) {
-		assert(*comp_start == '/');
-		while (*++comp_end && *comp_end != '/')
-			; /* nothing */
-		comp_len = comp_end - comp_start;
-
-		if (!strncmp("/",  comp_start, comp_len) ||
-		    !strncmp("/.", comp_start, comp_len))
-			goto next;
-
-		if (!strncmp("/..", comp_start, comp_len)) {
-			while (dst > buf && *--dst != '/')
-				; /* nothing */
-			goto next;
-		}
-
-		memmove(dst, comp_start, comp_len);
-		dst += comp_len;
-	next:
-		comp_start = comp_end;
-	}
-
-	if (dst == buf)
-		*dst++ = '/';
-
-	*dst = '\0';
-	return dst - buf;
-}
-
-/*
  * path = Canonical absolute path
  * prefix_list = Colon-separated list of absolute paths
  *
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 6e7501f..9498f72 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -7,41 +7,11 @@ test_description='Test various path utilities'
 
 . ./test-lib.sh
 
-norm_abs() {
-	test_expect_success "normalize absolute" \
-	"test \$(test-path-utils normalize_absolute_path '$1') = '$2'"
-}
-
 ancestor() {
 	test_expect_success "longest ancestor" \
 	"test \$(test-path-utils longest_ancestor_length '$1' '$2') = '$3'"
 }
 
-norm_abs "" /
-norm_abs / /
-norm_abs // /
-norm_abs /// /
-norm_abs /. /
-norm_abs /./ /
-norm_abs /./.. /
-norm_abs /../. /
-norm_abs /./../.// /
-norm_abs /dir/.. /
-norm_abs /dir/sub/../.. /
-norm_abs /dir /dir
-norm_abs /dir// /dir
-norm_abs /./dir /dir
-norm_abs /dir/. /dir
-norm_abs /dir///./ /dir
-norm_abs /dir//sub/.. /dir
-norm_abs /dir/sub/../ /dir
-norm_abs //dir/sub/../. /dir
-norm_abs /dir/s1/../s2/ /dir/s2
-norm_abs /d1/s1///s2/..//../s3/ /d1/s3
-norm_abs /d1/s1//../s2/../../d2 /d2
-norm_abs /d1/.../d2 /d1/.../d2
-norm_abs /d1/..././../d2 /d1/d2
-
 ancestor / "" -1
 ancestor / / -1
 ancestor /foo "" -1
diff --git a/test-path-utils.c b/test-path-utils.c
index 2c0f5a3..a470ee4 100644
--- a/test-path-utils.c
+++ b/test-path-utils.c
@@ -2,13 +2,6 @@
 
 int main(int argc, char **argv)
 {
-	if (argc == 3 && !strcmp(argv[1], "normalize_absolute_path")) {
-		char *buf = xmalloc(PATH_MAX + 1);
-		int rv = normalize_absolute_path(buf, argv[2]);
-		assert(strlen(buf) == rv);
-		puts(buf);
-	}
-
 	if (argc >= 2 && !strcmp(argv[1], "make_absolute_path")) {
 		while (argc > 2) {
 			puts(make_absolute_path(argv[2]));
-- 
1.6.1.2.350.g88cc

Re: [PATCH] fix t1504 on Windows

[Johannes Sixt]

René Scharfe schrieb:
> Johannes Sixt schrieb:
>> I also need this to complete this test:
>>
>> diff --git a/t/t1504-ceiling-dirs.sh b/t/t1504-ceiling-dirs.sh
>> index 9c9c4c9..e377d48 100755
>> --- a/t/t1504-ceiling-dirs.sh
>> +++ b/t/t1504-ceiling-dirs.sh
>> @@ -93,13 +93,13 @@ GIT_CEILING_DIRECTORIES="$TRASH_ROOT/subdi"
>>  test_prefix subdir_ceil_at_subdi_slash "sub/dir/"
>>
>>
>> -GIT_CEILING_DIRECTORIES="foo:$TRASH_ROOT/sub"
>> +GIT_CEILING_DIRECTORIES="/foo:$TRASH_ROOT/sub"
>>  test_fail second_of_two
>>
>>  GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub:/bar"
>>  test_fail first_of_two
>>
>> -GIT_CEILING_DIRECTORIES="foo:$TRASH_ROOT/sub:bar"
>> +GIT_CEILING_DIRECTORIES="/foo:$TRASH_ROOT/sub:/bar"
>>  test_fail second_of_three
> 
> I don't, which makes me uneasy -- the tests shouldn't depend on details
> in our setup. :-/

I updated my msysgit to the current master this moment, and I still need 
these two changes. Do you use an older msysgit? With 31d5dfeb0 
(2008-06-08) Steffen changed path mangling so that a text without a 
leading / or . is not converted. That's exactly what we see here.

>> That said, I'm in the process of preparing a series that includes your
>> patch and that does the proper cleanup and code moving that you again
>> didn't do :-/ But it turns out that this is non-trivial because of bash's
>> (MSYS's) I-know-better-what-is-a-path-and-what-not behavior. It will take
>> some time...
> 
> Glad to hear the first part, but what code moving do you mean?  Something
> like the following?
> 
> -- snip! --
> Remove the unused function normalize_absolute_path() and its tests.

No, that's not enough. I mean something more like this (but I'll not 
include the diff itself):

Johannes Sixt (4):
       Make test-path-utils more robust against incorrect use
       Move sanitary_path_copy to path.c (and rename to
         normalize_path_copy)
       Test and fix normalize_path_copy()
       Remove unused normalize_absolute_path()

René Scharfe (1):
       Fix t1504 on Windows

  cache.h                 |    2 +-
  path.c                  |  124 +++++++++++++++++++++++++------------
  setup.c                 |   88 +--------------------------
  t/t0060-path-utils.sh   |   26 ++++----
  t/t1504-ceiling-dirs.sh |    2 +-
  test-path-utils.c       |   12 +++-
  6 files changed, 107 insertions(+), 147 deletions(-)

-- Hannes

Re: [PATCH] fix t1504 on Windows

[René Scharfe]

Johannes Sixt schrieb:
> René Scharfe schrieb:
>> Johannes Sixt schrieb:
>>> -GIT_CEILING_DIRECTORIES="foo:$TRASH_ROOT/sub"
>>> +GIT_CEILING_DIRECTORIES="/foo:$TRASH_ROOT/sub"
>>>  test_fail second_of_two
>>>
>>>  GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub:/bar"
>>>  test_fail first_of_two
>>>
>>> -GIT_CEILING_DIRECTORIES="foo:$TRASH_ROOT/sub:bar"
>>> +GIT_CEILING_DIRECTORIES="/foo:$TRASH_ROOT/sub:/bar"
>>>  test_fail second_of_three
>>
>> I don't, which makes me uneasy -- the tests shouldn't depend on details
>> in our setup. :-/
> 
> I updated my msysgit to the current master this moment, and I still need
> these two changes. Do you use an older msysgit? With 31d5dfeb0
> (2008-06-08) Steffen changed path mangling so that a text without a
> leading / or . is not converted. That's exactly what we see here.

Oh, yes, good idea.  I never updated the msys part of my installation.

My "uneasiness" doesn't mean that I'm against your changes above, in any
case.

>>> That said, I'm in the process of preparing a series that includes your
>>> patch and that does the proper cleanup and code moving that you again
>>> didn't do :-/ But it turns out that this is non-trivial because of
>>> bash's
>>> (MSYS's) I-know-better-what-is-a-path-and-what-not behavior. It will
>>> take
>>> some time...

> Johannes Sixt (4):
>       Make test-path-utils more robust against incorrect use
>       Move sanitary_path_copy to path.c (and rename to
>         normalize_path_copy)
>       Test and fix normalize_path_copy()
>       Remove unused normalize_absolute_path()

Yeah, OK.  I was aiming for a quick short fix, deferring cleanups to
when I have more time.  I wonder what patches one and three do, though.

René

Re: [PATCH] fix t1504 on Windows

[René Scharfe]

Johannes Sixt schrieb:
> I also need this to complete this test:
> 
> diff --git a/t/t1504-ceiling-dirs.sh b/t/t1504-ceiling-dirs.sh
> index 9c9c4c9..e377d48 100755
> --- a/t/t1504-ceiling-dirs.sh
> +++ b/t/t1504-ceiling-dirs.sh
> @@ -93,13 +93,13 @@ GIT_CEILING_DIRECTORIES="$TRASH_ROOT/subdi"
>  test_prefix subdir_ceil_at_subdi_slash "sub/dir/"
> 
> 
> -GIT_CEILING_DIRECTORIES="foo:$TRASH_ROOT/sub"
> +GIT_CEILING_DIRECTORIES="/foo:$TRASH_ROOT/sub"
>  test_fail second_of_two
> 
>  GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub:/bar"
>  test_fail first_of_two
> 
> -GIT_CEILING_DIRECTORIES="foo:$TRASH_ROOT/sub:bar"
> +GIT_CEILING_DIRECTORIES="/foo:$TRASH_ROOT/sub:/bar"
>  test_fail second_of_three
> 

Just to confirm: after updating msys, I also need this patch to make
t1504 pass.

René

[PATCH 0/5] Consolidate path normalization functions

[Johannes Sixt]

This series:

 - moves sanitary_path_copy around, which accounts for the bulk
   of the change;

 - updates longest_ancestor_length(), the core function of
   GIT_CEILING_DIRECTORIES;

 - adjust the test suite, uncovers a buglet in sanitary_path_copy(),
   and fixes it.

 - removes normalize_absolute_path() that duplicated functionality
   of sanitary_path_copy().

As a side effect, this fixes GIT_CEILING_DIRECTORIES on Windows, where
this feature is currently broken.

Johannes Sixt (4):
  Make test-path-utils more robust against incorrect use
  Move sanitary_path_copy() to path.c and rename it to
    normalize_path_copy()
  Test and fix normalize_path_copy()
  Remove unused normalize_absolute_path()

René Scharfe (1):
  Fix t1504 on Windows

 cache.h                 |    2 +-
 path.c                  |  124 ++++++++++++++++++++++++++-------------
 setup.c                 |   88 +---------------------------
 t/t0060-path-utils.sh   |   33 +++++-----
 t/t1504-ceiling-dirs.sh |    6 +-
 test-path-utils.c       |   14 +++-
 6 files changed, 115 insertions(+), 152 deletions(-)

[PATCH 1/5] Make test-path-utils more robust against incorrect use

[Johannes Sixt]

Previously, this test utility happily returned with exit code 0 if garbage
was thrown at it. Now it reports failure if an unknown function name was
given on the command line.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 test-path-utils.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/test-path-utils.c b/test-path-utils.c
index 2c0f5a3..7e6fc8d 100644
--- a/test-path-utils.c
+++ b/test-path-utils.c
@@ -7,6 +7,7 @@ int main(int argc, char **argv)
 		int rv = normalize_absolute_path(buf, argv[2]);
 		assert(strlen(buf) == rv);
 		puts(buf);
+		return 0;
 	}
 
 	if (argc >= 2 && !strcmp(argv[1], "make_absolute_path")) {
@@ -15,12 +16,16 @@ int main(int argc, char **argv)
 			argc--;
 			argv++;
 		}
+		return 0;
 	}
 
 	if (argc == 4 && !strcmp(argv[1], "longest_ancestor_length")) {
 		int len = longest_ancestor_length(argv[2], argv[3]);
 		printf("%d\n", len);
+		return 0;
 	}
 
-	return 0;
+	fprintf(stderr, "%s: unknown function name: %s\n", argv[0],
+		argv[1] ? argv[1] : "(there was none)");
+	return 1;
 }
-- 
1.6.1.297.g9b01e

[PATCH 2/5] Move sanitary_path_copy() to path.c and rename it to normalize_path_copy()

[Johannes Sixt]

This function and normalize_absolute_path() do almost the same thing. The
former already works on Windows, but the latter crashes.

In subsequent changes we will remove normalize_absolute_path(). Here we
make the replacement function reusable. On the way we rename it to reflect
that it does some path normalization. Apart from that this is only moving
around code.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 cache.h |    1 +
 path.c  |   86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 setup.c |   88 +--------------------------------------------------------------
 3 files changed, 88 insertions(+), 87 deletions(-)

diff --git a/cache.h b/cache.h
index 2d889de..82fbb80 100644
--- a/cache.h
+++ b/cache.h
@@ -628,6 +628,7 @@ const char *make_absolute_path(const char *path);
 const char *make_nonrelative_path(const char *path);
 const char *make_relative_path(const char *abs, const char *base);
 int normalize_absolute_path(char *buf, const char *path);
+int normalize_path_copy(char *dst, const char *src);
 int longest_ancestor_length(const char *path, const char *prefix_list);
 
 /* Read and unpack a sha1 file into memory, write memory to a sha1 file */
diff --git a/path.c b/path.c
index 108d9e9..d0a3519 100644
--- a/path.c
+++ b/path.c
@@ -415,6 +415,92 @@ int normalize_absolute_path(char *buf, const char *path)
 	return dst - buf;
 }
 
+int normalize_path_copy(char *dst, const char *src)
+{
+	char *dst0;
+
+	if (has_dos_drive_prefix(src)) {
+		*dst++ = *src++;
+		*dst++ = *src++;
+	}
+	dst0 = dst;
+
+	if (is_dir_sep(*src)) {
+		*dst++ = '/';
+		while (is_dir_sep(*src))
+			src++;
+	}
+
+	for (;;) {
+		char c = *src;
+
+		/*
+		 * A path component that begins with . could be
+		 * special:
+		 * (1) "." and ends   -- ignore and terminate.
+		 * (2) "./"           -- ignore them, eat slash and continue.
+		 * (3) ".." and ends  -- strip one and terminate.
+		 * (4) "../"          -- strip one, eat slash and continue.
+		 */
+		if (c == '.') {
+			if (!src[1]) {
+				/* (1) */
+				src++;
+			} else if (is_dir_sep(src[1])) {
+				/* (2) */
+				src += 2;
+				while (is_dir_sep(*src))
+					src++;
+				continue;
+			} else if (src[1] == '.') {
+				if (!src[2]) {
+					/* (3) */
+					src += 2;
+					goto up_one;
+				} else if (is_dir_sep(src[2])) {
+					/* (4) */
+					src += 3;
+					while (is_dir_sep(*src))
+						src++;
+					goto up_one;
+				}
+			}
+		}
+
+		/* copy up to the next '/', and eat all '/' */
+		while ((c = *src++) != '\0' && !is_dir_sep(c))
+			*dst++ = c;
+		if (is_dir_sep(c)) {
+			*dst++ = '/';
+			while (is_dir_sep(c))
+				c = *src++;
+			src--;
+		} else if (!c)
+			break;
+		continue;
+
+	up_one:
+		/*
+		 * dst0..dst is prefix portion, and dst[-1] is '/';
+		 * go up one level.
+		 */
+		dst -= 2; /* go past trailing '/' if any */
+		if (dst < dst0)
+			return -1;
+		while (1) {
+			if (dst <= dst0)
+				break;
+			c = *dst--;
+			if (c == '/') {	/* MinGW: cannot be '\\' anymore */
+				dst += 2;
+				break;
+			}
+		}
+	}
+	*dst = '\0';
+	return 0;
+}
+
 /*
  * path = Canonical absolute path
  * prefix_list = Colon-separated list of absolute paths
diff --git a/setup.c b/setup.c
index dfda532..6c2deda 100644
--- a/setup.c
+++ b/setup.c
@@ -4,92 +4,6 @@
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
 
-static int sanitary_path_copy(char *dst, const char *src)
-{
-	char *dst0;
-
-	if (has_dos_drive_prefix(src)) {
-		*dst++ = *src++;
-		*dst++ = *src++;
-	}
-	dst0 = dst;
-
-	if (is_dir_sep(*src)) {
-		*dst++ = '/';
-		while (is_dir_sep(*src))
-			src++;
-	}
-
-	for (;;) {
-		char c = *src;
-
-		/*
-		 * A path component that begins with . could be
-		 * special:
-		 * (1) "." and ends   -- ignore and terminate.
-		 * (2) "./"           -- ignore them, eat slash and continue.
-		 * (3) ".." and ends  -- strip one and terminate.
-		 * (4) "../"          -- strip one, eat slash and continue.
-		 */
-		if (c == '.') {
-			if (!src[1]) {
-				/* (1) */
-				src++;
-			} else if (is_dir_sep(src[1])) {
-				/* (2) */
-				src += 2;
-				while (is_dir_sep(*src))
-					src++;
-				continue;
-			} else if (src[1] == '.') {
-				if (!src[2]) {
-					/* (3) */
-					src += 2;
-					goto up_one;
-				} else if (is_dir_sep(src[2])) {
-					/* (4) */
-					src += 3;
-					while (is_dir_sep(*src))
-						src++;
-					goto up_one;
-				}
-			}
-		}
-
-		/* copy up to the next '/', and eat all '/' */
-		while ((c = *src++) != '\0' && !is_dir_sep(c))
-			*dst++ = c;
-		if (is_dir_sep(c)) {
-			*dst++ = '/';
-			while (is_dir_sep(c))
-				c = *src++;
-			src--;
-		} else if (!c)
-			break;
-		continue;
-
-	up_one:
-		/*
-		 * dst0..dst is prefix portion, and dst[-1] is '/';
-		 * go up one level.
-		 */
-		dst -= 2; /* go past trailing '/' if any */
-		if (dst < dst0)
-			return -1;
-		while (1) {
-			if (dst <= dst0)
-				break;
-			c = *dst--;
-			if (c == '/') {	/* MinGW: cannot be '\\' anymore */
-				dst += 2;
-				break;
-			}
-		}
-	}
-	*dst = '\0';
-	return 0;
-}
-
 const char *prefix_path(const char *prefix, int len, const char *path)
 {
 	const char *orig = path;
@@ -101,7 +15,7 @@ const char *prefix_path(const char *prefix, int len, const char *path)
 			memcpy(sanitized, prefix, len);
 		strcpy(sanitized + len, path);
 	}
-	if (sanitary_path_copy(sanitized, sanitized))
+	if (normalize_path_copy(sanitized, sanitized))
 		goto error_out;
 	if (is_absolute_path(orig)) {
 		const char *work_tree = get_git_work_tree();
-- 
1.6.1.297.g9b01e

[PATCH 3/5] Fix GIT_CEILING_DIRECTORIES on Windows

[Johannes Sixt]

From: René Scharfe <rene.scharfe@lsrfire.ath.cx>

Using git with GIT_CEILING_DIRECTORIES crashed on Windows due to a failed
assertion in normalize_absolute_path(): This function expects absolute
paths to start with a slash, while on Windows they can start with a drive
letter or a backslash.

This fixes it by using the alternative, normalize_path_copy() instead,
which can handle Windows-style paths just fine.

Secondly, the portability macro PATH_SEP is used instead of expecting
colons to be used as path list delimiter.

The test script t1504 is also changed to help MSYS's bash recognize some
program arguments as path list. (MSYS's bash must translate POSIX-style
path lists to Windows-style path lists, and the heuristic did not catch
some cases.)

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
	I have adjusted the patch to fit into the series.
	I modified the commit message to pretend that the goal was
	to fix a feature instead of the test case ;) Hope that is
	fine with you.

	-- Hannes

 path.c                  |   11 ++++++-----
 t/t1504-ceiling-dirs.sh |    6 +++---
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/path.c b/path.c
index d0a3519..dc3807a 100644
--- a/path.c
+++ b/path.c
@@ -524,15 +524,16 @@ int longest_ancestor_length(const char *path, const char *prefix_list)
 		return -1;
 
 	for (colon = ceil = prefix_list; *colon; ceil = colon+1) {
-		for (colon = ceil; *colon && *colon != ':'; colon++);
+		for (colon = ceil; *colon && *colon != PATH_SEP; colon++);
 		len = colon - ceil;
 		if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))
 			continue;
 		strlcpy(buf, ceil, len+1);
-		len = normalize_absolute_path(buf, buf);
-		/* Strip "trailing slashes" from "/". */
-		if (len == 1)
-			len = 0;
+		if (normalize_path_copy(buf, buf) < 0)
+			continue;
+		len = strlen(buf);
+		if (len > 0 && buf[len-1] == '/')
+			buf[--len] = '\0';
 
 		if (!strncmp(path, buf, len) &&
 		    path[len] == '/' &&
diff --git a/t/t1504-ceiling-dirs.sh b/t/t1504-ceiling-dirs.sh
index 91b704a..e377d48 100755
--- a/t/t1504-ceiling-dirs.sh
+++ b/t/t1504-ceiling-dirs.sh
@@ -93,13 +93,13 @@ GIT_CEILING_DIRECTORIES="$TRASH_ROOT/subdi"
 test_prefix subdir_ceil_at_subdi_slash "sub/dir/"
 
 
-GIT_CEILING_DIRECTORIES="foo:$TRASH_ROOT/sub"
+GIT_CEILING_DIRECTORIES="/foo:$TRASH_ROOT/sub"
 test_fail second_of_two
 
-GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub:bar"
+GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub:/bar"
 test_fail first_of_two
 
-GIT_CEILING_DIRECTORIES="foo:$TRASH_ROOT/sub:bar"
+GIT_CEILING_DIRECTORIES="/foo:$TRASH_ROOT/sub:/bar"
 test_fail second_of_three
 
 
-- 
1.6.1.297.g9b01e

[PATCH 4/5] Test and fix normalize_path_copy()

[Johannes Sixt]

This changes the test-path-utils utility to invoke normalize_path_copy()
instead of normalize_absolute_path() because the latter is about to be
removed.

The test cases in t0060 are adjusted in two regards:

- normalize_path_copy() more often leaves a trailing slash in the result.
  This has no negative side effects because the new user of this function,
  longest_ancester_length(), already accounts for this behavior.

- The function can fail.

The tests uncover a flaw in normalize_path_copy(): If there are
sufficiently many '..' path components so that the root is reached, such as
in "/d1/s1/../../d2", then the leading slash was lost. This manifested
itself that (assuming there is a repository at /tmp/foo)

  $ git add /d1/../tmp/foo/some-file

reported 'pathspec is outside repository'. This is now fixed.

Moreover, the test case descriptions of t0060 now include the test data and
expected outcome.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 path.c                |   16 +++++-----------
 t/t0060-path-utils.sh |   33 +++++++++++++++++----------------
 test-path-utils.c     |    7 ++++---
 3 files changed, 26 insertions(+), 30 deletions(-)

diff --git a/path.c b/path.c
index dc3807a..16628b2 100644
--- a/path.c
+++ b/path.c
@@ -484,18 +484,12 @@ int normalize_path_copy(char *dst, const char *src)
 		 * dst0..dst is prefix portion, and dst[-1] is '/';
 		 * go up one level.
 		 */
-		dst -= 2; /* go past trailing '/' if any */
-		if (dst < dst0)
+		dst--;	/* go to trailing '/' */
+		if (dst <= dst0)
 			return -1;
-		while (1) {
-			if (dst <= dst0)
-				break;
-			c = *dst--;
-			if (c == '/') {	/* MinGW: cannot be '\\' anymore */
-				dst += 2;
-				break;
-			}
-		}
+		/* Windows: dst[-1] cannot be backslash anymore */
+		while (dst0 < dst && dst[-1] != '/')
+			dst--;
 	}
 	*dst = '\0';
 	return 0;
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 6e7501f..4ed1f0b 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -8,36 +8,37 @@ test_description='Test various path utilities'
 . ./test-lib.sh
 
 norm_abs() {
-	test_expect_success "normalize absolute" \
-	"test \$(test-path-utils normalize_absolute_path '$1') = '$2'"
+	test_expect_success "normalize absolute: $1 => $2" \
+	"test \"\$(test-path-utils normalize_path_copy '$1')\" = '$2'"
 }
 
 ancestor() {
-	test_expect_success "longest ancestor" \
-	"test \$(test-path-utils longest_ancestor_length '$1' '$2') = '$3'"
+	test_expect_success "longest ancestor: $1 $2 => $3" \
+	"test \"\$(test-path-utils longest_ancestor_length '$1' '$2')\" = '$3'"
 }
 
-norm_abs "" /
+norm_abs "" ""
 norm_abs / /
 norm_abs // /
 norm_abs /// /
 norm_abs /. /
 norm_abs /./ /
-norm_abs /./.. /
-norm_abs /../. /
-norm_abs /./../.// /
+norm_abs /./.. ++failed++
+norm_abs /../. ++failed++
+norm_abs /./../.// ++failed++
 norm_abs /dir/.. /
 norm_abs /dir/sub/../.. /
+norm_abs /dir/sub/../../.. ++failed++
 norm_abs /dir /dir
-norm_abs /dir// /dir
+norm_abs /dir// /dir/
 norm_abs /./dir /dir
-norm_abs /dir/. /dir
-norm_abs /dir///./ /dir
-norm_abs /dir//sub/.. /dir
-norm_abs /dir/sub/../ /dir
-norm_abs //dir/sub/../. /dir
-norm_abs /dir/s1/../s2/ /dir/s2
-norm_abs /d1/s1///s2/..//../s3/ /d1/s3
+norm_abs /dir/. /dir/
+norm_abs /dir///./ /dir/
+norm_abs /dir//sub/.. /dir/
+norm_abs /dir/sub/../ /dir/
+norm_abs //dir/sub/../. /dir/
+norm_abs /dir/s1/../s2/ /dir/s2/
+norm_abs /d1/s1///s2/..//../s3/ /d1/s3/
 norm_abs /d1/s1//../s2/../../d2 /d2
 norm_abs /d1/.../d2 /d1/.../d2
 norm_abs /d1/..././../d2 /d1/d2
diff --git a/test-path-utils.c b/test-path-utils.c
index 7e6fc8d..5168a8e 100644
--- a/test-path-utils.c
+++ b/test-path-utils.c
@@ -2,10 +2,11 @@
 
 int main(int argc, char **argv)
 {
-	if (argc == 3 && !strcmp(argv[1], "normalize_absolute_path")) {
+	if (argc == 3 && !strcmp(argv[1], "normalize_path_copy")) {
 		char *buf = xmalloc(PATH_MAX + 1);
-		int rv = normalize_absolute_path(buf, argv[2]);
-		assert(strlen(buf) == rv);
+		int rv = normalize_path_copy(buf, argv[2]);
+		if (rv)
+			buf = "++failed++";
 		puts(buf);
 		return 0;
 	}
-- 
1.6.1.297.g9b01e

[PATCH 5/5] Remove unused normalize_absolute_path()

[Johannes Sixt]

This function is now superseded by normalize_path_copy().

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 cache.h |    1 -
 path.c  |   51 ++++++---------------------------------------------
 2 files changed, 6 insertions(+), 46 deletions(-)

diff --git a/cache.h b/cache.h
index 82fbb80..8dcc53c 100644
--- a/cache.h
+++ b/cache.h
@@ -627,7 +627,6 @@ int is_directory(const char *);
 const char *make_absolute_path(const char *path);
 const char *make_nonrelative_path(const char *path);
 const char *make_relative_path(const char *abs, const char *base);
-int normalize_absolute_path(char *buf, const char *path);
 int normalize_path_copy(char *dst, const char *src);
 int longest_ancestor_length(const char *path, const char *prefix_list);
 
diff --git a/path.c b/path.c
index 16628b2..dd22370 100644
--- a/path.c
+++ b/path.c
@@ -363,58 +363,19 @@ const char *make_relative_path(const char *abs, const char *base)
 }
 
 /*
- * path = absolute path
- * buf = buffer of at least max(2, strlen(path)+1) bytes
- * It is okay if buf == path, but they should not overlap otherwise.
+ * It is okay if dst == src, but they should not overlap otherwise.
  *
- * Performs the following normalizations on path, storing the result in buf:
- * - Removes trailing slashes.
- * - Removes empty components.
+ * Performs the following normalizations on src, storing the result in dst:
+ * - Ensures that components are separated by '/' (Windows only)
+ * - Squashes sequences of '/'.
  * - Removes "." components.
  * - Removes ".." components, and the components the precede them.
- * "" and paths that contain only slashes are normalized to "/".
- * Returns the length of the output.
+ * Returns failure (non-zero) if a ".." component appears as first path
+ * component anytime during the normalization. Otherwise, returns success (0).
  *
  * Note that this function is purely textual.  It does not follow symlinks,
  * verify the existence of the path, or make any system calls.
  */
-int normalize_absolute_path(char *buf, const char *path)
-{
-	const char *comp_start = path, *comp_end = path;
-	char *dst = buf;
-	int comp_len;
-	assert(buf);
-	assert(path);
-
-	while (*comp_start) {
-		assert(*comp_start == '/');
-		while (*++comp_end && *comp_end != '/')
-			; /* nothing */
-		comp_len = comp_end - comp_start;
-
-		if (!strncmp("/",  comp_start, comp_len) ||
-		    !strncmp("/.", comp_start, comp_len))
-			goto next;
-
-		if (!strncmp("/..", comp_start, comp_len)) {
-			while (dst > buf && *--dst != '/')
-				; /* nothing */
-			goto next;
-		}
-
-		memmove(dst, comp_start, comp_len);
-		dst += comp_len;
-	next:
-		comp_start = comp_end;
-	}
-
-	if (dst == buf)
-		*dst++ = '/';
-
-	*dst = '\0';
-	return dst - buf;
-}
-
 int normalize_path_copy(char *dst, const char *src)
 {
 	char *dst0;
-- 
1.6.1.297.g9b01e

Re: [PATCH 4/5] Test and fix normalize_path_copy()

[Robin Rosenberg]

For Windows leading // is special indicating a UNC
path and should not be changed to /.

//foo/bar is different from /foo/bar

-- robin

Re: [PATCH 4/5] Test and fix normalize_path_copy()

[Johannes Sixt]

On Sonntag, 8. Februar 2009, Robin Rosenberg wrote:
> For Windows leading // is special indicating a UNC
> path and should not be changed to /.
>
> //foo/bar is different from /foo/bar

Indeed. But stripping the double-dash would bite you only if you are using 
GIT_CEILING_DIRECTORIES or if you tend to call eg. git add with absolute 
paths; and that only if your repository lives in a UNC share, which is a 
rather odd use-case.

In particular, it does *not* affect fetch and push etc. so that you can have a 
remote repository on an UNC share.

-- Hannes

Re: [PATCH 4/5] Test and fix normalize_path_copy()

[René Scharfe]

Johannes Sixt schrieb:
> Moreover, the test case descriptions of t0060 now include the test data and
> expected outcome.

The test still fails in a lot of cases for me, because "/" is translated to
the installation path of msys.  E.g. with the patch at the end, which shows
the results in addition to input and expected output, I get several cases
like this (the first test call checks the return code):

* FAIL 2: normalize absolute: '/' => '/'
	test 0 = 0 && test 'C:/Program Files (x86)/msysgit/msysgit' = '/'

* FAIL 32: longest ancestor: '/foo' '/' => '0'
	test 0 = 0 && test '38' = '0'

I'm not sure what to do about it.  Should test-path-utils translate it back
into "root is / is root"?  Should the test script look up "/" at the top
and just append this value to all paths?  Unpretty.


And then there's this:

* FAIL 7: normalize absolute: '/./..' => '++failed++'
	test 0 = 0 && test 'C:/Program Files (x86)/msysgit/msysgit' = '++failed++'
*   ok 8: normalize absolute: '/../.' => '++failed++'

A printf() added to the top of normalize_path_copy() tells me that you can
add as many "/.." as you want after a path starting with "/.", it will
always translate into the install directory of msys regardless.


Anyway, here's the patch mentioned above:

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 4ed1f0b..d5a38a6 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -8,13 +8,17 @@ test_description='Test various path utilities'
 . ./test-lib.sh
 
 norm_abs() {
-	test_expect_success "normalize absolute: $1 => $2" \
-	"test \"\$(test-path-utils normalize_path_copy '$1')\" = '$2'"
+	result=$(test-path-utils normalize_path_copy "$1")
+	rc=$?
+	test_expect_success "normalize absolute: '$1' => '$2'" \
+	"test $rc = 0 && test '$result' = '$2'"
 }
 
 ancestor() {
-	test_expect_success "longest ancestor: $1 $2 => $3" \
-	"test \"\$(test-path-utils longest_ancestor_length '$1' '$2')\" = '$3'"
+	result=$(test-path-utils longest_ancestor_length "$1" "$2")
+	rc=$?
+	test_expect_success "longest ancestor: '$1' '$2' => '$3'" \
+	"test $rc = 0 && test '$result' = '$3'"
 }
 
 norm_abs "" ""

Re: [PATCH 4/5] Test and fix normalize_path_copy()

[Johannes Sixt]

On Sonntag, 8. Februar 2009, René Scharfe wrote:
> Johannes Sixt schrieb:
> > Moreover, the test case descriptions of t0060 now include the test data
> > and expected outcome.
>
> The test still fails in a lot of cases for me, because "/" is translated to
> the installation path of msys.

To fix t0060 on Windows was not even on the radar of this series.

That said, I'm slowly working on a series that fixes or skips many tests on 
Windows. I currently have the patch below in my tree.

-- Hannes

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 4ed1f0b..dd2d241 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -7,41 +7,89 @@ test_description='Test various path utilities'
 
 . ./test-lib.sh
 
-norm_abs() {
-	test_expect_success "normalize absolute: $1 => $2" \
+norm_path() {
+	test_expect_success "normalize path: $1 => $2" \
 	"test \"\$(test-path-utils normalize_path_copy '$1')\" = '$2'"
 }
+xnorm_path() { say fix this: "$*"; }
+
+# On Windows, we are using MSYS's bash, which mangles the paths.
+# Absolute paths are anchored at the MSYS installation directory,
+# which means that the path / accounts for this many characters:
+rootoff=$(test-path-utils normalize_path_copy / | wc -c)
+# Account for the trailing LF:
+if test "$rootoff" = 2; then
+	rootoff=	# we are on Unix
+else
+	rootoff=$(($rootoff-1))
+fi
 
 ancestor() {
-	test_expect_success "longest ancestor: $1 $2 => $3" \
-	"test \"\$(test-path-utils longest_ancestor_length '$1' '$2')\" = '$3'"
+	# We do some math with the expected ancestor length.
+	expected=$3
+	if test -n "$rootoff" && ! test "$expected" = -1; then
+		expected=$(($expected+$rootoff))
+	fi
+	test_expect_success "longest ancestor: $1 $2 => $expected" \
+	"actual=\$(test-path-utils longest_ancestor_length '$1' '$2') &&
+	 test \"\$actual\" = '$expected'"
 }
 
-norm_abs "" ""
-norm_abs / /
-norm_abs // /
-norm_abs /// /
-norm_abs /. /
-norm_abs /./ /
-norm_abs /./.. ++failed++
-norm_abs /../. ++failed++
-norm_abs /./../.// ++failed++
-norm_abs /dir/.. /
-norm_abs /dir/sub/../.. /
-norm_abs /dir/sub/../../.. ++failed++
-norm_abs /dir /dir
-norm_abs /dir// /dir/
-norm_abs /./dir /dir
-norm_abs /dir/. /dir/
-norm_abs /dir///./ /dir/
-norm_abs /dir//sub/.. /dir/
-norm_abs /dir/sub/../ /dir/
-norm_abs //dir/sub/../. /dir/
-norm_abs /dir/s1/../s2/ /dir/s2/
-norm_abs /d1/s1///s2/..//../s3/ /d1/s3/
-norm_abs /d1/s1//../s2/../../d2 /d2
-norm_abs /d1/.../d2 /d1/.../d2
-norm_abs /d1/..././../d2 /d1/d2
+do_abs=t
+case $(uname -s) in
+*MINGW*) do_abs=
+esac
+
+norm_path "" ""
+norm_path . ""
+norm_path ./ ""
+norm_path ./. ""
+norm_path .. ++failed++
+norm_path ../sub ++failed++
+norm_path dir/.. ""
+norm_path dir/sub/../.. ""
+norm_path dir/sub/../../.. ++failed++
+norm_path dir dir
+norm_path dir// dir/
+norm_path ./dir dir
+norm_path dir/. dir/
+norm_path dir///./ dir/
+norm_path dir//sub/.. dir/
+norm_path dir/sub/../ dir/
+norm_path dir/sub/../. dir/
+norm_path dir/s1/../s2/ dir/s2/
+norm_path d1/s1///s2/..//../s3/ d1/s3/
+norm_path d1/s1//../s2/../../d2 d2
+norm_path d1/.../d2 d1/.../d2
+norm_path d1/..././../d2 d1/d2
+
+if test -n "$do_abs"; then
+norm_path "" ""
+norm_path / /
+norm_path // /
+norm_path /// /
+norm_path /. /
+norm_path /./ /
+norm_path /./.. ++failed++
+norm_path /../. ++failed++
+norm_path /./../.// ++failed++
+norm_path /dir/.. /
+norm_path /dir/sub/../.. /
+norm_path /dir/sub/../../.. ++failed++
+norm_path /dir /dir
+norm_path /dir// /dir/
+norm_path /./dir /dir
+norm_path /dir/. /dir/
+norm_path /dir///./ /dir/
+norm_path /dir//sub/.. /dir/
+norm_path /dir/sub/../ /dir/
+norm_path //dir/sub/../. /dir/
+norm_path /dir/s1/../s2/ /dir/s2/
+norm_path /d1/s1///s2/..//../s3/ /d1/s3/
+norm_path /d1/s1//../s2/../../d2 /d2
+norm_path /d1/.../d2 /d1/.../d2
+norm_path /d1/..././../d2 /d1/d2
+fi
 
 ancestor / "" -1
 ancestor / / -1
@@ -80,9 +128,9 @@ ancestor /foo/bar /:/foo:/bar/ 4
 ancestor /foo/bar /foo:/:/bar/ 4
 ancestor /foo/bar /:/bar/:/fo 0
 ancestor /foo/bar /:/bar/ 0
-ancestor /foo/bar :://foo/. 4
-ancestor /foo/bar :://foo/.:: 4
-ancestor /foo/bar //foo/./::/bar 4
-ancestor /foo/bar ::/bar -1
+ancestor /foo/bar .:/foo/. 4
+ancestor /foo/bar .:/foo/.:.: 4
+ancestor /foo/bar /foo/./:.:/bar 4
+ancestor /foo/bar .:/bar -1
 
 test_done