[PATCH] Handle double slashes in make_relative_path()

[PATCH] Handle double slashes in make_relative_path()

[Thomas Rast]

If you say

  git --git-dir=/some//path --work-tree=/some/ add <somefile>

then setup_work_tree() will call into make_relative_path() with
abs="/some//path" and base="/some".  (Note how the latter has already
lost its trailing slash.  One unfortunate user managed to trigger this
because his $HOME ended in a slash.)

This means that when checking whether 'abs' is a path under 'base', we
need to skip *two* slashes where the previous code only accounted for
one.  Fix it to handle an arbitrary number of slashes at that
position.

Noticed-by: eldenz on freenode
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 path.c              |    6 +++---
 t/t1501-worktree.sh |    6 ++++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/path.c b/path.c
index 2ec950b..a195bab 100644
--- a/path.c
+++ b/path.c
@@ -400,10 +400,10 @@ int set_shared_perm(const char *path, int mode)
 	baselen = strlen(base);
 	if (prefixcmp(abs, base))
 		return abs;
-	if (abs[baselen] == '/')
-		baselen++;
-	else if (base[baselen - 1] != '/')
+	if (abs[baselen] != '/' && base[baselen - 1] != '/')
 		return abs;
+	while (abs[baselen] == '/')
+		baselen++;
 	strcpy(buf, abs + baselen);
 	return buf;
 }
diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index 74e6443..9df3012 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh
@@ -189,4 +189,10 @@ test_expect_success 'absolute pathspec should fail gracefully' '
 	)
 '
 
+test_expect_success 'make_relative_path handles double slashes in GIT_DIR' '
+	: > dummy_file
+	echo git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file &&
+	git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file
+'
+
 test_done
-- 
1.6.6.1.532.g594fe

Re: [PATCH] Handle double slashes in make_relative_path()

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> diff --git a/path.c b/path.c
> index 2ec950b..a195bab 100644
> --- a/path.c
> +++ b/path.c
> @@ -400,10 +400,10 @@ int set_shared_perm(const char *path, int mode)
>  	baselen = strlen(base);
>  	if (prefixcmp(abs, base))
>  		return abs;
> -	if (abs[baselen] == '/')
> -		baselen++;
> -	else if (base[baselen - 1] != '/')
> +	if (abs[baselen] != '/' && base[baselen - 1] != '/')
>  		return abs;
> +	while (abs[baselen] == '/')
> +		baselen++;
>  	strcpy(buf, abs + baselen);
>  	return buf;
>  }

Curious; why does your hunk header says set_shared_perm() while this is a
patch to make_relative_path()?  Do you run a broken git with funny
funcname regexp pattern?

The function takes two paths, an early part of abs is supposed to
match base; otherwise abs is not a path under base and the function
returns the full path of abs.

Now what is the goal of this patch?  To allow people to have duplicated
slashes at random places in either abs or base, or is it only interested
in a particular input that is malformed?  If the latter, what is the
permitted non-canonical input?

If abs were "/a//b/c" and base were "/a/b", then the combination is
rejected by prefixcmp() and full "/a//b/c" is returned.  Is it the
intended behaviour of the patch?

I would actually have expected to see something like this, but I haven't
even compile tested it, so... 

 path.c |   29 ++++++++++++++++++++---------
 1 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/path.c b/path.c
index 2ec950b..1c3570c 100644
--- a/path.c
+++ b/path.c
@@ -394,17 +394,28 @@ int set_shared_perm(const char *path, int mode)
 const char *make_relative_path(const char *abs, const char *base)
 {
 	static char buf[PATH_MAX + 1];
-	int baselen;
+	int i = 0, j = 0;
+
 	if (!base)
 		return abs;
-	baselen = strlen(base);
-	if (prefixcmp(abs, base))
-		return abs;
-	if (abs[baselen] == '/')
-		baselen++;
-	else if (base[baselen - 1] != '/')
-		return abs;
-	strcpy(buf, abs + baselen);
+	while (base[i]) {
+		if (base[i] == '/') {
+			if (abs[j] != '/')
+				return abs;
+			while (base[i] == '/')
+				i++;
+			while (abs[j] == '/')
+				j++;
+			continue;
+		} else if (abs[j] != base[i]) {
+			return abs;
+		}
+		i++;
+		j++;
+	}
+	while (abs[j] == '/')
+		j++;
+	strcpy(buf, abs + j);
 	return buf;
 }
 

