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

[Michael Haggerty <mhagger@alum.mit.edu>]

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/