[PATCH 0/3] tests: fix tests broken if a " " is in the checkout dir's path

[PATCH 0/3] tests: fix tests broken if a " " is in the checkout dir's path

[Ævar Arnfjörð Bjarmason]

The 1/3 here fixes regressions in v2.37.0-rc0, and with 2-3/3 the test
suite passes with the "git" directory named e.g. "git ~ @ checkout"
when running:

	make NO_PERL=Y test

The "NO_PERL=Y" being because several git-svn tests still fail, for
reasons I haven't looked into.

Ævar Arnfjörð Bjarmason (3):
  tests: add missing double quotes to included library paths
  test-lib.sh: fix prepend_var() quoting issue
  config tests: fix harmless but broken "rm -r" cleanup

 t/t1300-config.sh         | 2 +-
 t/t3700-add.sh            | 2 +-
 t/t3903-stash.sh          | 2 +-
 t/t7609-mergetool--lib.sh | 2 +-
 t/test-lib.sh             | 4 ++--
 5 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.36.1.1239.gfba91521d90

[PATCH 1/3] tests: add missing double quotes to included library paths

[Ævar Arnfjörð Bjarmason]

Fix two inclusions added in d42bab442d7 (core.fsyncmethod: tests for
batch mode, 2022-04-04) that needed to be quoted, and the same sort of
issue in a242c150ebb (vimdiff: integrate layout tests in the unit
tests framework ('t' folder), 2022-03-30). Both were first released
with v2.37.0-rc0.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t3700-add.sh            | 2 +-
 t/t3903-stash.sh          | 2 +-
 t/t7609-mergetool--lib.sh | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 8979c8a5f03..8689b48589c 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -8,7 +8,7 @@ test_description='Test of git add, including the -- option.'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-. $TEST_DIRECTORY/lib-unique-files.sh
