# [PATCH 0/3] tests: fix tests broken if a " " is in the checkout dir's path 12 messages from 2022-06-21 to 2022-06-30. Participants: Ævar Arnfjörð Bjarmason, Junio C Hamano. Thread: https://gitlist.dev/t/58045 ## Ævar Arnfjörð Bjarmason, 2022-06-21 22:21 Subject: [PATCH 0/3] tests: fix tests broken if a " " is in the checkout dir's path Message-ID: URL: https://gitlist.dev/e/cover-0.3-00000000000-20220621T221928Z-avarab%40gmail.com ``` 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 ``` ## Ævar Arnfjörð Bjarmason, 2022-06-21 22:21 Subject: [PATCH 1/3] tests: add missing double quotes to included library paths Message-ID: URL: https://gitlist.dev/e/patch-1.3-05ba5e7f837-20220621T221928Z-avarab%40gmail.com In-Reply-To: ``` 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 --- 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 ``` ## Ævar Arnfjörð Bjarmason, 2022-06-21 22:21 Subject: [PATCH 2/3] test-lib.sh: fix prepend_var() quoting issue Message-ID: URL: https://gitlist.dev/e/patch-2.3-e06bf0cdfbe-20220621T221928Z-avarab%40gmail.com In-Reply-To: ``` 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 --- 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 ``` ## Ævar Arnfjörð Bjarmason, 2022-06-21 22:21 Subject: [PATCH 3/3] config tests: fix harmless but broken "rm -r" cleanup Message-ID: URL: https://gitlist.dev/e/patch-3.3-f787b19f8c2-20220621T221928Z-avarab%40gmail.com In-Reply-To: ``` 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 --- 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 ``` ## Junio C Hamano, 2022-06-21 22:34 Subject: Re: [PATCH 3/3] config tests: fix harmless but broken "rm -r" cleanup Message-ID: URL: https://gitlist.dev/e/xmqq4k0dfxxm.fsf%40gitster.g In-Reply-To: ``` Ævar Arnfjörð Bjarmason 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? ``` ## Junio C Hamano, 2022-06-21 22:42 Subject: Re: [PATCH 1/3] tests: add missing double quotes to included library paths Message-ID: URL: https://gitlist.dev/e/xmqqzgi5ej0d.fsf%40gitster.g In-Reply-To: ``` Ævar Arnfjörð Bjarmason 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 > --- > 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 > ' ``` ## Junio C Hamano, 2022-06-22 05:37 Subject: Re: [PATCH 3/3] config tests: fix harmless but broken "rm -r" cleanup Message-ID: URL: https://gitlist.dev/e/xmqq8rpp8div.fsf%40gitster.g In-Reply-To: ``` Junio C Hamano writes: > Ævar Arnfjörð Bjarmason 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 ``` ## Ævar Arnfjörð Bjarmason, 2022-06-30 10:18 Subject: [PATCH v2 1/3] tests: add missing double quotes to included library paths Message-ID: URL: https://gitlist.dev/e/patch-v2-1.3-f4ef137d076-20220630T101646Z-avarab%40gmail.com In-Reply-To: ``` 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 --- 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 ``` ## Ævar Arnfjörð Bjarmason, 2022-06-30 10:18 Subject: [PATCH v2 0/3] tests: fix tests broken if a " " is in the checkout dir's path Message-ID: URL: https://gitlist.dev/e/cover-v2-0.3-00000000000-20220630T101646Z-avarab%40gmail.com In-Reply-To: ``` 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 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 ## 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 ``` ## Ævar Arnfjörð Bjarmason, 2022-06-30 10:18 Subject: [PATCH v2 2/3] test-lib.sh: fix prepend_var() quoting issue Message-ID: URL: https://gitlist.dev/e/patch-v2-2.3-b56ededf1b8-20220630T101646Z-avarab%40gmail.com In-Reply-To: ``` 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 --- 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 ``` ## Ævar Arnfjörð Bjarmason, 2022-06-30 10:18 Subject: [PATCH v2 3/3] config tests: fix harmless but broken "rm -r" cleanup Message-ID: URL: https://gitlist.dev/e/patch-v2-3.3-d3f65326701-20220630T101646Z-avarab%40gmail.com In-Reply-To: ``` 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 --- 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 ``` ## Junio C Hamano, 2022-06-30 20:52 Subject: Re: [PATCH v2 3/3] config tests: fix harmless but broken "rm -r" cleanup Message-ID: URL: https://gitlist.dev/e/xmqq8rpdeuwq.fsf%40gitster.g In-Reply-To: ``` Ævar Arnfjörð Bjarmason 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. ```