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

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

[Michael Haggerty]

Second version of patches, incorporating gratefully received advice
from Hannes:

* Moved existing tests of real_path() from t0000 to t0060, and add new
  tests to t0060 instead of t0000.

* Adjusted the regexp used to construct a top-level directory that is
  needed for tests.

* Skip some tests when the POSIX prerequisite is not satisfied.

Michael Haggerty (8):
  t0060: move tests of real_path() from t0000 to here
  t0060: verify that absolute_path() fails if passed the empty string
  absolute_path(): reject the empty string
  t0060: verify that real_path() fails if passed the empty string
  real_path(): reject the empty string
  t0060: verify that real_path() works correctly with absolute paths
  real_path(): properly handle nonexistent top-level paths
  t0060: verify that real_path() removes extra slashes

 abspath.c             |  9 +++++++--
 t/t0000-basic.sh      | 18 -----------------
 t/t0060-path-utils.sh | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 20 deletions(-)

-- 
1.7.11.3

[PATCH v2 1/8] t0060: move tests of real_path() from t0000 to here

[Michael Haggerty]

Suggested by: Johannes Sixt <j6t@kdbg.org>

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t0000-basic.sh      | 18 ------------------
 t/t0060-path-utils.sh | 19 +++++++++++++++++++
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index ccb5435..ae6a3f0 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -450,24 +450,6 @@ test_expect_success 'update-index D/F conflict' '
 	test $numpath0 = 1
 '
 
-test_expect_success SYMLINKS 'real path works as expected' '
-	mkdir first &&
-	ln -s ../.git first/.git &&
-	mkdir second &&
-	ln -s ../first second/other &&
-	mkdir third &&
-	dir="$(cd .git; pwd -P)" &&
-	dir2=third/../second/other/.git &&
-	test "$dir" = "$(test-path-utils real_path $dir2)" &&
-	file="$dir"/index &&
-	test "$file" = "$(test-path-utils real_path $dir2/index)" &&
-	basename=blub &&
-	test "$dir/$basename" = "$(cd .git && test-path-utils real_path "$basename")" &&
-	ln -s ../first/file .git/syml &&
-	sym="$(cd first; pwd -P)"/file &&
-	test "$sym" = "$(test-path-utils real_path "$dir2/syml")"
-'
-
 test_expect_success 'very long name in the index handled sanely' '
 
 	a=a && # 1
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 53cf1f8..c8db144 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -139,4 +139,23 @@ test_expect_success 'strip_path_suffix' '
 	test c:/msysgit = $(test-path-utils strip_path_suffix \
 		c:/msysgit/libexec//git-core libexec/git-core)
 '
+
+test_expect_success SYMLINKS 'real path works as expected' '
+	mkdir first &&
+	ln -s ../.git first/.git &&
+	mkdir second &&
+	ln -s ../first second/other &&
+	mkdir third &&
+	dir="$(cd .git; pwd -P)" &&
+	dir2=third/../second/other/.git &&
+	test "$dir" = "$(test-path-utils real_path $dir2)" &&
+	file="$dir"/index &&
+	test "$file" = "$(test-path-utils real_path $dir2/index)" &&
+	basename=blub &&
+	test "$dir/$basename" = "$(cd .git && test-path-utils real_path "$basename")" &&
+	ln -s ../first/file .git/syml &&
+	sym="$(cd first; pwd -P)"/file &&
+	test "$sym" = "$(test-path-utils real_path "$dir2/syml")"
+'
+
 test_done
-- 
1.7.11.3

[PATCH v2 2/8] t0060: verify that absolute_path() fails if passed the empty string

[Michael Haggerty]

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

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

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index c8db144..d91e516 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -140,6 +140,10 @@ test_expect_success 'strip_path_suffix' '
 		c:/msysgit/libexec//git-core libexec/git-core)
 '
 
+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 v2 3/8] absolute_path(): reject the empty string

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 abspath.c             | 4 +++-
 t/t0060-path-utils.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/t0060-path-utils.sh b/t/t0060-path-utils.sh
index d91e516..924aa60 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -140,7 +140,7 @@ test_expect_success 'strip_path_suffix' '
 		c:/msysgit/libexec//git-core libexec/git-core)
 '
 
-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

Re: [PATCH v2 3/8] absolute_path(): reject the empty string

[Junio C Hamano]

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

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

