patchtest-lib: make sure TEST_DIRECTORY has no trailing slash

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(+)

Štěpán Němec <stepnem@smrk.net> writes:

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(-)

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

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(-)

On Tue, 03 Oct 2023 14:57:39 -0700 Junio C. Hamano wrote:

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:

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(-)

On Wed, 04 Oct 2023 09:19:40 -0700 Junio C. Hamano wrote:

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:

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:

Oh my, of course... thanks.

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:

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.