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

On Wed, Jun 16, 2021 at 11:49:20AM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

[...] 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 don't have much of an opinion on other uses of the scratch dir, not
having seen them. I agree if we do have one and are paying the cost for
it already, then using it here isn't a big deal.

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

Yep, me too. The nice thing about it is that we can swap out the
solution pretty easily if it turns out not to be. I don't know how good
our coverage of obscure shells is in CI, though (e.g., on your gcc
build-farm stuff, are we mostly using system shells?).

quoted

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.

My main complaint is that it's a real gotcha for the callers. They have
to remember to do this cd-to-scratch (or whatever technique we use).
That's cumbersome, and if they forget, their call is wrong in a really
subtle way that basic testing isn't likely to uncover.

-Peff