[PATCH] test-lib: fix "$remove_trash" regression and match_pattern_list() bugs

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

My edc23840b0b (test-lib: bring $remove_trash out of retirement,
2021-05-10) caused a regression where we'd fail to handle
GIT_SKIP_TESTS variable content like 't????', since we were calling
match_pattern_list() from the t/ directory not the trash
directory.

We'd thus glob match in the directory containing our tests, whereas
before we'd do the match in our newly setup (and empty, aside from the
.git) trash directory.

This bug reveals a more general problem though, which I'm attempting
to solve here: We have other users of match_pattern_list() that have
always been broken, namely the --verbose-only=* and --valgrind-only=*
options added in ff09af3fb8f (test-lib: verbose mode for only tests
matching a pattern, 2013-06-23) and 5dfc368f5ec (test-lib: valgrind
for only tests matching a pattern, 2013-06-23).

Those options will try to match an argument like --verbose-only=1*
against a test number like "10", but since we run the match in our own
trash directory an earlier test creating a file like "1one.txt" will
break that option.

We cannot simply quote the $GIT_SKIP_TESTS" being passed to
match_pattern_list(), since we are relying on the $IFS semantics.

Let's instead setup a .test-lib-trash subdirectory under the trash
directory, and an "empty-dir" directory under that. Then let's run the
match_pattern_list() in a sub-shell in that directory.

This fixes the regression in my edc23840b0b, as well as the bugs we've
had in the --{verbose,valgrind}-only=* options. I have tests for this,
but they're a pain to support without my in-flight lib-subtest.sh
changes, I'll submit them once those land.

This has the added benefit of providing a new clean work area for
tests in general, so functions in test-lib-functions.sh can use it for
their own scratch files, instead of potentially tripping up the test
logic itself.

The drawback is that any tests that cares about the complete
cleanliness of its test are might need to be adjusted, as one ls-files
test here does. I think it's still worth it because that'll be one
special case (and a dotfile), as opposed to every test potentially
tripping over the likes of "expected.config" on a glob match of "*".

1. https://lore.kernel.org/git/1d003cac-83fa-0b63-f60e-55513ac45cf9@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Wed, Jun 16 2021, Junio C Hamano wrote:

> Interestingly enough, edc23840b0 (test-lib: bring $remove_trash out
> of retirement, 2021-05-10) cleanly reverts without being depended on
> by anything else in the series.

I think reverting edc23840b0 might be the best short-term fix to avoid
dealing with the breakage + get a quick fix down. I can confirm that
nothing else in the series relies on it.

This patch is an attempt at a more general solution to the problem,
which depending on what you think you might want to take instead. It
fixes not only the immediate regression, but as noted other existing
bugs in match_pattern_list() usage.

 t/t3000-ls-files-others.sh |  8 ++++++--
 t/test-lib-functions.sh    |  8 +++++---
 t/test-lib.sh              | 40 +++++++++++++++++++++++---------------
 3 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/t/t3000-ls-files-others.sh b/t/t3000-ls-files-others.sh
index 740ce56eab5..8c62da502df 100755
--- a/t/t3000-ls-files-others.sh
+++ b/t/t3000-ls-files-others.sh
@@ -37,6 +37,7 @@ test_expect_success 'setup: expected output' '
 	cat >expected1 <<-\EOF &&
 	expected1
 	expected2
+	expected2.noempty
 	expected3
 	output
 	path0
@@ -47,7 +48,10 @@ test_expect_success 'setup: expected output' '
 
 	sed -e "s|path2/file2|path2/|" <expected1 >expected2 &&
 	cp expected2 expected3 &&
-	echo path4/ >>expected2
+	echo path4/ >>expected2 &&
+
+	echo .test-lib-trash/ >expected2.noempty &&
+	cat expected2 >>expected2.noempty
 '
 
 test_expect_success 'ls-files --others' '
@@ -57,7 +61,7 @@ test_expect_success 'ls-files --others' '
 
 test_expect_success 'ls-files --others --directory' '
 	git ls-files --others --directory >output &&
-	test_cmp expected2 output
+	test_cmp expected2.noempty output
 '
 
 test_expect_success '--no-empty-directory hides empty directory' '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index f0448daa74b..33697b0df81 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1040,10 +1040,12 @@ test_cmp_config () {
 		GD="-C $1" &&
 		shift
 	fi &&
-	printf "%s\n" "$1" >expect.config &&
+	printf "%s\n" "$1" >"$TRASH_DIRECTORY_TEST_LIB"/expect.config &&
 	shift &&
