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

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

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

I think the more usual way would be to have an actual
test_expect_success block that creates the keys as a setup step
(possibly triggered by a function, since it's included via lib-gpg.sh).
If we don't want to decide whether we have the GPGSSH prereq until then,
then that test can call test_set_prereq. See the LONG_REF case in t1401
for an example.

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

There are three cases to consider:

- On systems where this prerequisite check fails, a key may or may not
  be created, but the tests that rely on the key won't be run, so it
  doesn't matter either way.

- On (clearly the mainline) systems where this check passes and there
  are no ACL problems, the key that's generated is stored with
  sufficiently restrictive permissions that the tests that rely on the
  key can pass.

- On my system, where ACLs are a problem, the prerequisite check passes,
  and a key is created, but it has permissions that are too permissive.
  As a result, when a test calls OpenSSH to use that key, OpenSSH
  refuses due to the permissions, and the test fails.

FWIW, that explanation makes perfect sense to me (and your patch seems
like the right thing to do).

-Peff