{"thread":{"id":"59214","subject":"[PATCH] test: make SYMLINKS prerequisite more robust","startedAt":"2023-02-08T19:40:08Z","lastAt":"2023-02-13T18:08:08Z","messageCount":9,"participants":["Junio C Hamano","Ævar Arnfjörð Bjarmason","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"471793","messageId":"xmqqwn4sq73f.fsf@gitster.g","threadId":"59214","inReplyTo":null,"subject":"[PATCH] test: make SYMLINKS prerequisite more robust","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-08T19:40:04Z","receivedAt":"2023-02-08T19:40:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I see many failures around SYMLINKS prerequisite in Windows tests.\nThere are too many to point at, but the pattern seems to be the\nsame.  Here is one example:\n\n  https://github.com/git/git/actions/runs/4127147009/jobs/7130175639#step:5:502\n\nwhere \"ln -s x y && test -h y\" succeeds and declares SYMLINKS\nlazy prerequisite is satisfied, but then it fails to run\n\n\t\"ln -s unrelated DS && git update-index --add DS\"\n\nwith:\n\n\terror: readlink(\"DS\"): Function not implemented\n\nI wonder if something like this is in order?\n\n----- >8 -----\n\nWe should not just ensure \"ln -s\" and \"test -h\" works, but readlink\nyields the expected value.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Makefile                 |  1 +\n t/helper/test-readlink.c | 19 +++++++++++++++++++\n t/helper/test-tool.c     |  1 +\n t/helper/test-tool.h     |  1 +\n t/test-lib.sh            |  3 ++-\n 5 files changed, 24 insertions(+), 1 deletion(-)\n create mode 100644 t/helper/test-readlink.c\n\ndiff --git a/Makefile b/Makefile\nindex 45bd6ac9c3..2261e56d31 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -837,6 +837,7 @@ TEST_BUILTINS_OBJS += test-reach.o\n TEST_BUILTINS_OBJS += test-read-cache.o\n TEST_BUILTINS_OBJS += test-read-graph.o\n TEST_BUILTINS_OBJS += test-read-midx.o\n+TEST_BUILTINS_OBJS += test-readlink.o\n TEST_BUILTINS_OBJS += test-ref-store.o\n TEST_BUILTINS_OBJS += test-reftable.o\n TEST_BUILTINS_OBJS += test-regex.o\ndiff --git a/t/helper/test-readlink.c b/t/helper/test-readlink.c\nnew file mode 100644\nindex 0000000000..c300dc8a1a\n--- /dev/null\n+++ b/t/helper/test-readlink.c\n@@ -0,0 +1,19 @@\n+#include \"test-tool.h\"\n+#include \"strbuf.h\"\n+\n+static const char *usage_msg = \"test-tool readlink file\";\n+\n+int cmd__readlink(int ac, const char **av)\n+{\n+\tstruct strbuf buf;\n+\tint ret;\n+\n+\tif (ac != 2 || !av[1])\n+\t\tusage(usage_msg);\n+\n+\tret = strbuf_readlink(&buf, av[1], 0);\n+\tif (!ret)\n+\t\tprintf(\"%s\\n\", buf.buf);\n+\tstrbuf_release(&buf);\n+\treturn ret;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex abe8a785eb..12054d13d4 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -64,6 +64,7 @@ static struct test_cmd cmds[] = {\n \t{ \"read-cache\", cmd__read_cache },\n \t{ \"read-graph\", cmd__read_graph },\n \t{ \"read-midx\", cmd__read_midx },\n+\t{ \"readlink\", cmd__readlink },\n \t{ \"ref-store\", cmd__ref_store },\n \t{ \"reftable\", cmd__reftable },\n \t{ \"rot13-filter\", cmd__rot13_filter },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex ea2672436c..efe382d48e 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -57,6 +57,7 @@ int cmd__reach(int argc, const char **argv);\n int cmd__read_cache(int argc, const char **argv);\n int cmd__read_graph(int argc, const char **argv);\n int cmd__read_midx(int argc, const char **argv);\n+int cmd__readlink(int argc, const char **argv);\n int cmd__ref_store(int argc, const char **argv);\n int cmd__rot13_filter(int argc, const char **argv);\n int cmd__reftable(int argc, const char **argv);\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 01e88781dd..c8094f643b 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1773,7 +1773,8 @@ test_lazy_prereq PIPE '\n \n test_lazy_prereq SYMLINKS '\n \t# test whether the filesystem supports symbolic links\n-\tln -s x y && test -h y\n+\tln -s x y && test -h y && test-tool readlink y >/dev/null &&\n+\ttest \"$(test-tool readlink y)\" = x\n '\n \n test_lazy_prereq SYMLINKS_WINDOWS '\n-- \n2.39.1-418-g7876265d61\n\n\n\n"},{"id":"471814","messageId":"230209.86k00rzqsz.gmgdl@evledraar.gmail.com","threadId":"59214","inReplyTo":"xmqqwn4sq73f.fsf@gitster.g","subject":"Re: [PATCH] test: make SYMLINKS prerequisite more robust","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-08T23:09:46Z","receivedAt":"2023-02-08T23:21:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 08 2023, Junio C Hamano wrote:\n\n> I see many failures around SYMLINKS prerequisite in Windows tests.\n> There are too many to point at, but the pattern seems to be the\n> same.  Here is one example:\n>\n>   https://github.com/git/git/actions/runs/4127147009/jobs/7130175639#step:5:502\n>\n> where \"ln -s x y && test -h y\" succeeds and declares SYMLINKS\n> lazy prerequisite is satisfied, but then it fails to run\n>\n> \t\"ln -s unrelated DS && git update-index --add DS\"\n>\n> with:\n>\n> \terror: readlink(\"DS\"): Function not implemented\n>\n> I wonder if something like this is in order?\n\nI don't have much to contribute on that front, but this is really\nmissing some \"why\", this worked before, why is it failing now? Do we\nhave any idea.\n\nWe use \"windows-latest\", at first I suspected that this was the\n2019->2022 migration, which seems to be happening around now:\nhttps://github.blog/changelog/2022-01-11-github-actions-jobs-running-on-windows-latest-are-now-running-on-windows-server-2022/\n\nBut looking at a previous successful run on master:\nhttps://github.com/git/git/actions/runs/4089369223/jobs/7052074004\n\nV.s. this current failure:\nhttps://github.com/git/git/actions/runs/4127146869/jobs/7130173444\n\nShows that both run 2022, and seem to be running the same software,\nexcept that in \"set up job\" this is different, it was:\n\n\tDownload action repository\n\t'git-for-windows/setup-git-for-windows-sdk@v1'\n\t(SHA:cbe017cd7ae39629bf4e34fce8b1ccd211fec009)\n\nNow:\n\n\tDownload action repository\n\t'git-for-windows/setup-git-for-windows-sdk@v1'\n\t(SHA:848609620edfa4c2fc64838b85fbe19e534236ee)\n\nI have no idea if that's related though...\n\n> diff --git a/t/helper/test-readlink.c b/t/helper/test-readlink.c\n> new file mode 100644\n> index 0000000000..c300dc8a1a\n> --- /dev/null\n> +++ b/t/helper/test-readlink.c\n> @@ -0,0 +1,19 @@\n> +#include \"test-tool.h\"\n> +#include \"strbuf.h\"\n> +\n> +static const char *usage_msg = \"test-tool readlink file\";\n> +\n> +int cmd__readlink(int ac, const char **av)\n> +{\n> +\tstruct strbuf buf;\n\nYou're leaving the strbuf uninitialized here, use STRBUF_INIT...\n\n> +\tint ret;\n> +\n> +\tif (ac != 2 || !av[1])\n> +\t\tusage(usage_msg);\n> +\n> +\tret = strbuf_readlink(&buf, av[1], 0);\n\n...it's used here, and there's no implicit strbuf_init() on\nstrbuf_readlink(). The first thing it does is inspect sb->alloc.\n\n> +\tif (!ret)\n> +\t\tprintf(\"%s\\n\", buf.buf);\n\nNit: puts(buf.buf);\n\nAll in all a simple helper, but isn't this redundant to \"test_readlink\"?\n\nThat requires perl, and once we have this we could just replace it, but\nthen let's do that here, no?\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 01e88781dd..c8094f643b 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1773,7 +1773,8 @@ test_lazy_prereq PIPE '\n>  \n>  test_lazy_prereq SYMLINKS '\n>  \t# test whether the filesystem supports symbolic links\n> -\tln -s x y && test -h y\n> +\tln -s x y && test -h y && test-tool readlink y >/dev/null &&\n> +\ttest \"$(test-tool readlink y)\" = x\n\nWhy get the exit code, and then proceed to hide the exit code with the\ntest \"$()\" pattern, we can just (untested):\n\n\techo x >expect &&\n\ttest-tool readlink y >actual &&\n\ttest_cmp expect actual\n\nIf you're trying to avoid leaving litter or cleaning up that's not\nneeded anymore with these lazy prereqs for a while now (they get their\nthrow-away temporary directory).\n"},{"id":"471830","messageId":"xmqqbkm3ppn8.fsf@gitster.g","threadId":"59214","inReplyTo":"230209.86k00rzqsz.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] test: make SYMLINKS prerequisite more robust","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-09T01:56:59Z","receivedAt":"2023-02-09T01:57:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> I wonder if something like this is in order?\n>\n> I don't have much to contribute on that front, but this is really\n> missing some \"why\", this worked before, why is it failing now? Do we\n> have any idea.\n\nYour guess is as good as mine.  I do not do Windows.\n\nThanks for independently noticing the uninitialized strbuf.  What I\nhave queued has it fixed already locally but there isn't much point\nto send another copy out to the list ;-).\n\n> All in all a simple helper, but isn't this redundant to \"test_readlink\"?\n\nNot at all.  We need to avoid the Perl one for this purpose.  What\nmatters is whether \"git\" considers if symlink is working.\n\nPerhaps our readlink(3) emulation we have in compat/ may hardcode\nour \"knowledge\" that symlink is not available in Windows, which may\nnot match what the POSIX XCU emulation in our Windows environment\noffers, which apparently ran \"ln -s x y && test -h y\" successfully,\nand who knows what test_readlink that is written in Perl thinks?  We\nare testing \"git\" with the test suite, so even if with some magic\nthat is still unknown to compat/mingw.h it knows how to read what\n\"ln -s x y\" left in \"y\", until compat/mingw.h::readlink() learns the\nsame trick, asking Perl to decide SYMLINKS prerequisite would not\nhelp our test scripts at all.\n\n> \techo x >expect &&\n> \ttest-tool readlink y >actual &&\n> \ttest_cmp expect actual\n>\n> If you're trying to avoid leaving litter or cleaning up that's not\n> needed anymore with these lazy prereqs for a while now (they get their\n> throw-away temporary directory).\n\nIndeed, I just did not want to add another cruft, but 'x' and 'y'\nare already such crufts, so I could have just done\n\n\tln -s x y &&\n\ttest -h y &&\n\ttest-tool readlink y >x &&\n\ttest $(cat x) = x\n\nto use one of them ;-)\n"},{"id":"471833","messageId":"230209.86ttzvy405.gmgdl@evledraar.gmail.com","threadId":"59214","inReplyTo":"xmqqbkm3ppn8.fsf@gitster.g","subject":"Re: [PATCH] test: make SYMLINKS prerequisite more robust","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-09T02:15:07Z","receivedAt":"2023-02-09T02:19:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 08 2023, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>>> I wonder if something like this is in order?\n>>\n>> I don't have much to contribute on that front, but this is really\n>> missing some \"why\", this worked before, why is it failing now? Do we\n>> have any idea.\n>\n> Your guess is as good as mine.  I do not do Windows.\n>\n> Thanks for independently noticing the uninitialized strbuf.  What I\n> have queued has it fixed already locally but there isn't much point\n> to send another copy out to the list ;-).\n>\n>> All in all a simple helper, but isn't this redundant to \"test_readlink\"?\n>\n> Not at all.  We need to avoid the Perl one for this purpose.  What\n> matters is whether \"git\" considers if symlink is working.\n>\n> Perhaps our readlink(3) emulation we have in compat/ may hardcode\n> our \"knowledge\" that symlink is not available in Windows, which may\n> not match what the POSIX XCU emulation in our Windows environment\n> offers, which apparently ran \"ln -s x y && test -h y\" successfully,\n> and who knows what test_readlink that is written in Perl thinks?\n\nYeah that's fair, I wonder why we haven't replaced this already...\n\nFWIW I think this is what perl will dispatch to on Windows, so it makes\nyour point, it has its own NIH Windows emulation layer:\nhttps://github.com/Perl/perl5/blob/blead/win32/win32.c#L1983-L2026\n\n> We\n> are testing \"git\" with the test suite, so even if with some magic\n> that is still unknown to compat/mingw.h it knows how to read what\n> \"ln -s x y\" left in \"y\", until compat/mingw.h::readlink() learns the\n> same trick, asking Perl to decide SYMLINKS prerequisite would not\n> help our test scripts at all.\n\nWe could always see if they return the same answer :) But not worth\npursuing in this case.\n\n>> If you're trying to avoid leaving litter or cleaning up that's not\n>> needed anymore with these lazy prereqs for a while now (they get their\n>> throw-away temporary directory).\n>\n> Indeed, I just did not want to add another cruft, but 'x' and 'y'\n> are already such crufts, so I could have just done\n>\n> \tln -s x y &&\n> \ttest -h y &&\n> \ttest-tool readlink y >x &&\n> \ttest $(cat x) = x\n>\n> to use one of them ;-)\n\nYeah, I mean you don't need to avoid cruft, because the whole directory\nis about to be rm -rf'd\n\nNow that I've dug it up I see you implemented that in 04083f278d1 (test:\nallow prerequisite to be evaluated lazily, 2012-07-26).\n\nI was recalling 53ff3b96a87 (tests: make sure nested lazy prereqs work\nreliably, 2020-11-18) as the more recent change, but that just solved a\nnested prereq edge case.\n\n"},{"id":"471886","messageId":"xmqqlel6laa1.fsf@gitster.g","threadId":"59214","inReplyTo":"230209.86ttzvy405.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] test: make SYMLINKS prerequisite more robust","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-09T22:54:46Z","receivedAt":"2023-02-09T22:55:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> We\n>> are testing \"git\" with the test suite, so even if with some magic\n>> that is still unknown to compat/mingw.h it knows how to read what\n>> \"ln -s x y\" left in \"y\", until compat/mingw.h::readlink() learns the\n>> same trick, asking Perl to decide SYMLINKS prerequisite would not\n>> help our test scripts at all.\n>\n> We could always see if they return the same answer :) But not worth\n> pursuing in this case.\n\nActually we probably should test if \"git\" can create a symbolic\nlink, because that is what matters more than what \"ln -s\" can or\ncannot do.  We also are interested in the \"symbolic link\" does\nbehave as a symbolic link, i.e. writing into it should modify the\ncontents of the target of the link.\n\nIn any case, here is what I have in mind.  There is only a symbolic\nlink 'y' without 'x' (because this happens in a new and empty\ndirectory for evaluating the lazy prerequisite), redirecting output\nfrom the command, which should be 'x', into 'y' will vivify file 'x'\nwith contents also 'x', and reading 'y' should yield 'x' because it\npoints at 'x'.\n\ndiff --git c/t/test-lib.sh w/t/test-lib.sh\nindex 6db377f68b..3fb5957bd2 100644\n--- c/t/test-lib.sh\n+++ w/t/test-lib.sh\n@@ -1773,7 +1773,9 @@ test_lazy_prereq PIPE '\n \n test_lazy_prereq SYMLINKS '\n \t# test whether the filesystem supports symbolic links\n-\tln -s x y && test -h y\n+\tln -s x y && test -h y && test-tool readlink y >y &&\n+\ttest \"$(cat y)\" = x &&\n+\ttest \"$(cat x)\" = x\n '\n \n test_lazy_prereq SYMLINKS_WINDOWS '\n"},{"id":"471931","messageId":"xmqqk00pjuli.fsf@gitster.g","threadId":"59214","inReplyTo":"xmqqbkm3ppn8.fsf@gitster.g","subject":"Re: [PATCH] test: make SYMLINKS prerequisite more robust","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-10T17:31:05Z","receivedAt":"2023-02-10T17:31:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>>> I wonder if something like this is in order?\n>>\n>> I don't have much to contribute on that front, but this is really\n>> missing some \"why\", this worked before, why is it failing now? Do we\n>> have any idea.\n>\n> Your guess is as good as mine.  I do not do Windows.\n\nThis morning, I notice that those CI jobs I ran last night that\nfailed with \"whoa, windows tests are somehow reporting that symlinks\nare available but not really\" issue the patch in this thread were\nattempting to address has passed even for branches like 'master' and\n'next' that do not yet have it, and it seems to be because you\nre-run these failed jobs.\n\nWhatever magic you used to fix these failing tests, thanks.\n\nDo you have an insight on why and how these were failing?  The patch\nin this thread was a band-aid without knowing why all of a sudden\n\"ln -s x y && test -h y\" started passing (while compat/mingw.c still\nsays readlink() is not supported).  If we know that such a breakage\nis not expected, we can drop this workaroun, which would be great.\n\nThanks.\n\n"},{"id":"471941","messageId":"230210.86o7q1uwqu.gmgdl@evledraar.gmail.com","threadId":"59214","inReplyTo":"xmqqk00pjuli.fsf@gitster.g","subject":"Re: [PATCH] test: make SYMLINKS prerequisite more robust","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-10T19:39:11Z","receivedAt":"2023-02-10T19:50:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Feb 10 2023, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>>\n>>>> I wonder if something like this is in order?\n>>>\n>>> I don't have much to contribute on that front, but this is really\n>>> missing some \"why\", this worked before, why is it failing now? Do we\n>>> have any idea.\n>>\n>> Your guess is as good as mine.  I do not do Windows.\n>\n> This morning, I notice that those CI jobs I ran last night that\n> failed with \"whoa, windows tests are somehow reporting that symlinks\n> are available but not really\" issue the patch in this thread were\n> attempting to address has passed even for branches like 'master' and\n> 'next' that do not yet have it, and it seems to be because you\n> re-run these failed jobs.\n>\n> Whatever magic you used to fix these failing tests, thanks.\n>\n> Do you have an insight on why and how these were failing?  The patch\n> in this thread was a band-aid without knowing why all of a sudden\n> \"ln -s x y && test -h y\" started passing (while compat/mingw.c still\n> says readlink() is not supported).  If we know that such a breakage\n> is not expected, we can drop this workaroun, which would be great.\n\nI'm not Johannes :) But as I noted upthread this failed when we went from:\n\n\tDownload action repository\n\t'git-for-windows/setup-git-for-windows-sdk@v1'\n\t(SHA:cbe017cd7ae39629bf4e34fce8b1ccd211fec009)\n\nTo:\n\n\tDownload action repository\n\t'git-for-windows/setup-git-for-windows-sdk@v1'\n\t(SHA:848609620edfa4c2fc64838b85fbe19e534236ee)\n\nAnd now our passing \"next\" has:\n\n\tDownload action repository\n\t'git-for-windows/setup-git-for-windows-sdk@v1'\n\t(SHA:a8e2a23eb07129d628ff6f9d5f11047b0662aeba)\n\nIf you then look at that range in that repository you'll find the\nrelease includes:\n\n\thttps://github.com/git-for-windows/setup-git-for-windows-sdk/pull/595\n        https://github.com/git-for-windows/setup-git-for-windows-sdk/pull/596\n\thttps://github.com/git-for-windows/setup-git-for-windows-sdk/pull/597\n\nThe release notes for the last of those then has:\n\n\t\"Fix issue with symlink restoration on windows\".\n\nWhich from some looking around seems like it might be the issue we've\nbeen seeing, it's this commit & PR:\n\n\thttps://github.com/actions/toolkit/commit/b2d865f18051b214475dae41766e9970fd68ca12\n\thttps://github.com/actions/toolkit/pull/1291\n\nAnd following that rabbit hole leads to Johannes noting that this (or a\nrelated change) was breaking GFW:\n\n\thttps://github.com/actions/toolkit/pull/1291/commits/2867e318d4d0de11b10a2887fb29dcf713559a71#r1098571737\n\nSo I think we can drop the workaround, at least as far as fixing the CI\nbreakages goes.\n"},{"id":"472005","messageId":"ef19e164-b0ba-531d-95e6-75ef61c4fc9d@gmx.de","threadId":"59214","inReplyTo":"xmqqk00pjuli.fsf@gitster.g","subject":"Re: [PATCH] test: make SYMLINKS prerequisite more robust","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2023-02-13T09:12:46Z","receivedAt":"2023-02-13T09:12:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 10 Feb 2023, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n> >\n> >>> I wonder if something like this is in order?\n> >>\n> >> I don't have much to contribute on that front, but this is really\n> >> missing some \"why\", this worked before, why is it failing now? Do we\n> >> have any idea.\n> >\n> > Your guess is as good as mine.  I do not do Windows.\n>\n> This morning, I notice that those CI jobs I ran last night that\n> failed with \"whoa, windows tests are somehow reporting that symlinks\n> are available but not really\" issue the patch in this thread were\n> attempting to address has passed even for branches like 'master' and\n> 'next' that do not yet have it, and it seems to be because you\n> re-run these failed jobs.\n>\n> Whatever magic you used to fix these failing tests, thanks.\n\nThe magic was to pester the maintainers of the dependency of the\n`setup-git-for-windows-sdk` GitHub Action that broke said action.\n\nBackground (you can skip this unless you're interested in details):\n\nSince Git's test suite relies so heavily on Unix shell\nscripting, which is really foreign on Windows, we have to use the MSYS2\nBash to run it. And when I say \"MSYS2 Bash\", it is short-hand for \"a Bash\nthat uses the POSIX emulation layer provided by the MSYS2 runtime\".\n\nThe MSYS2 runtime _does_ have support for actual symbolic links, it just\ndepends on certain Windows features (and then it still is not quite the\nsame as Unix symbolic links because on Windows, the symbolic links are\neither pointing to files or to directories and you have to specify that\nwhen creating them).\n\nThis support is toggled via the environment variable `MSYS`. To enable\nsupport for symbolic links, set it to `winsymlinks:nativestrict`.\n\nNow, the dependency of `setup-git-for-windows-sdk` that is used for\ncaching the minimal subset of Git for Windows' SDK needed for git/git's CI\nruns is called `@actions/cache`, and it recently saw a contribution to\nallow for caching symbolic links even on Windows.\n\nThis change set the `MSYS` environment variable in the way indicated\nabove. Unfortunately, it set it not only for the `tar` invocation that\nneeded it, but it was generous enough to extend that setting to the\nentirety of the containing workflow run. Affecting, among other things,\nGit's test suite.\n\nThe `@actions/cache` PR with the bug fix was actually already opened when\nI noticed the problem, which was only because I released a new version of\nthe `setup-git-for-windows-sdk` Action that supports Windows/ARM64 better\n(and which unfortunately dragged in the borked dependency). But that PR\nhad not been merged yet.\n\nOnce it had been merged, and a new version of that dependency had been\nfast-tracked (I want to believe that my gentle nudging played a part in\nexpediting this), I released a new version of the\n`setup-git-for-windows-sdk` Action and re-ran the failed workflow runs.\n\nAnd subsequently I ran out of time and could not update anyone about what\nI had done to fix this.\n\n> Do you have an insight on why and how these were failing?  The patch\n> in this thread was a band-aid without knowing why all of a sudden\n> \"ln -s x y && test -h y\" started passing (while compat/mingw.c still\n> says readlink() is not supported).  If we know that such a breakage\n> is not expected, we can drop this workaroun, which would be great.\n\nThe patch that started this thread looks a lot like papering over the\nissue to me. And a cleaner way to accomplish that would be to force-set\n`MSYS` to an explicit value.\n\nIt would be nice to eventually get to a point to have Git's test suite\npass with symbolic links enabled on Windows, but introducing a new test\nhelper won't do that.\n\nThe actual root cause of the failures we saw when\n`MSYS=winsymlinks:nativestrict` was graced onto us was that Git's test\nsuite creates a symbolic link to `/dev/null` and then expects Git's\n`opendir()` call on the symbolic link to do something which it does not do\non Windows.\n\nI _guess_ that the symptom is caused by `opendir()` resolving the symbolic\nlink and then trying to open the `NUL` device as a directory, which makes\nno sense whatsoever.\n\nBut I would contend that the actual culprit is (from the cursory look I\ntook) that Git's test suite is abusing `/dev/null` as being some kind of\nempty directory instead of explicitly creating an empty directory and\nusing _that_.\n\nThis is _not_ at all the full and thorough analysis I expect of code\nreviews, especially of my own reviews, please do not mistake my reply for\nthat. I have more pressing things to attend to than to fix what seems like\na \"wouldn't it be nice\" rather than an \"OMG must fix rn!\" problem, and\ntherefore I cannot justify spending more time on it for the time being.\n\nCiao,\nJohannes\n"},{"id":"472012","messageId":"xmqq7cwlcucg.fsf@gitster.g","threadId":"59214","inReplyTo":"ef19e164-b0ba-531d-95e6-75ef61c4fc9d@gmx.de","subject":"Re: [PATCH] test: make SYMLINKS prerequisite more robust","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-13T18:07:27Z","receivedAt":"2023-02-13T18:08:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> Whatever magic you used to fix these failing tests, thanks.\n>\n> The magic was to pester the maintainers of the dependency of the\n> `setup-git-for-windows-sdk` GitHub Action that broke said action.\n>\n> Background (you can skip this unless you're interested in details):\n>\n> Since Git's test suite relies so heavily on Unix shell\n> scripting, which is really foreign on Windows, we have to use the MSYS2\n> Bash to run it. And when I say \"MSYS2 Bash\", it is short-hand for \"a Bash\n> that uses the POSIX emulation layer provided by the MSYS2 runtime\".\n>\n> The MSYS2 runtime _does_ have support for actual symbolic links, it just\n> depends on certain Windows features (and then it still is not quite the\n> same as Unix symbolic links because on Windows, the symbolic links are\n> either pointing to files or to directories and you have to specify that\n> when creating them).\n\nOK, so bash runtime accidentally enabled symbolic links. But of\ncourse \"git\" thinks that Windows never do symbolic links by having\n{ENOSYS; return -1;} in compat/mingw.h for readlink and symlink,\nrunning any git operation that depends on SYMLINKS prerequisite\nwould not work and that is where it fails.  We do not even have to\nhit a test that points a symbolic link at any special file.\n\nThanks for fixing the environment.  The patch in the thread only\nmeant to ensure that the test suite does not get fooled by only bash\nsupporting symbolic links without updating \"git\" supporting any by\nbeing more careful.  Now \"test -h\" and \"ln -s\" behave without\ncontradiction with readlink(2)/symlink(2) emulation \"git\" uses, we\ncan safely drop it.\n\n\n"}]}