[PATCH 0/7] Fix some bugs in abspath.c

[PATCH 0/7] Fix some bugs in abspath.c

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

I really just wanted to tidy up filter_refs(), but I've been sucked
into a cascade of recursive yak shaving.  This is my first attempt to
pop the yak stack.

I want to use real_path() for making the handling of
GIT_CEILING_DIRECTORIES more robust, but I noticed that it is broken
for some cases that will be important in this context.  This patch
series adds some new tests of that function and fixes the ones that
are broken, including a similar breakage in absolute_path().  It
applies to the current master.

Please note that both absolute_path("") and real_path("") used to
return the current directory followed by a slash.  I believe that this
was a bug, and that it is more appropriate for both functions to
reject the empty string.  The evidence is as follows:

* If this were intended behavior, presumably the return value would
  *not* have a trailing slash.

* I couldn't find any callers that appeared to depend on the old
  behavior.

* The test suite runs without errors after the change.

But I didn't do a thorough code review to ensure that no callers ever
rely on the old behavior.  The most likely scenario would be if one of
these functions were used to parse something like $PATH, where the
empty string denotes the current directory, but I didn't see any
callers that seemed to be doing anything like this.

Michael Haggerty (7):
  t0000: verify that absolute_path() fails if passed the empty string
  absolute_path(): reject the empty string
  t0000: verify that real_path() fails if passed the empty string
  real_path(): reject the empty string
  t0000: verify that real_path() works correctly with absolute paths
  real_path(): properly handle nonexistent top-level paths
  t0000: verify that real_path() removes extra slashes

 abspath.c        | 12 ++++++++++--
 t/t0000-basic.sh | 31 ++++++++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 3 deletions(-)

-- 
1.7.11.3

[PATCH 1/7] t0000: verify that absolute_path() fails if passed the empty string

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

It doesn't, so mark the test as failing.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t0000-basic.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index ccb5435..7347fb8 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -450,6 +450,10 @@ test_expect_success 'update-index D/F conflict' '
 	test $numpath0 = 1
 '
 
+test_expect_failure 'absolute path rejects the empty string' '
+	test_must_fail test-path-utils absolute_path ""
+'
+
 test_expect_success SYMLINKS 'real path works as expected' '
 	mkdir first &&
 	ln -s ../.git first/.git &&
-- 
1.7.11.3

[PATCH 2/7] absolute_path(): reject the empty string

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>


Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 abspath.c        | 4 +++-
 t/t0000-basic.sh | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/abspath.c b/abspath.c
