Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure

From: Jeff King <peff@peff.net>
Date: Nov 5, 2021, 23:39 UTC
Message-ID: <YYXAwxmhrLLMBqa+@coredump.intra.peff.net>
In-Reply-To: <xmqqk0hmxyw0.fsf@gitster.g>

On Fri, Nov 05, 2021 at 11:04:15AM -0700, Junio C Hamano wrote:

Show 13 quoted lines

> > This is sort of a side note to your main issue, but I think that relying
> > on a lazy_prereq for side effects is an anti-pattern. We make no
> > promises about when or how often the prereqs might be run, and we try to
> > insulate them from the main tests (by putting them in a subshell and
> > switching their cwd).
> >
> > It does happen to work here because the prereq script writes directly to
> > $GNUPGHOME, and we run the lazy prereqs about when you'd expect. So I
> > don't think it's really in any danger of breaking, but it is definitely
> > not using the feature as it was intended. :)
> 
> This merely imitates what GPG lazy-prerequisite started and imitated
> by other existing signature backends.

Ah, you're right. I should have checked the other GPG ones. It looks like that happened recently-ish in b417ec5f22 (tests: turn GPG, GPGSM and RFC1991 into lazy prereqs, 2020-03-26).

Before that the code was run outside of any test block at all, which I think is even worse.

Show 5 quoted lines

> I'd expect that you need some "initialization" for a feature X as
> part of asking "is feature X usable in this environment?".  Reusing
> the result of the initialization for true tests is probably an
> optimization worth making.  As long as the question is answered for
> the true tests, that is [*].

Yes, though if it's possible, I think doing less work in the prereq check might be a good approach (like say, just checking gpg or openssh version if we can). It results in flakier prereqs that may say "yes, we have feature X" when we don't. But it gets a human's attention when it fails, rather than quietly skipping tests (which is the same point Adam is making downthread).

It definitely is not something to fiddle with at this point in the -rc cycle, though.

Show 14 quoted lines

> > Again, that's mostly a tangent to your issue, and maybe not worth
> > futzing with at this point in the release cycle. I'm mostly just
> > registering my surprise. ;)
> 
> My purist side is with you and share the surprise.  But my practical
> side says this is probably an optimization worth taking.  If prereq
> only checks "if we initialize the keys right way, we can use ssh
> signing" and then removes the key and the equivalent to .ssh/
> directory, and a real test does "Ok, prereq passes so we know ssh
> signing is to be tested.  Now initialize the .ssh/ equivalent and
> create key", a fix like Adam came up with must be duplicated in two
> (or more) places, one for the prereq that initializes the keys
> "right way", and one for each test script that prepares the key used
> for it.

To be clear, I wasn't suggesting doing the key setup twice. I was just suggesting moving it out of a lazy prereq into a real test_expect block that sets the prereq flag as a side effect. That just makes the timing and the fact of running it more deliberate on the part of the test scripts.

It's probably not worth the effort, though. I think my line of thinking is coming from the "purist" side, and doesn't have any practical benefit.

-Peff