[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

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

[Ævar Arnfjörð Bjarmason]

A re-roll of
https://lore.kernel.org/git/cover-0.3-00000000000-20220621T221928Z-avarab@gmail.com/;
Changes since v1:

 - Clarified IFS splitting issues in 1/3, as requested by Junio
 - Fixed $HOME quoting issue in 3/3, and also added the missing cleanup
   for $HOME/.gitconfig to that test.

Æ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         | 5 +++--
 t/t3700-add.sh            | 2 +-
 t/t3903-stash.sh          | 2 +-
 t/t7609-mergetool--lib.sh | 2 +-
 t/test-lib.sh             | 4 ++--
 5 files changed, 8 insertions(+), 7 deletions(-)

Range-diff against v1:
1:  05ba5e7f837 ! 1:  f4ef137d076 tests: add missing double quotes to included library paths
    @@ Metadata
      ## Commit message ##
         tests: add missing double quotes to included library paths
     
    -    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.
    +    Fix inclusion errors which would occur if the $TEST_DIRECTORY had $IFS
    +    whitespace in it.
    +
    +    See d42bab442d7 (core.fsyncmethod: tests for batch mode, 2022-04-04)
    +    and a242c150ebb (vimdiff: integrate layout tests in the unit tests
    +    framework ('t' folder), 2022-03-30) for the two relevant commits. Both
    +    were first released with v2.37.0-rc0 (and were also part of v2.37.0).
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
2:  e06bf0cdfbe = 2:  b56ededf1b8 test-lib.sh: fix prepend_var() quoting issue
3:  f787b19f8c2 ! 3:  d3f65326701 config tests: fix harmless but broken "rm -r" cleanup
    @@ Commit message
         fail to remove an existing directory, but in practice that probably
         never happened.
     
    +    Let's fix both the quoting issue, and the other issue cleanup issue in
    +    4179b4897f2, which is that we were attempting to clean up
    +    ~/.config/git, but weren't cleaing up ~/.gitconfig.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## t/t1300-config.sh ##
    @@ t/t1300-config.sh: 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" &&
    - 
    +-
    ++	test_when_finished rm -f \"\$HOME\"/.gitconfig &&
      	cat >"$HOME"/.gitconfig <<-EOF &&
      	[home]
    + 		config = true
    + 	EOF
    ++
    ++	test_when_finished rm -rf \"\$HOME\"/.config/git &&
    + 	mkdir -p "$HOME"/.config/git &&
    + 	cat >"$HOME"/.config/git/config <<-EOF &&
    + 	[xdg]
-- 
2.37.0.880.gf07d56b18ba

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

[Ævar Arnfjörð Bjarmason]

Fix inclusion errors which would occur if the $TEST_DIRECTORY had $IFS
whitespace in it.

See d42bab442d7 (core.fsyncmethod: tests for batch mode, 2022-04-04)
and a242c150ebb (vimdiff: integrate layout tests in the unit tests
framework ('t' folder), 2022-03-30) for the two relevant commits. Both
were first released with v2.37.0-rc0 (and were also part of v2.37.0).

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.37.0.880.gf07d56b18ba

[PATCH v2 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.37.0.880.gf07d56b18ba

[PATCH v2 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.

Let's fix both the quoting issue, and the other issue cleanup issue in
4179b4897f2, which is that we were attempting to clean up
~/.config/git, but weren't cleaing up ~/.gitconfig.

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

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index d3d9adbb3db..c6661e61af5 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2083,12 +2083,13 @@ 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 -f \"\$HOME\"/.gitconfig &&
 	cat >"$HOME"/.gitconfig <<-EOF &&
 	[home]
 		config = true
 	EOF
+
+	test_when_finished rm -rf \"\$HOME\"/.config/git &&
 	mkdir -p "$HOME"/.config/git &&
 	cat >"$HOME"/.config/git/config <<-EOF &&
 	[xdg]
-- 
2.37.0.880.gf07d56b18ba

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

[Junio C Hamano]

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

> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index d3d9adbb3db..c6661e61af5 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -2083,12 +2083,13 @@ 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 -f \"\$HOME\"/.gitconfig &&

This does look more correct than the original.

>  	cat >"$HOME"/.gitconfig <<-EOF &&
>  	[home]
>  		config = true
>  	EOF
> +
> +	test_when_finished rm -rf \"\$HOME\"/.config/git &&
>  	mkdir -p "$HOME"/.config/git &&
>  	cat >"$HOME"/.config/git/config <<-EOF &&
>  	[xdg]

Nicely done.  Will queue.

Thanks.