-	git $GD config "$@" >actual.config &&
-	test_cmp expect.config actual.config
+	git $GD config "$@" >"$TRASH_DIRECTORY_TEST_LIB"/actual.config &&
+	test_cmp \
+		"$TRASH_DIRECTORY_TEST_LIB"/expect.config \
+		"$TRASH_DIRECTORY_TEST_LIB"/actual.config
 }
 
 # test_cmp_bin - helper to compare binary files
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 54938c64279..7f284f56b1d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -260,6 +260,8 @@ case "$TRASH_DIRECTORY" in
 /*) ;; # absolute path is good
  *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
 esac
+TRASH_DIRECTORY_TEST_LIB="$TRASH_DIRECTORY/.test-lib-trash"
+TRASH_DIRECTORY_TEST_LIB_EMPTY="$TRASH_DIRECTORY_TEST_LIB/empty-dir"
 
 # If --stress was passed, run this test repeatedly in several parallel loops.
 if test "$GIT_TEST_STRESS_STARTED" = "done"
@@ -848,7 +850,8 @@ maybe_teardown_verbose () {
 last_verbose=t
 maybe_setup_verbose () {
 	test -z "$verbose_only" && return
-	if match_pattern_list $test_count $verbose_only
+	if (cd "$TRASH_DIRECTORY_TEST_LIB_EMPTY" &&
+	    match_pattern_list $test_count $verbose_only)
 	then
 		exec 4>&2 3>&1
 		# Emit a delimiting blank line when going from
@@ -878,7 +881,8 @@ maybe_setup_valgrind () {
 		return
 	fi
 	GIT_VALGRIND_ENABLED=
-	if match_pattern_list $test_count $valgrind_only
+	if (cd "$TRASH_DIRECTORY_TEST_LIB_EMPTY" &&
+	    match_pattern_list $test_count $valgrind_only)
 	then
 		GIT_VALGRIND_ENABLED=t
 	fi
@@ -1006,7 +1010,8 @@ test_finish_ () {
 test_skip () {
 	to_skip=
 	skipped_reason=
-	if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
+	if (cd "$TRASH_DIRECTORY_TEST_LIB_EMPTY" &&
+	    match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS)
 	then
 		to_skip=t
 		skipped_reason="GIT_SKIP_TESTS"
@@ -1177,7 +1182,7 @@ test_done () {
 			esac
 		fi
 
-		if test -z "$debug" && test -n "$remove_trash"
+		if test -z "$debug"
 		then
 			test -d "$TRASH_DIRECTORY" ||
 			error "Tests passed but trash directory already removed before test cleanup; aborting"
@@ -1342,11 +1347,22 @@ then
 	exit 1
 fi
 
+# Test repository
+rm -fr "$TRASH_DIRECTORY" || {
+	GIT_EXIT_OK=t
+	echo >&5 "FATAL: Cannot prepare test area"
+	exit 1
+}
+
+# Set up an early work area for the test code itself
+mkdir -p "$TRASH_DIRECTORY_TEST_LIB_EMPTY" >&3 2>&4 ||
+	error "cannot create test-lib.sh trash directory"
+
 # Are we running this test at all?
-remove_trash=
 this_test=${0##*/}
 this_test=${this_test%%-*}
-if match_pattern_list "$this_test" $GIT_SKIP_TESTS
+if (cd "$TRASH_DIRECTORY_TEST_LIB_EMPTY" &&
+    match_pattern_list "$this_test" $GIT_SKIP_TESTS)
 then
 	say_color info >&3 "skipping test $this_test altogether"
 	skip_all="skip all tests in $this_test"
@@ -1358,20 +1374,12 @@ HOME="$TRASH_DIRECTORY"
 GNUPGHOME="$HOME/gnupg-home-not-used"
 export HOME GNUPGHOME
 
-# Test repository
-rm -fr "$TRASH_DIRECTORY" || {
-	GIT_EXIT_OK=t
-	echo >&5 "FATAL: Cannot prepare test area"
-	exit 1
-}
-
-remove_trash=t
+# "We have the $TRASH_DIRECTORY already, but let's create a
+# $TRASH_DIRECTORY/.git
 if test -z "$TEST_NO_CREATE_REPO"
 then
 	git init "$TRASH_DIRECTORY" >&3 2>&4 ||
 	error "cannot run git init"
-else
-	mkdir -p "$TRASH_DIRECTORY"
 fi
 
 # Use -P to resolve symlinks in our working directory so that the cwd
-- 
2.32.0.555.g0268d380f7b