index f04ac18..5d62430 100644
--- a/abspath.c
+++ b/abspath.c
@@ -123,7 +123,9 @@ const char *absolute_path(const char *path)
 {
 	static char buf[PATH_MAX + 1];
 
-	if (is_absolute_path(path)) {
+	if (!*path) {
+		die("The empty string is not a valid path");
+	} else if (is_absolute_path(path)) {
 		if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
 			die("Too long path: %.*s", 60, path);
 	} else {
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 7347fb8..5963a6c 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -450,7 +450,7 @@ test_expect_success 'update-index D/F conflict' '
 	test $numpath0 = 1
 '
 
-test_expect_failure 'absolute path rejects the empty string' '
+test_expect_success 'absolute path rejects the empty string' '
 	test_must_fail test-path-utils absolute_path ""
 '
 
-- 
1.7.11.3

[PATCH 3/7] t0000: verify that real_path() fails if passed the empty string

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

It doesn't, so mark the test as failing.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t0000-basic.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 5963a6c..363e190 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -454,6 +454,10 @@ test_expect_success 'absolute path rejects the empty string' '
 	test_must_fail test-path-utils absolute_path ""
 '
 
+test_expect_failure 'real path rejects the empty string' '
+	test_must_fail test-path-utils real_path ""
+'
+
 test_expect_success SYMLINKS 'real path works as expected' '
 	mkdir first &&
 	ln -s ../.git first/.git &&
-- 
1.7.11.3

[PATCH 4/7] real_path(): reject the empty string

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>


Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 abspath.c        | 3 +++
 t/t0000-basic.sh | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/abspath.c b/abspath.c
index 5d62430..3e8325c 100644
--- a/abspath.c
+++ b/abspath.c
@@ -35,6 +35,9 @@ const char *real_path(const char *path)
 	if (path == buf || path == next_buf)
 		return path;
 
+	if (!*path)
+		die("The empty string is not a valid path");
+
 	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
 		die ("Too long path: %.*s", 60, path);
 
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 363e190..1a51634 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -454,7 +454,7 @@ test_expect_success 'absolute path rejects the empty string' '
 	test_must_fail test-path-utils absolute_path ""
 '
 
-test_expect_failure 'real path rejects the empty string' '
+test_expect_success 'real path rejects the empty string' '
 	test_must_fail test-path-utils real_path ""
 '
 
-- 
1.7.11.3

[PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

There is currently a bug: if passed an absolute top-level path that
doesn't exist (e.g., "/foo") it incorrectly interprets the path as a
relative path (e.g., returns "$(pwd)/foo").  So mark the test as
failing.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t0000-basic.sh | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 1a51634..ad002ee 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -458,7 +458,17 @@ test_expect_success 'real path rejects the empty string' '
 	test_must_fail test-path-utils real_path ""
 '
 
-test_expect_success SYMLINKS 'real path works as expected' '
+test_expect_failure 'real path works on absolute paths' '
+	nopath="hopefully-absent-path" &&
+	test "/" = "$(test-path-utils real_path "/")" &&
+	test "/$nopath" = "$(test-path-utils real_path "/$nopath")" &&
+	# Find an existing top-level directory for the remaining tests:
+	d=$(pwd -P | sed -e "s|^\(/[^/]*\)/.*|\1|") &&
+	test "$d" = "$(test-path-utils real_path "$d")" &&
+	test "$d/$nopath" = "$(test-path-utils real_path "$d/$nopath")"
+'
+
+test_expect_success SYMLINKS 'real path works on symlinks' '
 	mkdir first &&
 	ln -s ../.git first/.git &&
 	mkdir second &&
-- 
1.7.11.3

Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths

[Johannes Sixt]

Am 9/4/2012 10:14, schrieb mhagger@alum.mit.edu:
> From: Michael Haggerty <mhagger@alum.mit.edu>
> 
> There is currently a bug: if passed an absolute top-level path that
> doesn't exist (e.g., "/foo") it incorrectly interprets the path as a
> relative path (e.g., returns "$(pwd)/foo").  So mark the test as
> failing.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  t/t0000-basic.sh | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index 1a51634..ad002ee 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -458,7 +458,17 @@ test_expect_success 'real path rejects the empty string' '
>  	test_must_fail test-path-utils real_path ""
>  '
>  

These tests should really be in t0060-path-utils.sh.

> -test_expect_success SYMLINKS 'real path works as expected' '
> +test_expect_failure 'real path works on absolute paths' '
> +	nopath="hopefully-absent-path" &&
> +	test "/" = "$(test-path-utils real_path "/")" &&
> +	test "/$nopath" = "$(test-path-utils real_path "/$nopath")" &&

These tests fail on Windows because test-path-utils operates on a
DOS-style absolute path even if a POSIX style absolute path is passed as
argument. The best thing would be to move this to a separate test that is
protected by a POSIX prerequisite (which is set in t0060).

> +	# Find an existing top-level directory for the remaining tests:
> +	d=$(pwd -P | sed -e "s|^\(/[^/]*\)/.*|\1|") &&

pwd -P actually expands to pwd -W on Windows, and produces a DOS-style
path. I suggest to insert [^/]* to skip drive letter-colon like so:

	d=$(pwd -P | sed -e "s|^\([^/]*/[^/]*\)/.*|\1|") &&

> +	test "$d" = "$(test-path-utils real_path "$d")" &&
> +	test "$d/$nopath" = "$(test-path-utils real_path "$d/$nopath")"

Then these tests pass.

-- Hannes

Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths

[Michael Haggerty]

On 09/05/2012 10:40 AM, Johannes Sixt wrote:
> Am 9/4/2012 10:14, schrieb mhagger@alum.mit.edu:
>> From: Michael Haggerty <mhagger@alum.mit.edu>
>>
>> There is currently a bug: if passed an absolute top-level path that
>> doesn't exist (e.g., "/foo") it incorrectly interprets the path as a
>> relative path (e.g., returns "$(pwd)/foo").  So mark the test as
>> failing.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  t/t0000-basic.sh | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
>> index 1a51634..ad002ee 100755
>> --- a/t/t0000-basic.sh
>> +++ b/t/t0000-basic.sh
>> @@ -458,7 +458,17 @@ test_expect_success 'real path rejects the empty string' '
>>  	test_must_fail test-path-utils real_path ""
>>  '
>>  
> 
> These tests should really be in t0060-path-utils.sh.
> 
>> -test_expect_success SYMLINKS 'real path works as expected' '
>> +test_expect_failure 'real path works on absolute paths' '
>> +	nopath="hopefully-absent-path" &&
>> +	test "/" = "$(test-path-utils real_path "/")" &&
>> +	test "/$nopath" = "$(test-path-utils real_path "/$nopath")" &&
> 
> These tests fail on Windows because test-path-utils operates on a
> DOS-style absolute path even if a POSIX style absolute path is passed as
> argument. The best thing would be to move this to a separate test that is
> protected by a POSIX prerequisite (which is set in t0060).
> 
>> +	# Find an existing top-level directory for the remaining tests:
>> +	d=$(pwd -P | sed -e "s|^\(/[^/]*\)/.*|\1|") &&
> 
> pwd -P actually expands to pwd -W on Windows, and produces a DOS-style
> path. I suggest to insert [^/]* to skip drive letter-colon like so:
> 
> 	d=$(pwd -P | sed -e "s|^\([^/]*/[^/]*\)/.*|\1|") &&
> 
>> +	test "$d" = "$(test-path-utils real_path "$d")" &&
>> +	test "$d/$nopath" = "$(test-path-utils real_path "$d/$nopath")"
> 
> Then these tests pass.

Thanks for the help.  They will be incorporated in v2.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths

[Junio C Hamano]

mhagger@alum.mit.edu writes:

> From: Michael Haggerty <mhagger@alum.mit.edu>
>
> There is currently a bug: if passed an absolute top-level path that
> doesn't exist (e.g., "/foo") it incorrectly interprets the path as a
> relative path (e.g., returns "$(pwd)/foo").  So mark the test as
> failing.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  t/t0000-basic.sh | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index 1a51634..ad002ee 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -458,7 +458,17 @@ test_expect_success 'real path rejects the empty string' '
>  	test_must_fail test-path-utils real_path ""
>  '
>  
> -test_expect_success SYMLINKS 'real path works as expected' '
> +test_expect_failure 'real path works on absolute paths' '
> +	nopath="hopefully-absent-path" &&
> +	test "/" = "$(test-path-utils real_path "/")" &&
> +	test "/$nopath" = "$(test-path-utils real_path "/$nopath")" &&

You could perhaps do

	sfx=0 &&
        while test -e "/$nopath$sfx"
        do
		sfx=$(( $sfx + 1 ))
	done && nopath=$nopath$sfx &&
	test "/$nopath" = "$(test-path-utils real_path "/$nopath")" &&

if you really cared.

Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths

[Michael Haggerty]

On 09/07/2012 01:08 AM, Junio C Hamano wrote:
> mhagger@alum.mit.edu writes:
> 
>> From: Michael Haggerty <mhagger@alum.mit.edu>
>>
>> There is currently a bug: if passed an absolute top-level path that
>> doesn't exist (e.g., "/foo") it incorrectly interprets the path as a
>> relative path (e.g., returns "$(pwd)/foo").  So mark the test as
>> failing.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  t/t0000-basic.sh | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
>> index 1a51634..ad002ee 100755
>> --- a/t/t0000-basic.sh
>> +++ b/t/t0000-basic.sh
>> @@ -458,7 +458,17 @@ test_expect_success 'real path rejects the empty string' '
>>  	test_must_fail test-path-utils real_path ""
>>  '
>>  
>> -test_expect_success SYMLINKS 'real path works as expected' '
>> +test_expect_failure 'real path works on absolute paths' '
>> +	nopath="hopefully-absent-path" &&
>> +	test "/" = "$(test-path-utils real_path "/")" &&
>> +	test "/$nopath" = "$(test-path-utils real_path "/$nopath")" &&
> 
> You could perhaps do
> 
> 	sfx=0 &&
>         while test -e "/$nopath$sfx"
>         do
> 		sfx=$(( $sfx + 1 ))
> 	done && nopath=$nopath$sfx &&
> 	test "/$nopath" = "$(test-path-utils real_path "/$nopath")" &&
> 
> if you really cared.

The possibility is obvious.  Are you advocating it?

I considered that approach, but came to the opinion that it would be
overkill that would only complicate the code for no real advantage,
given that (1) I picked a name that is pretty implausible for an
existing file, (2) the test suite only reads the file, never writes it
(so there is no risk that a copy from a previous run gets left behind),
(3) it's only test suite code, and any failures would have minor
consequences.

Please let me know if you assess the situation differently.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> The possibility is obvious.  Are you advocating it?
>
> I considered that approach, but came to the opinion that it would be
> overkill that would only complicate the code for no real advantage,
> given that (1) I picked a name that is pretty implausible for an
> existing file, (2) the test suite only reads the file, never writes it
> (so there is no risk that a copy from a previous run gets left behind),
> (3) it's only test suite code, and any failures would have minor
> consequences.

(4) if it only runs once at the very beginning of the test and sets
a variable that is named prominently clear what it means and lives
throughout the test, then we do not even have to say "hopefully" and
appear lazy and loose to the readers of the test who wonders what
happens when the path does exist; doing so will help reducing the
noise on the mailing list in the future.

Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths

[Junio C Hamano]

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

> (4) if it only runs once at the very beginning of the test and sets
> a variable that is named prominently clear what it means and lives
> throughout the test, then we do not even have to say "hopefully" and
> appear lazy and loose to the readers of the test who wonders what
> happens when the path does exist; doing so will help reducing the
> noise on the mailing list in the future.

Having said that, I really do not deeply care either way.  If you
are rerollilng the series for other changes, I wouldn't shed tears
even if I do not see any change to this part.

[PATCH 6/7] real_path(): properly handle nonexistent top-level paths

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>


Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 abspath.c        | 5 ++++-
 t/t0000-basic.sh | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/abspath.c b/abspath.c
index 3e8325c..0e1cd7f 100644
--- a/abspath.c
+++ b/abspath.c
@@ -45,8 +45,11 @@ const char *real_path(const char *path)
 		if (!is_directory(buf)) {
 			char *last_slash = find_last_dir_sep(buf);
 			if (last_slash) {
-				*last_slash = '\0';
 				last_elem = xstrdup(last_slash + 1);
+				if (last_slash == buf)
+					last_slash[1] = '\0';
+				else
+					last_slash[0] = '\0';
 			} else {
 				last_elem = xstrdup(buf);
 				*buf = '\0';
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index ad002ee..d929578 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -458,7 +458,7 @@ test_expect_success 'real path rejects the empty string' '
 	test_must_fail test-path-utils real_path ""
 '
 
-test_expect_failure 'real path works on absolute paths' '
+test_expect_success 'real path works on absolute paths' '
 	nopath="hopefully-absent-path" &&
 	test "/" = "$(test-path-utils real_path "/")" &&
 	test "/$nopath" = "$(test-path-utils real_path "/$nopath")" &&
-- 
1.7.11.3

[PATCH 7/7] t0000: verify that real_path() removes extra slashes

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

These tests already pass, but make sure they don't break in the
future.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---

It would be great if somebody would check whether these tests pass on
Windows, and if not, give me a tip about how to fix them.

 t/t0000-basic.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index d929578..3c75e97 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -468,6 +468,17 @@ test_expect_success 'real path works on absolute paths' '
 	test "$d/$nopath" = "$(test-path-utils real_path "$d/$nopath")"
 '
 
+test_expect_success 'real path removes extra slashes' '
+	nopath="hopefully-absent-path" &&
+	test "/" = "$(test-path-utils real_path "///")" &&
+	test "/$nopath" = "$(test-path-utils real_path "///$nopath")" &&
+	# We need an existing top-level directory to use in the
+	# remaining tests.  Use the top-level ancestor of $(pwd):
+	d=$(pwd -P | sed -e "s|^\(/[^/]*\)/.*|\1|") &&
+	test "$d" = "$(test-path-utils real_path "//$d///")" &&
+	test "$d/$nopath" = "$(test-path-utils real_path "//$d///$nopath")"
+'
+
 test_expect_success SYMLINKS 'real path works on symlinks' '
 	mkdir first &&
 	ln -s ../.git first/.git &&
-- 
1.7.11.3

Re: [PATCH 7/7] t0000: verify that real_path() removes extra slashes

[Junio C Hamano]

mhagger@alum.mit.edu writes:

> From: Michael Haggerty <mhagger@alum.mit.edu>
>
> These tests already pass, but make sure they don't break in the
> future.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>
> It would be great if somebody would check whether these tests pass on
> Windows, and if not, give me a tip about how to fix them.

I think this (perhaps unwarranted) removal of the double leading
slashes is the root cause of

    http://thread.gmane.org/gmane.comp.version-control.git/204469

(people involved in that thread Cc'ed).

>  t/t0000-basic.sh | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index d929578..3c75e97 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -468,6 +468,17 @@ test_expect_success 'real path works on absolute paths' '
>  	test "$d/$nopath" = "$(test-path-utils real_path "$d/$nopath")"
>  '
>  
> +test_expect_success 'real path removes extra slashes' '
> +	nopath="hopefully-absent-path" &&
> +	test "/" = "$(test-path-utils real_path "///")" &&
> +	test "/$nopath" = "$(test-path-utils real_path "///$nopath")" &&
> +	# We need an existing top-level directory to use in the
> +	# remaining tests.  Use the top-level ancestor of $(pwd):
> +	d=$(pwd -P | sed -e "s|^\(/[^/]*\)/.*|\1|") &&
> +	test "$d" = "$(test-path-utils real_path "//$d///")" &&
> +	test "$d/$nopath" = "$(test-path-utils real_path "//$d///$nopath")"
> +'
> +
>  test_expect_success SYMLINKS 'real path works on symlinks' '
>  	mkdir first &&
>  	ln -s ../.git first/.git &&

Re: [PATCH 7/7] t0000: verify that real_path() removes extra slashes

[Nguyen Thai Ngoc Duy]

On Wed, Sep 5, 2012 at 1:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> mhagger@alum.mit.edu writes:
>
>> From: Michael Haggerty <mhagger@alum.mit.edu>
>>
>> These tests already pass, but make sure they don't break in the
>> future.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>
>> It would be great if somebody would check whether these tests pass on
>> Windows, and if not, give me a tip about how to fix them.
>
> I think this (perhaps unwarranted) removal of the double leading
> slashes is the root cause of
>
>     http://thread.gmane.org/gmane.comp.version-control.git/204469
>
> (people involved in that thread Cc'ed).

I gave up on that thread because I did not have a proper environment
to further troubleshoot. Thanks Mike for giving me the clue about
real_path(). I traced link_alt_odb_entry() and it does this:

	if (!is_absolute_path(entry) && relative_base) {
		strbuf_addstr(&pathbuf, real_path(relative_base));
		strbuf_addch(&pathbuf, '/');
	}
	strbuf_add(&pathbuf, entry, len);
	normalize_path_copy(pathbuf.buf, pathbuf.buf);

The culprit might be normalize_path_copy (I don't have Windows to test
whether real_path() strips the leading slash in //server/somewhere). I
tested normalize_path_copy() separately and it does strip leading
slashes, so it needs to be fixed. Something like maybe:

diff --git a/path.c b/path.c
index 66acd24..ad2881c 100644
--- a/path.c
+++ b/path.c
@@ -503,6 +503,10 @@ int normalize_path_copy(char *dst, const char *src)
                *dst++ = *src++;
                *dst++ = *src++;
        }
+#ifdef WIN32
+       else if (src[0] == '/' && src[1] == '/')
+               *dst++ = *src++;
+#endif
        dst0 = dst;

        if (is_dir_sep(*src)) {

I'm not suitable for following this up as I don't have environment to
test it. Maybe some msysgit guy want to step in?
-- 
Duy

Re: [PATCH 7/7] t0000: verify that real_path() removes extra slashes

[Nguyen Thai Ngoc Duy]

On Wed, Sep 5, 2012 at 6:05 PM, Orgad and Raizel Shaneh
<orgads@gmail.com> wrote:
> This seems to fix it. Just change src[x] == '/' to is_dir_sep(src[x]).

So you are able to test the changes. Would you mind submitting a patch
so other users benefit it as well?
-- 
Duy

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

Re: [PATCH 7/7] t0000: verify that real_path() removes extra slashes

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> diff --git a/path.c b/path.c
> index 66acd24..ad2881c 100644
> --- a/path.c
> +++ b/path.c
> @@ -503,6 +503,10 @@ int normalize_path_copy(char *dst, const char *src)
>                 *dst++ = *src++;
>                 *dst++ = *src++;
>         }
> +#ifdef WIN32
> +       else if (src[0] == '/' && src[1] == '/')
> +               *dst++ = *src++;
> +#endif

The two-byte copy we see above the context is conditional on a nice
abstraction "has_dos_drive_prefix()" so that we do not have to
suffer from these ugly ifdefs.  Could we do something similar?

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

Re: [PATCH 7/7] t0000: verify that real_path() removes extra slashes

[Nguyen Thai Ngoc Duy]

On Wed, Sep 05, 2012 at 08:23:58PM -0700, Junio C Hamano wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> 
> > diff --git a/path.c b/path.c
> > index 66acd24..ad2881c 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -503,6 +503,10 @@ int normalize_path_copy(char *dst, const char *src)
> >                 *dst++ = *src++;
> >                 *dst++ = *src++;
> >         }
> > +#ifdef WIN32
> > +       else if (src[0] == '/' && src[1] == '/')
> > +               *dst++ = *src++;
> > +#endif
> 
> The two-byte copy we see above the context is conditional on a nice
> abstraction "has_dos_drive_prefix()" so that we do not have to
> suffer from these ugly ifdefs.  Could we do something similar?

Just an idea. We could unify "[a-z]:" and "//host" into "dos root"
concept. That would teach other code paths about UNC paths too.

Replace has_dos_drive_prefix() with has_dos_root_prefix(). Because the
root component's length is not fixed, offset_1st_component() should be
used instead of hard coding "2".

Something like this. Totally untested.

-- 8< --
diff --git a/cache.h b/cache.h
index 67f28b4..7946489 100644
--- a/cache.h
+++ b/cache.h
@@ -711,7 +711,7 @@ extern char *expand_user_path(const char *path);
 const char *enter_repo(const char *path, int strict);
 static inline int is_absolute_path(const char *path)
 {
-	return is_dir_sep(path[0]) || has_dos_drive_prefix(path);
+	return is_dir_sep(path[0]) || has_dos_root_prefix(path);
 }
 int is_directory(const char *);
 const char *real_path(const char *path);
@@ -721,7 +721,7 @@ int normalize_path_copy(char *dst, const char *src);
 int longest_ancestor_length(const char *path, const char *prefix_list);
 char *strip_path_suffix(const char *path, const char *suffix);
 int daemon_avoid_alias(const char *path);
-int offset_1st_component(const char *path);
+int offset_1st_component(const char *path, int keep_root);
 
 /* object replacement */
 #define READ_SHA1_FILE_REPLACE 1
diff --git a/compat/basename.c b/compat/basename.c
index d8f8a3c..178b60d 100644
--- a/compat/basename.c
+++ b/compat/basename.c
@@ -5,8 +5,7 @@ char *gitbasename (char *path)
 {
 	const char *base;
 	/* Skip over the disk name in MSDOS pathnames. */
-	if (has_dos_drive_prefix(path))
-		path += 2;
+	path += offset_1st_component(path, 0);
 	for (base = path; *path; path++) {
 		if (is_dir_sep(*path))
 			base = path + 1;
diff --git a/compat/mingw.h b/compat/mingw.h
index 61a6521..1ca3e19 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -302,7 +302,9 @@ int winansi_fprintf(FILE *stream, const char *format, ...) __attribute__((format
  * git specific compatibility
  */
 
-#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
+#define has_dos_root_prefix(path) \
+	((is_dir_sep[(path)[0]] && is_dir_sep[(path)[1]]) \
+	 || (isalpha(*(path)) && (path)[1] == ':'))
 #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
 static inline char *mingw_find_last_dir_sep(const char *path)
 {
diff --git a/connect.c b/connect.c
index 55a85ad..3d9f7fe 100644
--- a/connect.c
+++ b/connect.c
@@ -521,7 +521,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 		end = host;
 
 	path = strchr(end, c);
-	if (path && !has_dos_drive_prefix(end)) {
+	if (path && (!isalpha(*end) || end[1] != ':')) {
 		if (c == ':') {
 			protocol = PROTO_SSH;
 			*path++ = '\0';
diff --git a/git-compat-util.h b/git-compat-util.h
index 35b095e..c6656e2 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -216,8 +216,8 @@ extern char *gitbasename(char *);
 #define STRIP_EXTENSION ""
 #endif
 
-#ifndef has_dos_drive_prefix
-#define has_dos_drive_prefix(path) 0
+#ifndef has_dos_root_prefix
+#define has_dos_root_prefix(path) 0
 #endif
 
 #ifndef is_dir_sep
diff --git a/path.c b/path.c
index 66acd24..0e4e2d7 100644
--- a/path.c
+++ b/path.c
@@ -498,11 +498,12 @@ const char *relative_path(const char *abs, const char *base)
 int normalize_path_copy(char *dst, const char *src)
 {
 	char *dst0;
+	int i, len;
 
-	if (has_dos_drive_prefix(src)) {
+	len = offset_1st_component(src, 1);
+	for (i = 0; i < len; i++)
 		*dst++ = *src++;
-		*dst++ = *src++;
-	}
+
 	dst0 = dst;
 
 	if (is_dir_sep(*src)) {
@@ -702,9 +703,18 @@ int daemon_avoid_alias(const char *p)
 	}
 }
 
-int offset_1st_component(const char *path)
+int offset_1st_component(const char *path, int keep_root)
 {
-	if (has_dos_drive_prefix(path))
-		return 2 + is_dir_sep(path[2]);
-	return is_dir_sep(path[0]);
+	if (has_dos_root_prefix(path)) {
+		if (path[1] == ':')
+			return 2 + (keep_root ? 0 : is_dir_sep(path[2]));
+		else {
+			const char *s = strchr(path + 2, '/');
+			/* //host is considered a "drive" */
+			if (s)
+				return s - path + (keep_root ? 0 : 1);
+		}
+	}
+
+	return keep_root ? 0 : is_dir_sep(path[0]);
 }
diff --git a/read-cache.c b/read-cache.c
index 2f8159f..d4d339a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -755,7 +755,7 @@ int verify_path(const char *path)
 {
 	char c;
 
-	if (has_dos_drive_prefix(path))
+	if (has_dos_root_prefix(path))
 		return 0;
 
 	goto inside;
diff --git a/setup.c b/setup.c
index 9139bee..e010cf8 100644
--- a/setup.c
+++ b/setup.c
@@ -26,7 +26,7 @@ static char *prefix_path_gently(const char *prefix, int len, const char *path)
 		if (!work_tree)
 			goto error_out;
 		len = strlen(work_tree);
-		root_len = offset_1st_component(work_tree);
+		root_len = offset_1st_component(work_tree, 0);
 		total = strlen(sanitized) + 1;
 		if (strncmp(sanitized, work_tree, len) ||
 		    (len > root_len && sanitized[len] != '\0' && sanitized[len] != '/')) {
@@ -587,7 +587,7 @@ static const char *setup_bare_git_dir(char *cwd, int offset, int len, int *nongi
 	if (offset != len) {
 		if (chdir(cwd))
 			die_errno("Cannot come back to cwd");
-		root_len = offset_1st_component(cwd);
+		root_len = offset_1st_component(cwd, 0);
 		cwd[offset > root_len ? offset : root_len] = '\0';
 		set_git_dir(cwd);
 	}
@@ -654,7 +654,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 		return setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);
 
 	ceil_offset = longest_ancestor_length(cwd, env_ceiling_dirs);
-	if (ceil_offset < 0 && has_dos_drive_prefix(cwd))
+	if (ceil_offset < 0 && has_dos_root_prefix(cwd))
 		ceil_offset = 1;
 
 	/*
diff --git a/sha1_file.c b/sha1_file.c
index af5cfbd..21ce020 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -102,7 +102,7 @@ int mkdir_in_gitdir(const char *path)
 
 int safe_create_leading_directories(char *path)
 {
-	char *pos = path + offset_1st_component(path);
+	char *pos = path + offset_1st_component(path, 0);
 	struct stat st;
 
 	while (pos) {
diff --git a/transport.c b/transport.c
index 1811b50..5141e8f 100644
--- a/transport.c
+++ b/transport.c
@@ -869,7 +869,7 @@ static int is_local(const char *url)
 	const char *colon = strchr(url, ':');
 	const char *slash = strchr(url, '/');
 	return !colon || (slash && slash < colon) ||
-		has_dos_drive_prefix(url);
+		has_dos_root_prefix(url);
 }
 
 static int is_file(const char *url)
-- 8< --

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

Re: [PATCH 7/7] t0000: verify that real_path() removes extra slashes

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> Just an idea. We could unify "[a-z]:" and "//host" into "dos root"
> concept. That would teach other code paths about UNC paths too.
> ...
> diff --git a/path.c b/path.c
> index 66acd24..0e4e2d7 100644
> --- a/path.c
> +++ b/path.c
> @@ -498,11 +498,12 @@ const char *relative_path(const char *abs, const char *base)
>  int normalize_path_copy(char *dst, const char *src)
>  {
>  	char *dst0;
> +	int i, len;
>  
> -	if (has_dos_drive_prefix(src)) {
> +	len = offset_1st_component(src, 1);
> +	for (i = 0; i < len; i++)
>  		*dst++ = *src++;
> -		*dst++ = *src++;
> -	}
> +
>  	dst0 = dst;

Modulo that I suspect you could get rid of offset_1st_component()
altogether and has_dos_drive_prefix() return the length of the "d:"
or "//d" part (which needs to be copied literally regardless of the
"normalization"), what you suggest feels like the right approach.
Why do you need the "keep_root" parameter and do things differently
depending on the setting by the way?  Wouldn't "skip the root level
when computing the offset of the first path component" something the
caller can easily decide to do or not to do, and wouldn't it make
the semantics of the function cleaner and simpler by making it do
only one thing and one thing well?

Re: Re: [PATCH 7/7] t0000: verify that real_path() removes extra slashes

[Nguyen Thai Ngoc Duy]

On Fri, Sep 7, 2012 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Modulo that I suspect you could get rid of offset_1st_component()
> altogether and has_dos_drive_prefix() return the length of the "d:"
> or "//d" part (which needs to be copied literally regardless of the
> "normalization"), what you suggest feels like the right approach.
> Why do you need the "keep_root" parameter and do things differently
> depending on the setting by the way?

That's how offset_1st_component() originally works, root slash if
present is counted.

> Wouldn't "skip the root level
> when computing the offset of the first path component" something the
> caller can easily decide to do or not to do, and wouldn't it make
> the semantics of the function cleaner and simpler by making it do
> only one thing and one thing well?

Yeah. I'll have a closer look later and see if we can simplify the function.
-- 
Duy

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

Re: [PATCH 7/7] t0000: verify that real_path() removes extra slashes

[Johannes Sixt]

Am 9/4/2012 10:14, schrieb mhagger@alum.mit.edu:
> From: Michael Haggerty <mhagger@alum.mit.edu>
> 
> These tests already pass, but make sure they don't break in the
> future.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> 
> It would be great if somebody would check whether these tests pass on
> Windows, and if not, give me a tip about how to fix them.
> 
>  t/t0000-basic.sh | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index d929578..3c75e97 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -468,6 +468,17 @@ test_expect_success 'real path works on absolute paths' '
>  	test "$d/$nopath" = "$(test-path-utils real_path "$d/$nopath")"
>  '
>  
> +test_expect_success 'real path removes extra slashes' '
> +	nopath="hopefully-absent-path" &&
> +	test "/" = "$(test-path-utils real_path "///")" &&
> +	test "/$nopath" = "$(test-path-utils real_path "///$nopath")" &&

Same here: test-path-utils operates on a DOS-style absolute path.
Furthermore, as Junio pointed out elsewhere, it is desirable that slashes
(dir-separators) at the beginning are not collapsed if there are exactly
two of them. Three or more can be collapsed to a single slash.

> +	# We need an existing top-level directory to use in the
> +	# remaining tests.  Use the top-level ancestor of $(pwd):
> +	d=$(pwd -P | sed -e "s|^\(/[^/]*\)/.*|\1|") &&

Same here: Account for the drive letter-colon.

> +	test "$d" = "$(test-path-utils real_path "//$d///")" &&
> +	test "$d/$nopath" = "$(test-path-utils real_path "//$d///$nopath")"

Since $d is a DOS-style path on Windows, //$d means something entirely
different than $d. You should split the test in two: One test checks that
slashes at the end or before $nopath are collapsed (this must work on
Windows as well), and another test protected by POSIX prerequisite to
check that slashes at the beginning are collapsed.

-- Hannes

Re: [PATCH 7/7] t0000: verify that real_path() removes extra slashes

[Michael Haggerty]

On 09/05/2012 10:40 AM, Johannes Sixt wrote:
> Am 9/4/2012 10:14, schrieb mhagger@alum.mit.edu:
>> From: Michael Haggerty <mhagger@alum.mit.edu>
>>
>> These tests already pass, but make sure they don't break in the
>> future.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>
>> It would be great if somebody would check whether these tests pass on
>> Windows, and if not, give me a tip about how to fix them.
>>
>>  t/t0000-basic.sh | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
>> index d929578..3c75e97 100755
>> --- a/t/t0000-basic.sh
>> +++ b/t/t0000-basic.sh
>> @@ -468,6 +468,17 @@ test_expect_success 'real path works on absolute paths' '
>>  	test "$d/$nopath" = "$(test-path-utils real_path "$d/$nopath")"
>>  '
>>  
>> +test_expect_success 'real path removes extra slashes' '
>> +	nopath="hopefully-absent-path" &&
>> +	test "/" = "$(test-path-utils real_path "///")" &&
>> +	test "/$nopath" = "$(test-path-utils real_path "///$nopath")" &&
> 
> Same here: test-path-utils operates on a DOS-style absolute path.

ACK.

> Furthermore, as Junio pointed out elsewhere, it is desirable that slashes
> (dir-separators) at the beginning are not collapsed if there are exactly
> two of them. Three or more can be collapsed to a single slash.

The empirical behavior of real_path() on Linux is (assuming "/tmp"
exists and "/foo" does not)

                         Before my changes       After my changes
real_path("/tmp")        /tmp                    /tmp
real_path("//tmp")       /tmp                    /tmp
real_path("/foo")        $(pwd)/foo              /foo
real_path("//foo")       /foo                    /foo

real_path("/tmp/bar")    /tmp/bar                /tmp/bar
real_path("//tmp/bar")   /tmp/bar                /tmp/bar
real_path("/foo/bar")    --dies--                --dies--
real_path("//foo/bar")   --dies--                --dies--

So I think that my changes makes things strictly better (albeit probably
not perfect).

real_path() relies on chdir() / getcwd() to canonicalize the path,
except for one case:

If the specified path is not itself a directory, then it strips off the
last component of the name, tries chdir() / getcwd() on the shortened
path, then pastes the last component on again.  The stripping off is
done by find_last_dir_sep(), which literally just looks for the last '/'
(or for Windows also '\') in the string.  Please note that the pasting
back is done using '/' as a separator.

So I think that the only problematic case is a path that only includes a
single group of slashes, like "//foo" or maybe "c:\\foo", and also is
not is_directory() [1].  What should be the algorithm for such cases,
and how should it depend on the platform?  (And does it really matter?
Top-level Linux paths are usually directories.  Are Windows-style
network shares also considered directories in the sense of is_directory()?)

I will make an easy improvement: not to remove *any* slashes when
stripping the last path component from the end of the path (letting
chdir() deal with them).  This results in the same results for Linux and
perhaps more hope under Windows:

On Linux: "//foo" -> (chdir("//"), "foo") -> ("/", "foo") -> "/foo"

On Windows: "//foo" -> (chdir("//"), "foo") -> (?, "foo") -> ?
(what is the result of chdir("//") then getcwd()?)

On Windows: "c:\foo" -> (chdir("c:\"), "foo") -> (?, "foo") -> ?
(what is the result of chdir("c:\") then getcwd()?)

>> +	# We need an existing top-level directory to use in the
>> +	# remaining tests.  Use the top-level ancestor of $(pwd):
>> +	d=$(pwd -P | sed -e "s|^\(/[^/]*\)/.*|\1|") &&
> 
> Same here: Account for the drive letter-colon.

ACK

>> +	test "$d" = "$(test-path-utils real_path "//$d///")" &&
>> +	test "$d/$nopath" = "$(test-path-utils real_path "//$d///$nopath")"
> 
> Since $d is a DOS-style path on Windows, //$d means something entirely
> different than $d. You should split the test in two: One test checks that
> slashes at the end or before $nopath are collapsed (this must work on
> Windows as well), and another test protected by POSIX prerequisite to
> check that slashes at the beginning are collapsed.

Done.

Thanks again for your help.

Michael

[1] If there is more than one group of slashes in the name, like
"//foo//bar" or "c:\\foo\\bar" then:

* All but the last groups of slashes are handled by
  chdir("//foo/")/getcwd() and presumably de-duplicated or not as
  appropriate

* The extras from the last group of slashes are trailing slashes in
  the string passed to chdir() and are therefore removed.

so everything should be OK.

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 0/7] Fix some bugs in abspath.c

[Junio C Hamano]

mhagger@alum.mit.edu writes:

> From: Michael Haggerty <mhagger@alum.mit.edu>
>
> I really just wanted to tidy up filter_refs(), but I've been sucked
> into a cascade of recursive yak shaving.  This is my first attempt to
> pop the yak stack.

Thanks.

> Please note that both absolute_path("") and real_path("") used to
> return the current directory followed by a slash.  I believe that this
> was a bug, and that it is more appropriate for both functions to
> reject the empty string.  The evidence is as follows:
>
> * If this were intended behavior, presumably the return value would
>   *not* have a trailing slash.

That is weak.  The only thing you can infer from that observation is
that the presense or absense of trailing '/' would not make any
difference to the caller who wanted a path to the cwd (and is more
convenient if the call is made so that a path relative to the cwd is
tucked after it).

> * I couldn't find any callers that appeared to depend on the old
>   behavior.

That is a very good argument (especially if the audit were
thorough).

I would be tempted to say that we should die() on "" for now, cook
the result outside "master" for a few weeks while auditing the
callchains, and see if any of them complains.

Re: [PATCH 0/7] Fix some bugs in abspath.c

[Junio C Hamano]

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

> mhagger@alum.mit.edu writes:
>
>> Please note that both absolute_path("") and real_path("") used to
>> return the current directory followed by a slash.  I believe that this
>> was a bug, and that it is more appropriate for both functions to
>> reject the empty string.  The evidence is as follows:
>>
>> * If this were intended behavior, presumably the return value would
>>   *not* have a trailing slash.
>
> That is weak.  The only thing you can infer from that observation is
> that the presense or absense of trailing '/' would not make any
> difference to the caller who wanted a path to the cwd (and is more
> convenient if the call is made so that a path relative to the cwd is
> tucked after it).

I still wonder why you want to reject "" as an input, as it could be
argued that it is a valid way to say the current directory.

What does realpath(3) do?

    ... goes and looks ...

The realpath(3) wants an existing path, and "" naturally gives NOENT,
so we cannot really draw parallel from it.

Ok, let's do this again.

    $ ./test-path-utils absolute_path ""
    /srv/git/git.git/
    $ ./test-path-utils absolute_path "foo"
    /srv/git/git.git/foo

so a caller has to be prepared to see the returned path sometimes
terminated with slash and sometimes without, to form an absolute
path to the file "bar" in the directory it gives to these functions
(i.e. "/srv/git/git.git/bar" vs "/srv/git/git.git/foo/bar").

That is a reasonably good sign that either nobody calls these
functions with "" or existing callers are buggy and would not handle
that case well and need to be fixed anyway.

So I am fine with die() in your patch.

Thanks.

>> * I couldn't find any callers that appeared to depend on the old
>>   behavior.
>
> That is a very good argument (especially if the audit were
> thorough).
>
> I would be tempted to say that we should die() on "" for now, cook
> the result outside "master" for a few weeks while auditing the
> callchains, and see if any of them complains.