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

[Adam Dinwoodie <adam@dinwoodie.org>]

On Fri, 5 Nov 2021 at 18:04, Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> writes:
>
> > 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.

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.