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

From: Jeff King <peff@peff.net>
Date: Jun 16, 2021, 11:43 UTC
Message-ID: <YMnj7+nWQHSY6fTS@coredump.intra.peff.net>
In-Reply-To: <87pmwmxd6f.fsf@evledraar.gmail.com>

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

Show 10 quoted lines

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

Show 7 quoted lines

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