Re: [BUG] range expressions in GIT_SKIP_TESTS are broken in master (was [BUG] question mark in GIT_SKIP_TESTS is broken in master)

Junio C Hamano [off-list ref] writes:

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.

Ævar?

With the crude debugging aid patch (attached at the end) applied,
running

    $ GIT_SKIP_TESTS='t?000' sh -x t0000-basic.sh -v

will show something interesting in the trace.

    ++ this_test=t0000
    ++ _s_k_i_p_='t?000'
    ++ match_pattern_list t0000 t5000

The variable $GIT_SKIP_TESTS on this line:

    if match_pattern_list "$this_test" $GIT_SKIP_TESTS

globs to t5000.  We don't quote the variable because we want them
separated at $IFS boundaries, but we didn't want the glob specials
in its value to take any effect.  Sigh.

The reason why edc23840b0 appears to break this is probably because
we are still in $TEST_DIRECTORY when this match_pattern_list is
executed; before that change, we've created $TRASH_DIRECTORY and
chdir'd there already, and when we check "do we want to skip all?",
there is nothing for the glob to match.

That also explains why GIT_SKIP_TESTS="t000?" appears to work.
There is no such filesystem entity directly in $TEST_DIRECTORY.

    $ echo t000? t00?0 t0?00 t?000
    t000? t00?0 t0200 t5000

diff --git i/t/test-lib.sh w/t/test-lib.sh
index 54938c6427..8ee0540532 100644
--- i/t/test-lib.sh
+++ w/t/test-lib.sh

@@ -1346,13 +1346,17 @@ fi
 remove_trash=
 this_test=${0##*/}
 this_test=${this_test%%-*}
+_s_k_i_p_=$GIT_SKIP_TESTS
+
 if 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"
 	test_done
 fi
 
+exit
+
 # Last-minute variable setup
 HOME="$TRASH_DIRECTORY"
 GNUPGHOME="$HOME/gnupg-home-not-used"

On Wed, Jun 16, 2021 at 01:12:09PM +0900, Junio C Hamano wrote:

The variable $GIT_SKIP_TESTS on this line:

    if match_pattern_list "$this_test" $GIT_SKIP_TESTS

globs to t5000.  We don't quote the variable because we want them
separated at $IFS boundaries, but we didn't want the glob specials
in its value to take any effect.  Sigh.

Good find.

It's surprisingly hard to do field-splitting without pathname globbing
in pure shell. I couldn't find a way without using "set -f". That's in
POSIX, but it feels funny tweaking a global that can effect how other
code runs. We can at least constraint it to a subshell close to the
point of use:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 54938c6427..093104d04f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -732,14 +732,15 @@ match_pattern_list () {
 	arg="$1"
 	shift
 	test -z "$*" && return 1
-	for pattern_
+	(set -f
+	for pattern_ in $*
 	do
 		case "$arg" in
 		$pattern_)
-			return 0
+			exit 0
 		esac
 	done
-	return 1
+	exit 1)
 }
 
 match_test_selector_list () {

@@ -848,7 +849,7 @@ maybe_teardown_verbose () {
 last_verbose=t
 maybe_setup_verbose () {
 	test -z "$verbose_only" && return
-	if match_pattern_list $test_count $verbose_only
+	if match_pattern_list $test_count "$verbose_only"
 	then
 		exec 4>&2 3>&1
 		# Emit a delimiting blank line when going from

@@ -878,7 +879,7 @@ maybe_setup_valgrind () {
 		return
 	fi
 	GIT_VALGRIND_ENABLED=
-	if match_pattern_list $test_count $valgrind_only
+	if match_pattern_list $test_count "$valgrind_only"
 	then
 		GIT_VALGRIND_ENABLED=t
 	fi

@@ -1006,7 +1007,7 @@ test_finish_ () {
 test_skip () {
 	to_skip=
 	skipped_reason=
-	if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
+	if match_pattern_list $this_test.$test_count "$GIT_SKIP_TESTS"
 	then
 		to_skip=t
 		skipped_reason="GIT_SKIP_TESTS"

@@ -1346,7 +1347,7 @@ fi
 remove_trash=
 this_test=${0##*/}
 this_test=${this_test%%-*}
-if match_pattern_list "$this_test" $GIT_SKIP_TESTS
+if 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"

If that isn't portable for some reason, I think we could fall back on
splitting with an external tool. You can't feed the result as a function
argument (you run into the same problem!) but you can use "read" to
split on newlines, like:

  echo "$GIT_SKIP_TESTS" |
  tr ' ' '\n' |
  while read pattern
  do
    echo "got $pattern"
  done

That does put the shell code on the right-hand side of a pipe, which
means it's constrained in terms of setting variables, etc. But that
would be acceptable for our use here.

I dunno. Maybe somebody else can come up with something more clever (or
maybe I am just missing something obvious).

-Peff