{"thread":{"id":"56846","subject":"[PATCH] t/lib-git.sh: fix ACL-related permissions failure","startedAt":"2021-11-04T19:26:06Z","lastAt":"2021-11-13T14:43:58Z","messageCount":28,"participants":["Adam Dinwoodie","Junio C Hamano","Ramsay Jones","Fabian Stelzer","Jeff King","Carlo Arenas","Kerry, Richard"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"440476","messageId":"20211104192533.2520-1-adam@dinwoodie.org","threadId":"56846","inReplyTo":null,"subject":"[PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2021-11-04T19:25:33Z","receivedAt":"2021-11-04T19:26:06Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"SSH keys are expected to be created with very restrictive permissions,\nand SSH commands will fail if the permissions are not appropriate. When\ncreating a directory for SSH keys in test scripts, attempt to clear any\nACLs that might otherwise cause the private key to inherit less\nrestrictive permissions than it requires.\n\nThis change is required in particular to avoid tests relating to SSH\nsigning failing in Cygwin.\n\nSigned-off-by: Adam Dinwoodie \nHelped-by: Fabian Stelzer \n---\n t/lib-gpg.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex f99ef3e859..1d8e5b5b7e 100644\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -106,6 +106,7 @@ test_lazy_prereq GPGSSH '\n \ttest $? = 0 || exit 1;\n \tmkdir -p \"${GNUPGHOME}\" &&\n \tchmod 0700 \"${GNUPGHOME}\" &&\n+\t(setfacl -k \"${GNUPGHOME}\" 2>/dev/null || true) &&\n \tssh-keygen -t ed25519 -N \"\" -C \"git ed25519 key\" -f \"${GPGSSH_KEY_PRIMARY}\" >/dev/null &&\n \techo \"\\\"principal with number 1\\\" $(cat \"${GPGSSH_KEY_PRIMARY}.pub\")\" >> \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n \tssh-keygen -t rsa -b 2048 -N \"\" -C \"git rsa2048 key\" -f \"${GPGSSH_KEY_SECONDARY}\" >/dev/null &&\n-- \n2.33.0\n\n"},{"id":"440478","messageId":"xmqq7ddn3dlt.fsf@gitster.g","threadId":"56846","inReplyTo":"20211104192533.2520-1-adam@dinwoodie.org","subject":"Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-04T19:49:50Z","receivedAt":"2021-11-04T19:49:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Dinwoodie writes:\n\n> SSH keys are expected to be created with very restrictive permissions,\n> and SSH commands will fail if the permissions are not appropriate. When\n> creating a directory for SSH keys in test scripts, attempt to clear any\n> ACLs that might otherwise cause the private key to inherit less\n> restrictive permissions than it requires.\n\nAll of the above makes sense as an explanation as to why the\nssh-keygen command may be unhappy with the $GNUPGHOME directory that\nis prepared here, but ...\n\n> This change is required in particular to avoid tests relating to SSH\n> signing failing in Cygwin.\n\n... I am not quite sure how this explains \"tests relating to ssh\nsigning failing on Cygwin\". After all, this piece of code is\nlazy_prereq, which means that ssh-keygen in this block that fails\n(due to a less restrictive permissions) would merely mean that tests\nthat are protected with GPGSSH prerequisite will be skipped without\ncausing test failures. After all that is the whole point of\ncomputing prereq on the fly.\n\n> Signed-off-by: Adam Dinwoodie \n> Helped-by: Fabian Stelzer \n\nPlease order these chronologically, i.e. Fabian helped and the patch\nwas finished, and finally you sent with your sign off.\n\n> ---\n> t/lib-gpg.sh | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\n> index f99ef3e859..1d8e5b5b7e 100644\n> --- a/t/lib-gpg.sh\n> +++ b/t/lib-gpg.sh\n> @@ -106,6 +106,7 @@ test_lazy_prereq GPGSSH '\n> \ttest $? = 0 || exit 1;\n> \tmkdir -p \"${GNUPGHOME}\" &&\n> \tchmod 0700 \"${GNUPGHOME}\" &&\n> +\t(setfacl -k \"${GNUPGHOME}\" 2>/dev/null || true) &&\n> \tssh-keygen -t ed25519 -N \"\" -C \"git ed25519 key\" -f \"${GPGSSH_KEY_PRIMARY}\" >/dev/null &&\n> \techo \"\\\"principal with number 1\\\" $(cat \"${GPGSSH_KEY_PRIMARY}.pub\")\" >> \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n> \tssh-keygen -t rsa -b 2048 -N \"\" -C \"git rsa2048 key\" -f \"${GPGSSH_KEY_SECONDARY}\" >/dev/null &&\n\nThere are other uses of ssh-keygen in the real tests but presumably\nthey just use the GNUPGHOME directory prepared with this lazy_prereq\nblock, and \"setfacl -k\" here would have wiped any possible loosening\nof permission, and that is why this is the only place that needs a\nchange, right? That fact might deserve recording in the proposed\nlog message.\n\nThanks.\n"},{"id":"440479","messageId":"xmqqzgqj1yff.fsf@gitster.g","threadId":"56846","inReplyTo":"xmqq7ddn3dlt.fsf@gitster.g","subject":"Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-04T20:03:00Z","receivedAt":"2021-11-04T20:03:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> This change is required in particular to avoid tests relating to SSH\n>> signing failing in Cygwin.\n>\n> ... I am not quite sure how this explains \"tests relating to ssh\n> signing failing on Cygwin\". After all, this piece of code is\n> lazy_prereq, which means that ssh-keygen in this block that fails\n> (due to a less restrictive permissions) would merely mean that tests\n> that are protected with GPGSSH prerequisite will be skipped without\n> causing test failures. After all that is the whole point of\n> computing prereq on the fly.\n\nThe reason why I wondered about the above is that it can be an\nindication of another breakage, namely, that we may have tests that\nrequire a working ssh-keygen but are by mistake not protected with\nGPGSSH prerequisite.\n\nThe test_lazy_prereq block you touched may refrain from setting the\nprerequisite on your system (due to the faulty test here that you\ntouched), but if we had such unprotected tests, we still will run\nssh signing tests and they would fail, due to the lack of the\nprerequisite.\n\nAnd fixing the prereq block alone will hide that other breakage, at\nleast on your system. Hence my question.\n\nThanks.\n"},{"id":"440480","messageId":"6acb22bc-a90c-b8b6-2e7d-d7e17ba595ea@ramsayjones.plus.com","threadId":"56846","inReplyTo":"20211104192533.2520-1-adam@dinwoodie.org","subject":"Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2021-11-04T20:09:03Z","receivedAt":"2021-11-04T20:09:09Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Hi Adam,\n\nOn 04/11/2021 19:25, Adam Dinwoodie wrote:\n> SSH keys are expected to be created with very restrictive permissions,\n> and SSH commands will fail if the permissions are not appropriate. When\n> creating a directory for SSH keys in test scripts, attempt to clear any\n> ACLs that might otherwise cause the private key to inherit less\n> restrictive permissions than it requires.\n\nI was somewhat surprised to see your report, since all these tests\npassed without issue for me on '-rc0'! :D (64-bit cygwin only).\n\nSo, the difference seems to be down to FS ACLs, Hmmm ...\n\n(BTW, I am on windows 10 21H1)\n\nATB,\nRamsay Jones\n"},{"id":"440489","messageId":"20211104223633.5j556ggfga43myz5@fs","threadId":"56846","inReplyTo":"xmqqzgqj1yff.fsf@gitster.g","subject":"Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-04T22:36:33Z","receivedAt":"2021-11-04T22:36:38Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 04.11.2021 13:03, Junio C Hamano wrote:\n>Junio C Hamano writes:\n>\n>>> This change is required in particular to avoid tests relating to SSH\n>>> signing failing in Cygwin.\n>>\n>> ... I am not quite sure how this explains \"tests relating to ssh\n>> signing failing on Cygwin\". After all, this piece of code is\n>> lazy_prereq, which means that ssh-keygen in this block that fails\n>> (due to a less restrictive permissions) would merely mean that tests\n>> that are protected with GPGSSH prerequisite will be skipped without\n>> causing test failures. After all that is the whole point of\n>> computing prereq on the fly.\n>\n>The reason why I wondered about the above is that it can be an\n>indication of another breakage, namely, that we may have tests that\n>require a working ssh-keygen but are by mistake not protected with\n>GPGSSH prerequisite.\n>\n>The test_lazy_prereq block you touched may refrain from setting the\n>prerequisite on your system (due to the faulty test here that you\n>touched), but if we had such unprotected tests, we still will run\n>ssh signing tests and they would fail, due to the lack of the\n>prerequisite.\n>\n>And fixing the prereq block alone will hide that other breakage, at\n>least on your system. Hence my question.\n>\n>Thanks.\n\nThe problem is that the ssh-keygen in the layz_prereq will succeed but\nmight create a private key with world readable permissions. Only the\nremaining tests using this key will then fail with a \"your private key\npermissions are too restrictive\" like error. If we would like to make\nsure in the prereq that the keys actually work fine we would need to do\na signing operation with them in it.\n\nSomething like the following call would be enough:\necho \"test\" | ssh-keygen -Y sign -f $GPGSSHKEY_PRIMARY -n \"git\" \n\nNot sure if we want to go that far though. The setfacl seems fine to me\notherwise.\n"},{"id":"440506","messageId":"xmqqtugrxdo1.fsf@gitster.g","threadId":"56846","inReplyTo":"20211104223633.5j556ggfga43myz5@fs","subject":"Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-05T07:30:22Z","receivedAt":"2021-11-05T07:30:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer writes:\n\n> The problem is that the ssh-keygen in the layz_prereq will succeed but\n> might create a private key with world readable permissions. Only the\n> remaining tests using this key will then fail with a \"your private key\n> permissions are too restrictive\" like error. If we would like to make\n> sure in the prereq that the keys actually work fine we would need to do\n> a signing operation with them in it.\n\nThat sounds like a right thing to do, with or without the setfacl fix.\n\n>\n> Something like the following call would be enough:\n> echo \"test\" | ssh-keygen -Y sign -f $GPGSSHKEY_PRIMARY -n \"git\" \n> Not sure if we want to go that far though. The setfacl seems fine to me\n> otherwise.\n\n"},{"id":"440521","messageId":"20211105112525.GA25887@dinwoodie.org","threadId":"56846","inReplyTo":"xmqq7ddn3dlt.fsf@gitster.g","subject":"Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2021-11-05T11:25:25Z","receivedAt":"2021-11-05T11:25:43Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Thursday 04 November 2021 at 12:49 pm -0700, Junio C Hamano wrote:\n> Adam Dinwoodie writes:\n> \n> > SSH keys are expected to be created with very restrictive permissions,\n> > and SSH commands will fail if the permissions are not appropriate. When\n> > creating a directory for SSH keys in test scripts, attempt to clear any\n> > ACLs that might otherwise cause the private key to inherit less\n> > restrictive permissions than it requires.\n> \n> All of the above makes sense as an explanation as to why the\n> ssh-keygen command may be unhappy with the $GNUPGHOME directory that\n> is prepared here, but ...\n> \n> > This change is required in particular to avoid tests relating to SSH\n> > signing failing in Cygwin.\n> \n> ... I am not quite sure how this explains \"tests relating to ssh\n> signing failing on Cygwin\". After all, this piece of code is\n> lazy_prereq, which means that ssh-keygen in this block that fails\n> (due to a less restrictive permissions) would merely mean that tests\n> that are protected with GPGSSH prerequisite will be skipped without\n> causing test failures. After all that is the whole point of\n> computing prereq on the fly.\n\nThe issue is that the prerequisite check isn't _just_ checking a\nprerequisite: it's also creating an SSH key that's used without further\nmodification by the tests.\n\nThere are three cases to consider:\n\n- On systems where this prerequisite check fails, a key may or may not\n be created, but the tests that rely on the key won't be run, so it\n doesn't matter either way.\n\n- On (clearly the mainline) systems where this check passes and there\n are no ACL problems, the key that's generated is stored with\n sufficiently restrictive permissions that the tests that rely on the\n key can pass.\n\n- On my system, where ACLs are a problem, the prerequisite check passes,\n and a key is created, but it has permissions that are too permissive.\n As a result, when a test calls OpenSSH to use that key, OpenSSH\n refuses due to the permissions, and the test fails.\n\n> > Signed-off-by: Adam Dinwoodie \n> > Helped-by: Fabian Stelzer \n> \n> Please order these chronologically, i.e. Fabian helped and the patch\n> was finished, and finally you sent with your sign off.\n\nShall do! I'll resubmit a corrected version as soon as all the other\ndiscussions about this patch seem tidied up.\n\n> > ---\n> > t/lib-gpg.sh | 1 +\n> > 1 file changed, 1 insertion(+)\n> >\n> > diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\n> > index f99ef3e859..1d8e5b5b7e 100644\n> > --- a/t/lib-gpg.sh\n> > +++ b/t/lib-gpg.sh\n> > @@ -106,6 +106,7 @@ test_lazy_prereq GPGSSH '\n> > \ttest $? = 0 || exit 1;\n> > \tmkdir -p \"${GNUPGHOME}\" &&\n> > \tchmod 0700 \"${GNUPGHOME}\" &&\n> > +\t(setfacl -k \"${GNUPGHOME}\" 2>/dev/null || true) &&\n> > \tssh-keygen -t ed25519 -N \"\" -C \"git ed25519 key\" -f \"${GPGSSH_KEY_PRIMARY}\" >/dev/null &&\n> > \techo \"\\\"principal with number 1\\\" $(cat \"${GPGSSH_KEY_PRIMARY}.pub\")\" >> \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n> > \tssh-keygen -t rsa -b 2048 -N \"\" -C \"git rsa2048 key\" -f \"${GPGSSH_KEY_SECONDARY}\" >/dev/null &&\n> \n> There are other uses of ssh-keygen in the real tests but presumably\n> they just use the GNUPGHOME directory prepared with this lazy_prereq\n> block, and \"setfacl -k\" here would have wiped any possible loosening\n> of permission, and that is why this is the only place that needs a\n> change, right? That fact might deserve recording in the proposed\n> log message.\n\nMore than that: the uses of ssh-keygen elsewhere are calling `ssh-keygen\n-l`. That command doesn't generate a key at all, it merely calculates\nthe fingerprint from the keys that are generated with the ssh-keygen\ncommands in this prerequisite check.\n\nI think it's unusual that this test_lazy_prereq check isn't just\nchecking the state of the system but is creating the keys that will be\nused in later tests, but I didn't think to document that in my log\nbecause that was clearly a decision that had been made earlier. But I'm\nvery happy to make that logic clearer in my commit message!\n"},{"id":"440522","messageId":"20211105114747.GB25887@dinwoodie.org","threadId":"56846","inReplyTo":"6acb22bc-a90c-b8b6-2e7d-d7e17ba595ea@ramsayjones.plus.com","subject":"Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2021-11-05T11:47:47Z","receivedAt":"2021-11-05T11:47:54Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Thursday 04 November 2021 at 08:09 pm +0000, Ramsay Jones wrote:\n> Hi Adam,\n> \n> On 04/11/2021 19:25, Adam Dinwoodie wrote:\n> > SSH keys are expected to be created with very restrictive permissions,\n> > and SSH commands will fail if the permissions are not appropriate. When\n> > creating a directory for SSH keys in test scripts, attempt to clear any\n> > ACLs that might otherwise cause the private key to inherit less\n> > restrictive permissions than it requires.\n> \n> I was somewhat surprised to see your report, since all these tests\n> passed without issue for me on '-rc0'! :D (64-bit cygwin only).\n> \n> So, the difference seems to be down to FS ACLs, Hmmm ...\n> \n> (BTW, I am on windows 10 21H1)\n\nI'm running these tests in subdirectories in the temporary drive on\nDv4-size Windows 11 Pro Gen2 Azure VMs. I'm spinning up fresh VMs and\nusing new Cygwin installations regularly, in the name of build\nreproducibility; I'm vaguely working on automating more and more of the\nCygwin Git test and release processes.\n\n(At some point now they're becoming available, I'll probably shift to\nDdv5 Azure VMs for this work; I very much doubt that'll make a\ndifference, but I note it for the sake of completeness. Longer-term,\nI'm hoping to swap to using GitHub Actions to do most of the heavy\nlifting.)\n\nThis isn't the first time I've seen similar problems in this environment\nthat haven't been spotted elsewhere: see a1e03535db (t4129: fix\nsetfacl-related permissions failure, 2020-12-23).\n\nThe `getfacl` output for the temporary drive, from Cygwin's perspective,\nis as below; I'm `cd`ing into that directory and getting the Git\nrepositories by running `git clone https://github.com/git/git` from\nthere.\n\n```\n# file: /cygdrive/d\n# owner: NETWORK SERVICE\n# group: NETWORK SERVICE\nuser::r-x\ngroup::r-x\ngroup:SYSTEM:rwx #effective:r-x\ngroup:Administrators:rwx #effective:r-x\ngroup:Users:r-x\nmask::r-x\nother::r-x\ndefault:user::rwx\ndefault:group::---\ndefault:group:SYSTEM:rwx\ndefault:group:Administrators:rwx\ndefault:group:Users:rwx\ndefault:mask::rwx\ndefault:other::r-x\n```\n\nI'm honestly not sure what it is that means I keep hitting these\nproblems with this setup. I've managed to avoid needing anything but\nthe most cursory knowledge of extended permissions handling,\nparticularly for Cygwin where one has to contend with both the\nunderlying OS's interpretation of file permissions and with the Cygwin\nlayer's reinterpretations. I can't say I'm keen to get a deep working\nknowledge of how all these pieces interact!\n"},{"id":"440524","messageId":"YYUeKt0xQm/6QT+w@coredump.intra.peff.net","threadId":"56846","inReplyTo":"20211105112525.GA25887@dinwoodie.org","subject":"Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-05T12:06:02Z","receivedAt":"2021-11-05T12:06:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 05, 2021 at 11:25:25AM +0000, Adam Dinwoodie wrote:\n\n> > ... I am not quite sure how this explains \"tests relating to ssh\n> > signing failing on Cygwin\". After all, this piece of code is\n> > lazy_prereq, which means that ssh-keygen in this block that fails\n> > (due to a less restrictive permissions) would merely mean that tests\n> > that are protected with GPGSSH prerequisite will be skipped without\n> > causing test failures. After all that is the whole point of\n> > computing prereq on the fly.\n> \n> The issue is that the prerequisite check isn't _just_ checking a\n> prerequisite: it's also creating an SSH key that's used without further\n> modification by the tests.\n\nThis is sort of a side note to your main issue, but I think that relying\non a lazy_prereq for side effects is an anti-pattern. We make no\npromises about when or how often the prereqs might be run, and we try to\ninsulate them from the main tests (by putting them in a subshell and\nswitching their cwd).\n\nIt does happen to work here because the prereq script writes directly to\n$GNUPGHOME, and we run the lazy prereqs about when you'd expect. So I\ndon't think it's really in any danger of breaking, but it is definitely\nnot using the feature as it was intended. :)\n\nI think the more usual way would be to have an actual\ntest_expect_success block that creates the keys as a setup step\n(possibly triggered by a function, since it's included via lib-gpg.sh).\nIf we don't want to decide whether we have the GPGSSH prereq until then,\nthen that test can call test_set_prereq. See the LONG_REF case in t1401\nfor an example.\n\nAgain, that's mostly a tangent to your issue, and maybe not worth\nfutzing with at this point in the release cycle. I'm mostly just\nregistering my surprise. ;)\n\n> There are three cases to consider:\n> \n> - On systems where this prerequisite check fails, a key may or may not\n> be created, but the tests that rely on the key won't be run, so it\n> doesn't matter either way.\n> \n> - On (clearly the mainline) systems where this check passes and there\n> are no ACL problems, the key that's generated is stored with\n> sufficiently restrictive permissions that the tests that rely on the\n> key can pass.\n> \n> - On my system, where ACLs are a problem, the prerequisite check passes,\n> and a key is created, but it has permissions that are too permissive.\n> As a result, when a test calls OpenSSH to use that key, OpenSSH\n> refuses due to the permissions, and the test fails.\n\nFWIW, that explanation makes perfect sense to me (and your patch seems\nlike the right thing to do).\n\n-Peff\n"},{"id":"440525","messageId":"20211105121323.6fbyxoibaeyeshtw@fs","threadId":"56846","inReplyTo":"YYUeKt0xQm/6QT+w@coredump.intra.peff.net","subject":"Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-05T12:13:23Z","receivedAt":"2021-11-05T12:13:28Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 05.11.2021 08:06, Jeff King wrote:\n>On Fri, Nov 05, 2021 at 11:25:25AM +0000, Adam Dinwoodie wrote:\n>\n>> > ... I am not quite sure how this explains \"tests relating to ssh\n>> > signing failing on Cygwin\". After all, this piece of code is\n>> > lazy_prereq, which means that ssh-keygen in this block that fails\n>> > (due to a less restrictive permissions) would merely mean that tests\n>> > that are protected with GPGSSH prerequisite will be skipped without\n>> > causing test failures. After all that is the whole point of\n>> > computing prereq on the fly.\n>>\n>> The issue is that the prerequisite check isn't _just_ checking a\n>> prerequisite: it's also creating an SSH key that's used without further\n>> modification by the tests.\n>\n>This is sort of a side note to your main issue, but I think that relying\n>on a lazy_prereq for side effects is an anti-pattern. We make no\n>promises about when or how often the prereqs might be run, and we try to\n>insulate them from the main tests (by putting them in a subshell and\n>switching their cwd).\n>\n>It does happen to work here because the prereq script writes directly to\n>$GNUPGHOME, and we run the lazy prereqs about when you'd expect. So I\n>don't think it's really in any danger of breaking, but it is definitely\n>not using the feature as it was intended. :)\n>\n>I think the more usual way would be to have an actual\n>test_expect_success block that creates the keys as a setup step\n>(possibly triggered by a function, since it's included via lib-gpg.sh).\n>If we don't want to decide whether we have the GPGSSH prereq until then,\n>then that test can call test_set_prereq. See the LONG_REF case in t1401\n>for an example.\n\nI was not aware of this. I assumed prereq not just meant checking for\npre requisites but also setting them up. Since i still have a follow up\npatch series in progress i will keep that in mind and move the actual\nsetup code into a function in lib-gpg. There are gpg ssh tests in\nmultiple different test files so i didn't want to create keys for each\nof them repeatedly. And i'm not a fan of checking in those file (like\nthe gpg keyring for testing) since it also needs documentation on how it\nwas generated that is not quaranteed to match how it was done.\n\nWe still could add the actual sign test into the prereq for now.\nA `echo \"test\" | ssh-keygen -Y sign -f $GPGSSHKEY_PRIMARY -n \"git\"`\nwill make sure that the keys actually work.\n\n>\n>Again, that's mostly a tangent to your issue, and maybe not worth\n>futzing with at this point in the release cycle. I'm mostly just\n>registering my surprise. ;)\n>\n>> There are three cases to consider:\n>>\n>> - On systems where this prerequisite check fails, a key may or may not\n>> be created, but the tests that rely on the key won't be run, so it\n>> doesn't matter either way.\n>>\n>> - On (clearly the mainline) systems where this check passes and there\n>> are no ACL problems, the key that's generated is stored with\n>> sufficiently restrictive permissions that the tests that rely on the\n>> key can pass.\n>>\n>> - On my system, where ACLs are a problem, the prerequisite check passes,\n>> and a key is created, but it has permissions that are too permissive.\n>> As a result, when a test calls OpenSSH to use that key, OpenSSH\n>> refuses due to the permissions, and the test fails.\n>\n>FWIW, that explanation makes perfect sense to me (and your patch seems\n>like the right thing to do).\n>\n\n\n>-Peff\n"},{"id":"440538","messageId":"xmqqk0hmxyw0.fsf@gitster.g","threadId":"56846","inReplyTo":"YYUeKt0xQm/6QT+w@coredump.intra.peff.net","subject":"Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-05T18:04:15Z","receivedAt":"2021-11-05T18:04:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Nov 05, 2021 at 11:25:25AM +0000, Adam Dinwoodie wrote:\n>\n>> > ... I am not quite sure how this explains \"tests relating to ssh\n>> > signing failing on Cygwin\". After all, this piece of code is\n>> > lazy_prereq, which means that ssh-keygen in this block that fails\n>> > (due to a less restrictive permissions) would merely mean that tests\n>> > that are protected with GPGSSH prerequisite will be skipped without\n>> > causing test failures. After all that is the whole point of\n>> > computing prereq on the fly.\n>> \n>> The issue is that the prerequisite check isn't _just_ checking a\n>> prerequisite: it's also creating an SSH key that's used without further\n>> modification by the tests.\n>\n> This is sort of a side note to your main issue, but I think that relying\n> on a lazy_prereq for side effects is an anti-pattern. We make no\n> promises about when or how often the prereqs might be run, and we try to\n> insulate them from the main tests (by putting them in a subshell and\n> switching their cwd).\n>\n> It does happen to work here because the prereq script writes directly to\n> $GNUPGHOME, and we run the lazy prereqs about when you'd expect. So I\n> don't think it's really in any danger of breaking, but it is definitely\n> not using the feature as it was intended. :)\n\nThis merely imitates what GPG lazy-prerequisite started and imitated\nby other existing signature backends.\n\nI'd expect that you need some \"initialization\" for a feature X as\npart of asking \"is feature X usable in this environment?\". Reusing\nthe result of the initialization for true tests is probably an\noptimization worth making. As long as the question is answered for\nthe true tests, that is [*].\n\n side note: so being able to create a key alone, without\n verifying the resulting key is usable, is a no-no. That is why\n I said it is a good idea to check if the resulting key is usable\n inside the lazy-prereq.\n\n> Again, that's mostly a tangent to your issue, and maybe not worth\n> futzing with at this point in the release cycle. I'm mostly just\n> registering my surprise. ;)\n\nMy purist side is with you and share the surprise. But my practical\nside says this is probably an optimization worth taking. If prereq\nonly checks \"if we initialize the keys right way, we can use ssh\nsigning\" and then removes the key and the equivalent to .ssh/\ndirectory, and a real test does \"Ok, prereq passes so we know ssh\nsigning is to be tested. Now initialize the .ssh/ equivalent and\ncreate key\", a fix like Adam came up with must be duplicated in two\n(or more) places, one for the prereq that initializes the keys\n\"right way\", and one for each test script that prepares the key used\nfor it.\n"},{"id":"440539","messageId":"xmqqfssaxyfn.fsf@gitster.g","threadId":"56846","inReplyTo":"20211105112525.GA25887@dinwoodie.org","subject":"Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-05T18:14:04Z","receivedAt":"2021-11-05T18:14:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Dinwoodie writes:\n\n> On Thursday 04 November 2021 at 12:49 pm -0700, Junio C Hamano wrote:\n>> Adam Dinwoodie writes:\n>> \n>> > SSH keys are expected to be created with very restrictive permissions,\n>> > and SSH commands will fail if the permissions are not appropriate. When\n>> > creating a directory for SSH keys in test scripts, attempt to clear any\n>> > ACLs that might otherwise cause the private key to inherit less\n>> > restrictive permissions than it requires.\n>> \n>> All of the above makes sense as an explanation as to why the\n>> ssh-keygen command may be unhappy with the $GNUPGHOME directory that\n>> is prepared here, but ...\n>> \n>> > This change is required in particular to avoid tests relating to SSH\n>> > signing failing in Cygwin.\n>> \n>> ... I am not quite sure how this explains \"tests relating to ssh\n>> signing failing on Cygwin\". After all, this piece of code is\n>> lazy_prereq, which means that ssh-keygen in this block that fails\n>> (due to a less restrictive permissions) would merely mean that tests\n>> that are protected with GPGSSH prerequisite will be skipped without\n>> causing test failures. After all that is the whole point of\n>> computing prereq on the fly.\n>\n> The issue is that the prerequisite check isn't _just_ checking a\n> prerequisite: it's also creating an SSH key that's used without further\n> modification by the tests.\n>\n> There are three cases to consider:\n>\n> - On systems where this prerequisite check fails, a key may or may not\n> be created, but the tests that rely on the key won't be run, so it\n> doesn't matter either way.\n>\n> - On (clearly the mainline) systems where this check passes and there\n> are no ACL problems, the key that's generated is stored with\n> sufficiently restrictive permissions that the tests that rely on the\n> key can pass.\n>\n> - On my system, where ACLs are a problem, the prerequisite check passes,\n> and a key is created, but it has permissions that are too permissive.\n> As a result, when a test calls OpenSSH to use that key, OpenSSH\n> refuses due to the permissions, and the test fails.\n\nMakes sense. If we can update the commit log message so that the\nabove three points are clear to readers without asking (all three\nmay not necessarily need to be spelled out in the bulletted list\nform), that would be great.\n"},{"id":"440542","messageId":"CA+kUOa=vqFNXe2QKc8K31OLL0zkEsK7wAk6hPMxjQJNVM7PsGQ@mail.gmail.com","threadId":"56846","inReplyTo":"xmqqk0hmxyw0.fsf@gitster.g","subject":"Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2021-11-05T18:49:14Z","receivedAt":"2021-11-05T18:49:52Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Fri, 5 Nov 2021 at 18:04, Junio C Hamano wrote:\n>\n> Jeff King writes:\n>\n> > On Fri, Nov 05, 2021 at 11:25:25AM +0000, Adam Dinwoodie wrote:\n> >\n> >> > ... I am not quite sure how this explains \"tests relating to ssh\n> >> > signing failing on Cygwin\". After all, this piece of code is\n> >> > lazy_prereq, which means that ssh-keygen in this block that fails\n> >> > (due to a less restrictive permissions) would merely mean that tests\n> >> > that are protected with GPGSSH prerequisite will be skipped without\n> >> > causing test failures. After all that is the whole point of\n> >> > computing prereq on the fly.\n> >>\n> >> The issue is that the prerequisite check isn't _just_ checking a\n> >> prerequisite: it's also creating an SSH key that's used without further\n> >> modification by the tests.\n> >\n> > This is sort of a side note to your main issue, but I think that relying\n> > on a lazy_prereq for side effects is an anti-pattern. We make no\n> > promises about when or how often the prereqs might be run, and we try to\n> > insulate them from the main tests (by putting them in a subshell and\n> > switching their cwd).\n> >\n> > It does happen to work here because the prereq script writes directly to\n> > $GNUPGHOME, and we run the lazy prereqs about when you'd expect. So I\n> > don't think it's really in any danger of breaking, but it is definitely\n> > not using the feature as it was intended. :)\n>\n> This merely imitates what GPG lazy-prerequisite started and imitated\n> by other existing signature backends.\n>\n> I'd expect that you need some \"initialization\" for a feature X as\n> part of asking \"is feature X usable in this environment?\". Reusing\n> the result of the initialization for true tests is probably an\n> optimization worth making. As long as the question is answered for\n> the true tests, that is [*].\n>\n> side note: so being able to create a key alone, without\n> verifying the resulting key is usable, is a no-no. That is why\n> I said it is a good idea to check if the resulting key is usable\n> inside the lazy-prereq.\n\nI'm not convinced by this. Or at least, I'm convinced by the\nprinciple, but wary of the implications.\n\nTake this case, for example: the function being tested by the\nGPGSSH-gated tests is function that should work on Cygwin. If there\nwere a regression, running the tests on Cygwin ought to catch it, and\nin this instance the tests failing meant that we caught a bug. On this\noccasion it was a bug in the test library rather than the function\nthat most Git users care about, but I don't think there's anything\ninherent about this situation that means it couldn't have been a\nfunctional bug.\n\nHowever, if the prerequisite checks had not only created the key but\nalso verified it could be used, in this scenario these tests would\nhave been skipped. The function the tests are exercising would still\nwork, and users would therefore expect it to continue working, but the\nonly chance we'd have to spot any future regressions is if they're hit\nin some other environment or someone spots the tests being skipped by\ntrawling through the reams of test output to check what tests are\nbeing skipped.\n\nThis is probably a much broader conversation. I remember when I first\nstarted packaging Git for Cygwin, I produced a release that didn't\nhave support for HTTPS URLs due to a missing dependency in my build\nenvironment. The build and test suite all passed -- it assumed I just\nwanted to build a release that didn't have HTTPS support -- so some\nrelatively critical function was silently skipped. I don't know how to\navoid that sort of issue other than relying on (a) user bug (or at\nleast missing function) reports and (b) folk building Git for\nthemselves/others periodically going through the output of the\nconfigure scripts and the skipped subtests to make sure only expected\nthings get missed; neither of those options seem great to me.\n"},{"id":"440545","messageId":"xmqqv916wh7t.fsf@gitster.g","threadId":"56846","inReplyTo":"CA+kUOa=vqFNXe2QKc8K31OLL0zkEsK7wAk6hPMxjQJNVM7PsGQ@mail.gmail.com","subject":"Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-05T19:11:18Z","receivedAt":"2021-11-05T19:11:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Dinwoodie writes:\n\n> This is probably a much broader conversation. I remember when I first\n> started packaging Git for Cygwin, I produced a release that didn't\n> have support for HTTPS URLs due to a missing dependency in my build\n> environment. The build and test suite all passed -- it assumed I just\n> wanted to build a release that didn't have HTTPS support -- so some\n> relatively critical function was silently skipped. I don't know how to\n> avoid that sort of issue other than relying on (a) user bug (or at\n> least missing function) reports and (b) folk building Git for\n> themselves/others periodically going through the output of the\n> configure scripts and the skipped subtests to make sure only expected\n> things get missed; neither of those options seem great to me.\n\nI agree with you that there needs a good way to enumerate what the\nunsatisfied prerequisites for a particular build are. That would\nhave helped in your HTTPS situation.\n\nBut that is a separate issue how we should determine a lazy\nprerequisite for any feature is satisified.\n\n\"We have this feature that our code utilizes. If it is not working\ncorrectly, then we can expect our code that depends on it would not\nwork, and it is no use testing\" is what the test prerequisite system\ntries to achieve. That is quite different from \"the frotz feature\ncould work here as we see a binary /usr/bin/frotz installed, so\nlet's go test our code that depends on it---we'll find out if the\ninstalled frotz is not what we expect, or way too old to help our\ncode, as the test will break and let us notice.\"\n"},{"id":"440546","messageId":"CA+kUOamwQmK6te66sE+EVLPhwmBFZ+CXC9p=HJ4y0KC0wnkNsQ@mail.gmail.com","threadId":"56846","inReplyTo":"xmqqv916wh7t.fsf@gitster.g","subject":"Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2021-11-05T19:24:53Z","receivedAt":"2021-11-05T19:25:35Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Fri, 5 Nov 2021 at 19:11, Junio C Hamano wrote:\n>\n> Adam Dinwoodie writes:\n>\n> > This is probably a much broader conversation. I remember when I first\n> > started packaging Git for Cygwin, I produced a release that didn't\n> > have support for HTTPS URLs due to a missing dependency in my build\n> > environment. The build and test suite all passed -- it assumed I just\n> > wanted to build a release that didn't have HTTPS support -- so some\n> > relatively critical function was silently skipped. I don't know how to\n> > avoid that sort of issue other than relying on (a) user bug (or at\n> > least missing function) reports and (b) folk building Git for\n> > themselves/others periodically going through the output of the\n> > configure scripts and the skipped subtests to make sure only expected\n> > things get missed; neither of those options seem great to me.\n>\n> I agree with you that there needs a good way to enumerate what the\n> unsatisfied prerequisites for a particular build are. That would\n> have helped in your HTTPS situation.\n>\n> But that is a separate issue how we should determine a lazy\n> prerequisite for any feature is satisified.\n>\n> \"We have this feature that our code utilizes. If it is not working\n> correctly, then we can expect our code that depends on it would not\n> work, and it is no use testing\" is what the test prerequisite system\n> tries to achieve. That is quite different from \"the frotz feature\n> could work here as we see a binary /usr/bin/frotz installed, so\n> let's go test our code that depends on it---we'll find out if the\n> installed frotz is not what we expect, or way too old to help our\n> code, as the test will break and let us notice.\"\n\nI can see how they're separate problems, but they seem related to me.\nIf OpenSSH were not installed on my system, Git would be compiled\nwithout this function and the tests would be skipped. If OpenSSH is\ninstalled but the prerequisite check fails, Git will be compiled with\nthe function, but the tests will be skipped. In the first case,\nfunction some users might depend on will be missing; in the second,\nthe function will be nominally present but we won't be sure it's\nactually working as expected. Both issues would be avoided if the\ntests were always run, because suddenly both sorts of silent failure\nbecome noisy.\n\nI'm not actually advocating that -- running all tests all the time\nwould clearly cause far more problems than it would solve! -- but\nthat's why I'm seeing these as two sides of the same coin, and\nproblems that might have a single shared solution.\n"},{"id":"440547","messageId":"20211105193106.3195-1-adam@dinwoodie.org","threadId":"56846","inReplyTo":"20211104192533.2520-1-adam@dinwoodie.org","subject":"[PATCH v2] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2021-11-05T19:31:06Z","receivedAt":"2021-11-05T19:31:45Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"As well as checking that the relevant functionality is available, the\nGPGSSH prerequisite check creates the SSH keys that are used by the test\nfunctions it gates. If these keys are created in a directory that\nhas a default Access Control List, the key files can inherit those\npermissions.\n\nThis can result in a scenario where the private keys are created\nsuccessfully, so the prerequisite check passes and the tests are run,\nbut the key files have permissions that are too permissive, meaning\nOpenSSH will refuse to load them and the tests will fail.\n\nTo avoid this happening, before creating the keys, clear any default ACL\nset on the directory that will contain them. This step allowed to fail;\nif setfacl isn't present, that's a very likely indicator that the\nfilesystem in question simply doesn't support default ACLs.\n\nHelped-by: Fabian Stelzer \nSigned-off-by: Adam Dinwoodie \n---\n t/lib-gpg.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex f99ef3e859..1d8e5b5b7e 100644\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -106,6 +106,7 @@ test_lazy_prereq GPGSSH '\n \ttest $? = 0 || exit 1;\n \tmkdir -p \"${GNUPGHOME}\" &&\n \tchmod 0700 \"${GNUPGHOME}\" &&\n+\t(setfacl -k \"${GNUPGHOME}\" 2>/dev/null || true) &&\n \tssh-keygen -t ed25519 -N \"\" -C \"git ed25519 key\" -f \"${GPGSSH_KEY_PRIMARY}\" >/dev/null &&\n \techo \"\\\"principal with number 1\\\" $(cat \"${GPGSSH_KEY_PRIMARY}.pub\")\" >> \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n \tssh-keygen -t rsa -b 2048 -N \"\" -C \"git rsa2048 key\" -f \"${GPGSSH_KEY_SECONDARY}\" >/dev/null &&\n-- \n2.33.0\n\n"},{"id":"440550","messageId":"CAPUEsphasEqT=qfHgO1o4LpzFSWcdhmtTk3o+hYaGNTs35EQKw@mail.gmail.com","threadId":"56846","inReplyTo":"CA+kUOamwQmK6te66sE+EVLPhwmBFZ+CXC9p=HJ4y0KC0wnkNsQ@mail.gmail.com","subject":"Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-05T21:00:32Z","receivedAt":"2021-11-05T21:00:46Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Fri, Nov 5, 2021 at 1:16 PM Adam Dinwoodie wrote:\n\n> If OpenSSH were not installed on my system, Git would be compiled\n> without this function and the tests would be skipped.\n\nthat is correct for the http dependency (because it is a library that\ngets linked in), but not for the OpenSSH dependency, which is just\ninvoking the binary at runtime.\n\nRegardless of what you have in your build environment the code will be\ncompiled in (and tested or not), and will fail instead at runtime if\nOpenSSH is not installed.\n\nCarlo\n"},{"id":"440551","messageId":"xmqqk0hmwc0c.fsf@gitster.g","threadId":"56846","inReplyTo":"20211105193106.3195-1-adam@dinwoodie.org","subject":"Re: [PATCH v2] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-05T21:03:47Z","receivedAt":"2021-11-05T21:03:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Dinwoodie writes:\n\n> As well as checking that the relevant functionality is available, the\n> GPGSSH prerequisite check creates the SSH keys that are used by the test\n> functions it gates. If these keys are created in a directory that\n> has a default Access Control List, the key files can inherit those\n> permissions.\n>\n> This can result in a scenario where the private keys are created\n> successfully, so the prerequisite check passes and the tests are run,\n> but the key files have permissions that are too permissive, meaning\n> OpenSSH will refuse to load them and the tests will fail.\n\nThat may indicate that \"private keys are created successfully\" is a\nbit too optimistic. A key that did not exist but now exists indeed\nwas created, but if it cannot be used in tests, calling it\n\"successfully created\" is a bit too charitable, I would say.\n\n ... where the private keys appear to have been created\n successfully, but at the runtime OpenSSH will refuse to load\n these keys due to permissions that are too loose. In other\n words, the keys created here are not usable. Yet the lazy_prereq\n is set, pretending all is well, and makes the real tests fail.\n\nAnd when described that way, we'd realize that \"setfacl -k\" solution\nmay be closing one known way that a key, that seemingly was created\nsuccessfully, can be unusable in real tests, but it is not\naddressing the root cause of the breakage you observed---the\nlazy_prereq is not set based on what really matters, i.e. is the key\nusable to sign and verify?\n\n> To avoid this happening, before creating the keys, clear any default ACL\n\n\"happening\" -> \"from happening\"\n\n> set on the directory that will contain them. This step allowed to fail;\n\n\"allowed\" -> \"is allowed\".\n\n> if setfacl isn't present, that's a very likely indicator that the\n> filesystem in question simply doesn't support default ACLs.\n\nTrue. Or setfacl command fails to futz with the ACL for whatever\nreason, in which case you may still have the \"we 'successfully'\ncreated a key, but it turns out that it was unusable in real tests\"\nproblem. As long as the lazy_prereq is not set to pretend that all\nis well, we won't see test breakage noise that distracts those who\nare watching for breakage due to \"git\". And that is why we want to\nadd \"is the key really usable\" check before the lazy_prereq declares\na success.\n\n> Helped-by: Fabian Stelzer \n> Signed-off-by: Adam Dinwoodie \n> ---\n> t/lib-gpg.sh | 1 +\n> 1 file changed, 1 insertion(+)\n\nOther than that, the above explanation reads well.\n\nThanks.\n\n>\n> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\n> index f99ef3e859..1d8e5b5b7e 100644\n> --- a/t/lib-gpg.sh\n> +++ b/t/lib-gpg.sh\n> @@ -106,6 +106,7 @@ test_lazy_prereq GPGSSH '\n> \ttest $? = 0 || exit 1;\n> \tmkdir -p \"${GNUPGHOME}\" &&\n> \tchmod 0700 \"${GNUPGHOME}\" &&\n> +\t(setfacl -k \"${GNUPGHOME}\" 2>/dev/null || true) &&\n> \tssh-keygen -t ed25519 -N \"\" -C \"git ed25519 key\" -f \"${GPGSSH_KEY_PRIMARY}\" >/dev/null &&\n> \techo \"\\\"principal with number 1\\\" $(cat \"${GPGSSH_KEY_PRIMARY}.pub\")\" >> \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n> \tssh-keygen -t rsa -b 2048 -N \"\" -C \"git rsa2048 key\" -f \"${GPGSSH_KEY_SECONDARY}\" >/dev/null &&\n"},{"id":"440552","messageId":"676553a5-2119-45bd-007d-40bb0802a263@ramsayjones.plus.com","threadId":"56846","inReplyTo":"20211105114747.GB25887@dinwoodie.org","subject":"Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2021-11-05T21:44:15Z","receivedAt":"2021-11-05T21:44:24Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 05/11/2021 11:47, Adam Dinwoodie wrote:\n> On Thursday 04 November 2021 at 08:09 pm +0000, Ramsay Jones wrote:\n>> Hi Adam,\n>>\n>> On 04/11/2021 19:25, Adam Dinwoodie wrote:\n>>> SSH keys are expected to be created with very restrictive permissions,\n>>> and SSH commands will fail if the permissions are not appropriate. When\n>>> creating a directory for SSH keys in test scripts, attempt to clear any\n>>> ACLs that might otherwise cause the private key to inherit less\n>>> restrictive permissions than it requires.\n>>\n>> I was somewhat surprised to see your report, since all these tests\n>> passed without issue for me on '-rc0'! :D (64-bit cygwin only).\n>>\n>> So, the difference seems to be down to FS ACLs, Hmmm ...\n>>\n>> (BTW, I am on windows 10 21H1)\n\nJust FYI, tests t4202, t5534 and t6200 all pass for me without issue\non both of the -rc0 and -rc1 builds.\n\n> I'm running these tests in subdirectories in the temporary drive on\n> Dv4-size Windows 11 Pro Gen2 Azure VMs. I'm spinning up fresh VMs and\n> using new Cygwin installations regularly, in the name of build\n> reproducibility; I'm vaguely working on automating more and more of the\n> Cygwin Git test and release processes.\n> \n> (At some point now they're becoming available, I'll probably shift to\n> Ddv5 Azure VMs for this work; I very much doubt that'll make a\n> difference, but I note it for the sake of completeness. Longer-term,\n> I'm hoping to swap to using GitHub Actions to do most of the heavy\n> lifting.)\n> \n> This isn't the first time I've seen similar problems in this environment\n> that haven't been spotted elsewhere: see a1e03535db (t4129: fix\n> setfacl-related permissions failure, 2020-12-23).\n> \n> The `getfacl` output for the temporary drive, from Cygwin's perspective,\n> is as below; I'm `cd`ing into that directory and getting the Git\n> repositories by running `git clone https://github.com/git/git` from\n> there.\n\nHeh, yeah, given the setup above, I'm not exactly shocked that you\nare running into permission problems ... ;-)\n\n> ```\n> # file: /cygdrive/d\n> # owner: NETWORK SERVICE\n> # group: NETWORK SERVICE\n> user::r-x\n> group::r-x\n> group:SYSTEM:rwx #effective:r-x\n> group:Administrators:rwx #effective:r-x\n> group:Users:r-x\n> mask::r-x\n> other::r-x\n> default:user::rwx\n> default:group::---\n> default:group:SYSTEM:rwx\n> default:group:Administrators:rwx\n> default:group:Users:rwx\n> default:mask::rwx\n> default:other::r-x\n> ```\n\nI have been using cygwin since the 'beta-8' days (windows NT 3.51, so about\n1997 or so) and have run into several permission problems over the years.\nSo, in order to finesse these issues, I find it best to keep it simple.\nI do not move outside of my cygwin installation (at C:\\cygwin64), which\neven includes my home directory and all git repos.\n\nSo, for me:\n \n $ echo $HOME\n /home/ramsay\n $ cygpath -w /home/ramsay\n C:\\cygwin64\\home\\ramsay\n $ \n \n $ getfacl /cygdrive/c/cygwin64\n # file: /cygdrive/c/cygwin64\n # owner: ramsay\n # group: None\n user::rwx\n group::r-x\n other::r-x\n default:user::rwx\n default:group::r-x\n default:other::r-x\n \n $ id\n uid=1001(ramsay) gid=513(None) groups=513(None),545(Users),4(INTERACTIVE),66049(CONSOLE LOGON),11(Authenticated Users),15(This Organization),113(Local account),4095(CurrentSession),66048(LOCAL),262154(NTLM Authentication),401408(Medium Mandatory Level)\n $\n\n> I'm honestly not sure what it is that means I keep hitting these\n> problems with this setup. I've managed to avoid needing anything but\n> the most cursory knowledge of extended permissions handling,\n> particularly for Cygwin where one has to contend with both the\n> underlying OS's interpretation of file permissions and with the Cygwin\n> layer's reinterpretations. I can't say I'm keen to get a deep working\n> knowledge of how all these pieces interact!\n\nI'm definitely no expert, but even with my current setup, I have had\npermission problems. I used to 'ssh' into cygwin from Linux so that\nI could build/test git on Linux/cygwin at the same time - that worked\nfine for many many years, until a test was added that failed when I\nwas remotely logged-in to cygwin, but passed when I was actually directly\nlogged-in on the windows laptop. I don't remember the details, but ever\nsince I have been having to run the tests locally.\n\n[When remotely logged in:\n\n $ id\n uid=1001(ramsay) gid=513(None) groups=513(None),114(Local account and member of Administrators group),0(root),545(Users),2(NETWORK),11(Authenticated Users),15(This Organization),113(Local account),4095(CurrentSession),262154(NTLM Authentication),405504(High Mandatory Level)\n $ \n\nYes, I am still using the 'privileged user' account for the 'sshd' service.\nI suppose I should re-configure it to use the LOCAL ACCOUNT and test again,\nbut, well, if it ain't broke ... ;-)\n]\n\nATB,\nRamsay Jones\n\n"},{"id":"440561","messageId":"YYXAwxmhrLLMBqa+@coredump.intra.peff.net","threadId":"56846","inReplyTo":"xmqqk0hmxyw0.fsf@gitster.g","subject":"Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-05T23:39:47Z","receivedAt":"2021-11-05T23:39:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 05, 2021 at 11:04:15AM -0700, Junio C Hamano wrote:\n\n> > This is sort of a side note to your main issue, but I think that relying\n> > on a lazy_prereq for side effects is an anti-pattern. We make no\n> > promises about when or how often the prereqs might be run, and we try to\n> > insulate them from the main tests (by putting them in a subshell and\n> > switching their cwd).\n> >\n> > It does happen to work here because the prereq script writes directly to\n> > $GNUPGHOME, and we run the lazy prereqs about when you'd expect. So I\n> > don't think it's really in any danger of breaking, but it is definitely\n> > not using the feature as it was intended. :)\n> \n> This merely imitates what GPG lazy-prerequisite started and imitated\n> by other existing signature backends.\n\nAh, you're right. I should have checked the other GPG ones. It looks\nlike that happened recently-ish in b417ec5f22 (tests: turn GPG, GPGSM\nand RFC1991 into lazy prereqs, 2020-03-26).\n\nBefore that the code was run outside of any test block at all, which I\nthink is even worse.\n\n> I'd expect that you need some \"initialization\" for a feature X as\n> part of asking \"is feature X usable in this environment?\". Reusing\n> the result of the initialization for true tests is probably an\n> optimization worth making. As long as the question is answered for\n> the true tests, that is [*].\n\nYes, though if it's possible, I think doing less work in the prereq\ncheck might be a good approach (like say, just checking gpg or openssh\nversion if we can). It results in flakier prereqs that may say \"yes, we\nhave feature X\" when we don't. But it gets a human's attention when\nit fails, rather than quietly skipping tests (which is the same point\nAdam is making downthread).\n\nIt definitely is not something to fiddle with at this point in the -rc\ncycle, though.\n\n> > Again, that's mostly a tangent to your issue, and maybe not worth\n> > futzing with at this point in the release cycle. I'm mostly just\n> > registering my surprise. ;)\n> \n> My purist side is with you and share the surprise. But my practical\n> side says this is probably an optimization worth taking. If prereq\n> only checks \"if we initialize the keys right way, we can use ssh\n> signing\" and then removes the key and the equivalent to .ssh/\n> directory, and a real test does \"Ok, prereq passes so we know ssh\n> signing is to be tested. Now initialize the .ssh/ equivalent and\n> create key\", a fix like Adam came up with must be duplicated in two\n> (or more) places, one for the prereq that initializes the keys\n> \"right way\", and one for each test script that prepares the key used\n> for it.\n\nTo be clear, I wasn't suggesting doing the key setup twice. I was just\nsuggesting moving it out of a lazy prereq into a real test_expect block\nthat sets the prereq flag as a side effect. That just makes the timing\nand the fact of running it more deliberate on the part of the test\nscripts.\n\nIt's probably not worth the effort, though. I think my line of thinking\nis coming from the \"purist\" side, and doesn't have any practical\nbenefit.\n\n-Peff\n"},{"id":"440562","messageId":"YYXD3NESdiDI4B6G@coredump.intra.peff.net","threadId":"56846","inReplyTo":"CA+kUOa=vqFNXe2QKc8K31OLL0zkEsK7wAk6hPMxjQJNVM7PsGQ@mail.gmail.com","subject":"Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-05T23:53:00Z","receivedAt":"2021-11-05T23:53:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 05, 2021 at 06:49:14PM +0000, Adam Dinwoodie wrote:\n\n> This is probably a much broader conversation. I remember when I first\n> started packaging Git for Cygwin, I produced a release that didn't\n> have support for HTTPS URLs due to a missing dependency in my build\n> environment. The build and test suite all passed -- it assumed I just\n> wanted to build a release that didn't have HTTPS support -- so some\n> relatively critical function was silently skipped. I don't know how to\n> avoid that sort of issue other than relying on (a) user bug (or at\n> least missing function) reports and (b) folk building Git for\n> themselves/others periodically going through the output of the\n> configure scripts and the skipped subtests to make sure only expected\n> things get missed; neither of those options seem great to me.\n\nThe HTTP tests in particular have a knob for this, as I was worried\nabout this kind of situation when we introduced auto-enabling of network\ntests back in 83d842dc8c (tests: turn on network daemon tests by\ndefault, 2014-02-10). The solution there was to make the knob a\ntri-state: the default is \"auto\", which will try to probe whether we\nhave a working apache setup, but setting it to \"true\" will complain if\nthat setup fails.\n\nNow that's not a perfect solution:\n\n - you have to know to flip the switch to \"true\". For an old switch\n like HTTP, that's easy. But somebody packaging Git might not even\n realize GPGSSH was a new thing.\n\n - The \"true\" knob only covers probing of the environment. If you\n accidentally build with NO_CURL, we'd still quietly skip the tests.\n It might be reasonable to change this.\n\n - In your particular case, it probably would not have helped anyway\n because we don't have any specific HTTPS tests (there is an option\n to set up the default server with SSL, but I didn't even realize\n that until just now; I wonder if it actually works).\n\nSo I dunno. I guess because of point 1, having an allow-known-skips list\nwould be more helpful. That gives you the opportunity to examine new\nprereqs and decide if they ought to be skipped or not in your setup.\n\n-Peff\n"},{"id":"440667","messageId":"AS8PR02MB730266C2B87493F2AF712D989C919@AS8PR02MB7302.eurprd02.prod.outlook.com","threadId":"56846","inReplyTo":"xmqqk0hmwc0c.fsf@gitster.g","subject":"RE: [PATCH v2] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Kerry, Richard","fromEmail":"richard.kerry@atos.net","sentAt":"2021-11-08T16:40:29Z","receivedAt":"2021-11-08T16:47:43Z","isPatch":true,"sender":{"key":"richard.kerry@atos.net","avatar":null},"body":"> \n> > To avoid this happening, before creating the keys, clear any default\n> > ACL\n> \n> \"happening\" -> \"from happening\"\n> \n\nNo, original is correct.\n\nTo avoid this happening.\nTo keep this from happening.\nTo prevent this happening. \nTo prevent this from happening. \n\nWould I think all be correct.\n\"to avoid from\" is not right.\n\nRegards,\nRichard,\n\n"},{"id":"440673","messageId":"xmqqbl2uv4ri.fsf@gitster.g","threadId":"56846","inReplyTo":"AS8PR02MB730266C2B87493F2AF712D989C919@AS8PR02MB7302.eurprd02.prod.outlook.com","subject":"Re: [PATCH v2] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-08T19:14:41Z","receivedAt":"2021-11-08T19:14:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kerry, Richard\" writes:\n\n>> \n>> > To avoid this happening, before creating the keys, clear any default\n>> > ACL\n>> \n>> \"happening\" -> \"from happening\"\n>> \n>\n> No, original is correct.\n>\n> To avoid this happening.\n> To keep this from happening.\n> To prevent this happening. \n> To prevent this from happening. \n>\n> Would I think all be correct.\n> \"to avoid from\" is not right.\n\nBut I meant to say \"to avoid this from happening\", not \"to avoid\nfrom\", which I gree is not right.\n"},{"id":"440752","messageId":"AS8PR02MB73028E498D0AB831FE8028E89C929@AS8PR02MB7302.eurprd02.prod.outlook.com","threadId":"56846","inReplyTo":"xmqqbl2uv4ri.fsf@gitster.g","subject":"RE: [PATCH v2] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Kerry, Richard","fromEmail":"richard.kerry@atos.net","sentAt":"2021-11-09T17:23:12Z","receivedAt":"2021-11-09T17:23:22Z","isPatch":true,"sender":{"key":"richard.kerry@atos.net","avatar":null},"body":"\n\n> -----Original Message-----\n> From: Junio C Hamano \n> Sent: 08 November 2021 19:15\n> To: Kerry, Richard \n> Cc: Adam Dinwoodie ; git@vger.kernel.org; Fabian\n> Stelzer \n> Subject: Re: [PATCH v2] t/lib-git.sh: fix ACL-related permissions failure\n> \n> Caution! External email. Do not open attachments or click links, unless this\n> email comes from a known sender and you know the content is safe.\n> \n> \"Kerry, Richard\" writes:\n> \n> >>\n> >> > To avoid this happening, before creating the keys, clear any\n> >> > default ACL\n> >>\n> >> \"happening\" -> \"from happening\"\n> >>\n> >\n> > No, original is correct.\n> >\n> > To avoid this happening.\n> > To keep this from happening.\n> > To prevent this happening.\n> > To prevent this from happening.\n> >\n> > Would I think all be correct.\n> > \"to avoid from\" is not right.\n> \n> But I meant to say \"to avoid this from happening\", not \"to avoid from\", which\n> I agree is not right.\n\n\"to avoid this from happening\" is wrong.\n\"to avoid this happening\" is right.\nOr my other examples, with more or less the same meaning.\n\nI phrased it as \"to avoid from\" as an example of the verb in its basic form. You were entirely clear what you meant - I was merely trying to give examples of what I think is wrong.\n\nAs a native English speaker I grew up without being taught formal grammar, so I can say something is wrong without being able to explain why in a formal way.....\n\nI'd guess from his name that Adam is also a native English speaker.\n\nRegards,\nRichard.\n\n\n\n"},{"id":"440757","messageId":"xmqqk0hh2nui.fsf@gitster.g","threadId":"56846","inReplyTo":"AS8PR02MB73028E498D0AB831FE8028E89C929@AS8PR02MB7302.eurprd02.prod.outlook.com","subject":"Re: [PATCH v2] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-09T18:19:49Z","receivedAt":"2021-11-09T18:20:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kerry, Richard\" writes:\n\n> \"to avoid this from happening\" is wrong.\n> \"to avoid this happening\" is right.\n\nAh, of course. I somehow mixed it up with \"to prevent\".\n\n"},{"id":"440985","messageId":"20211112160101.xm7xi4474pgybrh4@fs","threadId":"56846","inReplyTo":"xmqqv916wh7t.fsf@gitster.g","subject":"[RFC PATCH] lib-test: show failed prereq was Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-12T16:01:01Z","receivedAt":"2021-11-12T16:01:07Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 05.11.2021 12:11, Junio C Hamano wrote:\n>Adam Dinwoodie writes:\n>\n>> This is probably a much broader conversation. I remember when I first\n>> started packaging Git for Cygwin, I produced a release that didn't\n>> have support for HTTPS URLs due to a missing dependency in my build\n>> environment. The build and test suite all passed -- it assumed I just\n>> wanted to build a release that didn't have HTTPS support -- so some\n>> relatively critical function was silently skipped. I don't know how to\n>> avoid that sort of issue other than relying on (a) user bug (or at\n>> least missing function) reports and (b) folk building Git for\n>> themselves/others periodically going through the output of the\n>> configure scripts and the skipped subtests to make sure only expected\n>> things get missed; neither of those options seem great to me.\n>\n>I agree with you that there needs a good way to enumerate what the\n>unsatisfied prerequisites for a particular build are. That would\n>have helped in your HTTPS situation.\n>\n\nSorry for not replying earlier. I've been sick the last couple of days\nand only slowly getting up to speed again. I will improve the prereq\ntests in a new commit in the other patch series still in progress that\ni'll shortly reroll.\n\nAs for the general prereq issue i ran into that as well during\ndevelopment. When you depend on other patches / a specific version of\nssh-keygen for git I always have to remember to set the path correctly\nor the tests might silently be ignored by the missing prereq. Usually\nnot a problem for single test runs, but when i run the full suite before\nsending something.\n\nSo, here's a simple rfc patch to maybe start with addressing this issue. \n\n\nFrom 0e7e57e546ec969d31094405aecafd1b1f3cf4d8 Mon Sep 17 00:00:00 2001\nFrom: Fabian Stelzer \nDate: Fri, 12 Nov 2021 16:41:30 +0100\nSubject: [RFC PATCH 1/2] test-lib: show failed prereq summary\n\nAdd failed prereqs to the test results.\nAggregate and then show them with the totals.\n\nSigned-off-by: Fabian Stelzer \n---\n t/aggregate-results.sh | 12 ++++++++++++\n t/test-lib.sh | 4 ++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/t/aggregate-results.sh b/t/aggregate-results.sh\nindex 7913e206ed..ad531cc75d 100755\n--- a/t/aggregate-results.sh\n+++ b/t/aggregate-results.sh\n@@ -6,6 +6,7 @@ success=0\n failed=0\n broken=0\n total=0\n+missing_prereq=\n \n while read file\n do\n@@ -30,10 +31,21 @@ do\n \t\t\tbroken=$(($broken + $value)) ;;\n \t\ttotal)\n \t\t\ttotal=$(($total + $value)) ;;\n+\t\tmissing_prereq)\n+\t\t\tmissing_prereq=\"$missing_prereq $value\" ;;\n \t\tesac\n \tdone <\"$file\"\n done\n \n+if test -n \"$missing_prereq\"\n+then\n+\tunique_missing_prereq=$(\n+\t\techo $missing_prereq | tr -s \",\" | \\\n+\t\tsed -e 's/ //g' -e 's/^,//' -e 's/,$//' -e 's/,/\\n/g' \\\n+\t\t| sort | uniq | paste -s -d ',')\n+\tprintf \"\\nmissing prereq: $unique_missing_prereq\\n\\n\"\n+fi\n+\n if test -n \"$failed_tests\"\n then\n \tprintf \"\\nfailed test(s):$failed_tests\\n\\n\"\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 2679a7596a..472387afec 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -669,6 +669,8 @@ test_fixed=0\n test_broken=0\n test_success=0\n \n+test_missing_prereq=\n+\n test_external_has_tap=0\n \n die () {\n@@ -1068,6 +1070,7 @@ test_skip () {\n \t\tthen\n \t\t\tof_prereq=\" of $test_prereq\"\n \t\tfi\n+\t\ttest_missing_prereq=\"$missing_prereq,$test_missing_prereq\"\n \t\tskipped_reason=\"missing $missing_prereq${of_prereq}\"\n \tfi\n \n@@ -1175,6 +1178,7 @@ test_done () {\n \t\tfixed $test_fixed\n \t\tbroken $test_broken\n \t\tfailed $test_failure\n+\t\tmissing_prereq $test_missing_prereq\n \n \t\tEOF\n \tfi\n-- \n2.31.1\n\n\n\nFrom d13d4c8ccbd832e1d62044b18b8b771f6586ee2a Mon Sep 17 00:00:00 2001\nFrom: Fabian Stelzer \nDate: Fri, 12 Nov 2021 16:43:18 +0100\nSubject: [RFC PATCH 2/2] test-lib: introduce required prereq for test runs\n\nAllows setting GIT_TEST_REQUIRE_PREREQ to a number of prereqs that must\nsucceed for this run. Otherwise the test run will abort.\n\nSigned-off-by: Fabian Stelzer \n---\n t/test-lib-functions.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex eef2262a36..d65995cd15 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -669,6 +669,14 @@ test_have_prereq () {\n \t\t\tsatisfied_this_prereq=t\n \t\t\t;;\n \t\t*)\n+\t\t\tif ! test -z $GIT_TEST_REQUIRE_PREREQ\n+\t\t\tthen\n+\t\t\t\tcase \",$GIT_TEST_REQUIRE_PREREQ,\" in\n+\t\t\t\t*,$prerequisite,*)\n+\t\t\t\t\terror \"required prereq $prerequisite failed\"\n+\t\t\t\t\t;;\n+\t\t\t\tesac\n+\t\t\tfi\n \t\t\tsatisfied_this_prereq=\n \t\tesac\n \n-- \n2.31.1\n\n"},{"id":"441052","messageId":"xmqqk0hcmvql.fsf@gitster.g","threadId":"56846","inReplyTo":"20211112160101.xm7xi4474pgybrh4@fs","subject":"Re: [RFC PATCH] lib-test: show failed prereq was Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-13T06:10:26Z","receivedAt":"2021-11-13T06:10:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer writes:\n\n> As for the general prereq issue i ran into that as well during\n> development. When you depend on other patches / a specific version of\n> ssh-keygen for git I always have to remember to set the path correctly\n> or the tests might silently be ignored by the missing prereq. Usually\n> not a problem for single test runs, but when i run the full suite before\n> sending something.\n\nThis will become a handy tool for everybody, not just for those on\nminority and/or exotic platforms. When someone prepares a plain\nvanilla fresh box and build Git from the source for the first time\non the box, it is fairly easy to end up with a castrated version of\nGit, without knowing what is missing. This is especially so when\nautoconf is used, but even without using autoconf, if you do not\nhave libsvn Perl modules, svn binary, or cvs binary installed, our\ntests treat it as a signal that you are uninterested in SVN or CVS\ninterop tests, rather than flagging it as an error. Being able to\nsee what is automatically skipped would be a good way to sanity\ncheck what you actually have vs what you thought you had. For\nexample, I just found out that I am still running CVS interop tests\nin my installation.\n\n> Subject: [RFC PATCH 1/2] test-lib: show failed prereq summary\n>\n> Add failed prereqs to the test results.\n> Aggregate and then show them with the totals.\n>\n> Signed-off-by: Fabian Stelzer \n> ---\n> t/aggregate-results.sh | 12 ++++++++++++\n> t/test-lib.sh | 4 ++++\n> 2 files changed, 16 insertions(+)\n>\n> diff --git a/t/aggregate-results.sh b/t/aggregate-results.sh\n> index 7913e206ed..ad531cc75d 100755\n> --- a/t/aggregate-results.sh\n> +++ b/t/aggregate-results.sh\n> @@ -6,6 +6,7 @@ success=0\n> failed=0\n> broken=0\n> total=0\n> +missing_prereq=\n> \n> while read file\n> do\n> @@ -30,10 +31,21 @@ do\n> \t\t\tbroken=$(($broken + $value)) ;;\n> \t\ttotal)\n> \t\t\ttotal=$(($total + $value)) ;;\n> +\t\tmissing_prereq)\n> +\t\t\tmissing_prereq=\"$missing_prereq $value\" ;;\n\nIt is unclear yet what shape $value has at this point (because we\nhaven't seen what is in test-lib.sh), but we accumulate them in the\n$missing_prereq variable, separated by a space. Also, I notice that\n$missing_prereq will begin with a space when it is not empty, which\nprobably is not a big deal.\n\n> \t\tesac\n> \tdone <\"$file\"\n> done\n> \n> +if test -n \"$missing_prereq\"\n> +then\n> +\tunique_missing_prereq=$(\n> +\t\techo $missing_prereq | tr -s \",\" | \\\n\nYou do not need backslash there; the line ends with '|' and shell\nknows you haven't completed the pipeline yet, so it will go on to\nread the next line. The same for the next line; instead of adding\na backslash and breaking the line after it, just have the pipe there\nand you can break the line safely without a backslash.\n\n> +\t\tsed -e 's/ //g' -e 's/^,//' -e 's/,$//' -e 's/,/\\n/g' \\\n> +\t\t| sort | uniq | paste -s -d ',')\n\nI suspect you are making more work than necessary for yourself by\nchoosing to use SP when accumulating values in $missing_prereq\nvariable. If you used comma instead, \"tr -s ','\" here will make a\nneat sequence of tokens separated with one comma each, possibly with\none extra comma at the beginning and at the end if some $value were\nempty.\n\nWould something like this work better, I wonder?\n\n\tunique_missing_prereq=$(\n echo \"$missing_prereq\" |\n tr -s \",\" \"\\012\" |\n grep -v \"^$\" |\n sort -u |\n paste -s -d ,\n\t)\n\n> +\tprintf \"\\nmissing prereq: $unique_missing_prereq\\n\\n\"\n\nI think it is possible that a $missing_prereq that is not empty\nstill yields an empty $unique_missing_prereq. If $value read from\nthe files all are empty strings, $missing_prereq will have many SP\n(or comma if you take my earlier suggestion), but no actual prereq\nwill remain after the \"unique\" thing is computed. I think this\nprintf should be shown only when $unique_missing_prereq is not\nempty.\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 2679a7596a..472387afec 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -669,6 +669,8 @@ test_fixed=0\n> test_broken=0\n> test_success=0\n> \n> +test_missing_prereq=\n> +\n> test_external_has_tap=0\n> \n> die () {\n> @@ -1068,6 +1070,7 @@ test_skip () {\n> \t\tthen\n> \t\t\tof_prereq=\" of $test_prereq\"\n> \t\tfi\n> +\t\ttest_missing_prereq=\"$missing_prereq,$test_missing_prereq\"\n\nOK. We accumulate in $test_missing_prereq what is in missing_prereq\n(assigned in test_have_prereq check). I notice that over there, it\ntakes pains to make sure it uses only one comma between each token,\nwithout excess leading or trailing comma, but we are not taking the\nsame care here. It would be OK as we'd run \"tr -s ,\" on the side\nthat reads the output, but looks somewhat sloppy.\n\n> \t\tskipped_reason=\"missing $missing_prereq${of_prereq}\"\n> \tfi\n> \n> @@ -1175,6 +1178,7 @@ test_done () {\n> \t\tfixed $test_fixed\n> \t\tbroken $test_broken\n> \t\tfailed $test_failure\n> +\t\tmissing_prereq $test_missing_prereq\n> \n> \t\tEOF\n\nAnd this part is quite obvious, after having read the consumer side\nalready.\n\nNicely done.\n\n> \tfi\n> -- \n> 2.31.1\n>\n> From d13d4c8ccbd832e1d62044b18b8b771f6586ee2a Mon Sep 17 00:00:00 2001\n> From: Fabian Stelzer \n> Date: Fri, 12 Nov 2021 16:43:18 +0100\n> Subject: [RFC PATCH 2/2] test-lib: introduce required prereq for test runs\n>\n> Allows setting GIT_TEST_REQUIRE_PREREQ to a number of prereqs that must\n> succeed for this run. Otherwise the test run will abort.\n\nI am not quite sure what the sentence means, so let me read the code\nfirst before commenting.\n\n> Signed-off-by: Fabian Stelzer \n> ---\n> t/test-lib-functions.sh | 8 ++++++++\n> 1 file changed, 8 insertions(+)\n>\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index eef2262a36..d65995cd15 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -669,6 +669,14 @@ test_have_prereq () {\n> \t\t\tsatisfied_this_prereq=t\n> \t\t\t;;\n> \t\t*)\n\nAt this point, we know $prerequisite we are looking at (note that\nwhat is written as a guard for a particular test might be negated,\ne.g. \"test_expect_success !WINDOWS 'title' 'code'\" that runs on\nnon-WINDOWS platforms, but here the negation has been stripped away,\nso the test says \"I require to be on non-Windows\", but this new code\nonly knows that WINDOWS prereq has failed)\n\n> +\t\t\tif ! test -z $GIT_TEST_REQUIRE_PREREQ\n\nWhy not \n\n\tif test -n \"$GIT_TEST_REQUIRE_PREREQ\"\n\n?\n\n\n> +\t\t\tthen\n> +\t\t\t\tcase \",$GIT_TEST_REQUIRE_PREREQ,\" in\n> +\t\t\t\t*,$prerequisite,*)\n> +\t\t\t\t\terror \"required prereq $prerequisite failed\"\n> +\t\t\t\t\t;;\n\nSo GIT_TEST_REQUIRE_PREREQ could be set to a comma separated list of\nprerequisites, e.g. WINDOWS,PDP11,CRAY, and we see if $prerequisite\nwe have just found out is missing is any one of them. And abort the\ntest if that is true. Makes sense, except for the negation. You\nwant to be able to say GIT_TEST_REQUIRE_PREREQ=!WINDOWS,PERL to\nrequire that you are not on Windows and have PERL, for example.\n\nPerhaps this new block should be moved a bit further down in the\ncode, i.e.\n\n|\t\ttotal_prereq=$(($total_prereq + 1))\n|\t\tcase \"$satisfied_prereq\" in\n|\t\t*\" $prerequisite \"*)\n|\t\t\tsatisfied_this_prereq=t\n|\t\t\t;;\n|\t\t*)\n\n... you are inserting the new code here, but don't do that yet, and ...\n\n|\t\t\tsatisfied_this_prereq=\n|\t\tesac\n|\n|\t\tcase \"$satisfied_this_prereq,$negative_prereq\" in\n|\t\tt,|,t)\n|\t\t\tok_prereq=$(($ok_prereq + 1))\n|\t\t\t;;\n|\t\t*)\n|\t\t\t# Keep a list of missing prerequisites; restore\n|\t\t\t# the negative marker if necessary.\n|\t\t\tprerequisite=${negative_prereq:+!}$prerequisite\n\n... do it here instead? We have restored the negation in prerequisite \nat this point, so we can say\n\n\t\t\tcase \",$GIT_TEST_REQUIRE_PREREQ,\" in\n\t\t\t*,$prerequisite,*)\n\t\t\t\terror you do not have $prerequisite.\n\t\t\t\t;;\n\t\t\tesac\n\nsafely here, before we accumulate it into $missing_prereq variable.\n\n|\t\t\tif test -z \"$missing_prereq\"\n|\t\t\tthen\n|\t\t\t\tmissing_prereq=$prerequisite\n|\t\t\telse\n|\t\t\t\tmissing_prereq=\"$prerequisite,$missing_prereq\"\n|\t\t\tfi\n|\t\tesac\n\nThanks for working on this.\nLooking good.\n"},{"id":"441068","messageId":"20211113144351.3rsbogowax36iatz@fs","threadId":"56846","inReplyTo":"xmqqk0hcmvql.fsf@gitster.g","subject":"Re: [RFC PATCH] lib-test: show failed prereq was Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-13T14:43:51Z","receivedAt":"2021-11-13T14:43:58Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 12.11.2021 22:10, Junio C Hamano wrote:\n>Fabian Stelzer writes:\n>\n>> As for the general prereq issue i ran into that as well during\n>> development. When you depend on other patches / a specific version of\n>> ssh-keygen for git I always have to remember to set the path correctly\n>> or the tests might silently be ignored by the missing prereq. Usually\n>> not a problem for single test runs, but when i run the full suite before\n>> sending something.\n>\n>This will become a handy tool for everybody, not just for those on\n>minority and/or exotic platforms. When someone prepares a plain\n>vanilla fresh box and build Git from the source for the first time\n>on the box, it is fairly easy to end up with a castrated version of\n>Git, without knowing what is missing. This is especially so when\n>autoconf is used, but even without using autoconf, if you do not\n>have libsvn Perl modules, svn binary, or cvs binary installed, our\n>tests treat it as a signal that you are uninterested in SVN or CVS\n>interop tests, rather than flagging it as an error. Being able to\n>see what is automatically skipped would be a good way to sanity\n>check what you actually have vs what you thought you had. For\n>example, I just found out that I am still running CVS interop tests\n>in my installation.\n>\n>> Subject: [RFC PATCH 1/2] test-lib: show failed prereq summary\n>>\n>> Add failed prereqs to the test results.\n>> Aggregate and then show them with the totals.\n>>\n>\n>> +\t\tsed -e 's/ //g' -e 's/^,//' -e 's/,$//' -e 's/,/\\n/g' \\\n>> +\t\t| sort | uniq | paste -s -d ',')\n>\n>I suspect you are making more work than necessary for yourself by\n>choosing to use SP when accumulating values in $missing_prereq\n>variable. If you used comma instead, \"tr -s ','\" here will make a\n>neat sequence of tokens separated with one comma each, possibly with\n>one extra comma at the beginning and at the end if some $value were\n>empty.\n\nYou are right. I'll change it to ',' as well. It makes the following\nunique logic easier.\n\n>\n>Would something like this work better, I wonder?\n>\n>\tunique_missing_prereq=$(\n> echo \"$missing_prereq\" |\n> tr -s \",\" \"\\012\" |\n> grep -v \"^$\" |\n> sort -u |\n> paste -s -d ,\n>\t)\n>\n\nOk. Took me a moment to understand since i didn't realize tr did the\nnewline expansion as well. But yeah, this is nicer.\n\n>> +\tprintf \"\\nmissing prereq: $unique_missing_prereq\\n\\n\"\n>\n>I think it is possible that a $missing_prereq that is not empty\n>still yields an empty $unique_missing_prereq. If $value read from\n>the files all are empty strings, $missing_prereq will have many SP\n>(or comma if you take my earlier suggestion), but no actual prereq\n>will remain after the \"unique\" thing is computed. I think this\n>printf should be shown only when $unique_missing_prereq is not\n>empty.\n\nTrue. I'll add an if.\n\n>> +\t\ttest_missing_prereq=\"$missing_prereq,$test_missing_prereq\"\n>\n>OK. We accumulate in $test_missing_prereq what is in missing_prereq\n>(assigned in test_have_prereq check). I notice that over there, it\n>takes pains to make sure it uses only one comma between each token,\n>without excess leading or trailing comma, but we are not taking the\n>same care here. It would be OK as we'd run \"tr -s ,\" on the side\n>that reads the output, but looks somewhat sloppy.\n\nOk, i'll use the same logic as in the test_have_prereq func here as\nwell.\n\n>>\n>> From d13d4c8ccbd832e1d62044b18b8b771f6586ee2a Mon Sep 17 00:00:00 2001\n>> From: Fabian Stelzer \n>> Date: Fri, 12 Nov 2021 16:43:18 +0100\n>> Subject: [RFC PATCH 2/2] test-lib: introduce required prereq for test runs\n>>\n>> Allows setting GIT_TEST_REQUIRE_PREREQ to a number of prereqs that must\n>> succeed for this run. Otherwise the test run will abort.\n>\n>I am not quite sure what the sentence means, so let me read the code\n>first before commenting.\n>\n>At this point, we know $prerequisite we are looking at (note that\n>what is written as a guard for a particular test might be negated,\n>e.g. \"test_expect_success !WINDOWS 'title' 'code'\" that runs on\n>non-WINDOWS platforms, but here the negation has been stripped away,\n>so the test says \"I require to be on non-Windows\", but this new code\n>only knows that WINDOWS prereq has failed)\n\nI will write some clearer commit messages and then re-send as a normal\npatch.\n\n>\n>> +\t\t\tif ! test -z $GIT_TEST_REQUIRE_PREREQ\n>\n>Why not\n>\n>\tif test -n \"$GIT_TEST_REQUIRE_PREREQ\"\n>\n>?\n\nObviously, yes...\n\n>\n>\n>> +\t\t\tthen\n>> +\t\t\t\tcase \",$GIT_TEST_REQUIRE_PREREQ,\" in\n>> +\t\t\t\t*,$prerequisite,*)\n>> +\t\t\t\t\terror \"required prereq $prerequisite failed\"\n>> +\t\t\t\t\t;;\n>\n>So GIT_TEST_REQUIRE_PREREQ could be set to a comma separated list of\n>prerequisites, e.g. WINDOWS,PDP11,CRAY, and we see if $prerequisite\n>we have just found out is missing is any one of them. And abort the\n>test if that is true. Makes sense, except for the negation. You\n>want to be able to say GIT_TEST_REQUIRE_PREREQ=!WINDOWS,PERL to\n>require that you are not on Windows and have PERL, for example.\n>\n>Perhaps this new block should be moved a bit further down in the\n>code, i.e.\n\nThanks, yes i did not notice the negation issue.\n\n>Thanks for working on this.\n>Looking good.\n\nThanks for your review.\n"}]}