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

Junio C Hamano [off-list ref] 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 <redacted>
---
 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

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, ".");

fredagen den 22 januari 2010 09.36.13 skrev  Johannes Sixt:

Junio C Hamano schrieb:

quoted

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

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

lördagen den 23 januari 2010 14.09.29 skrev  Johannes Sixt:

On Samstag, 23. Januar 2010, Robin Rosenberg wrote:

quoted

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

On Samstag, 23. Januar 2010, Robin Rosenberg wrote:

lördagen den 23 januari 2010 14.09.29 skrev  Johannes Sixt:

quoted

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

Johannes Sixt [off-list ref] writes:

On Samstag, 23. Januar 2010, Robin Rosenberg wrote:

quoted

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.