| Message ID | 20231003082323.3002663-1-stepnem@smrk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | test-lib: make sure TEST_DIRECTORY has no trailing slash | expand |
Štěpán Němec <stepnem@smrk.net> writes: > Turns out having `pwd` (which TEST_DIRECTORY defaults to) print $PWD > with a trailing slash isn't very difficult, in my case it went something > like > > ; tmux new-window -c ~/src/git/t/ > [in the new window] > ; sh ./t0000-basic.sh > PANIC: Running in a /home/stepnem/src/git/t/ that doesn't end in '/t'? > ; pwd > /home/stepnem/src/git/t/ > > (tmux(1) apparently sets PWD in the environment in addition to calling > chdir(2), which seems enough to make at least some shells preserve the > trailing slash in `pwd` output.) > > Strip the trailing slash, if present, to prevent bailing out with the > PANIC message in such cases. > > Signed-off-by: Štěpán Němec <stepnem@smrk.net> > --- > t/test-lib.sh | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 1656c9eed006..3b6f1a17e349 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -35,6 +35,7 @@ else > # needing to exist. > TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) || exit 1 > fi > +TEST_DIRECTORY="${TEST_DIRECTORY%/}" > if test -z "$TEST_OUTPUT_DIRECTORY" > then > # Similarly, override this to store the test-results subdir > > base-commit: d0e8084c65cbf949038ae4cc344ac2c2efd77415 While this would certainly squelch the particular test on TEST_DIRECTORY I am not sure if a safer fix would be to fix your PWD when it has a trailing slash. Your tests with this patch may be taking unintended branch when they do something like if test "$TRASH_DIRECTORY" = "$(pwd)" then ... fi as TRASH_DIRECTORY, whose default is derived from TEST_DIRECTORY and thanks to your fix it now does not have an extra slash in it, does not have a trailing slash but your $(pwd) or $PWD would have the trailing slash, simply because the patch only fixes TEST_DIRECTORY without fixing PWD. I wonder if this would be a safer alternative, or is it doing too much more than what is necessary? t/test-lib.sh | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git c/t/test-lib.sh w/t/test-lib.sh index 1656c9eed0..6ec42eab0d 100644 --- c/t/test-lib.sh +++ w/t/test-lib.sh @@ -22,19 +22,26 @@ then # ensure that TEST_DIRECTORY is an absolute path so that it # is valid even if the current working directory is changed TEST_DIRECTORY=$(pwd) -else - # The TEST_DIRECTORY will always be the path to the "t" - # directory in the git.git checkout. This is overridden by - # e.g. t/lib-subtest.sh, but only because its $(pwd) is - # different. Those tests still set "$TEST_DIRECTORY" to the - # same path. - # - # See use of "$GIT_BUILD_DIR" and "$TEST_DIRECTORY" below for - # hard assumptions about "$GIT_BUILD_DIR/t" existing and being - # the "$TEST_DIRECTORY", and e.g. "$TEST_DIRECTORY/helper" - # needing to exist. - TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) || exit 1 fi + +# The TEST_DIRECTORY will always be the path to the "t" +# directory in the git.git checkout. This is overridden by +# e.g. t/lib-subtest.sh, but only because its $(pwd) is +# different. Those tests still set "$TEST_DIRECTORY" to the +# same path. +# +# See use of "$GIT_BUILD_DIR" and "$TEST_DIRECTORY" below for +# hard assumptions about "$GIT_BUILD_DIR/t" existing and being +# the "$TEST_DIRECTORY", and e.g. "$TEST_DIRECTORY/helper" +# needing to exist. +# +# Also, apparently a shell spawned in some tricky way can be +# talked into keeping a slash at the end of its $PWD. +# +# All of the above can be worked around by doing an extra chdir +# and asking where we ended up to be. +TEST_DIRECTORY=$(cd "$TEST_DIRECTORY/." && pwd) || exit 1 + if test -z "$TEST_OUTPUT_DIRECTORY" then # Similarly, override this to store the test-results subdir
Junio C Hamano <gitster@pobox.com> writes: > I wonder if this would be a safer alternative, or is it doing too > much more than what is necessary? An alternative with much smaller blast radius would be to do this. Hopefully, by going "$(pwd)/." before asking the value returned by the `pwd` command, we can make sure that the trailing slash is removed (or at least $(pwd) and $TEST_DIRECTORY should be identical after this is done). t/test-lib.sh | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git c/t/test-lib.sh w/t/test-lib.sh index 1656c9eed0..d159358b21 100644 --- c/t/test-lib.sh +++ w/t/test-lib.sh @@ -19,9 +19,13 @@ # t/ subdirectory and are run in 'trash directory' subdirectory. if test -z "$TEST_DIRECTORY" then - # ensure that TEST_DIRECTORY is an absolute path so that it + # Ensure that TEST_DIRECTORY is an absolute path so that it # is valid even if the current working directory is changed - TEST_DIRECTORY=$(pwd) + # Some environments can talk the shell into keeping trailing + # slash in $PWD---go there and ask where we are to work it + # around, as we expect TEST_DIRECTORY and PWD are both + # canonical and can textually be compared for equality + TEST_DIRECTORY=$(cd "$(pwd)/." && pwd) else # The TEST_DIRECTORY will always be the path to the "t" # directory in the git.git checkout. This is overridden by
On Tue, 03 Oct 2023 14:57:39 -0700 Junio C. Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> I wonder if this would be a safer alternative, or is it doing too >> much more than what is necessary? > > An alternative with much smaller blast radius would be to do this. > > Hopefully, by going "$(pwd)/." before asking the value returned by > the `pwd` command, we can make sure that the trailing slash is > removed (or at least $(pwd) and $TEST_DIRECTORY should be identical > after this is done). > > > > t/test-lib.sh | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git c/t/test-lib.sh w/t/test-lib.sh > index 1656c9eed0..d159358b21 100644 > --- c/t/test-lib.sh > +++ w/t/test-lib.sh > @@ -19,9 +19,13 @@ > # t/ subdirectory and are run in 'trash directory' subdirectory. > if test -z "$TEST_DIRECTORY" > then > - # ensure that TEST_DIRECTORY is an absolute path so that it > + # Ensure that TEST_DIRECTORY is an absolute path so that it > # is valid even if the current working directory is changed > - TEST_DIRECTORY=$(pwd) > + # Some environments can talk the shell into keeping trailing > + # slash in $PWD---go there and ask where we are to work it > + # around, as we expect TEST_DIRECTORY and PWD are both > + # canonical and can textually be compared for equality > + TEST_DIRECTORY=$(cd "$(pwd)/." && pwd) > else > # The TEST_DIRECTORY will always be the path to the "t" > # directory in the git.git checkout. This is overridden by Yes, actually, AFAICT just $(cd . && pwd) fixes things (and saves a few syscalls), and I agree this is a much better approach than my naive fix. And functionally it should be equivalent to your other solution that did this unconditionally, because the else branch (TEST_DIRECTORY set) was already cd-ing anyway. Thanks, Štěpán
Štěpán Němec <stepnem@smrk.net> writes: > Yes, actually, AFAICT just $(cd . && pwd) fixes things (and saves a few > syscalls), and I agree this is a much better approach than my naive fix. Actually I was still being silly. We sometimes do val=$(cd there && do something there) so that we can get the output from a command in a different directory _without_ having to move our current directory. But the point of this current topic is that we _need_ to convince the shell that the path to our current directory is a canonical one without trailing slash, so my silly 'cd "$(pwd)/."' (or your "cd .") should be done outside the command expansion, or the canonicalized $PWD will only appear inside the $() and the next reference to $(pwd) or $PWD in the test script will still give the path with the trailing slash, that is textually different from $TEST_DIRECTORY. I wonder if this works better for you. We would be sure that $PWD and $TEST_DIRECTORY (when the latter is not imported from the environment) are the same, so "your cwd that does not end with /t and has a trailing slash after it" would be gone. Any $PWD or $(pwd) the tests refer to later in the step will also lack the unwanted trailing slash. As long as "cd ." is sufficient to cause the shell reexamine and canonicalize the $PWD, that is. Thanks. t/test-lib.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git c/t/test-lib.sh w/t/test-lib.sh index 1656c9eed0..a7045e028c 100644 --- c/t/test-lib.sh +++ w/t/test-lib.sh @@ -19,6 +19,11 @@ # t/ subdirectory and are run in 'trash directory' subdirectory. if test -z "$TEST_DIRECTORY" then + # It is reported that some shells spawned in tricky ways can + # give $PWD with a trailing slash. An explicit chdir hopefully + # would wake them out of their hallucination. + cd . + # ensure that TEST_DIRECTORY is an absolute path so that it # is valid even if the current working directory is changed TEST_DIRECTORY=$(pwd) @@ -626,7 +631,6 @@ fi # Protect ourselves from common misconfiguration to export # CDPATH into the environment unset CDPATH - unset GREP_OPTIONS unset UNZIP
On Wed, 04 Oct 2023 09:19:40 -0700 Junio C. Hamano wrote: > Štěpán Němec <stepnem@smrk.net> writes: > >> Yes, actually, AFAICT just $(cd . && pwd) fixes things (and saves a few >> syscalls), and I agree this is a much better approach than my naive fix. > > Actually I was still being silly. We sometimes do > > val=$(cd there && do something there) > > so that we can get the output from a command in a different > directory _without_ having to move our current directory. But the > point of this current topic is that we _need_ to convince the shell > that the path to our current directory is a canonical one without > trailing slash, so my silly 'cd "$(pwd)/."' (or your "cd .") should > be done outside the command expansion, or the canonicalized $PWD will > only appear inside the $() and the next reference to $(pwd) or $PWD > in the test script will still give the path with the trailing slash, > that is textually different from $TEST_DIRECTORY. > > I wonder if this works better for you. We would be sure that $PWD > and $TEST_DIRECTORY (when the latter is not imported from the > environment) are the same, so "your cwd that does not end with /t > and has a trailing slash after it" would be gone. Any $PWD or $(pwd) > the tests refer to later in the step will also lack the unwanted > trailing slash. As long as "cd ." is sufficient to cause the shell > reexamine and canonicalize the $PWD, that is. > > Thanks. > > t/test-lib.sh | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git c/t/test-lib.sh w/t/test-lib.sh > index 1656c9eed0..a7045e028c 100644 > --- c/t/test-lib.sh > +++ w/t/test-lib.sh > @@ -19,6 +19,11 @@ > # t/ subdirectory and are run in 'trash directory' subdirectory. > if test -z "$TEST_DIRECTORY" > then > + # It is reported that some shells spawned in tricky ways can > + # give $PWD with a trailing slash. An explicit chdir hopefully > + # would wake them out of their hallucination. > + cd . > + > # ensure that TEST_DIRECTORY is an absolute path so that it > # is valid even if the current working directory is changed > TEST_DIRECTORY=$(pwd) > @@ -626,7 +631,6 @@ fi > # Protect ourselves from common misconfiguration to export > # CDPATH into the environment > unset CDPATH > - > unset GREP_OPTIONS > unset UNZIP Yes, this is even simpler and more obvious, although for some reason, the subshell version works just as well, i.e., with just TEST_DIRECTORY=$(cd . && pwd) (no cd in the parent) in test-lib.sh and cat <<EOF >./tslash.sh #!/bin/sh test_description='yada yada' . ./test-lib.sh test_expect_success 'check TEST_DIRECTORY for trailing slash' ' echo "$TEST_DIRECTORY" && test "$TEST_DIRECTORY" = "${TEST_DIRECTORY%/}" ' test_done EOF I get ; echo ${TEST_DIRECTORY-unset} unset ; pwd /home/stepnem/src/git/t/ ; echo $PWD /home/stepnem/src/git/t/ ; sh ./tslash.sh -v Initialized empty Git repository in /home/stepnem/src/git/t/trash directory.tslash/.git/ expecting success of slash.1 'check TEST_DIRECTORY for trailing slash': echo "$TEST_DIRECTORY" && test "$TEST_DIRECTORY" = "${TEST_DIRECTORY%/}" /home/stepnem/src/git/t ok 1 - check TEST_DIRECTORY for trailing slash # passed all 1 test(s) 1..1 ; pwd /home/stepnem/src/git/t/ ; echo $PWD /home/stepnem/src/git/t/ [and, needless to say, reverting the changes still triggers the panic: ; sh ./tslash.sh PANIC: Running in a /home/stepnem/src/git/t/ that doesn't end in '/t'? ] Go figure... In any case, for what it's worth, any of your versions is Tested-by: Štěpán Němec <stepnem@smrk.net> Thanks, Štěpán
Štěpán Němec <stepnem@smrk.net> writes: > Yes, this is even simpler and more obvious, although for some reason, > the subshell version works just as well, i.e., with just > TEST_DIRECTORY=$(cd . && pwd) (no cd in the parent) in test-lib.sh and > > cat <<EOF >./tslash.sh > #!/bin/sh > test_description='yada yada' > > . ./test-lib.sh > > test_expect_success 'check TEST_DIRECTORY for trailing slash' ' > echo "$TEST_DIRECTORY" && > test "$TEST_DIRECTORY" = "${TEST_DIRECTORY%/}" > ' > > test_done > EOF If the only thing you checked was if TEST_DIRECTORY has or does not have a trailing slash, then it is totally understandable how and why the chdir inside subshell works. The value $(pwd) in the subshell returns, that is assigned to TEST_DIRECTORY, is canonicalized. But the outer shell still thinks $(pwd) is with trailing slash, and the above does not test it. test_expect_success 'check $PWD for trailing slash' ' echo "$PWD" && test "$PWD" = "${PWD%/}" ' The primary reason why I said I was silly doing it in a subshell was because I wanted to make sure that any test that refers to $PWD or $(pwd) later in the code would not get upset the same way with trailing slash after $PWD or $(pwd). As long as it is not overdone, it is a good practice to consider possibilities of similar problems triggered by the same root cause, which we may not have got bitten by yet, and preventing it from happening with the same fix. Thanks.
On Wed, 04 Oct 2023 10:15:30 -0700 Junio C. Hamano wrote: > Štěpán Němec <stepnem@smrk.net> writes: > >> Yes, this is even simpler and more obvious, although for some reason, >> the subshell version works just as well, i.e., with just >> TEST_DIRECTORY=$(cd . && pwd) (no cd in the parent) in test-lib.sh and >> >> cat <<EOF >./tslash.sh >> #!/bin/sh >> test_description='yada yada' >> >> . ./test-lib.sh >> >> test_expect_success 'check TEST_DIRECTORY for trailing slash' ' >> echo "$TEST_DIRECTORY" && >> test "$TEST_DIRECTORY" = "${TEST_DIRECTORY%/}" >> ' >> >> test_done >> EOF > > If the only thing you checked was if TEST_DIRECTORY has or does not > have a trailing slash, then it is totally understandable how and why > the chdir inside subshell works. The value $(pwd) in the subshell > returns, that is assigned to TEST_DIRECTORY, is canonicalized. But > the outer shell still thinks $(pwd) is with trailing slash, and the > above does not test it. Oh my, of course... thanks. > > test_expect_success 'check $PWD for trailing slash' ' > echo "$PWD" && > test "$PWD" = "${PWD%/}" > ' > The primary reason why I said I was silly doing it in a subshell was > because I wanted to make sure that any test that refers to $PWD or > $(pwd) later in the code would not get upset the same way with > trailing slash after $PWD or $(pwd). The $PWD inside the test is the trash directory, though, so that's not the problematic $PWD with a trailing slash any more, so I guess this problem can't really happen in a test that follows the '. ./test-lib.sh' (which does cd -P "$TRASH_DIRECTORY") convention?
Štěpán Němec <stepnem@smrk.net> writes: > The $PWD inside the test is the trash directory, though, so that's not > the problematic $PWD with a trailing slash any more, so I guess this > problem can't really happen in a test that follows the '. ./test-lib.sh' > (which does cd -P "$TRASH_DIRECTORY") convention? Yeah, after the dot-include of test-lib.sh returns, everything should be safe. What happens inside test-lib.sh (and worse yet, inside future versions of that file) before that "cd -P" is what the extra-carefulness protects against.
diff --git a/t/test-lib.sh b/t/test-lib.sh index 1656c9eed006..3b6f1a17e349 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -35,6 +35,7 @@ else # needing to exist. TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) || exit 1 fi +TEST_DIRECTORY="${TEST_DIRECTORY%/}" if test -z "$TEST_OUTPUT_DIRECTORY" then # Similarly, override this to store the test-results subdir
Turns out having `pwd` (which TEST_DIRECTORY defaults to) print $PWD with a trailing slash isn't very difficult, in my case it went something like ; tmux new-window -c ~/src/git/t/ [in the new window] ; sh ./t0000-basic.sh PANIC: Running in a /home/stepnem/src/git/t/ that doesn't end in '/t'? ; pwd /home/stepnem/src/git/t/ (tmux(1) apparently sets PWD in the environment in addition to calling chdir(2), which seems enough to make at least some shells preserve the trailing slash in `pwd` output.) Strip the trailing slash, if present, to prevent bailing out with the PANIC message in such cases. Signed-off-by: Štěpán Němec <stepnem@smrk.net> --- t/test-lib.sh | 1 + 1 file changed, 1 insertion(+) base-commit: d0e8084c65cbf949038ae4cc344ac2c2efd77415