I think I asked why this matters (iow, why it is the right thing to
do to reject an empty string, instead of treating it as "the current
directory") in the previous round.  I would have expected to find
the answer be above the S-o-b line here.

The same comment applies to the other patch for real_path().

>  abspath.c             | 4 +++-
>  t/t0060-path-utils.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/t0060-path-utils.sh b/t/t0060-path-utils.sh
> index d91e516..924aa60 100755
> --- a/t/t0060-path-utils.sh
> +++ b/t/t0060-path-utils.sh
> @@ -140,7 +140,7 @@ test_expect_success 'strip_path_suffix' '
>  		c:/msysgit/libexec//git-core libexec/git-core)
>  '
>  
> -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 ""
>  '

Re: [PATCH v2 3/8] absolute_path(): reject the empty string

[Michael Haggerty]

On 09/07/2012 01:09 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
> 
> I think I asked why this matters (iow, why it is the right thing to
> do to reject an empty string, instead of treating it as "the current
> directory") in the previous round.  I would have expected to find
> the answer be above the S-o-b line here.

The reasons that the change is desirable:

1. The empty string is not a legitimate path according to POSIX; e.g.,
see Linux's path_resolution(7):

   Empty pathname
       In the original UNIX, the empty pathname referred to the current
       directory.   Nowadays  POSIX  decrees  that  an  empty  pathname
       must not be resolved successfully.  Linux returns ENOENT in this
       case.

Accordingly, comparable standard functions like realpath(3) reject the
empty string.

2. The functions did not handle the empty path consistently with the way
they handled other paths (namely, the return value contained a trailing
slash).

3. This unusual behavior was undocumented.

The above points let me to the conclusion that the anomalous handling of
the empty string was a bug in the implementation rather than an intended
behavior.  Moreover, a quick check of callers didn't turn up any that
seemed to rely on the strange behavior.

Do you want a re-roll with this verbiage added to the commit messages of
the two relevant commits?

Michael

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

Re: [PATCH v2 3/8] absolute_path(): reject the empty string

[Junio C Hamano]

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

> On 09/07/2012 01:09 AM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> 
>>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>>> ---
>> 
>> I think I asked why this matters (iow, why it is the right thing to
>> do to reject an empty string, instead of treating it as "the current
>> directory") in the previous round.  I would have expected to find
>> the answer be above the S-o-b line here.
>
> The reasons that the change is desirable:
>
> 1. The empty string is not a legitimate path according to POSIX; e.g.,
> see Linux's path_resolution(7):
>
>    Empty pathname
>        In the original UNIX, the empty pathname referred to the current
>        directory.   Nowadays  POSIX  decrees  that  an  empty  pathname
>        must not be resolved successfully.  Linux returns ENOENT in this
>        case.
>
> Accordingly, comparable standard functions like realpath(3) reject the
> empty string.
>
> 2. The functions did not handle the empty path consistently with the way
> they handled other paths (namely, the return value contained a trailing
> slash).
>
> 3. This unusual behavior was undocumented.
>
> The above points let me to the conclusion that the anomalous handling of
> the empty string was a bug in the implementation rather than an intended
> behavior.  Moreover, a quick check of callers didn't turn up any that
> seemed to rely on the strange behavior.
>
> Do you want a re-roll with this verbiage added to the commit messages of
> the two relevant commits?

What the function used to do does not really matter when we are
trying to see if the behaviour of the new implementation makes
sense, even though it is a good supporting argument to help us judge
that this change will not cause regressions to existing callers.

A two sentence paragraph, with "realpath(3) from POSIX.1 rejects an
empty string as input" as the primary justification, with "no
existing callers passes an empty string and expects to get the
current directory" as a supporting argument, should suffice, I would
think.

[PATCH v2 4/8] t0060: verify that real_path() fails if passed the empty string

[Michael Haggerty]

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

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

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 924aa60..1118056 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -144,6 +144,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 v2 5/8] real_path(): reject the empty string

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 abspath.c             | 3 +++
 t/t0060-path-utils.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/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 1118056..fab5ea2 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -144,7 +144,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 v2 6/8] t0060: verify that real_path() works correctly with absolute paths

[Michael Haggerty]

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.

These tests are skipped on Windows because test-path-utils operates on
a DOS-style absolute path even if a POSIX style absolute path is
passed as argument.

Adjusted for Windows by: Johannes Sixt <j6t@kdbg.org>

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

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index fab5ea2..3121691 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -148,7 +148,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 POSIX '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

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

[Michael Haggerty]

The change has two points:

1. Do not strip off a leading slash, because that erroneously turns an
   absolute path into a relative path.

2. Do not remove slashes from groups of multiple slashes; instead let
   chdir() handle them.  It could be, for example, that it wants to
   leave leading double-slashes alone.

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

diff --git a/abspath.c b/abspath.c
index 3e8325c..05f2d79 100644
--- a/abspath.c
+++ b/abspath.c
@@ -45,8 +45,8 @@ 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);
+				last_slash[1] = '\0';
 			} else {
 				last_elem = xstrdup(buf);
 				*buf = '\0';
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 3121691..30361f9 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -148,7 +148,7 @@ test_expect_success 'real path rejects the empty string' '
 	test_must_fail test-path-utils real_path ""
 '
 
-test_expect_failure POSIX 'real path works on absolute paths' '
+test_expect_success POSIX '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 v2 8/8] t0060: verify that real_path() removes extra slashes

[Michael Haggerty]

Adjusted for Windows by: Johannes Sixt <j6t@kdbg.org>

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t0060-path-utils.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 30361f9..e40f764 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -158,6 +158,24 @@ test_expect_success POSIX 'real path works on absolute paths' '
 	test "$d/$nopath" = "$(test-path-utils real_path "$d/$nopath")"
 '
 
+test_expect_success POSIX 'real path removes extra leading slashes' '
+	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 'real path removes other extra slashes' '
+	nopath="hopefully-absent-path" &&
+	# 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 &&
-- 
1.7.11.3

[PATCH 9/8] t0060: split absolute path test in two to exercise some of it on Windows

[Johannes Sixt]

Only the first half of the test works only on POSIX, the second half
passes on Windows as well.

A later test "real path removes other extra slashes" looks very similar,
but it does not make sense to split it in the same way: When two slashes
are prepended in front of an absolute DOS-style path on Windows, the
meaning of the path is changed (//server/share style), so that the test
cannot pass on Windows.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 The series passes for me as is, but one test needs POSIX only in
 the first half. This patch splits it in two.

 t/t0060-path-utils.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index e40f764..4ef2345 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -148,10 +148,14 @@ test_expect_success 'real path rejects the empty string' '
 	test_must_fail test-path-utils real_path ""
 '
 
-test_expect_success POSIX 'real path works on absolute paths' '
+test_expect_success POSIX 'real path works on absolute paths 1' '
 	nopath="hopefully-absent-path" &&
 	test "/" = "$(test-path-utils real_path "/")" &&
-	test "/$nopath" = "$(test-path-utils real_path "/$nopath")" &&
+	test "/$nopath" = "$(test-path-utils real_path "/$nopath")"
+'
+
+test_expect_success 'real path works on absolute paths 2' '
+	nopath="hopefully-absent-path" &&
 	# 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")" &&
-- 
1.7.12.1493.g247bc0e

Re: [PATCH 9/8] t0060: split absolute path test in two to exercise some of it on Windows

[Michael Haggerty]

Looks fine to me.  Thanks!

Michael

On 09/09/2012 05:42 PM, Johannes Sixt wrote:
> Only the first half of the test works only on POSIX, the second half
> passes on Windows as well.
> 
> A later test "real path removes other extra slashes" looks very similar,
> but it does not make sense to split it in the same way: When two slashes
> are prepended in front of an absolute DOS-style path on Windows, the
> meaning of the path is changed (//server/share style), so that the test
> cannot pass on Windows.
> 
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  The series passes for me as is, but one test needs POSIX only in
>  the first half. This patch splits it in two.
> 
>  t/t0060-path-utils.sh | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
> index e40f764..4ef2345 100755
> --- a/t/t0060-path-utils.sh
> +++ b/t/t0060-path-utils.sh
> @@ -148,10 +148,14 @@ test_expect_success 'real path rejects the empty string' '
>  	test_must_fail test-path-utils real_path ""
>  '
>  
> -test_expect_success POSIX 'real path works on absolute paths' '
> +test_expect_success POSIX 'real path works on absolute paths 1' '
>  	nopath="hopefully-absent-path" &&
>  	test "/" = "$(test-path-utils real_path "/")" &&
> -	test "/$nopath" = "$(test-path-utils real_path "/$nopath")" &&
> +	test "/$nopath" = "$(test-path-utils real_path "/$nopath")"
> +'
> +
> +test_expect_success 'real path works on absolute paths 2' '
> +	nopath="hopefully-absent-path" &&
>  	# 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")" &&
> 


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