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

On Wed, Jun 16 2021, Jeff King wrote:

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

quoted

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.