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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 3, 2023, 21:21 UTC
Message-ID: <xmqqwmw3wgeo.fsf@gitster.g>
In-Reply-To: <20231003082323.3002663-1-stepnem@smrk.net>

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

Show 37 quoted lines

> 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