Re: [PATCH] Handle double slashes in make_relative_path()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I would actually have expected to see something like this, but I haven't
> even compile tested it, so... 

Ok, here is a compile and "make test" tested one, together with your
addition to the test script.

I am still curious how you managed to end up with a wrong function name in
the context header, though.  The patch below has "set_shared_perm" because
that is the header we find before the context of the hunk, so it is sort
of understandable; we might want to squelch the hunk header string when
the first context line of the hunk already matches the funcname pattern,
though.

-- >8 --
Subject: ignore duplicated slashes in make_relative_path()

The function takes two paths, an early part of abs is supposed to match
base; otherwise abs is not a path under base and the function returns the
full path of abs.  The caller can easily confuse the implementation by
giving duplicated and needless slashes in these path arguments.

Credit for test script, motivation and initial patch goes to Thomas Rast,
but the bugs in the implementation of this patch are mine..

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 path.c              |   32 +++++++++++++++++++++++---------
 t/t1501-worktree.sh |    6 ++++++
 2 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/path.c b/path.c
index 2ec950b..5906fa3 100644
--- a/path.c
+++ b/path.c
@@ -394,17 +394,31 @@ int set_shared_perm(const char *path, int mode)
 const char *make_relative_path(const char *abs, const char *base)
 {
 	static char buf[PATH_MAX + 1];
-	int baselen;
+	int i = 0, j = 0;
+
 	if (!base)
 		return abs;
-	baselen = strlen(base);
-	if (prefixcmp(abs, base))
-		return abs;
-	if (abs[baselen] == '/')
-		baselen++;
-	else if (base[baselen - 1] != '/')
-		return abs;
-	strcpy(buf, abs + baselen);
+	while (base[i]) {
+		if (base[i] == '/') {
+			if (abs[j] != '/')
+				return abs;
+			while (base[i] == '/')
+				i++;
+			while (abs[j] == '/')
+				j++;
+			continue;
+		} else if (abs[j] != base[i]) {
+			return abs;
+		}
+		i++;
+		j++;
+	}
+	while (abs[j] == '/')
+		j++;
+	if (!abs[j])
+		strcpy(buf, ".");
+	else
+		strcpy(buf, abs + j);
 	return buf;
 }
 
diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index 74e6443..9df3012 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh
@@ -189,4 +189,10 @@ test_expect_success 'absolute pathspec should fail gracefully' '
 	)
 '
 
+test_expect_success 'make_relative_path handles double slashes in GIT_DIR' '
+	: > dummy_file
+	echo git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file &&
+	git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file
+'
+
 test_done

Re: [PATCH] Handle double slashes in make_relative_path()

[Johannes Sixt]

Junio C Hamano schrieb:
> Credit for test script, motivation and initial patch goes to Thomas Rast,
> but the bugs in the implementation of this patch are mine..

And with this squashed in it has fewer of them ;-) and is more portable.
The bug was that /foo was incorrectly stripped from /foobar.

-- Hannes

diff --git a/path.c b/path.c
index 78ab54a..3cb19c7 100644
--- a/path.c
+++ b/path.c
@@ -396,15 +396,15 @@ const char *make_relative_path(const char *abs, const char *base)
 	static char buf[PATH_MAX + 1];
 	int i = 0, j = 0;

