[PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash

[PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash

[Štěpán Němec]

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
-- 
2.42.0

Re: [PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash

[Junio C Hamano]

Š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

Re: [PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash

[Junio C Hamano]

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

Re: [PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash

[Štěpán Němec]

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

Re: [PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash

[Junio C Hamano]

Š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
 

Re: [PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash

[Štěpán Němec]

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

Re: [PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash

[Junio C Hamano]

Š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.

Re: [PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash

[Štěpán Němec]

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

Re: [PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash

[Junio C Hamano]

Š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.