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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 16, 2021, 09:49 UTC
Message-ID: <87pmwmxd6f.fsf@evledraar.gmail.com>
In-Reply-To: <YMm4F2uKZ4Dv3C4p@coredump.intra.peff.net>

On Wed, Jun 16 2021, Jeff King wrote:

Show 17 quoted lines

> On Wed, Jun 16, 2021 at 10:24:13AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> 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. :)

Yeah, I sent it before seeing yours.

> 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.[...]

Not just unlikely, impossible. Yes it's possible in the general case, but in the match_pattern_list we are always normalizing e.g. ./t1234-some-test.sh t t1234, and the other match cases are test numbers etc. It doesn't need to deal with the general case.

> [...] 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.

Yeah, in isolation I'd agree, but given the desire (with existing code and other in-flight code) to have a scratch are for the test library code itself simply creating such a directory seems like a good thing to have in general, and once we have it we can use the subshell trick, or just "set -f" I suppose and use the scratch dir for other stuff.

I am a bit wary of this being our first ever use of "set -f", but maybe it's sufficiently portable.

> 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.

I think it's rather clean to not quote it, i.e. to have the loop get a list of item and then things to match, it would also make it easier to e.g. port it to a native C program.