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

From: Jeff King <peff@peff.net>
Date: Jun 16, 2021, 08:29 UTC
Message-ID: <YMm2Wlb+eJDL7+ua@coredump.intra.peff.net>
In-Reply-To: <xmqq4kdysb1i.fsf@gitster.g>

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

Show 7 quoted lines

> 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