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

[Jeff King <peff@peff.net>]

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

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

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