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

On Fri, 5 Nov 2021 at 18:04, Junio C Hamano [off-list ref] wrote:

Jeff King [off-list ref] writes:

quoted

On Fri, Nov 05, 2021 at 11:25:25AM +0000, Adam Dinwoodie wrote:

quoted

quoted

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

I'm not convinced by this. Or at least, I'm convinced by the
principle, but wary of the implications.

Take this case, for example: the function being tested by the
GPGSSH-gated tests is function that should work on Cygwin. If there
were a regression, running the tests on Cygwin ought to catch it, and
in this instance the tests failing meant that we caught a bug. On this
occasion it was a bug in the test library rather than the function
that most Git users care about, but I don't think there's anything
inherent about this situation that means it couldn't have been a
functional bug.

However, if the prerequisite checks had not only created the key but
also verified it could be used, in this scenario these tests would
have been skipped. The function the tests are exercising would still
work, and users would therefore expect it to continue working, but the
only chance we'd have to spot any future regressions is if they're hit
in some other environment or someone spots the tests being skipped by
trawling through the reams of test output to check what tests are
being skipped.

This is probably a much broader conversation. I remember when I first
started packaging Git for Cygwin, I produced a release that didn't
have support for HTTPS URLs due to a missing dependency in my build
environment. The build and test suite all passed -- it assumed I just
wanted to build a release that didn't have HTTPS support -- so some
relatively critical function was silently skipped. I don't know how to
avoid that sort of issue other than relying on (a) user bug (or at
least missing function) reports and (b) folk building Git for
themselves/others periodically going through the output of the
configure scripts and the skipped subtests to make sure only expected
things get missed; neither of those options seem great to me.