-	if (!base)
+	if (!base || !base[0])
 		return abs;
 	while (base[i]) {
-		if (base[i] == '/') {
-			if (abs[j] != '/')
+		if (is_dir_sep(base[i])) {
+			if (!is_dir_sep(abs[j]))
 				return abs;
-			while (base[i] == '/')
+			while (is_dir_sep(base[i]))
 				i++;
-			while (abs[j] == '/')
+			while (is_dir_sep(abs[j]))
 				j++;
 			continue;
 		} else if (abs[j] != base[i]) {
@@ -413,7 +413,14 @@ const char *make_relative_path(const char *abs, const char *base)
 		i++;
 		j++;
 	}
-	while (abs[j] == '/')
+	if (
+	    /* "/foo" is a prefix of "/foo" */
+	    abs[j] &&
+	    /* "/foo" is not a prefix of "/foobar" */
+	    !is_dir_sep(base[i-1]) && !is_dir_sep(abs[j])
+	   )
+		return abs;
+	while (is_dir_sep(abs[j]))
 		j++;
 	if (!abs[j])
 		strcpy(buf, ".");

Re: [PATCH] Handle double slashes in make_relative_path()

[Robin Rosenberg]

fredagen den 22 januari 2010 09.36.13 skrev  Johannes Sixt:
> Junio C Hamano schrieb:
> > Credit for test script, motivation and initial patch goes to Thomas Rast,
> > but the bugs in the implementation of this patch are mine..
> 
> And with this squashed in it has fewer of them ;-) and is more portable.
> The bug was that /foo was incorrectly stripped from /foobar.

It seems this function does something unhealthy when you pass a path of the 
form //server/share. On windows dropping the double // at the beginning makes
it a different path since // is the UNC prefix.

I'm not sure git on windows actually works with UNC-prefix anyway, so my point 
may be moot, but having even more places to fix to make it work doesn't help.

-- robin

Re: [PATCH] Handle double slashes in make_relative_path()

[Johannes Sixt]

On Samstag, 23. Januar 2010, Robin Rosenberg wrote:
> It seems this function does something unhealthy when you pass a path of the
> form //server/share. On windows dropping the double // at the beginning
> makes it a different path since // is the UNC prefix.

There is no problem in practice.

The function returns either the input unmodified, or it strips also at least 
one directory component, except when base is only "/" (or "//" or "///"...). 
I said in practice, because on Windows it does not make sense to invoke git 
with (literally)

   git --git-dir=//server/share/repo.git --work-tree=/ ...

i.e., without a drive prefix before the slash of --work-tree.

-- Hannes

Re: [PATCH] Handle double slashes in make_relative_path()

[Robin Rosenberg]

lördagen den 23 januari 2010 14.09.29 skrev  Johannes Sixt:
> On Samstag, 23. Januar 2010, Robin Rosenberg wrote:
> > It seems this function does something unhealthy when you pass a path of
> > the form //server/share. On windows dropping the double // at the
> > beginning makes it a different path since // is the UNC prefix.
> 
> There is no problem in practice.
> 
> The function returns either the input unmodified, or it strips also at
>  least one directory component, except when base is only "/" (or "//" or
>  "///"...). I said in practice, because on Windows it does not make sense
>  to invoke git with (literally)
> 
>    git --git-dir=//server/share/repo.git --work-tree=/ ...
> 
> i.e., without a drive prefix before the slash of --work-tree.

Why not? //foo/bar/z is just as valid and useful a path as x:/z. 

Defining a drive-letter with msysgit is tricky because I have to find one that 
is available and then also restart every msys bash instance to make msys
see it.

-- robin

Re: [PATCH] Handle double slashes in make_relative_path()

[Johannes Sixt]

On Samstag, 23. Januar 2010, Robin Rosenberg wrote:
> lördagen den 23 januari 2010 14.09.29 skrev  Johannes Sixt:
> > The function returns either the input unmodified, or it strips also at
> >  least one directory component, except when base is only "/" (or "//" or
> >  "///"...). I said in practice, because on Windows it does not make sense
> >  to invoke git with (literally)
> >
> >    git --git-dir=//server/share/repo.git --work-tree=/ ...
> >
> > i.e., without a drive prefix before the slash of --work-tree.
>
> Why not? //foo/bar/z is just as valid and useful a path as x:/z.

Fortunately, make_relative_path() does not have the slightest problem 
with //foo/bar/z, either as value of abs (the path to make relative) or as 
base (the path to strip from abs).

-- Hannes

Re: [PATCH] Handle double slashes in make_relative_path()

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> On Samstag, 23. Januar 2010, Robin Rosenberg wrote:
>> It seems this function does something unhealthy when you pass a path of the
>> form //server/share. On windows dropping the double // at the beginning
>> makes it a different path since // is the UNC prefix.
>
> There is no problem in practice.
>
> The function returns either the input unmodified, or it strips also at least 
> one directory component, except when base is only "/" (or "//" or "///"...). 
> I said in practice, because on Windows it does not make sense to invoke git 
> with (literally)
>
>    git --git-dir=//server/share/repo.git --work-tree=/ ...
>
> i.e., without a drive prefix before the slash of --work-tree.

If you did this:

    git --git-dir=//reposerver/repo.git --work-tree=//buildserver/workarea

then we would say "one is not a prefix of the other", so it would be
fine.  At least I don't think the "recover from unintentionally doubled
slashes in user supplied path" fix is introducing any new problem in that
case.

If on the other hand, if you did this:

    git --git-dir=//server/repo.git --work-tree=//server/workarea

that also would be Ok.

I think one issue is what happens when you did this:

    cd //server
    git --git-dir=//server/repo/repo.git --work-tree=repo

Does msysgit implementation figures out that the work tree is located at
"//server/repo" when get_git_work_tree() is asked to produce an absolute
path so that it can be compared with //server/repo/repo.git with the code?
If it does (with the leading double slash), then "doubled slahses fix" is
a regression we should do something about it.  If it doesn't, then it
probably doesn't matter.

Re: [PATCH] Handle double slashes in make_relative_path()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I think one issue is what happens when you did this:
>
>     cd //server
>     git --git-dir=//server/repo/repo.git --work-tree=repo
>
> Does msysgit implementation figures out that the work tree is located at
> "//server/repo" when get_git_work_tree() is asked to produce an absolute
> path so that it can be compared with //server/repo/repo.git with the code?
> If it does (with the leading double slash), then "doubled slahses fix" is
> a regression we should do something about it.  If it doesn't, then it
> probably doesn't matter.

Nah, I wasn't thinking straight.  What happens if you did this?

	git --git-dir=//git/repo/repo.git --work-tree=/git/repo

where "//git/repo" is on the "git server" and you are working in local
hierarchy "/git/repo"?

Re: [PATCH] Handle double slashes in make_relative_path()

[Johannes Sixt]

On Samstag, 23. Januar 2010, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> > I think one issue is what happens when you did this:
> >
> >     cd //server
> >     git --git-dir=//server/repo/repo.git --work-tree=repo
> >
> > Does msysgit implementation figures out that the work tree is located at
> > "//server/repo" when get_git_work_tree() is asked to produce an absolute
> > path so that it can be compared with //server/repo/repo.git with the
> > code? If it does (with the leading double slash), then "doubled slahses
> > fix" is a regression we should do something about it.  If it doesn't,
> > then it probably doesn't matter.
>
> Nah, I wasn't thinking straight.  What happens if you did this?
>
> 	git --git-dir=//git/repo/repo.git --work-tree=/git/repo
>
> where "//git/repo" is on the "git server" and you are working in local
> hierarchy "/git/repo"?

Ah, right, this would not do the right thing. (But I can't verify this claim 
right now.)

The problem is that /git/repo without a drive prefix is a valid path and it 
means the path that begins at the same drive that CWD currently is. I would 
not dismiss this form of paths as too exotic, so we should care about them. 
OTOH, it can be worked around easily by the user (just insert the drive 
prefix). Dunno...

-- Hannes

Re: [PATCH] Handle double slashes in make_relative_path()

[Sverre Rabbelier]

Heya,

On Sat, Jan 23, 2010 at 21:41, Johannes Sixt <j6t@kdbg.org> wrote:

> OTOH, it can be worked around easily by the user (just insert the drive
> prefix). Dunno...

I think it's preferable to keep the old behavior where we fail if the
user gives us an invalid argument, rather than fix a user error and
break on a a valid argument instead. I think we should be correct
first, and try and fix incorrect user behavior after.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] Handle double slashes in make_relative_path()