+. "$TEST_DIRECTORY"/lib-unique-files.sh
 
 # Test the file mode "$1" of the file "$2" in the index.
 test_mode_in_index () {
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 20e94881964..2a4c3fd61c0 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -9,7 +9,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 . ./test-lib.sh
-. $TEST_DIRECTORY/lib-unique-files.sh
+. "$TEST_DIRECTORY"/lib-unique-files.sh
 
 test_expect_success 'usage on cmd and subcommand invalid option' '
 	test_expect_code 129 git stash --invalid-option 2>usage &&
diff --git a/t/t7609-mergetool--lib.sh b/t/t7609-mergetool--lib.sh
index d848fe6442b..330d6d603d7 100755
--- a/t/t7609-mergetool--lib.sh
+++ b/t/t7609-mergetool--lib.sh
@@ -7,7 +7,7 @@ Testing basic merge tools options'
 . ./test-lib.sh
 
 test_expect_success 'mergetool --tool=vimdiff creates the expected layout' '
-	. $GIT_BUILD_DIR/mergetools/vimdiff &&
+	. "$GIT_BUILD_DIR"/mergetools/vimdiff &&
 	run_unit_tests
 '
 
-- 
2.36.1.1239.gfba91521d90

[PATCH 2/3] test-lib.sh: fix prepend_var() quoting issue

[Ævar Arnfjörð Bjarmason]

Fix a quoting issue in the function introduced in
b9638d7286f (test-lib: make $GIT_BUILD_DIR an absolute path,
2022-02-27), running the test suite where the git checkout was on a
path with e.g. a space in it would fail.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 55857af601b..8cabb4d10f9 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -57,14 +57,14 @@ fi
 #
 #	prepend_var VAR : VALUE
 prepend_var () {
-	eval "$1=$3\${$1:+${3:+$2}\$$1}"
+	eval "$1=\"$3\${$1:+${3:+$2}\$$1}\""
 }
 
 # If [AL]SAN is in effect we want to abort so that we notice
 # problems. The GIT_SAN_OPTIONS variable can be used to set common
 # defaults shared between [AL]SAN_OPTIONS.
 prepend_var GIT_SAN_OPTIONS : abort_on_error=1
-prepend_var GIT_SAN_OPTIONS : strip_path_prefix=\"$GIT_BUILD_DIR/\"
+prepend_var GIT_SAN_OPTIONS : strip_path_prefix="$GIT_BUILD_DIR/"
 
 # If we were built with ASAN, it may complain about leaks
 # of program-lifetime variables. Disable it by default to lower
-- 
2.36.1.1239.gfba91521d90

[PATCH 3/3] config tests: fix harmless but broken "rm -r" cleanup

[Ævar Arnfjörð Bjarmason]

The "test_when_finished" cleanup phase added in 4179b4897f2 (config:
allow overriding of global and system configuration, 2021-04-19) has
never worked as intended, firstly the ".config/git" is a directory, so
we'd need the "-r" flag, but more importantly the $HOME variable
wasn't properly quoted.

We'd thus end up trying to remove the "trash" part of "trash
directory", which wouldn't fail with "-f", since "rm -f" won't fail on
non-existing files.

It's possible that this would have caused an actual failure if someone
had a $HOME with a space character in it, such that our "rm -f" would
fail to remove an existing directory, but in practice that probably
never happened.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1300-config.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index d3d9adbb3db..da4d03813f1 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2083,7 +2083,7 @@ test_expect_success '--show-scope with --show-origin' '
 '
 
 test_expect_success 'override global and system config' '
-	test_when_finished rm -f "$HOME"/.config/git &&
+	test_when_finished "rm -rf \"$HOME\"/.config/git" &&
 
 	cat >"$HOME"/.gitconfig <<-EOF &&
 	[home]
-- 
2.36.1.1239.gfba91521d90

Re: [PATCH 3/3] config tests: fix harmless but broken "rm -r" cleanup

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>  test_expect_success 'override global and system config' '
> -	test_when_finished rm -f "$HOME"/.config/git &&
> +	test_when_finished "rm -rf \"$HOME\"/.config/git" &&

As this string is evaled, isn't it safer to defer dereferencing the
environment variable at the runtime by quoting the dollar-sign, too?

I.e.

	test_when_finished rm -rf \"\$HOME\"/.config/git &&

or something?

Re: [PATCH 1/3] tests: add missing double quotes to included library paths

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Fix two inclusions added in d42bab442d7 (core.fsyncmethod: tests for
> batch mode, 2022-04-04) that needed to be quoted, and the same sort of
> issue in a242c150ebb (vimdiff: integrate layout tests in the unit
> tests framework ('t' folder), 2022-03-30). Both were first released
> with v2.37.0-rc0.

Describe what symptom you may see if it is left unfixed.  IOW, if we
consider that having a whitespace in the path to the build directory
is just as crazy as having a single-quote, it would make this patch
unnecessary,  Make it easier for readers to decide, by saying
something like

	As TEST_DIRECTORY is (typically) path to t/ in the extracted
	source tree, it can have funny characters like $IFS whitespace
	in it.

at least.

Thanks.

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t3700-add.sh            | 2 +-
>  t/t3903-stash.sh          | 2 +-
>  t/t7609-mergetool--lib.sh | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index 8979c8a5f03..8689b48589c 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -8,7 +8,7 @@ test_description='Test of git add, including the -- option.'
>  TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  
> -. $TEST_DIRECTORY/lib-unique-files.sh
> +. "$TEST_DIRECTORY"/lib-unique-files.sh
>  
>  # Test the file mode "$1" of the file "$2" in the index.
>  test_mode_in_index () {
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 20e94881964..2a4c3fd61c0 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -9,7 +9,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>  
>  . ./test-lib.sh
> -. $TEST_DIRECTORY/lib-unique-files.sh
> +. "$TEST_DIRECTORY"/lib-unique-files.sh
>  
>  test_expect_success 'usage on cmd and subcommand invalid option' '
>  	test_expect_code 129 git stash --invalid-option 2>usage &&
> diff --git a/t/t7609-mergetool--lib.sh b/t/t7609-mergetool--lib.sh
> index d848fe6442b..330d6d603d7 100755
> --- a/t/t7609-mergetool--lib.sh
> +++ b/t/t7609-mergetool--lib.sh
> @@ -7,7 +7,7 @@ Testing basic merge tools options'
>  . ./test-lib.sh
>  
>  test_expect_success 'mergetool --tool=vimdiff creates the expected layout' '
> -	. $GIT_BUILD_DIR/mergetools/vimdiff &&
> +	. "$GIT_BUILD_DIR"/mergetools/vimdiff &&
>  	run_unit_tests
>  '

Re: [PATCH 3/3] config tests: fix harmless but broken "rm -r" cleanup

[Junio C Hamano]

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

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>>  test_expect_success 'override global and system config' '
>> -	test_when_finished rm -f "$HOME"/.config/git &&
>> +	test_when_finished "rm -rf \"$HOME\"/.config/git" &&
>
> As this string is evaled, isn't it safer to defer dereferencing the
> environment variable at the runtime by quoting the dollar-sign, too?
>
> I.e.
>
> 	test_when_finished rm -rf \"\$HOME\"/.config/git &&
>
> or something?

It probably would matter if $HOME had a double-quote in it, I think