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
next prev parent 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.