Re: [PATCH] test-lib: fix "$remove_trash" regression and match_pattern_list() bugs

From: Jeff King <peff@peff.net>
Date: Jun 16, 2021, 08:36 UTC
Message-ID: <YMm4F2uKZ4Dv3C4p@coredump.intra.peff.net>
In-Reply-To: <patch-1.1-436c723f4f8-20210616T082030Z-avarab@gmail.com>

On Wed, Jun 16, 2021 at 10:24:13AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 11 quoted lines

> Those options will try to match an argument like --verbose-only=1*
> against a test number like "10", but since we run the match in our own
> trash directory an earlier test creating a file like "1one.txt" will
> break that option.
> 
> We cannot simply quote the $GIT_SKIP_TESTS" being passed to
> match_pattern_list(), since we are relying on the $IFS semantics.
> 
> Let's instead setup a .test-lib-trash subdirectory under the trash
> directory, and an "empty-dir" directory under that. Then let's run the
> match_pattern_list() in a sub-shell in that directory.

Looks like my email just crossed with this one. Your "cd to an empty directory" is a fun version of my "maybe somebody can think of something clever" statement. :)

As a general solution, it does fail if the globs may contain things that look like absolute paths, but that is quite unlikely for our use case here. Still, I kind of like the "set -f" version because it doesn't need the extra directory which could cause problems with "ls-files -o", etc, as you mentioned. You could also create the empty directory on the fly, though if "set -f" works portably, that seems less complicated to me.

Whatever the expansion mechanism, I do think it's worth having callers quote "$GIT_SKIP_TESTS" and then performing the expansion within match_pattern_list. Then the nasty mechanics are all in that one place.

-Peff