[Thomas Rast]

On Saturday 23 January 2010 22:01:38 Sverre Rabbelier wrote:
> Heya,
> 
> On Sat, Jan 23, 2010 at 21:41, Johannes Sixt <j6t@kdbg.org> wrote:
> 
> > OTOH, it can be worked around easily by the user (just insert the drive
> > prefix). Dunno...
> 
> I think it's preferable to keep the old behavior where we fail if the
> user gives us an invalid argument, rather than fix a user error and
> break on a a valid argument instead. I think we should be correct
> first, and try and fix incorrect user behavior after.

I can't really comment on the Windows side of things, but I tried to
come up with some more data points.

POSIX specifies that multiple slashes must be treated as if they were
a single slash (except as in the next bullet point).  Leading _double_
slashes may be treated implementation-dependently. [1]  Non-leading
double slashes do not seem to be specified.

There's a manpage path_resolution(7) on my system, which can also be
found on the web quite easily, e.g. [2].  It doesn't say anything
about multiple slashes, but experimentally my Linux resolves them as
if they were single slashes (even a leading double slash).

Junio's patch is already in maint, so I suppose we're in the somewhat
unfortunate situation where the old version didn't work in all cases
on Linux, but the current one breaks on Windows in some cases.  Then
again, shouldn't windows get special support to figure out that /c/foo
[3] is a prefix of /foo and vice versa, assuming you're currently in
C:?


