All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Andrei Rybak" <rybak.a.v@gmail.com>,
	git@vger.kernel.org
Subject: Re: [BUG] range expressions in GIT_SKIP_TESTS are broken in master (was [BUG] question mark in GIT_SKIP_TESTS is broken in master)
Date: Wed, 16 Jun 2021 04:29:14 -0400	[thread overview]
Message-ID: <YMm2Wlb+eJDL7+ua@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqq4kdysb1i.fsf@gitster.g>

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

  reply	other threads:[~2021-06-16  8:29 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-15 20:55 [BUG] question mark in GIT_SKIP_TESTS is broken in master Andrei Rybak
2021-06-15 23:28 ` [BUG] range expressions in GIT_SKIP_TESTS are broken in master (was [BUG] question mark in GIT_SKIP_TESTS is broken in master) Andrei Rybak
2021-06-16  3:28   ` Junio C Hamano
2021-06-16  4:12     ` Junio C Hamano
2021-06-16  8:29       ` Jeff King [this message]
2021-06-16  9:19         ` Junio C Hamano
2021-06-16  8:24     ` [PATCH] test-lib: fix "$remove_trash" regression and match_pattern_list() bugs Ævar Arnfjörð Bjarmason
2021-06-16  8:36       ` Jeff King
2021-06-16  9:22         ` Junio C Hamano
2021-06-16 10:23           ` Jeff King
2021-06-16 10:24             ` Jeff King
2021-06-16 11:38             ` Ævar Arnfjörð Bjarmason
2021-06-16 11:50               ` Jeff King
2021-06-17  0:29                 ` Junio C Hamano
2021-06-16  9:49         ` Ævar Arnfjörð Bjarmason
2021-06-16 11:43           ` Jeff King
2021-06-17  0:36           ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YMm2Wlb+eJDL7+ua@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=rybak.a.v@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.