{"thread":{"id":"55939","subject":"[BUG] question mark in GIT_SKIP_TESTS is broken in master","startedAt":"2021-06-15T20:55:58Z","lastAt":"2021-06-17T00:36:42Z","messageCount":17,"participants":["Andrei Rybak","Junio C Hamano","Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"427528","messageId":"1d003cac-83fa-0b63-f60e-55513ac45cf9@gmail.com","threadId":"55939","inReplyTo":null,"subject":"[BUG] question mark in GIT_SKIP_TESTS is broken in master","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2021-06-15T20:55:48Z","receivedAt":"2021-06-15T20:55:58Z","isPatch":false,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Hi,\n\nI've taken an example of how to use GIT_SKIP_TESTS directly from\nt/README:\n\n     $ GIT_SKIP_TESTS='t[0-4]??? t91?? t9200.8' make\n\nand noticed that t0000 is still being run.  I've written a script to\nbisect the issue:\n\n\n     #/bin/sh\n\n     (\n             cd t\n             GIT_SKIP_TESTS='t[0-4]??? t91?? t9200.8' make | \\\n                     head | grep 'SKIP skip all tests in t0000'\n     )\n\n\nand it bisected to edc23840b0 (test-lib: bring $remove_trash out of\nretirement, 2021-05-10) which was merged to master in 2019256717 (\"Merge\nbranch 'ab/test-lib-updates'\", 2021-06-14).\n\nNote, that spelling out the tests number in full works, i.e.\n\n     $ GIT_SKIP_TESTS=t0000 make\n\nwon't run t0000.\n"},{"id":"427544","messageId":"6980e906-8076-1436-ecdb-6775eff55d39@gmail.com","threadId":"55939","inReplyTo":"1d003cac-83fa-0b63-f60e-55513ac45cf9@gmail.com","subject":"[BUG] range expressions in GIT_SKIP_TESTS are broken in master (was [BUG] question mark in GIT_SKIP_TESTS is broken in master)","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2021-06-15T23:28:33Z","receivedAt":"2021-06-15T23:28:40Z","isPatch":false,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 15/06/2021 22:55, Andrei Rybak wrote:\n> Hi,\n> \n> I've taken an example of how to use GIT_SKIP_TESTS directly from\n> t/README:\n> \n>      $ GIT_SKIP_TESTS='t[0-4]??? t91?? t9200.8' make\n> \n> and noticed that t0000 is still being run.  I've written a script to\n> bisect the issue:\n> \n> \n>      #/bin/sh\n> \n>      (\n>              cd t\n>              GIT_SKIP_TESTS='t[0-4]??? t91?? t9200.8' make | \\\n>                      head | grep 'SKIP skip all tests in t0000'\n>      )\n> \n> \n> and it bisected to edc23840b0 (test-lib: bring $remove_trash out of\n> retirement, 2021-05-10) which was merged to master in 2019256717 (\"Merge\n> branch 'ab/test-lib-updates'\", 2021-06-14).\n> \n> Note, that spelling out the tests number in full works, i.e.\n> \n>      $ GIT_SKIP_TESTS=t0000 make\n> \n> won't run t0000.\n\nI sent this originally with incorrect subject. Question marks, e.g.\n\n     $ GIT_SKIP_TESTS='t000?' make\n\nwork.  It is the range expressions, like [0-9] that are broken:\n\n     $ GIT_SKIP_TESTS='t[0-9]000' make\n"},{"id":"427569","messageId":"xmqqa6nqsd2i.fsf@gitster.g","threadId":"55939","inReplyTo":"6980e906-8076-1436-ecdb-6775eff55d39@gmail.com","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)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-16T03:28:21Z","receivedAt":"2021-06-16T03:28:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrei Rybak <rybak.a.v@gmail.com> writes:\n\n> I sent this originally with incorrect subject. Question marks, e.g.\n>\n>     $ GIT_SKIP_TESTS='t000?' make\n>\n> work.  It is the range expressions, like [0-9] that are broken:\n>\n>     $ GIT_SKIP_TESTS='t[0-9]000' make\n\nThis reproduces for me, and\n\n      $ GIT_SKIP_TESTS=t?000' sh t0000-basic.sh\n      $ GIT_SKIP_TESTS=t0?00' sh t0000-basic.sh\n      $ GIT_SKIP_TESTS=t00?0' sh t0000-basic.sh\n\ntells me that \"Question marks work\" above is not exactly correct.\nIt is an exception that this\n\n      $ GIT_SKIP_TESTS=t000?' sh t0000-basic.sh\n\nhappens to skip.\n\nInterestingly enough, edc23840b0 (test-lib: bring $remove_trash out\nof retirement, 2021-05-10) cleanly reverts without being depended on\nby anything else in the series.\n\nÆvar?\n\nThanks.\n"},{"id":"427572","messageId":"xmqq4kdysb1i.fsf@gitster.g","threadId":"55939","inReplyTo":"xmqqa6nqsd2i.fsf@gitster.g","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)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-16T04:12:09Z","receivedAt":"2021-06-16T04:12:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Interestingly enough, edc23840b0 (test-lib: bring $remove_trash out\n> of retirement, 2021-05-10) cleanly reverts without being depended on\n> by anything else in the series.\n>\n> Ævar?\n\nWith the crude debugging aid patch (attached at the end) applied,\nrunning\n\n    $ GIT_SKIP_TESTS='t?000' sh -x t0000-basic.sh -v\n\nwill show something interesting in the trace.\n\n    ++ this_test=t0000\n    ++ _s_k_i_p_='t?000'\n    ++ match_pattern_list t0000 t5000\n\nThe variable $GIT_SKIP_TESTS on this line:\n\n    if match_pattern_list \"$this_test\" $GIT_SKIP_TESTS\n\nglobs to t5000.  We don't quote the variable because we want them\nseparated at $IFS boundaries, but we didn't want the glob specials\nin its value to take any effect.  Sigh.\n\nThe reason why edc23840b0 appears to break this is probably because\nwe are still in $TEST_DIRECTORY when this match_pattern_list is\nexecuted; before that change, we've created $TRASH_DIRECTORY and\nchdir'd there already, and when we check \"do we want to skip all?\",\nthere is nothing for the glob to match.\n\nThat also explains why GIT_SKIP_TESTS=\"t000?\" appears to work.\nThere is no such filesystem entity directly in $TEST_DIRECTORY.\n\n    $ echo t000? t00?0 t0?00 t?000\n    t000? t00?0 t0200 t5000\n\n\n\ndiff --git i/t/test-lib.sh w/t/test-lib.sh\nindex 54938c6427..8ee0540532 100644\n--- i/t/test-lib.sh\n+++ w/t/test-lib.sh\n@@ -1346,13 +1346,17 @@ fi\n remove_trash=\n this_test=${0##*/}\n this_test=${this_test%%-*}\n+_s_k_i_p_=$GIT_SKIP_TESTS\n+\n if match_pattern_list \"$this_test\" $GIT_SKIP_TESTS\n then\n \tsay_color info >&3 \"skipping test $this_test altogether\"\n \tskip_all=\"skip all tests in $this_test\"\n \ttest_done\n fi\n \n+exit\n+\n # Last-minute variable setup\n HOME=\"$TRASH_DIRECTORY\"\n GNUPGHOME=\"$HOME/gnupg-home-not-used\"\n"},{"id":"427587","messageId":"patch-1.1-436c723f4f8-20210616T082030Z-avarab@gmail.com","threadId":"55939","inReplyTo":"xmqqa6nqsd2i.fsf@gitster.g","subject":"[PATCH] test-lib: fix \"$remove_trash\" regression and match_pattern_list() bugs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-16T08:24:13Z","receivedAt":"2021-06-16T08:24:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"My edc23840b0b (test-lib: bring $remove_trash out of retirement,\n2021-05-10) caused a regression where we'd fail to handle\nGIT_SKIP_TESTS variable content like 't????', since we were calling\nmatch_pattern_list() from the t/ directory not the trash\ndirectory.\n\nWe'd thus glob match in the directory containing our tests, whereas\nbefore we'd do the match in our newly setup (and empty, aside from the\n.git) trash directory.\n\nThis bug reveals a more general problem though, which I'm attempting\nto solve here: We have other users of match_pattern_list() that have\nalways been broken, namely the --verbose-only=* and --valgrind-only=*\noptions added in ff09af3fb8f (test-lib: verbose mode for only tests\nmatching a pattern, 2013-06-23) and 5dfc368f5ec (test-lib: valgrind\nfor only tests matching a pattern, 2013-06-23).\n\nThose options will try to match an argument like --verbose-only=1*\nagainst a test number like \"10\", but since we run the match in our own\ntrash directory an earlier test creating a file like \"1one.txt\" will\nbreak that option.\n\nWe cannot simply quote the $GIT_SKIP_TESTS\" being passed to\nmatch_pattern_list(), since we are relying on the $IFS semantics.\n\nLet's instead setup a .test-lib-trash subdirectory under the trash\ndirectory, and an \"empty-dir\" directory under that. Then let's run the\nmatch_pattern_list() in a sub-shell in that directory.\n\nThis fixes the regression in my edc23840b0b, as well as the bugs we've\nhad in the --{verbose,valgrind}-only=* options. I have tests for this,\nbut they're a pain to support without my in-flight lib-subtest.sh\nchanges, I'll submit them once those land.\n\nThis has the added benefit of providing a new clean work area for\ntests in general, so functions in test-lib-functions.sh can use it for\ntheir own scratch files, instead of potentially tripping up the test\nlogic itself.\n\nThe drawback is that any tests that cares about the complete\ncleanliness of its test are might need to be adjusted, as one ls-files\ntest here does. I think it's still worth it because that'll be one\nspecial case (and a dotfile), as opposed to every test potentially\ntripping over the likes of \"expected.config\" on a glob match of \"*\".\n\n1. https://lore.kernel.org/git/1d003cac-83fa-0b63-f60e-55513ac45cf9@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nOn Wed, Jun 16 2021, Junio C Hamano wrote:\n\n> Interestingly enough, edc23840b0 (test-lib: bring $remove_trash out\n> of retirement, 2021-05-10) cleanly reverts without being depended on\n> by anything else in the series.\n\nI think reverting edc23840b0 might be the best short-term fix to avoid\ndealing with the breakage + get a quick fix down. I can confirm that\nnothing else in the series relies on it.\n\nThis patch is an attempt at a more general solution to the problem,\nwhich depending on what you think you might want to take instead. It\nfixes not only the immediate regression, but as noted other existing\nbugs in match_pattern_list() usage.\n\n t/t3000-ls-files-others.sh |  8 ++++++--\n t/test-lib-functions.sh    |  8 +++++---\n t/test-lib.sh              | 40 +++++++++++++++++++++++---------------\n 3 files changed, 35 insertions(+), 21 deletions(-)\n\ndiff --git a/t/t3000-ls-files-others.sh b/t/t3000-ls-files-others.sh\nindex 740ce56eab5..8c62da502df 100755\n--- a/t/t3000-ls-files-others.sh\n+++ b/t/t3000-ls-files-others.sh\n@@ -37,6 +37,7 @@ test_expect_success 'setup: expected output' '\n \tcat >expected1 <<-\\EOF &&\n \texpected1\n \texpected2\n+\texpected2.noempty\n \texpected3\n \toutput\n \tpath0\n@@ -47,7 +48,10 @@ test_expect_success 'setup: expected output' '\n \n \tsed -e \"s|path2/file2|path2/|\" <expected1 >expected2 &&\n \tcp expected2 expected3 &&\n-\techo path4/ >>expected2\n+\techo path4/ >>expected2 &&\n+\n+\techo .test-lib-trash/ >expected2.noempty &&\n+\tcat expected2 >>expected2.noempty\n '\n \n test_expect_success 'ls-files --others' '\n@@ -57,7 +61,7 @@ test_expect_success 'ls-files --others' '\n \n test_expect_success 'ls-files --others --directory' '\n \tgit ls-files --others --directory >output &&\n-\ttest_cmp expected2 output\n+\ttest_cmp expected2.noempty output\n '\n \n test_expect_success '--no-empty-directory hides empty directory' '\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex f0448daa74b..33697b0df81 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1040,10 +1040,12 @@ test_cmp_config () {\n \t\tGD=\"-C $1\" &&\n \t\tshift\n \tfi &&\n-\tprintf \"%s\\n\" \"$1\" >expect.config &&\n+\tprintf \"%s\\n\" \"$1\" >\"$TRASH_DIRECTORY_TEST_LIB\"/expect.config &&\n \tshift &&\n-\tgit $GD config \"$@\" >actual.config &&\n-\ttest_cmp expect.config actual.config\n+\tgit $GD config \"$@\" >\"$TRASH_DIRECTORY_TEST_LIB\"/actual.config &&\n+\ttest_cmp \\\n+\t\t\"$TRASH_DIRECTORY_TEST_LIB\"/expect.config \\\n+\t\t\"$TRASH_DIRECTORY_TEST_LIB\"/actual.config\n }\n \n # test_cmp_bin - helper to compare binary files\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 54938c64279..7f284f56b1d 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -260,6 +260,8 @@ case \"$TRASH_DIRECTORY\" in\n /*) ;; # absolute path is good\n  *) TRASH_DIRECTORY=\"$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY\" ;;\n esac\n+TRASH_DIRECTORY_TEST_LIB=\"$TRASH_DIRECTORY/.test-lib-trash\"\n+TRASH_DIRECTORY_TEST_LIB_EMPTY=\"$TRASH_DIRECTORY_TEST_LIB/empty-dir\"\n \n # If --stress was passed, run this test repeatedly in several parallel loops.\n if test \"$GIT_TEST_STRESS_STARTED\" = \"done\"\n@@ -848,7 +850,8 @@ maybe_teardown_verbose () {\n last_verbose=t\n maybe_setup_verbose () {\n \ttest -z \"$verbose_only\" && return\n-\tif match_pattern_list $test_count $verbose_only\n+\tif (cd \"$TRASH_DIRECTORY_TEST_LIB_EMPTY\" &&\n+\t    match_pattern_list $test_count $verbose_only)\n \tthen\n \t\texec 4>&2 3>&1\n \t\t# Emit a delimiting blank line when going from\n@@ -878,7 +881,8 @@ maybe_setup_valgrind () {\n \t\treturn\n \tfi\n \tGIT_VALGRIND_ENABLED=\n-\tif match_pattern_list $test_count $valgrind_only\n+\tif (cd \"$TRASH_DIRECTORY_TEST_LIB_EMPTY\" &&\n+\t    match_pattern_list $test_count $valgrind_only)\n \tthen\n \t\tGIT_VALGRIND_ENABLED=t\n \tfi\n@@ -1006,7 +1010,8 @@ test_finish_ () {\n test_skip () {\n \tto_skip=\n \tskipped_reason=\n-\tif match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS\n+\tif (cd \"$TRASH_DIRECTORY_TEST_LIB_EMPTY\" &&\n+\t    match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS)\n \tthen\n \t\tto_skip=t\n \t\tskipped_reason=\"GIT_SKIP_TESTS\"\n@@ -1177,7 +1182,7 @@ test_done () {\n \t\t\tesac\n \t\tfi\n \n-\t\tif test -z \"$debug\" && test -n \"$remove_trash\"\n+\t\tif test -z \"$debug\"\n \t\tthen\n \t\t\ttest -d \"$TRASH_DIRECTORY\" ||\n \t\t\terror \"Tests passed but trash directory already removed before test cleanup; aborting\"\n@@ -1342,11 +1347,22 @@ then\n \texit 1\n fi\n \n+# Test repository\n+rm -fr \"$TRASH_DIRECTORY\" || {\n+\tGIT_EXIT_OK=t\n+\techo >&5 \"FATAL: Cannot prepare test area\"\n+\texit 1\n+}\n+\n+# Set up an early work area for the test code itself\n+mkdir -p \"$TRASH_DIRECTORY_TEST_LIB_EMPTY\" >&3 2>&4 ||\n+\terror \"cannot create test-lib.sh trash directory\"\n+\n # Are we running this test at all?\n-remove_trash=\n this_test=${0##*/}\n this_test=${this_test%%-*}\n-if match_pattern_list \"$this_test\" $GIT_SKIP_TESTS\n+if (cd \"$TRASH_DIRECTORY_TEST_LIB_EMPTY\" &&\n+    match_pattern_list \"$this_test\" $GIT_SKIP_TESTS)\n then\n \tsay_color info >&3 \"skipping test $this_test altogether\"\n \tskip_all=\"skip all tests in $this_test\"\n@@ -1358,20 +1374,12 @@ HOME=\"$TRASH_DIRECTORY\"\n GNUPGHOME=\"$HOME/gnupg-home-not-used\"\n export HOME GNUPGHOME\n \n-# Test repository\n-rm -fr \"$TRASH_DIRECTORY\" || {\n-\tGIT_EXIT_OK=t\n-\techo >&5 \"FATAL: Cannot prepare test area\"\n-\texit 1\n-}\n-\n-remove_trash=t\n+# \"We have the $TRASH_DIRECTORY already, but let's create a\n+# $TRASH_DIRECTORY/.git\n if test -z \"$TEST_NO_CREATE_REPO\"\n then\n \tgit init \"$TRASH_DIRECTORY\" >&3 2>&4 ||\n \terror \"cannot run git init\"\n-else\n-\tmkdir -p \"$TRASH_DIRECTORY\"\n fi\n \n # Use -P to resolve symlinks in our working directory so that the cwd\n-- \n2.32.0.555.g0268d380f7b\n\n"},{"id":"427588","messageId":"YMm2Wlb+eJDL7+ua@coredump.intra.peff.net","threadId":"55939","inReplyTo":"xmqq4kdysb1i.fsf@gitster.g","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)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-16T08:29:14Z","receivedAt":"2021-06-16T08:29:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 16, 2021 at 01:12:09PM +0900, Junio C Hamano wrote:\n\n> The variable $GIT_SKIP_TESTS on this line:\n> \n>     if match_pattern_list \"$this_test\" $GIT_SKIP_TESTS\n> \n> globs to t5000.  We don't quote the variable because we want them\n> separated at $IFS boundaries, but we didn't want the glob specials\n> in its value to take any effect.  Sigh.\n\nGood find.\n\nIt's surprisingly hard to do field-splitting without pathname globbing\nin pure shell. I couldn't find a way without using \"set -f\". That's in\nPOSIX, but it feels funny tweaking a global that can effect how other\ncode runs. We can at least constraint it to a subshell close to the\npoint of use:\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 54938c6427..093104d04f 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -732,14 +732,15 @@ match_pattern_list () {\n \targ=\"$1\"\n \tshift\n \ttest -z \"$*\" && return 1\n-\tfor pattern_\n+\t(set -f\n+\tfor pattern_ in $*\n \tdo\n \t\tcase \"$arg\" in\n \t\t$pattern_)\n-\t\t\treturn 0\n+\t\t\texit 0\n \t\tesac\n \tdone\n-\treturn 1\n+\texit 1)\n }\n \n match_test_selector_list () {\n@@ -848,7 +849,7 @@ maybe_teardown_verbose () {\n last_verbose=t\n maybe_setup_verbose () {\n \ttest -z \"$verbose_only\" && return\n-\tif match_pattern_list $test_count $verbose_only\n+\tif match_pattern_list $test_count \"$verbose_only\"\n \tthen\n \t\texec 4>&2 3>&1\n \t\t# Emit a delimiting blank line when going from\n@@ -878,7 +879,7 @@ maybe_setup_valgrind () {\n \t\treturn\n \tfi\n \tGIT_VALGRIND_ENABLED=\n-\tif match_pattern_list $test_count $valgrind_only\n+\tif match_pattern_list $test_count \"$valgrind_only\"\n \tthen\n \t\tGIT_VALGRIND_ENABLED=t\n \tfi\n@@ -1006,7 +1007,7 @@ test_finish_ () {\n test_skip () {\n \tto_skip=\n \tskipped_reason=\n-\tif match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS\n+\tif match_pattern_list $this_test.$test_count \"$GIT_SKIP_TESTS\"\n \tthen\n \t\tto_skip=t\n \t\tskipped_reason=\"GIT_SKIP_TESTS\"\n@@ -1346,7 +1347,7 @@ fi\n remove_trash=\n this_test=${0##*/}\n this_test=${this_test%%-*}\n-if match_pattern_list \"$this_test\" $GIT_SKIP_TESTS\n+if match_pattern_list \"$this_test\" \"$GIT_SKIP_TESTS\"\n then\n \tsay_color info >&3 \"skipping test $this_test altogether\"\n \tskip_all=\"skip all tests in $this_test\"\n\nIf that isn't portable for some reason, I think we could fall back on\nsplitting with an external tool. You can't feed the result as a function\nargument (you run into the same problem!) but you can use \"read\" to\nsplit on newlines, like:\n\n  echo \"$GIT_SKIP_TESTS\" |\n  tr ' ' '\\n' |\n  while read pattern\n  do\n    echo \"got $pattern\"\n  done\n\nThat does put the shell code on the right-hand side of a pipe, which\nmeans it's constrained in terms of setting variables, etc. But that\nwould be acceptable for our use here.\n\nI dunno. Maybe somebody else can come up with something more clever (or\nmaybe I am just missing something obvious).\n\n-Peff\n"},{"id":"427589","messageId":"YMm4F2uKZ4Dv3C4p@coredump.intra.peff.net","threadId":"55939","inReplyTo":"patch-1.1-436c723f4f8-20210616T082030Z-avarab@gmail.com","subject":"Re: [PATCH] test-lib: fix \"$remove_trash\" regression and match_pattern_list() bugs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-16T08:36:39Z","receivedAt":"2021-06-16T08:36:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 16, 2021 at 10:24:13AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Those options will try to match an argument like --verbose-only=1*\n> against a test number like \"10\", but since we run the match in our own\n> trash directory an earlier test creating a file like \"1one.txt\" will\n> break that option.\n> \n> We cannot simply quote the $GIT_SKIP_TESTS\" being passed to\n> match_pattern_list(), since we are relying on the $IFS semantics.\n> \n> Let's instead setup a .test-lib-trash subdirectory under the trash\n> directory, and an \"empty-dir\" directory under that. Then let's run the\n> match_pattern_list() in a sub-shell in that directory.\n\nLooks like my email just crossed with this one. Your \"cd to an empty\ndirectory\" is a fun version of my \"maybe somebody can think of something\nclever\" statement. :)\n\nAs a general solution, it does fail if the globs may contain things that\nlook like absolute paths, but that is quite unlikely for our use case\nhere. Still, I kind of like the \"set -f\" version because it doesn't need\nthe extra directory which could cause problems with \"ls-files -o\", etc,\nas you mentioned. You could also create the empty directory on the fly,\nthough if \"set -f\" works portably, that seems less complicated to me.\n\nWhatever the expansion mechanism, I do think it's worth having callers\nquote \"$GIT_SKIP_TESTS\" and then performing the expansion within\nmatch_pattern_list. Then the nasty mechanics are all in that one place.\n\n-Peff\n"},{"id":"427593","messageId":"xmqqim2ep3ov.fsf@gitster.g","threadId":"55939","inReplyTo":"YMm2Wlb+eJDL7+ua@coredump.intra.peff.net","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)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-16T09:19:12Z","receivedAt":"2021-06-16T09:19:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> It's surprisingly hard to do field-splitting without pathname globbing\n> in pure shell. I couldn't find a way without using \"set -f\". That's in\n> POSIX, but it feels funny tweaking a global that can effect how other\n> code runs. We can at least constraint it to a subshell close to the\n> point of use:\n> ...\n> -\tfor pattern_\n> +\t(set -f\n> +\tfor pattern_ in $*\n>  \tdo\n>  \t\tcase \"$arg\" in\n>  \t\t$pattern_)\n> -\t\t\treturn 0\n> +\t\t\texit 0\n>  \t\tesac\n>  \tdone\n> -\treturn 1\n> +\texit 1)\n>  }\n\nNice.  \"set -f\" is what I wanted to find myself but couldn't, when I\nwrote the message you are responding to.\n\n> -if match_pattern_list \"$this_test\" $GIT_SKIP_TESTS\n> +if match_pattern_list \"$this_test\" \"$GIT_SKIP_TESTS\"\n\nOK.  'for pattern_ in $*' that flattens $* allows us to quote it\nhere, passing it as a single argument without globbing.\n\n\n"},{"id":"427594","messageId":"xmqqeed2p3jx.fsf@gitster.g","threadId":"55939","inReplyTo":"YMm4F2uKZ4Dv3C4p@coredump.intra.peff.net","subject":"Re: [PATCH] test-lib: fix \"$remove_trash\" regression and match_pattern_list() bugs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-16T09:22:10Z","receivedAt":"2021-06-16T09:22:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... Still, I kind of like the \"set -f\" version because it doesn't need\n> the extra directory which could cause problems with \"ls-files -o\", etc,\n> as you mentioned. You could also create the empty directory on the fly,\n> though if \"set -f\" works portably, that seems less complicated to me.\n\nFWIW, I share that.\n"},{"id":"427597","messageId":"YMnRC8v4RLDk2+K9@coredump.intra.peff.net","threadId":"55939","inReplyTo":"xmqqeed2p3jx.fsf@gitster.g","subject":"Re: [PATCH] test-lib: fix \"$remove_trash\" regression and match_pattern_list() bugs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-16T10:23:07Z","receivedAt":"2021-06-16T10:23:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 16, 2021 at 06:22:10PM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > ... Still, I kind of like the \"set -f\" version because it doesn't need\n> > the extra directory which could cause problems with \"ls-files -o\", etc,\n> > as you mentioned. You could also create the empty directory on the fly,\n> > though if \"set -f\" works portably, that seems less complicated to me.\n> \n> FWIW, I share that.\n\nHere it is with some cosmetic cleanups and a commit message. I don't\nmean to preempt further discussion if Ævar prefers another route, but I\nwant to make sure we didn't stall out.\n\nI'm a little curious whether the bug could be triggered before the\nrecent move to running match_pattern_list outside of the trash directory\n(i.e., whether there is some test that creates a sufficiently\nconfusing-looking file in the filesystem; t0000 would be a likely\ncandidate). But not curious enough to comb through the test suite\nlooking for candidates. :)\n\n-- >8 --\nSubject: [PATCH] test-lib: avoid accidental globbing in match_pattern_list()\n\nWe have a custom match_pattern_list() function which we use for matching\ntest names (like \"t1234\") against glob-like patterns (like \"t1???\") for\n$GIT_SKIP_TESTS, --verbose-only, etc.\n\nThose patterns may have multiple whitespace-separated elements (e.g.,\n\"t0* t1234 t5?78\"). The callers of match_pattern_list thus pass the\nstrings unquoted, so that the shell does the usual field-splitting into\nseparate arguments.\n\nBut this also means the shell will do the usual globbing for each\nargument, which can result in us seeing an expansion based on what's in\nthe filesystem, rather than the real pattern. For example, if I have the\npath \"t5000\" in the filesystem, and you feed the pattern \"t?0000\", that\n_should_ match the string \"t0000\", but it won't after the shell has\nexpanded it to \"t5000\".\n\nThis has been a bug ever since that function was introduced. But it\ndidn't usually trigger since we typically use the function inside the\ntrash directory, which has a very limited set of files that are unlikely\nto match. It became a lot easier to trigger after edc23840b0 (test-lib:\nbring $remove_trash out of retirement, 2021-05-10), because now we match\n$GIT_SKIP_TESTS before even entering the trash directory. So the t5000\nexample above can be seen with:\n\n  GIT_SKIP_TESTS=t?000 ./t0000-basic.sh\n\nwhich should skip all tests but doesn't.\n\nWe can fix this by using \"set -f\" to ask the shell not to glob (which is\nin POSIX, so should hopefully be portable enough). We only want to do\nthis in a subshell (to avoid polluting the rest of the script), which\nmeans we need to get the whole string intact into the match_pattern_list\nfunction by quoting it. Arguably this is a good idea anyway, since it\nmakes it much more obvious that we intend to split, and it's not simply\nsloppy scripting.\n\nDiagnosed-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/test-lib.sh | 34 ++++++++++++++++++++++------------\n 1 file changed, 22 insertions(+), 12 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 54938c6427..a7badbf1dd 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -732,14 +732,24 @@ match_pattern_list () {\n \targ=\"$1\"\n \tshift\n \ttest -z \"$*\" && return 1\n-\tfor pattern_\n-\tdo\n-\t\tcase \"$arg\" in\n-\t\t$pattern_)\n-\t\t\treturn 0\n-\t\tesac\n-\tdone\n-\treturn 1\n+\t# We need to use \"$*\" to get field-splitting, but we want to\n+\t# disable globbing, since we are matching against an arbitrary\n+\t# $arg, not what's in the filesystem. Using \"set -f\" accomplishes\n+\t# that, but we must do it in a subshell to avoid impacting the\n+\t# rest of the script. The exit value of the subshell becomes\n+\t# the function's return value.\n+\t(\n+\t\tset -f\n+\t\tfor pattern_ in $*\n+\t\tdo\n+\t\t\tcase \"$arg\" in\n+\t\t\t$pattern_)\n+\t\t\t\texit 0\n+\t\t\t\t;;\n+\t\t\tesac\n+\t\tdone\n+\t\texit 1\n+\t)\n }\n \n match_test_selector_list () {\n@@ -848,7 +858,7 @@ maybe_teardown_verbose () {\n last_verbose=t\n maybe_setup_verbose () {\n \ttest -z \"$verbose_only\" && return\n-\tif match_pattern_list $test_count $verbose_only\n+\tif match_pattern_list $test_count \"$verbose_only\"\n \tthen\n \t\texec 4>&2 3>&1\n \t\t# Emit a delimiting blank line when going from\n@@ -878,7 +888,7 @@ maybe_setup_valgrind () {\n \t\treturn\n \tfi\n \tGIT_VALGRIND_ENABLED=\n-\tif match_pattern_list $test_count $valgrind_only\n+\tif match_pattern_list $test_count \"$valgrind_only\"\n \tthen\n \t\tGIT_VALGRIND_ENABLED=t\n \tfi\n@@ -1006,7 +1016,7 @@ test_finish_ () {\n test_skip () {\n \tto_skip=\n \tskipped_reason=\n-\tif match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS\n+\tif match_pattern_list $this_test.$test_count \"$GIT_SKIP_TESTS\"\n \tthen\n \t\tto_skip=t\n \t\tskipped_reason=\"GIT_SKIP_TESTS\"\n@@ -1346,7 +1356,7 @@ fi\n remove_trash=\n this_test=${0##*/}\n this_test=${this_test%%-*}\n-if match_pattern_list \"$this_test\" $GIT_SKIP_TESTS\n+if match_pattern_list \"$this_test\" \"$GIT_SKIP_TESTS\"\n then\n \tsay_color info >&3 \"skipping test $this_test altogether\"\n \tskip_all=\"skip all tests in $this_test\"\n-- \n2.32.0.559.g998c91e3d7\n\n"},{"id":"427598","messageId":"YMnRYJnMAe5/TIlF@coredump.intra.peff.net","threadId":"55939","inReplyTo":"YMnRC8v4RLDk2+K9@coredump.intra.peff.net","subject":"Re: [PATCH] test-lib: fix \"$remove_trash\" regression and match_pattern_list() bugs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-16T10:24:32Z","receivedAt":"2021-06-16T10:24:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 16, 2021 at 06:23:07AM -0400, Jeff King wrote:\n\n> But this also means the shell will do the usual globbing for each\n> argument, which can result in us seeing an expansion based on what's in\n> the filesystem, rather than the real pattern. For example, if I have the\n> path \"t5000\" in the filesystem, and you feed the pattern \"t?0000\", that\n> _should_ match the string \"t0000\", but it won't after the shell has\n> expanded it to \"t5000\".\n\nWhoops, that pattern should be \"t?000\", of course.\n\nIt's correct in the runnable example later on.\n\n-Peff\n"},{"id":"427600","messageId":"87pmwmxd6f.fsf@evledraar.gmail.com","threadId":"55939","inReplyTo":"YMm4F2uKZ4Dv3C4p@coredump.intra.peff.net","subject":"Re: [PATCH] test-lib: fix \"$remove_trash\" regression and match_pattern_list() bugs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-16T09:49:20Z","receivedAt":"2021-06-16T11:27:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 16 2021, Jeff King wrote:\n\n> On Wed, Jun 16, 2021 at 10:24:13AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Those options will try to match an argument like --verbose-only=1*\n>> against a test number like \"10\", but since we run the match in our own\n>> trash directory an earlier test creating a file like \"1one.txt\" will\n>> break that option.\n>> \n>> We cannot simply quote the $GIT_SKIP_TESTS\" being passed to\n>> match_pattern_list(), since we are relying on the $IFS semantics.\n>> \n>> Let's instead setup a .test-lib-trash subdirectory under the trash\n>> directory, and an \"empty-dir\" directory under that. Then let's run the\n>> match_pattern_list() in a sub-shell in that directory.\n>\n> Looks like my email just crossed with this one. Your \"cd to an empty\n> directory\" is a fun version of my \"maybe somebody can think of something\n> clever\" statement. :)\n\nYeah, I sent it before seeing yours.\n\n> As a general solution, it does fail if the globs may contain things that\n> look like absolute paths, but that is quite unlikely for our use case\n> here.[...]\n\nNot just unlikely, impossible. Yes it's possible in the general case,\nbut in the match_pattern_list we are always normalizing\ne.g. ./t1234-some-test.sh t t1234, and the other match cases are test\nnumbers etc. It doesn't need to deal with the general case.\n\n> [...] Still, I kind of like the \"set -f\" version because it doesn't need\n> the extra directory which could cause problems with \"ls-files -o\", etc,\n> as you mentioned. You could also create the empty directory on the fly,\n> though if \"set -f\" works portably, that seems less complicated to me.\n\nYeah, in isolation I'd agree, but given the desire (with existing code\nand other in-flight code) to have a scratch are for the test library\ncode itself simply creating such a directory seems like a good thing to\nhave in general, and once we have it we can use the subshell trick, or\njust \"set -f\" I suppose and use the scratch dir for other stuff.\n\nI am a bit wary of this being our first ever use of \"set -f\", but maybe\nit's sufficiently portable.\n\n> Whatever the expansion mechanism, I do think it's worth having callers\n> quote \"$GIT_SKIP_TESTS\" and then performing the expansion within\n> match_pattern_list. Then the nasty mechanics are all in that one place.\n\nI think it's rather clean to not quote it, i.e. to have the loop get a\nlist of item and then things to match, it would also make it easier to\ne.g. port it to a native C program.\n"},{"id":"427602","messageId":"YMnj7+nWQHSY6fTS@coredump.intra.peff.net","threadId":"55939","inReplyTo":"87pmwmxd6f.fsf@evledraar.gmail.com","subject":"Re: [PATCH] test-lib: fix \"$remove_trash\" regression and match_pattern_list() bugs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-16T11:43:43Z","receivedAt":"2021-06-16T11:43:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 16, 2021 at 11:49:20AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > [...] Still, I kind of like the \"set -f\" version because it doesn't need\n> > the extra directory which could cause problems with \"ls-files -o\", etc,\n> > as you mentioned. You could also create the empty directory on the fly,\n> > though if \"set -f\" works portably, that seems less complicated to me.\n> \n> Yeah, in isolation I'd agree, but given the desire (with existing code\n> and other in-flight code) to have a scratch are for the test library\n> code itself simply creating such a directory seems like a good thing to\n> have in general, and once we have it we can use the subshell trick, or\n> just \"set -f\" I suppose and use the scratch dir for other stuff.\n\nI don't have much of an opinion on other uses of the scratch dir, not\nhaving seen them. I agree if we do have one and are paying the cost for\nit already, then using it here isn't a big deal.\n\n> I am a bit wary of this being our first ever use of \"set -f\", but maybe\n> it's sufficiently portable.\n\nYep, me too. The nice thing about it is that we can swap out the\nsolution pretty easily if it turns out not to be. I don't know how good\nour coverage of obscure shells is in CI, though (e.g., on your gcc\nbuild-farm stuff, are we mostly using system shells?).\n\n> > Whatever the expansion mechanism, I do think it's worth having callers\n> > quote \"$GIT_SKIP_TESTS\" and then performing the expansion within\n> > match_pattern_list. Then the nasty mechanics are all in that one place.\n> \n> I think it's rather clean to not quote it, i.e. to have the loop get a\n> list of item and then things to match, it would also make it easier to\n> e.g. port it to a native C program.\n\nMy main complaint is that it's a real gotcha for the callers. They have\nto remember to do this cd-to-scratch (or whatever technique we use).\nThat's cumbersome, and if they forget, their call is wrong in a really\nsubtle way that basic testing isn't likely to uncover.\n\n-Peff\n"},{"id":"427603","messageId":"87k0muxcd7.fsf@evledraar.gmail.com","threadId":"55939","inReplyTo":"YMnRC8v4RLDk2+K9@coredump.intra.peff.net","subject":"Re: [PATCH] test-lib: fix \"$remove_trash\" regression and match_pattern_list() bugs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-16T11:38:55Z","receivedAt":"2021-06-16T11:44:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 16 2021, Jeff King wrote:\n\n> On Wed, Jun 16, 2021 at 06:22:10PM +0900, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > ... Still, I kind of like the \"set -f\" version because it doesn't need\n>> > the extra directory which could cause problems with \"ls-files -o\", etc,\n>> > as you mentioned. You could also create the empty directory on the fly,\n>> > though if \"set -f\" works portably, that seems less complicated to me.\n>> \n>> FWIW, I share that.\n>\n> Here it is with some cosmetic cleanups and a commit message. I don't\n> mean to preempt further discussion if Ævar prefers another route, but I\n> want to make sure we didn't stall out.\n\nI mildly prefer mine as noted in\nhttps://lore.kernel.org/git/87pmwmxd6f.fsf@evledraar.gmail.com/; but I'd\nmainly prefer just to fix the immediate breakage on master, so whatever\nvariant of reverting, taking yours or mine Junio prefers I'm fine with.\n\nJust inline comments on the patch:\n\n> @@ -732,14 +732,24 @@ match_pattern_list () {\n>  \targ=\"$1\"\n>  \tshift\n>  \ttest -z \"$*\" && return 1\n> -\tfor pattern_\n> -\tdo\n> -\t\tcase \"$arg\" in\n> -\t\t$pattern_)\n> -\t\t\treturn 0\n> -\t\tesac\n> -\tdone\n> -\treturn 1\n> +\t# We need to use \"$*\" to get field-splitting, but we want to\n> +\t# disable globbing, since we are matching against an arbitrary\n> +\t# $arg, not what's in the filesystem. Using \"set -f\" accomplishes\n> +\t# that, but we must do it in a subshell to avoid impacting the\n> +\t# rest of the script. The exit value of the subshell becomes\n> +\t# the function's return value.\n> +\t(\n> +\t\tset -f\n> +\t\tfor pattern_ in $*\n> +\t\tdo\n> +\t\t\tcase \"$arg\" in\n> +\t\t\t$pattern_)\n> +\t\t\t\texit 0\n> +\t\t\t\t;;\n> +\t\t\tesac\n> +\t\tdone\n> +\t\texit 1\n> +\t)\n>  }\n\nWhy not just start with a ret=1, set ret=0 if we have a match and break\nfrom the loop, and then do a \"set +f\" afterwards? I.e. is there an\nactual need for the subshell here.\n\nI'm mildly paranoid about a new \"set -<flag>\" in the codebase for vague\nfears of portability (as noted in my linked message), but whatever shell\nsupports \"set -<flag>\" surely supports the inverse with \"set +<flag>\",\nno?\n"},{"id":"427604","messageId":"YMnllMXvRJKSdGID@coredump.intra.peff.net","threadId":"55939","inReplyTo":"87k0muxcd7.fsf@evledraar.gmail.com","subject":"Re: [PATCH] test-lib: fix \"$remove_trash\" regression and match_pattern_list() bugs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-16T11:50:44Z","receivedAt":"2021-06-16T11:50:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 16, 2021 at 01:38:55PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > +\t# We need to use \"$*\" to get field-splitting, but we want to\n> > +\t# disable globbing, since we are matching against an arbitrary\n> > +\t# $arg, not what's in the filesystem. Using \"set -f\" accomplishes\n> > +\t# that, but we must do it in a subshell to avoid impacting the\n> > +\t# rest of the script. The exit value of the subshell becomes\n> > +\t# the function's return value.\n> > +\t(\n> > +\t\tset -f\n> > +\t\tfor pattern_ in $*\n> > +\t\tdo\n> > +\t\t\tcase \"$arg\" in\n> > +\t\t\t$pattern_)\n> > +\t\t\t\texit 0\n> > +\t\t\t\t;;\n> > +\t\t\tesac\n> > +\t\tdone\n> > +\t\texit 1\n> > +\t)\n> >  }\n> \n> Why not just start with a ret=1, set ret=0 if we have a match and break\n> from the loop, and then do a \"set +f\" afterwards? I.e. is there an\n> actual need for the subshell here.\n\nMy thought was that the subshell takes us back to the original state,\nregardless of what it was. As opposed to \"set +f\" which takes us back to\na particular state. But it is unlikely that we'd have done a global \"set\n-f\" before calling this, so maybe that is being overly conservative.\n\n> I'm mildly paranoid about a new \"set -<flag>\" in the codebase for vague\n> fears of portability (as noted in my linked message), but whatever shell\n> supports \"set -<flag>\" surely supports the inverse with \"set +<flag>\",\n> no?\n\nYes, I think we can assume that if it supports one, it supports the\nother.\n\n-Peff\n"},{"id":"427645","messageId":"xmqqlf79nxk1.fsf@gitster.g","threadId":"55939","inReplyTo":"YMnllMXvRJKSdGID@coredump.intra.peff.net","subject":"Re: [PATCH] test-lib: fix \"$remove_trash\" regression and match_pattern_list() bugs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-17T00:29:18Z","receivedAt":"2021-06-17T00:29:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> My thought was that the subshell takes us back to the original state,\n> regardless of what it was. As opposed to \"set +f\" which takes us back to\n> a particular state. But it is unlikely that we'd have done a global \"set\n> -f\" before calling this, so maybe that is being overly conservative.\n\nOverly conservative, yes, but if it is not too much overhead, I\nthink it is a good practice anyway.\n\n"},{"id":"427646","messageId":"xmqqh7hxnx7v.fsf@gitster.g","threadId":"55939","inReplyTo":"87pmwmxd6f.fsf@evledraar.gmail.com","subject":"Re: [PATCH] test-lib: fix \"$remove_trash\" regression and match_pattern_list() bugs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-17T00:36:36Z","receivedAt":"2021-06-17T00:36:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> Whatever the expansion mechanism, I do think it's worth having callers\n>> quote \"$GIT_SKIP_TESTS\" and then performing the expansion within\n>> match_pattern_list. Then the nasty mechanics are all in that one place.\n>\n> I think it's rather clean to not quote it, i.e. to have the loop get a\n> list of item and then things to match, it would also make it easier to\n> e.g. port it to a native C program.\n\nSorry, I am not sure how it can work without quoting $GIT_SKIP_TESTS\nand exposing the details of how we disable globbing when the caller\ncalls match_pattern_list.\n\nA list of item in $GIT_SKIP_TESTS would not be able to pass t?000 in\nit intact to the loop that processes each of the item on the list.\n\n\tfor pattern in $GIT_SKIP_TESTS\n\tdo\n\t\tattempt to match with $pattern\n\tdone\n\nwill see t5000 in $pattern due to globbing.  It's not like \"$@\"\ntrick can be applied to any plain variable.\n"}]}