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

quoted

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.

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.

quoted

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