Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths
On 09/07/2012 01:08 AM, Junio C Hamano wrote: mhag...@alum.mit.edu writes: From: Michael Haggerty mhag...@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 mhag...@alum.mit.edu --- t/t-basic.sh | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/t/t-basic.sh b/t/t-basic.sh index 1a51634..ad002ee 100755 --- a/t/t-basic.sh +++ b/t/t-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 mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths
Michael Haggerty mhag...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths
Junio C Hamano gits...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths
mhag...@alum.mit.edu writes: From: Michael Haggerty mhag...@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 mhag...@alum.mit.edu --- t/t-basic.sh | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/t/t-basic.sh b/t/t-basic.sh index 1a51634..ad002ee 100755 --- a/t/t-basic.sh +++ b/t/t-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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths
Am 9/4/2012 10:14, schrieb mhag...@alum.mit.edu: From: Michael Haggerty mhag...@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 mhag...@alum.mit.edu --- t/t-basic.sh | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/t/t-basic.sh b/t/t-basic.sh index 1a51634..ad002ee 100755 --- a/t/t-basic.sh +++ b/t/t-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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths
From: Michael Haggerty mhag...@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 mhag...@alum.mit.edu --- t/t-basic.sh | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/t/t-basic.sh b/t/t-basic.sh index 1a51634..ad002ee 100755 --- a/t/t-basic.sh +++ b/t/t-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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