[1] http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap04.html#tag_04_11
Actually the server doesn't work for me, but google has a cached copy:
http://209.85.129.132/search?q=cache:QUuajH7Dp5gJ:www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap04.html+http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap04.html#tag_04_11&cd=1&hl=en&ct=clnk&gl=uk

[2] http://www.kernel.org/doc/man-pages/online/pages/man7/path_resolution.7.html

[3] Don't blame me if I didn't get that syntax right, I'm actively
trying to forget I ever used Windows and it shows.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] Handle double slashes in make_relative_path()

[Bernhard R. Link]

* Thomas Rast <trast@student.ethz.ch> [100124 14:58]:
> POSIX specifies that multiple slashes must be treated as if they were
> a single slash (except as in the next bullet point).  Leading _double_
> slashes may be treated implementation-dependently. [1]  Non-leading
> double slashes do not seem to be specified.

http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html#tag_03_266

| 3.266 Pathname
|
| A character string that is used to identify a file. In the context of
| IEEE Std 1003.1-2001, a pathname consists of, at most, {PATH_MAX} bytes,
| including the terminating null byte. It has an optional beginning
| slash, followed by zero or more filenames separated by slashes. A
| pathname may optionally contain one or more trailing slashes.
| Multiple successive slashes are considered to be the same as one
| slash.

Re: [PATCH] Handle double slashes in make_relative_path()

[Junio C Hamano]

"Bernhard R. Link" <brlink@debian.org> writes:

> http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html#tag_03_266
>
> | 3.266 Pathname
> |
> | A character string that is used to identify a file. In the context of
> | IEEE Std 1003.1-2001, a pathname consists of, at most, {PATH_MAX} bytes,
> | including the terminating null byte. It has an optional beginning
> | slash, followed by zero or more filenames separated by slashes. A
> | pathname may optionally contain one or more trailing slashes.
> | Multiple successive slashes are considered to be the same as one
> | slash.

Which is a bit stale, and there is an update that is crucial when
discussing the issue that is the topic of this thread.

Try this one instead, especially the last ", except for" which is an
important clarification.

http://www.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html

| 3.266 Pathname
| 
| A character string that is used to identify a file. In the context of
| POSIX.1-2008, a pathname may be limited to {PATH_MAX} bytes, including the
| terminating null byte. It has an optional beginning <slash>, followed by
| zero or more filenames separated by <slash> characters. A pathname may
| optionally contain one or more trailing <slash> characters. Multiple
| successive <slash> characters are considered to be the same as one
| <slash>, except for the case of exactly two leading <slash> characters.

And 4.12 in the same issue of POSIX says pathnames with exactly two
leading slashes may be handled in an implemenation defined manner
(i.e. three or more means the same thing as a single slash).  Confusing
;-)

Re: [PATCH] Handle double slashes in make_relative_path()

[Johannes Sixt]

On Samstag, 23. Januar 2010, Johannes Sixt wrote:
> On Samstag, 23. Januar 2010, Junio C Hamano wrote:
> > What happens if you did this?
> >
> > 	git --git-dir=//git/repo/repo.git --work-tree=/git/repo
> >
> > where "//git/repo" is on the "git server" and you are working in local
> > hierarchy "/git/repo"?
>
> Ah, right, this would not do the right thing. (But I can't verify this
> claim right now.)

I tested it, and it does the right thing. The reason is that before 
setup_work_tree() calls make_relative_path(), the --work-tree argument has 
been processed by make_absolute_path(), which adds the drive prefix.

As long as setup_work_tree() remains the only caller of make_relative_path(), 
we are safe.

-- Hannes

Re: [PATCH] Handle double slashes in make_relative_path()

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> On Samstag, 23. Januar 2010, Johannes Sixt wrote:
>> On Samstag, 23. Januar 2010, Junio C Hamano wrote:
>> > What happens if you did this?
>> >
>> > 	git --git-dir=//git/repo/repo.git --work-tree=/git/repo
>> >
>> > where "//git/repo" is on the "git server" and you are working in local
>> > hierarchy "/git/repo"?
>>
>> Ah, right, this would not do the right thing. (But I can't verify this
>> claim right now.)
>
> I tested it, and it does the right thing. The reason is that before 
> setup_work_tree() calls make_relative_path(), the --work-tree argument has 
> been processed by make_absolute_path(), which adds the drive prefix.
>
> As long as setup_work_tree() remains the only caller of make_relative_path(), 
> we are safe.

