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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 5, 2021, 18:04 UTC
Message-ID: <xmqqk0hmxyw0.fsf@gitster.g>
In-Reply-To: <YYUeKt0xQm/6QT+w@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 24 quoted lines

> On Fri, Nov 05, 2021 at 11:25:25AM +0000, Adam Dinwoodie wrote:
>
>> > ... I am not quite sure how this explains "tests relating to ssh
>> > signing failing on Cygwin".  After all, this piece of code is
>> > lazy_prereq, which means that ssh-keygen in this block that fails
>> > (due to a less restrictive permissions) would merely mean that tests
>> > that are protected with GPGSSH prerequisite will be skipped without
>> > causing test failures.  After all that is the whole point of
>> > computing prereq on the fly.
>> 
>> The issue is that the prerequisite check isn't _just_ checking a
>> prerequisite: it's also creating an SSH key that's used without further
>> modification by the tests.
>
> 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.

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 [*].

    side note: so being able to create a key alone, without
    verifying the resulting key is usable, is a no-no.  That is why
    I said it is a good idea to check if the resulting key is usable
    inside the lazy-prereq.

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