Thanks; I think a more correct description of your findings is:

 - msysgit's make_absolute_path() does the right thing (i.e. adds "drive
   prefix" to "git/repo" given to --work-tree); and

 - as long as the callers feed what the platform considers absolute paths
   in abs and base, make_relative_path() does the right thing.

So I think we are Ok.  We _might_ want to add Windows-only test at the
beginning of make_relative_path() to make sure that the both inputs have
double-slashes at the beginning to catch future broken callers, but I
think that is a separate topic, and we don't have to do that as long as
setup_work_tree(0 remains the only caller, as you said.

Re: [PATCH] Handle double slashes in make_relative_path()

[Robin Rosenberg]

söndagen den 24 januari 2010 19.31.01 skrev  Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> > On Samstag, 23. Januar 2010, Johannes Sixt wrote:
> >> On Samstag, 23. Januar 2010, Junio C Hamano wrote:
> >> > What happens if you did this?
> >> >
> >> > 	git --git-dir=//git/repo/repo.git --work-tree=/git/repo
> >> >
> >> > where "//git/repo" is on the "git server" and you are working in local
> >> > hierarchy "/git/repo"?
> >>
> >> Ah, right, this would not do the right thing. (But I can't verify this
> >> claim right now.)
> >
> > I tested it, and it does the right thing. The reason is that before
> > setup_work_tree() calls make_relative_path(), the --work-tree argument
> > has been processed by make_absolute_path(), which adds the drive prefix.
> >
> > As long as setup_work_tree() remains the only caller of
> > make_relative_path(), we are safe.
> 
> Thanks; I think a more correct description of your findings is:
> 
>  - msysgit's make_absolute_path() does the right thing (i.e. adds "drive
>    prefix" to "git/repo" given to --work-tree); and
> 
>  - as long as the callers feed what the platform considers absolute paths
>    in abs and base, make_relative_path() does the right thing.
> 
> So I think we are Ok.  We _might_ want to add Windows-only test at the
> beginning of make_relative_path() to make sure that the both inputs have
> double-slashes at the beginning to catch future broken callers, but I
> think that is a separate topic, and we don't have to do that as long as
> setup_work_tree(0 remains the only caller, as you said.

Separate patch posted since this problem occurs in more than one place. 

-- robin

Re: [PATCH] Handle double slashes in make_relative_path()

[Thomas Rast]

On Friday 22 January 2010 02:40:41 Junio C Hamano wrote:
> 
> Now what is the goal of this patch?  To allow people to have duplicated
> slashes at random places in either abs or base, or is it only interested
> in a particular input that is malformed?  If the latter, what is the
> permitted non-canonical input?
> 
> If abs were "/a//b/c" and base were "/a/b", then the combination is
> rejected by prefixcmp() and full "/a//b/c" is returned.  Is it the
> intended behaviour of the patch?
> 
> I would actually have expected to see [a real fix that handles
> duplicate slashes in all instances.]

It's not about *permitted* input; the problem is simply that the
current function gives back *bogus* paths, which causes git to fail.
So I only went for the minimal patch to fix this.

Not handling the abs="/a//b/c" base="/a/b" case seemed ok to me since
that was never turned as a relative "c", hence there would not be any
speed loss (nor gain) from my patch.

Does that answer the question?

As for your patch, thanks for coming up with a real fix.  I read the
amended version, and it seems correct to me.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] Handle double slashes in make_relative_path()

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> It's not about *permitted* input; the problem is simply that the
> current function gives back *bogus* paths, which causes git to fail.
> So I only went for the minimal patch to fix this.

With that logic a minimal patch would have been not to call the function
at all, as apparently the caller seem to be able to cope with absolute
paths returned when they could be made relative, no?

In other words, it wasn't obvious to me if the minimal patch avoided
returning a bogus result claiming that is a path relative to the base
directory and instead returned an absolute path (which might be suboptimal
but way better than giving a wrong thing back) in _all_ cases, or only
just on _some_ cases but not others, and if it was the latter, what are
the cases that it did better than the original.

> As for your patch, thanks for coming up with a real fix.  I read the
> amended version, and it seems correct to me.

By "amended", I take it to mean the fix-up by Hannes.  I'll queue one
for 'maint'.

Thanks.