{"thread":{"id":"51556","subject":"RE: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","startedAt":"2019-07-30T17:08:53Z","lastAt":"2019-07-31T21:23:00Z","messageCount":24,"participants":["Randall S. Becker","Junio C Hamano","Matheus Tavares Bernardino","Jeff King","Todd Zullinger","SZEDER Gábor","René Scharfe"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"379584","messageId":"049a01d546f9$70be7a30$523b6e90$@nexbridge.com","threadId":"51556","inReplyTo":null,"subject":"RE: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2019-07-30T17:08:37Z","receivedAt":"2019-07-30T17:08:53Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"Hi All,\n\nA preview of the situation with testing 2.23.0.rc0 on NonStop is not great. We have had some new failures right off the bat on our NonStop platforms. This is a preview of what we find within the first bit of testing. The tests run a long time, so more to come.\n\nt0016: oidmap\n\nSubtest 6 had an ordering issue. We do not know whether the problem is the code or the test result not keeping up with the code changes.\n--- expect      2019-07-30 16:56:36 +0000\n+++ actual      2019-07-30 16:56:36 +0000\n@@ -1,6 +1,6 @@\n NULL\n NULL\n NULL\n+7c7cd714e262561f73f3079dfca4e8724682ac21 3\n 139b20d8e6c5b496de61f033f642d0e3dbff528d 2\n d79ce1670bdcb76e6d1da2ae095e890ccb326ae9 1\n-7c7cd714e262561f73f3079dfca4e8724682ac21 3\n\n\nt0066: dir-iterator\n\nSubtest 4 depends on a non-portable error code. ENOENT is not guaranteed to be the same on all systems. It is 4002 on NonStop and 2 on many other machines.\n--- expected-inexistent-path-output     2019-07-30 16:58:51 +0000\n+++ actual-inexistent-path-output       2019-07-30 16:58:50 +0000\n@@ -1 +1 @@\n-dir_iterator_begin failure: 2\n+dir_iterator_begin failure: 4002\n\nSubtest 5 also depends on a non-portable error code. ENOTDIR is not guaranteed to be the same on all systems. It is 4020 on NonStop and 20 on many other machines.\n--- expected-non-dir-output     2019-07-30 16:58:51 +0000\n+++ actual-non-dir-output       2019-07-30 16:58:51 +0000\n@@ -1 +1 @@\n-dir_iterator_begin failure: 20\n+dir_iterator_begin failure: 4020\n\nRandall\n\n-- Brief whoami:\n NonStop developer since approximately 211288444200000000\n UNIX developer since approximately 421664400\n-- In my real life, I talk too much.\n\n\n\n"},{"id":"379586","messageId":"xmqq7e7zton5.fsf@gitster-ct.c.googlers.com","threadId":"51556","inReplyTo":"049a01d546f9$70be7a30$523b6e90$@nexbridge.com","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-30T17:31:42Z","receivedAt":"2019-07-30T17:31:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Randall S. Becker\" <rsbecker@nexbridge.com> writes:\n\n> t0066: dir-iterator\n>\n> Subtest 4 depends on a non-portable error code. ENOENT is not guaranteed ...\n> Subtest 5 also depends on a non-portable error code. ENOTDIR is not gua...\n\nYikes, and sorry.  I've become somewhat complacent after relying on\nhow good our other reviewers are, pretty much ignored the new code\nin fringes like t/helper/, and failed catch an obvious amateurish\nmistake like this one.\n\nI do not think of a portable way to map an int ENOENT to a string\n\"ENOENT\", but there are only only two errors test-dir-iterator test\ncode cares about, so perhaps a patch like the following may be\nsufficient.\n\nI wonder if a tool like sparse can help us catch a pattern that\nfeeds errno to \"%d\" format.\n\n t/helper/test-dir-iterator.c | 11 ++++++++++-\n t/t0066-dir-iterator.sh      |  4 ++--\n 2 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c\nindex a5b96cb0dc..c7c30664da 100644\n--- a/t/helper/test-dir-iterator.c\n+++ b/t/helper/test-dir-iterator.c\n@@ -4,6 +4,15 @@\n #include \"iterator.h\"\n #include \"dir-iterator.h\"\n \n+static const char *error_name(int error_number)\n+{\n+\tswitch (error_number) {\n+\tcase ENOENT: return \"ENOENT\";\n+\tcase ENOTDIR: return \"ENOTDIR\";\n+\tdefault: return \"ESOMETHINGELSE\";\n+\t}\n+}\n+\n /*\n  * usage:\n  * tool-test dir-iterator [--follow-symlinks] [--pedantic] directory_path\n@@ -31,7 +40,7 @@ int cmd__dir_iterator(int argc, const char **argv)\n \tditer = dir_iterator_begin(path.buf, flags);\n \n \tif (!diter) {\n-\t\tprintf(\"dir_iterator_begin failure: %d\\n\", errno);\n+\t\tprintf(\"dir_iterator_begin failure: %s\\n\", error_name(errno));\n \t\texit(EXIT_FAILURE);\n \t}\n \ndiff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh\nindex 9354d3f1ed..92910e4e6c 100755\n--- a/t/t0066-dir-iterator.sh\n+++ b/t/t0066-dir-iterator.sh\n@@ -55,13 +55,13 @@ test_expect_success 'dir-iterator should list files in the correct order' '\n test_expect_success 'begin should fail upon inexistent paths' '\n \ttest_must_fail test-tool dir-iterator ./inexistent-path \\\n \t\t>actual-inexistent-path-output &&\n-\techo \"dir_iterator_begin failure: 2\" >expected-inexistent-path-output &&\n+\techo \"dir_iterator_begin failure: ENOENT\" >expected-inexistent-path-output &&\n \ttest_cmp expected-inexistent-path-output actual-inexistent-path-output\n '\n \n test_expect_success 'begin should fail upon non directory paths' '\n \ttest_must_fail test-tool dir-iterator ./dir/b >actual-non-dir-output &&\n-\techo \"dir_iterator_begin failure: 20\" >expected-non-dir-output &&\n+\techo \"dir_iterator_begin failure: ENOTDIR\" >expected-non-dir-output &&\n \ttest_cmp expected-non-dir-output actual-non-dir-output\n '\n \n"},{"id":"379590","messageId":"CAHd-oW6unEGZck9jWW9qF7DhXUUxTB=h6Ra8ukLVP+odjs9DrQ@mail.gmail.com","threadId":"51556","inReplyTo":"xmqq7e7zton5.fsf@gitster-ct.c.googlers.com","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-07-30T18:09:01Z","receivedAt":"2019-07-30T18:09:15Z","isPatch":false,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, Junio and Randall\n\nOn Tue, Jul 30, 2019 at 2:31 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Randall S. Becker\" <rsbecker@nexbridge.com> writes:\n>\n> > t0066: dir-iterator\n> >\n> > Subtest 4 depends on a non-portable error code. ENOENT is not guaranteed ...\n> > Subtest 5 also depends on a non-portable error code. ENOTDIR is not gua...\n>\n> Yikes, and sorry.  I've become somewhat complacent after relying on\n> how good our other reviewers are, pretty much ignored the new code\n> in fringes like t/helper/, and failed catch an obvious amateurish\n> mistake like this one.\n>\n> I do not think of a portable way to map an int ENOENT to a string\n> \"ENOENT\", but there are only only two errors test-dir-iterator test\n> code cares about, so perhaps a patch like the following may be\n> sufficient.\n\nSorry for that. I totally overlooked the non-portability of error\ncodes when making the patch. But the proposed fix seems very good. And\nhaving the literal string at the test instead of a number is much more\nsignificant, as well.\n\n> I wonder if a tool like sparse can help us catch a pattern that\n> feeds errno to \"%d\" format.\n>\n>  t/helper/test-dir-iterator.c | 11 ++++++++++-\n>  t/t0066-dir-iterator.sh      |  4 ++--\n>  2 files changed, 12 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c\n> index a5b96cb0dc..c7c30664da 100644\n> --- a/t/helper/test-dir-iterator.c\n> +++ b/t/helper/test-dir-iterator.c\n> @@ -4,6 +4,15 @@\n>  #include \"iterator.h\"\n>  #include \"dir-iterator.h\"\n>\n> +static const char *error_name(int error_number)\n> +{\n> +       switch (error_number) {\n> +       case ENOENT: return \"ENOENT\";\n> +       case ENOTDIR: return \"ENOTDIR\";\n> +       default: return \"ESOMETHINGELSE\";\n> +       }\n> +}\n> +\n>  /*\n>   * usage:\n>   * tool-test dir-iterator [--follow-symlinks] [--pedantic] directory_path\n> @@ -31,7 +40,7 @@ int cmd__dir_iterator(int argc, const char **argv)\n>         diter = dir_iterator_begin(path.buf, flags);\n>\n>         if (!diter) {\n> -               printf(\"dir_iterator_begin failure: %d\\n\", errno);\n> +               printf(\"dir_iterator_begin failure: %s\\n\", error_name(errno));\n>                 exit(EXIT_FAILURE);\n>         }\n>\n> diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh\n> index 9354d3f1ed..92910e4e6c 100755\n> --- a/t/t0066-dir-iterator.sh\n> +++ b/t/t0066-dir-iterator.sh\n> @@ -55,13 +55,13 @@ test_expect_success 'dir-iterator should list files in the correct order' '\n>  test_expect_success 'begin should fail upon inexistent paths' '\n>         test_must_fail test-tool dir-iterator ./inexistent-path \\\n>                 >actual-inexistent-path-output &&\n> -       echo \"dir_iterator_begin failure: 2\" >expected-inexistent-path-output &&\n> +       echo \"dir_iterator_begin failure: ENOENT\" >expected-inexistent-path-output &&\n>         test_cmp expected-inexistent-path-output actual-inexistent-path-output\n>  '\n>\n>  test_expect_success 'begin should fail upon non directory paths' '\n>         test_must_fail test-tool dir-iterator ./dir/b >actual-non-dir-output &&\n> -       echo \"dir_iterator_begin failure: 20\" >expected-non-dir-output &&\n> +       echo \"dir_iterator_begin failure: ENOTDIR\" >expected-non-dir-output &&\n>         test_cmp expected-non-dir-output actual-non-dir-output\n>  '\n>\n"},{"id":"379591","messageId":"04a401d54702$05624290$1026c7b0$@nexbridge.com","threadId":"51556","inReplyTo":"xmqq7e7zton5.fsf@gitster-ct.c.googlers.com","subject":"RE: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2019-07-30T18:10:02Z","receivedAt":"2019-07-30T18:10:14Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On July 30, 2019 1:32 PM, Junio C Hamano wrote:\n> \"Randall S. Becker\" <rsbecker@nexbridge.com> writes:\n> \n> > t0066: dir-iterator\n> >\n> > Subtest 4 depends on a non-portable error code. ENOENT is not\n> guaranteed ...\n> > Subtest 5 also depends on a non-portable error code. ENOTDIR is not\ngua...\n> \n> Yikes, and sorry.  I've become somewhat complacent after relying on how\n> good our other reviewers are, pretty much ignored the new code in fringes\n> like t/helper/, and failed catch an obvious amateurish mistake like this\none.\n> \n> I do not think of a portable way to map an int ENOENT to a string\n\"ENOENT\",\n> but there are only only two errors test-dir-iterator test code cares\nabout, so\n> perhaps a patch like the following may be sufficient.\n> \n> I wonder if a tool like sparse can help us catch a pattern that feeds\nerrno to\n> \"%d\" format.\n> \n>  t/helper/test-dir-iterator.c | 11 ++++++++++-\n>  t/t0066-dir-iterator.sh      |  4 ++--\n>  2 files changed, 12 insertions(+), 3 deletions(-)\n> \n> diff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c\nindex\n> a5b96cb0dc..c7c30664da 100644\n> --- a/t/helper/test-dir-iterator.c\n> +++ b/t/helper/test-dir-iterator.c\n> @@ -4,6 +4,15 @@\n>  #include \"iterator.h\"\n>  #include \"dir-iterator.h\"\n> \n> +static const char *error_name(int error_number) {\n> +\tswitch (error_number) {\n> +\tcase ENOENT: return \"ENOENT\";\n> +\tcase ENOTDIR: return \"ENOTDIR\";\n> +\tdefault: return \"ESOMETHINGELSE\";\n> +\t}\n> +}\n> +\n>  /*\n>   * usage:\n>   * tool-test dir-iterator [--follow-symlinks] [--pedantic] directory_path\n@@ -\n> 31,7 +40,7 @@ int cmd__dir_iterator(int argc, const char **argv)\n>  \tditer = dir_iterator_begin(path.buf, flags);\n> \n>  \tif (!diter) {\n> -\t\tprintf(\"dir_iterator_begin failure: %d\\n\", errno);\n> +\t\tprintf(\"dir_iterator_begin failure: %s\\n\",\nerror_name(errno));\n>  \t\texit(EXIT_FAILURE);\n>  \t}\n> \n> diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh index\n> 9354d3f1ed..92910e4e6c 100755\n> --- a/t/t0066-dir-iterator.sh\n> +++ b/t/t0066-dir-iterator.sh\n> @@ -55,13 +55,13 @@ test_expect_success 'dir-iterator should list files in\n> the correct order' '\n>  test_expect_success 'begin should fail upon inexistent paths' '\n>  \ttest_must_fail test-tool dir-iterator ./inexistent-path \\\n>  \t\t>actual-inexistent-path-output &&\n> -\techo \"dir_iterator_begin failure: 2\" >expected-inexistent-path-\n> output &&\n> +\techo \"dir_iterator_begin failure: ENOENT\"\n> +>expected-inexistent-path-output &&\n>  \ttest_cmp expected-inexistent-path-output actual-inexistent-path-\n> output  '\n> \n>  test_expect_success 'begin should fail upon non directory paths' '\n>  \ttest_must_fail test-tool dir-iterator ./dir/b >actual-non-dir-output\n> &&\n> -\techo \"dir_iterator_begin failure: 20\" >expected-non-dir-output &&\n> +\techo \"dir_iterator_begin failure: ENOTDIR\" >expected-non-dir-\n> output &&\n>  \ttest_cmp expected-non-dir-output actual-non-dir-output  '\n> \n\nSeems reasonable. Better than trying to use strerror(), which previously\n(I'm not sure whether it was this project or another) had a similar mapping\nissue because the error text does not match either.\n\n"},{"id":"379592","messageId":"xmqq36intlpj.fsf@gitster-ct.c.googlers.com","threadId":"51556","inReplyTo":"04a401d54702$05624290$1026c7b0$@nexbridge.com","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-30T18:35:04Z","receivedAt":"2019-07-30T18:35:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Randall S. Becker\" <rsbecker@nexbridge.com> writes:\n\n> On July 30, 2019 1:32 PM, Junio C Hamano wrote:\n>> \n>> I wonder if a tool like sparse can help us catch a pattern that feeds\n> errno to\n>> \"%d\" format.\n>>  ...\n> Seems reasonable. Better than trying to use strerror(), which previously\n> (I'm not sure whether it was this project or another) had a similar mapping\n> issue because the error text does not match either.\n\nYup, that was also us X-<.\n\nHere is what I queued for now.\n\n-- >8 --\nSubject: [PATCH] test-dir-iterator: do not assume errno values\n\nA few tests printed 'errno' as an integer and compared with\nhardcoded integers; this is obviously not portable.\n\nA two things to note are:\n\n - the string obtained by strerror() is not portable, and cannot be\n   used for the purpose of these tests.\n\n - there unfortunately isn't a portable way to map error numbers to\n   error names.\n\nAs we only care about a few selected errors, just map the error\nnumber to the name before emitting for comparison.\n\nReported-by: Randall S. Becker <rsbecker@nexbridge.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/helper/test-dir-iterator.c | 11 ++++++++++-\n t/t0066-dir-iterator.sh      |  4 ++--\n 2 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c\nindex a5b96cb0dc..c7c30664da 100644\n--- a/t/helper/test-dir-iterator.c\n+++ b/t/helper/test-dir-iterator.c\n@@ -4,6 +4,15 @@\n #include \"iterator.h\"\n #include \"dir-iterator.h\"\n \n+static const char *error_name(int error_number)\n+{\n+\tswitch (error_number) {\n+\tcase ENOENT: return \"ENOENT\";\n+\tcase ENOTDIR: return \"ENOTDIR\";\n+\tdefault: return \"ESOMETHINGELSE\";\n+\t}\n+}\n+\n /*\n  * usage:\n  * tool-test dir-iterator [--follow-symlinks] [--pedantic] directory_path\n@@ -31,7 +40,7 @@ int cmd__dir_iterator(int argc, const char **argv)\n \tditer = dir_iterator_begin(path.buf, flags);\n \n \tif (!diter) {\n-\t\tprintf(\"dir_iterator_begin failure: %d\\n\", errno);\n+\t\tprintf(\"dir_iterator_begin failure: %s\\n\", error_name(errno));\n \t\texit(EXIT_FAILURE);\n \t}\n \ndiff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh\nindex 9354d3f1ed..92910e4e6c 100755\n--- a/t/t0066-dir-iterator.sh\n+++ b/t/t0066-dir-iterator.sh\n@@ -55,13 +55,13 @@ test_expect_success 'dir-iterator should list files in the correct order' '\n test_expect_success 'begin should fail upon inexistent paths' '\n \ttest_must_fail test-tool dir-iterator ./inexistent-path \\\n \t\t>actual-inexistent-path-output &&\n-\techo \"dir_iterator_begin failure: 2\" >expected-inexistent-path-output &&\n+\techo \"dir_iterator_begin failure: ENOENT\" >expected-inexistent-path-output &&\n \ttest_cmp expected-inexistent-path-output actual-inexistent-path-output\n '\n \n test_expect_success 'begin should fail upon non directory paths' '\n \ttest_must_fail test-tool dir-iterator ./dir/b >actual-non-dir-output &&\n-\techo \"dir_iterator_begin failure: 20\" >expected-non-dir-output &&\n+\techo \"dir_iterator_begin failure: ENOTDIR\" >expected-non-dir-output &&\n \ttest_cmp expected-non-dir-output actual-non-dir-output\n '\n \n-- \n2.23.0-rc0-134-gc2d418fd54\n\n\n"},{"id":"379602","messageId":"20190730194515.GA1088@sigill.intra.peff.net","threadId":"51556","inReplyTo":"049a01d546f9$70be7a30$523b6e90$@nexbridge.com","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-30T19:45:15Z","receivedAt":"2019-07-30T19:45:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 30, 2019 at 01:08:37PM -0400, Randall S. Becker wrote:\n\n> t0016: oidmap\n> \n> Subtest 6 had an ordering issue. We do not know whether the problem is the code or the test result not keeping up with the code changes.\n> --- expect      2019-07-30 16:56:36 +0000\n> +++ actual      2019-07-30 16:56:36 +0000\n> @@ -1,6 +1,6 @@\n>  NULL\n>  NULL\n>  NULL\n> +7c7cd714e262561f73f3079dfca4e8724682ac21 3\n>  139b20d8e6c5b496de61f033f642d0e3dbff528d 2\n>  d79ce1670bdcb76e6d1da2ae095e890ccb326ae9 1\n> -7c7cd714e262561f73f3079dfca4e8724682ac21 3\n\nThis one is very curious. It's iterating a hash, which _seems_ like it\nwould produce non-deterministic output. But neither this test nor the\nhashmap test it is based on sorts the output, and they pass consistently\nfor me. I assume that's because while hash ordering is not guaranteed,\nit happens to be the same as long the pattern of inserts is the same\n(with our implementation, which does not do any hash randomization).\n\nBut I am scratching my head as to what could be different on your\nplatform that would cause a different ordering (especially when the\nhashmap test this is based on doesn't get one!).\n\nI guess in some sense it may not be worth tracking down, and we should\njust sort the output of a hash iteration unconditionally when comparing\nit to expected output.\n\n-Peff\n"},{"id":"379603","messageId":"20190730194938.GZ4545@pobox.com","threadId":"51556","inReplyTo":"049a01d546f9$70be7a30$523b6e90$@nexbridge.com","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-07-30T19:49:38Z","receivedAt":"2019-07-30T19:49:43Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi,\n\n[added Christian, SZEDER, and Jeff to Cc as author and\nhelpers on the newly-added t0016-oidmap]\n\nRandall S. Becker wrote:\n> A preview of the situation with testing 2.23.0.rc0 on\n> NonStop is not great. We have had some new failures right\n> off the bat on our NonStop platforms. This is a preview of\n> what we find within the first bit of testing. The tests\n> run a long time, so more to come.\n> \n> t0016: oidmap\n> \n> Subtest 6 had an ordering issue. We do not know whether\n> the problem is the code or the test result not keeping up\n> with the code changes.\n>\n> --- expect      2019-07-30 16:56:36 +0000\n> +++ actual      2019-07-30 16:56:36 +0000\n> @@ -1,6 +1,6 @@\n>  NULL\n>  NULL\n>  NULL\n> +7c7cd714e262561f73f3079dfca4e8724682ac21 3\n>  139b20d8e6c5b496de61f033f642d0e3dbff528d 2\n>  d79ce1670bdcb76e6d1da2ae095e890ccb326ae9 1\n> -7c7cd714e262561f73f3079dfca4e8724682ac21 3\n\nI hit the same failure while building for Fedora on the\ns390x architecture.  I have not dug into it much yet, but\nperhaps this is an endianess issue?\n\n-- \nTodd\n"},{"id":"379604","messageId":"20190730200203.GA4882@sigill.intra.peff.net","threadId":"51556","inReplyTo":"20190730194938.GZ4545@pobox.com","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-30T20:02:03Z","receivedAt":"2019-07-30T20:02:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 30, 2019 at 03:49:38PM -0400, Todd Zullinger wrote:\n\n> > Subtest 6 had an ordering issue. We do not know whether\n> > the problem is the code or the test result not keeping up\n> > with the code changes.\n> >\n> > --- expect      2019-07-30 16:56:36 +0000\n> > +++ actual      2019-07-30 16:56:36 +0000\n> > @@ -1,6 +1,6 @@\n> >  NULL\n> >  NULL\n> >  NULL\n> > +7c7cd714e262561f73f3079dfca4e8724682ac21 3\n> >  139b20d8e6c5b496de61f033f642d0e3dbff528d 2\n> >  d79ce1670bdcb76e6d1da2ae095e890ccb326ae9 1\n> > -7c7cd714e262561f73f3079dfca4e8724682ac21 3\n> \n> I hit the same failure while building for Fedora on the\n> s390x architecture.  I have not dug into it much yet, but\n> perhaps this is an endianess issue?\n\nAh, of course. Our oid hashing is done by just picking off the first\nbytes of the sha1, and it doesn't care about endianness (because these\nare just internal-to-memory hashes).\n\nWe _could_ reconcile that like this:\n\ndiff --git a/hashmap.h b/hashmap.h\nindex 8424911566..493229ac54 100644\n--- a/hashmap.h\n+++ b/hashmap.h\n@@ -116,19 +116,11 @@ unsigned int memihash_cont(unsigned int hash_seed, const void *buf, size_t len);\n  * Converts a cryptographic hash (e.g. SHA-1) into an int-sized hash code\n  * for use in hash tables. Cryptographic hashes are supposed to have\n  * uniform distribution, so in contrast to `memhash()`, this just copies\n- * the first `sizeof(int)` bytes without shuffling any bits. Note that\n- * the results will be different on big-endian and little-endian\n- * platforms, so they should not be stored or transferred over the net.\n+ * the first `sizeof(int)` bytes without shuffling any bits.\n  */\n static inline unsigned int oidhash(const struct object_id *oid)\n {\n-\t/*\n-\t * Equivalent to 'return *(unsigned int *)oid->hash;', but safe on\n-\t * platforms that don't support unaligned reads.\n-\t */\n-\tunsigned int hash;\n-\tmemcpy(&hash, oid->hash, sizeof(hash));\n-\treturn hash;\n+\treturn get_be32(oid->hash);\n }\n \n /*\ndiff --git a/t/t0016-oidmap.sh b/t/t0016-oidmap.sh\nindex bbe719e950..6656db9d69 100755\n--- a/t/t0016-oidmap.sh\n+++ b/t/t0016-oidmap.sh\n@@ -93,9 +93,9 @@ put three 3\n iterate\" \"NULL\n NULL\n NULL\n+$(git rev-parse three) 3\n $(git rev-parse two) 2\n-$(git rev-parse one) 1\n-$(git rev-parse three) 3\"\n+$(git rev-parse one) 1\"\n \n '\n \n\nwhich not only fixes this test but any other hash-based oddities. I\nwonder if it's appreciably less efficient. I'll bet I could nerd-snipe\nRené into doing a bunch of measurements and explorations of the\ndisassembled code. ;)\n\n-Peff\n"},{"id":"379606","messageId":"04b201d54715$0180a0f0$0481e2d0$@nexbridge.com","threadId":"51556","inReplyTo":"20190730194515.GA1088@sigill.intra.peff.net","subject":"RE: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2019-07-30T20:25:56Z","receivedAt":"2019-07-30T20:26:14Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On July 30, 2019 3:45 PM, Jeff King wrote:\n> To: Randall S. Becker <rsbecker@nexbridge.com>\n> Cc: 'Junio C Hamano' <gitster@pobox.com>; git@vger.kernel.org; git-\n> packagers@googlegroups.com\n> Subject: Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop\n> \n> On Tue, Jul 30, 2019 at 01:08:37PM -0400, Randall S. Becker wrote:\n> \n> > t0016: oidmap\n> >\n> > Subtest 6 had an ordering issue. We do not know whether the problem is\n> the code or the test result not keeping up with the code changes.\n> > --- expect      2019-07-30 16:56:36 +0000\n> > +++ actual      2019-07-30 16:56:36 +0000\n> > @@ -1,6 +1,6 @@\n> >  NULL\n> >  NULL\n> >  NULL\n> > +7c7cd714e262561f73f3079dfca4e8724682ac21 3\n> >  139b20d8e6c5b496de61f033f642d0e3dbff528d 2\n> >  d79ce1670bdcb76e6d1da2ae095e890ccb326ae9 1\n> > -7c7cd714e262561f73f3079dfca4e8724682ac21 3\n> \n> This one is very curious. It's iterating a hash, which _seems_ like it would\n> produce non-deterministic output. But neither this test nor the hashmap test\n> it is based on sorts the output, and they pass consistently for me. I assume\n> that's because while hash ordering is not guaranteed, it happens to be the\n> same as long the pattern of inserts is the same (with our implementation,\n> which does not do any hash randomization).\n> \n> But I am scratching my head as to what could be different on your platform\n> that would cause a different ordering (especially when the hashmap test this\n> is based on doesn't get one!).\n> \n> I guess in some sense it may not be worth tracking down, and we should just\n> sort the output of a hash iteration unconditionally when comparing it to\n> expected output.\n\nDefinitely a head scratcher. Is it possible that the bucket() function, which uses\n\n\tkey->hash & (map->tablesize - 1);\n\nmight better use\n\n\tkey->hash % (map->tablesize - 1);\n\nI have not seen a bucket computation done this way before so that surprised me, not that it should make a difference on hash-determinism. The only thing that might is an uninitialized stack variable, which on this platform's C compiler will not initialize. Global statics are always 0 unless otherwise specified, but I'm not sure about stack-local (but there's nothing wrong I can see in hashmap.c on those points.\n\nSorting the output seems like a safe option, providing that the hash is itself demonstrably solid otherwise.\n\nCheers,\nRandall\n\n"},{"id":"379608","messageId":"04b301d54715$3b371a90$b1a54fb0$@nexbridge.com","threadId":"51556","inReplyTo":"20190730194938.GZ4545@pobox.com","subject":"RE: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2019-07-30T20:27:33Z","receivedAt":"2019-07-30T20:27:51Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On July 30, 2019 3:50 PM, Todd Zullinger wrote:\n> To: Randall S. Becker <rsbecker@nexbridge.com>\n> Cc: 'Junio C Hamano' <gitster@pobox.com>; Christian Couder\n> <chriscool@tuxfamily.org>; SZEDER Gábor <szeder.dev@gmail.com>; Jeff\n> King <peff@peff.net>; git@vger.kernel.org; git-\n> packagers@googlegroups.com\n> Subject: Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop\n> \n> Hi,\n> \n> [added Christian, SZEDER, and Jeff to Cc as author and helpers on the\nnewly-\n> added t0016-oidmap]\n> \n> Randall S. Becker wrote:\n> > A preview of the situation with testing 2.23.0.rc0 on NonStop is not\n> > great. We have had some new failures right off the bat on our NonStop\n> > platforms. This is a preview of what we find within the first bit of\n> > testing. The tests run a long time, so more to come.\n> >\n> > t0016: oidmap\n> >\n> > Subtest 6 had an ordering issue. We do not know whether the problem is\n> > the code or the test result not keeping up with the code changes.\n> >\n> > --- expect      2019-07-30 16:56:36 +0000\n> > +++ actual      2019-07-30 16:56:36 +0000\n> > @@ -1,6 +1,6 @@\n> >  NULL\n> >  NULL\n> >  NULL\n> > +7c7cd714e262561f73f3079dfca4e8724682ac21 3\n> >  139b20d8e6c5b496de61f033f642d0e3dbff528d 2\n> >  d79ce1670bdcb76e6d1da2ae095e890ccb326ae9 1\n> > -7c7cd714e262561f73f3079dfca4e8724682ac21 3\n> \n> I hit the same failure while building for Fedora on the s390x\narchitecture.  I\n> have not dug into it much yet, but perhaps this is an endianess issue?\n\nMy platforms are BIGendian, so perhaps that's it. The bucket calculation\nuses bit arithmetic so that could contribute.\n\n"},{"id":"379610","messageId":"xmqqpnlrs1d7.fsf@gitster-ct.c.googlers.com","threadId":"51556","inReplyTo":"20190730200203.GA4882@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-30T20:39:48Z","receivedAt":"2019-07-30T20:39:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> @@ -116,19 +116,11 @@ unsigned int memihash_cont(unsigned int hash_seed, const void *buf, size_t len);\n>   * Converts a cryptographic hash (e.g. SHA-1) into an int-sized hash code\n>   * for use in hash tables. Cryptographic hashes are supposed to have\n>   * uniform distribution, so in contrast to `memhash()`, this just copies\n> - * the first `sizeof(int)` bytes without shuffling any bits. Note that\n> - * the results will be different on big-endian and little-endian\n> - * platforms, so they should not be stored or transferred over the net.\n\n;-)\n\n> + * the first `sizeof(int)` bytes without shuffling any bits.\n>   */\n>  static inline unsigned int oidhash(const struct object_id *oid)\n>  {\n> -\t/*\n> -\t * Equivalent to 'return *(unsigned int *)oid->hash;', but safe on\n> -\t * platforms that don't support unaligned reads.\n> -\t */\n> -\tunsigned int hash;\n> -\tmemcpy(&hash, oid->hash, sizeof(hash));\n> -\treturn hash;\n> +\treturn get_be32(oid->hash);\n>  }\n>  \n>  /*\n> diff --git a/t/t0016-oidmap.sh b/t/t0016-oidmap.sh\n> index bbe719e950..6656db9d69 100755\n> --- a/t/t0016-oidmap.sh\n> +++ b/t/t0016-oidmap.sh\n> @@ -93,9 +93,9 @@ put three 3\n>  iterate\" \"NULL\n>  NULL\n>  NULL\n> +$(git rev-parse three) 3\n>  $(git rev-parse two) 2\n> -$(git rev-parse one) 1\n> -$(git rev-parse three) 3\"\n> +$(git rev-parse one) 1\"\n>  \n>  '\n>\n> which not only fixes this test but any other hash-based oddities. I\n> wonder if it's appreciably less efficient. I'll bet I could nerd-snipe\n> René into doing a bunch of measurements and explorations of the\n> disassembled code. ;)\n\nI'd rather see us go in the direction of discouraging people from\nrelying on the hash order.\n"},{"id":"379612","messageId":"20190730205624.GR20404@szeder.dev","threadId":"51556","inReplyTo":"20190730200203.GA4882@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-07-30T20:56:24Z","receivedAt":"2019-07-30T20:56:31Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Jul 30, 2019 at 04:02:03PM -0400, Jeff King wrote:\n> On Tue, Jul 30, 2019 at 03:49:38PM -0400, Todd Zullinger wrote:\n> \n> > > Subtest 6 had an ordering issue. We do not know whether\n> > > the problem is the code or the test result not keeping up\n> > > with the code changes.\n> > >\n> > > --- expect      2019-07-30 16:56:36 +0000\n> > > +++ actual      2019-07-30 16:56:36 +0000\n> > > @@ -1,6 +1,6 @@\n> > >  NULL\n> > >  NULL\n> > >  NULL\n> > > +7c7cd714e262561f73f3079dfca4e8724682ac21 3\n> > >  139b20d8e6c5b496de61f033f642d0e3dbff528d 2\n> > >  d79ce1670bdcb76e6d1da2ae095e890ccb326ae9 1\n> > > -7c7cd714e262561f73f3079dfca4e8724682ac21 3\n> > \n> > I hit the same failure while building for Fedora on the\n> > s390x architecture.  I have not dug into it much yet, but\n> > perhaps this is an endianess issue?\n> \n> Ah, of course. Our oid hashing is done by just picking off the first\n> bytes of the sha1, and it doesn't care about endianness (because these\n> are just internal-to-memory hashes).\n\nYeah.\n\n> We _could_ reconcile that like this:\n\nDo we really want that, though?  It's a hashmap, after all, and the\norder of iteration over various hashmap implementations tends to be\narbitrary.  So an argument could be made that this test is overly\nspecific by expecting a particular order of elements (and perhaps by\nchecking the elements' oid as well), and it would be sufficient to\ncheck that it iterates over all elements, no matter the order (IOW\nsorting 'actual' before the comparison).\n\nOTOH, this is not just any hashmap, but an oidmap, and I could imagine\nthat there might be use cases where it would be beneficial if the\niteration order were to match the oid order (but don't know whether we\nactually have such a use case).\n\n\n> I\n> wonder if it's appreciably less efficient. I'll bet I could nerd-snipe\n> René into doing a bunch of measurements and explorations of the\n> disassembled code. ;)\n\nMaybe it shows up in an oidmap-specific performance test, but with all\nthat's usually going on in Git hashmap performance tends to be\nnegligible (e.g. it's rarely visible in flame graphs).\n\n"},{"id":"379628","messageId":"20190731005933.GA9610@sigill.intra.peff.net","threadId":"51556","inReplyTo":"20190730205624.GR20404@szeder.dev","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-31T00:59:34Z","receivedAt":"2019-07-31T00:59:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 30, 2019 at 10:56:24PM +0200, SZEDER Gábor wrote:\n\n> > Ah, of course. Our oid hashing is done by just picking off the first\n> > bytes of the sha1, and it doesn't care about endianness (because these\n> > are just internal-to-memory hashes).\n> \n> Yeah.\n> \n> > We _could_ reconcile that like this:\n> \n> Do we really want that, though?  It's a hashmap, after all, and the\n> order of iteration over various hashmap implementations tends to be\n> arbitrary.  So an argument could be made that this test is overly\n> specific by expecting a particular order of elements (and perhaps by\n> checking the elements' oid as well), and it would be sufficient to\n> check that it iterates over all elements, no matter the order (IOW\n> sorting 'actual' before the comparison).\n\nI'd agree that this test is being overly specific. I guess what I'm\nfeeling is a vague notion that it might be better if Git behaves\ndeterministically regardless of endian-ness. Not because it _should_\nmatter for this test, but there could literally be a bug on big-endian\nplatforms that nobody knows about because it's very rare for anybody to\ntest there.\n\nI admit that's pretty hand-wavy though. And there may actually be a\nbenefit in finding such a bug, because it means that some part of the\ncode (or a test) is relying on something it ought not to.\n\n> OTOH, this is not just any hashmap, but an oidmap, and I could imagine\n> that there might be use cases where it would be beneficial if the\n> iteration order were to match the oid order (but don't know whether we\n> actually have such a use case).\n\nI don't think we can promise anything about iteration order. This test\nis relying on the order at least being deterministic between runs, but\nif we added a new entry and had to grow the table, all bets are off.\n\nSo regardless of the endian thing above, it probably does make sense for\nany hashmap iteration output to be sorted before comparing. That goes\nfor t0011, too; it doesn't have this endian thing, but it looks to be\nrelying on hash order that could change if we swapped out hash\nfunctions.\n\n> > I\n> > wonder if it's appreciably less efficient. I'll bet I could nerd-snipe\n> > René into doing a bunch of measurements and explorations of the\n> > disassembled code. ;)\n> \n> Maybe it shows up in an oidmap-specific performance test, but with all\n> that's usually going on in Git hashmap performance tends to be\n> negligible (e.g. it's rarely visible in flame graphs).\n\nThat's my guess, too, but data trumps guesses (you'll note that I'm not\nvolunteering to _collect_ the data, though, which perhaps gives a sense\nof how invested I am in it. ;) ).\n\n-Peff\n"},{"id":"379630","messageId":"20190731012336.GA13880@sigill.intra.peff.net","threadId":"51556","inReplyTo":"20190731005933.GA9610@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-31T01:23:37Z","receivedAt":"2019-07-31T01:23:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 30, 2019 at 08:59:33PM -0400, Jeff King wrote:\n\n> > OTOH, this is not just any hashmap, but an oidmap, and I could imagine\n> > that there might be use cases where it would be beneficial if the\n> > iteration order were to match the oid order (but don't know whether we\n> > actually have such a use case).\n> \n> I don't think we can promise anything about iteration order. This test\n> is relying on the order at least being deterministic between runs, but\n> if we added a new entry and had to grow the table, all bets are off.\n> \n> So regardless of the endian thing above, it probably does make sense for\n> any hashmap iteration output to be sorted before comparing. That goes\n> for t0011, too; it doesn't have this endian thing, but it looks to be\n> relying on hash order that could change if we swapped out hash\n> functions.\n\nSo here's an actual patch.\n\n-- >8 --\nSubject: [PATCH] t: sort output of hashmap iteration\n\nThe iteration order of a hashmap is undefined, and may depend on things\nlike the exact set of items added, or the table has been grown or\nshrunk. In the case of an oidmap, it even depends on endianness, because\nwe take the oid hash by casting sha1 bytes directly into an unsigned\nint.\n\nLet's sort the test-tool output from any hash iterators. In the case of\nt0011, this is just future-proofing. But for t0016, it actually fixes a\nreported failure on the big-endian s390 and nonstop ports.\n\nI didn't bother to teach the helper functions to optionally sort output.\nThey are short enough that it's simpler to just repeat them inline for\nthe iteration tests than it is to add a --sort option.\n\nReported-by: Randall S. Becker <rsbecker@nexbridge.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t0011-hashmap.sh | 58 ++++++++++++++++++++++++++++------------------\n t/t0016-oidmap.sh  | 30 +++++++++++++++---------\n 2 files changed, 55 insertions(+), 33 deletions(-)\n\ndiff --git a/t/t0011-hashmap.sh b/t/t0011-hashmap.sh\nindex 9c96b3e3b1..5343ffd3f9 100755\n--- a/t/t0011-hashmap.sh\n+++ b/t/t0011-hashmap.sh\n@@ -170,31 +170,45 @@ NULL\n '\n \n test_expect_success 'iterate' '\n-\n-test_hashmap \"put key1 value1\n-put key2 value2\n-put fooBarFrotz value3\n-iterate\" \"NULL\n-NULL\n-NULL\n-key2 value2\n-key1 value1\n-fooBarFrotz value3\"\n-\n+\ttest-tool hashmap >actual.raw <<-\\EOF &&\n+\tput key1 value1\n+\tput key2 value2\n+\tput fooBarFrotz value3\n+\titerate\n+\tEOF\n+\n+\tcat >expect <<-\\EOF &&\n+\tNULL\n+\tNULL\n+\tNULL\n+\tfooBarFrotz value3\n+\tkey1 value1\n+\tkey2 value2\n+\tEOF\n+\n+\tsort <actual.raw >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'iterate (case insensitive)' '\n-\n-test_hashmap \"put key1 value1\n-put key2 value2\n-put fooBarFrotz value3\n-iterate\" \"NULL\n-NULL\n-NULL\n-fooBarFrotz value3\n-key2 value2\n-key1 value1\" ignorecase\n-\n+\ttest-tool hashmap ignorecase >actual.raw <<-\\EOF &&\n+\tput key1 value1\n+\tput key2 value2\n+\tput fooBarFrotz value3\n+\titerate\n+\tEOF\n+\n+\tcat >expect <<-\\EOF &&\n+\tNULL\n+\tNULL\n+\tNULL\n+\tfooBarFrotz value3\n+\tkey1 value1\n+\tkey2 value2\n+\tEOF\n+\n+\tsort <actual.raw >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'grow / shrink' '\ndiff --git a/t/t0016-oidmap.sh b/t/t0016-oidmap.sh\nindex bbe719e950..31f8276ba8 100755\n--- a/t/t0016-oidmap.sh\n+++ b/t/t0016-oidmap.sh\n@@ -86,17 +86,25 @@ NULL\"\n '\n \n test_expect_success 'iterate' '\n-\n-test_oidmap \"put one 1\n-put two 2\n-put three 3\n-iterate\" \"NULL\n-NULL\n-NULL\n-$(git rev-parse two) 2\n-$(git rev-parse one) 1\n-$(git rev-parse three) 3\"\n-\n+\ttest-tool oidmap >actual.raw <<-\\EOF &&\n+\tput one 1\n+\tput two 2\n+\tput three 3\n+\titerate\n+\tEOF\n+\n+\t# sort \"expect\" too so we do not rely on the order of particular oids\n+\tsort >expect <<-EOF &&\n+\tNULL\n+\tNULL\n+\tNULL\n+\t$(git rev-parse one) 1\n+\t$(git rev-parse two) 2\n+\t$(git rev-parse three) 3\n+\tEOF\n+\n+\tsort <actual.raw >actual &&\n+\ttest_cmp expect actual\n '\n \n test_done\n-- \n2.23.0.rc0.426.gbdee707ba7\n\n"},{"id":"379631","messageId":"20190731012746.GB13880@sigill.intra.peff.net","threadId":"51556","inReplyTo":"20190731012336.GA13880@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-31T01:27:46Z","receivedAt":"2019-07-31T01:27:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 30, 2019 at 09:23:36PM -0400, Jeff King wrote:\n\n>  test_expect_success 'iterate (case insensitive)' '\n> -\n> -test_hashmap \"put key1 value1\n> -put key2 value2\n> -put fooBarFrotz value3\n> -iterate\" \"NULL\n> -NULL\n> -NULL\n> -fooBarFrotz value3\n> -key2 value2\n> -key1 value1\" ignorecase\n\nBy the way, I found it curious that this test is repeated both with and\nwithout ignorecase, but in the case-insensitive we never have any\ncase-equivalent names!\n\nI guess it is testing that we preserved the name of fooBarFrotz, but I\nwould think that this:\n\ndiff --git a/t/t0011-hashmap.sh b/t/t0011-hashmap.sh\nindex 5343ffd3f9..2807dff644 100755\n--- a/t/t0011-hashmap.sh\n+++ b/t/t0011-hashmap.sh\n@@ -195,14 +195,15 @@ test_expect_success 'iterate (case insensitive)' '\n \tput key1 value1\n \tput key2 value2\n \tput fooBarFrotz value3\n+\tput foobarfrotz value4\n \titerate\n \tEOF\n \n \tcat >expect <<-\\EOF &&\n \tNULL\n \tNULL\n \tNULL\n-\tfooBarFrotz value3\n+\tfooBarFrotz value4\n \tkey1 value1\n \tkey2 value2\n \tEOF\n\non top of my other patch would be a good addition. However, it yields\nthis result:\n\n--- expect\t2019-07-31 01:26:07.284231899 +0000\n+++ actual\t2019-07-31 01:26:07.284231899 +0000\n@@ -1,6 +1,7 @@\n NULL\n NULL\n NULL\n-fooBarFrotz value4\n+foobarfrotz value4\n key1 value1\n key2 value2\n+value3\n\nThe first line is a matter of taste I think (whether to keep the\noriginal case or replace it with the new key). But the empty-keyed\nretention of value3 seems wrong. I'm not sure if this is a bug in the\nactual hashmap code, or just a weirdness with the way test-tool prints\nit.\n\nI'm going offline for a bit, so if anybody feels like following up on\nthe mystery, be my guest. Otherwise I may take a look later tonight.\n\n-Peff\n"},{"id":"379633","messageId":"20190731015917.GB4545@pobox.com","threadId":"51556","inReplyTo":"20190731012336.GA13880@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-07-31T01:59:17Z","receivedAt":"2019-07-31T01:59:30Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Jeff King wrote:\n> On Tue, Jul 30, 2019 at 08:59:33PM -0400, Jeff King wrote:\n> \n>>> OTOH, this is not just any hashmap, but an oidmap, and I could imagine\n>>> that there might be use cases where it would be beneficial if the\n>>> iteration order were to match the oid order (but don't know whether we\n>>> actually have such a use case).\n>> \n>> I don't think we can promise anything about iteration order. This test\n>> is relying on the order at least being deterministic between runs, but\n>> if we added a new entry and had to grow the table, all bets are off.\n>> \n>> So regardless of the endian thing above, it probably does make sense for\n>> any hashmap iteration output to be sorted before comparing. That goes\n>> for t0011, too; it doesn't have this endian thing, but it looks to be\n>> relying on hash order that could change if we swapped out hash\n>> functions.\n> \n> So here's an actual patch.\n\nAt the risk of showing my complete lack of knowledge about\nthese tests, I was wondering if the order mattered for the\nother tests in t0011 and t0016.  I had assumed it didn't and\nhad something like this for testing (and a similar change to\ntest_oidmap() in t0016):\n\n diff --git i/t/t0011-hashmap.sh w/t/t0011-hashmap.sh\n index 9c96b3e3b1..9ed1c4f14d 100755\n --- i/t/t0011-hashmap.sh\n +++ w/t/t0011-hashmap.sh\n @@ -4,8 +4,8 @@ test_description='test hashmap and string hash functions'\n  . ./test-lib.sh\n  \n  test_hashmap() {\n -\techo \"$1\" | test-tool hashmap $3 > actual &&\n -\techo \"$2\" > expect &&\n +\techo \"$1\" | test-tool hashmap $3 | sort >actual &&\n +\techo \"$2\" | sort >expect &&\n  \ttest_cmp expect actual\n  }\n\nYou've got a more comprehensive patch and a proper commit\nmessage, so this is really just a matter of curiosity.\n\n-- \nTodd\n"},{"id":"379634","messageId":"20190731032735.GA14684@sigill.intra.peff.net","threadId":"51556","inReplyTo":"20190731015917.GB4545@pobox.com","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-31T03:27:35Z","receivedAt":"2019-07-31T03:27:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 30, 2019 at 09:59:17PM -0400, Todd Zullinger wrote:\n\n> At the risk of showing my complete lack of knowledge about\n> these tests, I was wondering if the order mattered for the\n> other tests in t0011 and t0016.  I had assumed it didn't and\n> had something like this for testing (and a similar change to\n> test_oidmap() in t0016):\n> \n>  diff --git i/t/t0011-hashmap.sh w/t/t0011-hashmap.sh\n>  index 9c96b3e3b1..9ed1c4f14d 100755\n>  --- i/t/t0011-hashmap.sh\n>  +++ w/t/t0011-hashmap.sh\n>  @@ -4,8 +4,8 @@ test_description='test hashmap and string hash functions'\n>   . ./test-lib.sh\n>   \n>   test_hashmap() {\n>  -\techo \"$1\" | test-tool hashmap $3 > actual &&\n>  -\techo \"$2\" > expect &&\n>  +\techo \"$1\" | test-tool hashmap $3 | sort >actual &&\n>  +\techo \"$2\" | sort >expect &&\n>   \ttest_cmp expect actual\n>   }\n> \n> You've got a more comprehensive patch and a proper commit\n> message, so this is really just a matter of curiosity.\n\nI think the order does matter for those ones. E.g., the ones that run\n\"get\" want to make sure they're seeing the values in the same order in\nwhich they were requested.\n\nOne thing that makes it all a bit funky is that the \"put\" lines also\noutput the old value (which is what all those NULLs) are. And I think\nthat solves my \"value3\" puzzlement from earlier. It is not part of the\niteration at all, but rather the result of the duplicate \"put\".\n\nThat would perhaps be clearer if the \"hashmap\" tool actually did the\nsorting itself (so we'd sort _just_ the iteration, not the whole\noutput). Something like this, though I'm on the fence about whether it\nis worth it:\n\ndiff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\nindex aaf17b0ddf..9f6901666e 100644\n--- a/t/helper/test-hashmap.c\n+++ b/t/helper/test-hashmap.c\n@@ -2,6 +2,7 @@\n #include \"git-compat-util.h\"\n #include \"hashmap.h\"\n #include \"strbuf.h\"\n+#include \"string-list.h\"\n \n struct test_entry\n {\n@@ -221,10 +222,18 @@ int cmd__hashmap(int argc, const char **argv)\n \n \t\t} else if (!strcmp(\"iterate\", cmd)) {\n \n+\t\t\tstruct string_list sorted = STRING_LIST_INIT_NODUP;\n+\t\t\tstruct string_list_item *item;\n \t\t\tstruct hashmap_iter iter;\n \t\t\thashmap_iter_init(&map, &iter);\n-\t\t\twhile ((entry = hashmap_iter_next(&iter)))\n-\t\t\t\tprintf(\"%s %s\\n\", entry->key, get_value(entry));\n+\t\t\twhile ((entry = hashmap_iter_next(&iter))) {\n+\t\t\t\titem = string_list_append(&sorted, entry->key);\n+\t\t\t\titem->util = (void *)get_value(entry);\n+\t\t\t}\n+\t\t\tstring_list_sort(&sorted);\n+\t\t\tfor_each_string_list_item(item, &sorted)\n+\t\t\t\tprintf(\"%s %s\\n\", item->string, (const char *)item->util);\n+\t\t\tstring_list_clear(&sorted, 0);\n \n \t\t} else if (!strcmp(\"size\", cmd)) {\n \n"},{"id":"379635","messageId":"20190731035344.GA26019@sigill.intra.peff.net","threadId":"51556","inReplyTo":"20190731032735.GA14684@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-31T03:53:44Z","receivedAt":"2019-07-31T03:53:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 30, 2019 at 11:27:35PM -0400, Jeff King wrote:\n\n> That would perhaps be clearer if the \"hashmap\" tool actually did the\n> sorting itself (so we'd sort _just_ the iteration, not the whole\n> output). Something like this, though I'm on the fence about whether it\n> is worth it:\n> [...]\n\nAnd here it is for reference with the matching change in test-oidmap,\nand the adjustments necessary for the test scripts (from master, not\nfrom my earlier patch). I think I prefer the simpler \"just sort it all\"\nversion I posted with the commit message.\n\nThe post-image in t0016 may be a new low: a command substitution in a DQ\nstring fed to echo, itself inside a command substitution in a here-doc.\nYuck. :)\n\n---\ndiff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\nindex aaf17b0ddf..9f6901666e 100644\n--- a/t/helper/test-hashmap.c\n+++ b/t/helper/test-hashmap.c\n@@ -2,6 +2,7 @@\n #include \"git-compat-util.h\"\n #include \"hashmap.h\"\n #include \"strbuf.h\"\n+#include \"string-list.h\"\n \n struct test_entry\n {\n@@ -221,10 +222,18 @@ int cmd__hashmap(int argc, const char **argv)\n \n \t\t} else if (!strcmp(\"iterate\", cmd)) {\n \n+\t\t\tstruct string_list sorted = STRING_LIST_INIT_NODUP;\n+\t\t\tstruct string_list_item *item;\n \t\t\tstruct hashmap_iter iter;\n \t\t\thashmap_iter_init(&map, &iter);\n-\t\t\twhile ((entry = hashmap_iter_next(&iter)))\n-\t\t\t\tprintf(\"%s %s\\n\", entry->key, get_value(entry));\n+\t\t\twhile ((entry = hashmap_iter_next(&iter))) {\n+\t\t\t\titem = string_list_append(&sorted, entry->key);\n+\t\t\t\titem->util = (void *)get_value(entry);\n+\t\t\t}\n+\t\t\tstring_list_sort(&sorted);\n+\t\t\tfor_each_string_list_item(item, &sorted)\n+\t\t\t\tprintf(\"%s %s\\n\", item->string, (const char *)item->util);\n+\t\t\tstring_list_clear(&sorted, 0);\n \n \t\t} else if (!strcmp(\"size\", cmd)) {\n \ndiff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c\nindex 0acf99931e..5fd059fe90 100644\n--- a/t/helper/test-oidmap.c\n+++ b/t/helper/test-oidmap.c\n@@ -94,10 +94,19 @@ int cmd__oidmap(int argc, const char **argv)\n \n \t\t} else if (!strcmp(\"iterate\", cmd)) {\n \n+\t\t\tstruct string_list sorted = STRING_LIST_INIT_DUP;\n+\t\t\tstruct string_list_item *item;\n \t\t\tstruct oidmap_iter iter;\n \t\t\toidmap_iter_init(&map, &iter);\n-\t\t\twhile ((entry = oidmap_iter_next(&iter)))\n-\t\t\t\tprintf(\"%s %s\\n\", oid_to_hex(&entry->entry.oid), entry->name);\n+\t\t\twhile ((entry = oidmap_iter_next(&iter))) {\n+\t\t\t\titem = string_list_append(&sorted,\n+\t\t\t\t\t\t\t  oid_to_hex(&entry->entry.oid));\n+\t\t\t\titem->util = entry->name;\n+\t\t\t}\n+\t\t\tstring_list_sort(&sorted);\n+\t\t\tfor_each_string_list_item(item, &sorted)\n+\t\t\t\tprintf(\"%s %s\\n\", item->string, (const char *)item->util);\n+\t\t\tstring_list_clear(&sorted, 0);\n \n \t\t} else {\n \ndiff --git a/t/t0011-hashmap.sh b/t/t0011-hashmap.sh\nindex 9c96b3e3b1..1adcd7762d 100755\n--- a/t/t0011-hashmap.sh\n+++ b/t/t0011-hashmap.sh\n@@ -177,9 +177,9 @@ put fooBarFrotz value3\n iterate\" \"NULL\n NULL\n NULL\n-key2 value2\n+fooBarFrotz value3\n key1 value1\n-fooBarFrotz value3\"\n+key2 value2\"\n \n '\n \n@@ -192,8 +192,8 @@ iterate\" \"NULL\n NULL\n NULL\n fooBarFrotz value3\n-key2 value2\n-key1 value1\" ignorecase\n+key1 value1\n+key2 value2\" ignorecase\n \n '\n \ndiff --git a/t/t0016-oidmap.sh b/t/t0016-oidmap.sh\nindex bbe719e950..abeaa64159 100755\n--- a/t/t0016-oidmap.sh\n+++ b/t/t0016-oidmap.sh\n@@ -93,9 +93,14 @@ put three 3\n iterate\" \"NULL\n NULL\n NULL\n-$(git rev-parse two) 2\n-$(git rev-parse one) 1\n-$(git rev-parse three) 3\"\n+$(\n+\t# sort to avoid relying on exact oids\n+\t{\n+\t\techo \"$(git rev-parse one) 1\"\n+\t\techo \"$(git rev-parse two) 2\"\n+\t\techo \"$(git rev-parse three) 3\"\n+\t} | sort\n+)\"\n \n '\n \n"},{"id":"379636","messageId":"b32a178d-0255-6c82-a3fd-ced2ec828fc8@web.de","threadId":"51556","inReplyTo":"20190731032735.GA14684@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-07-31T04:06:02Z","receivedAt":"2019-07-31T04:06:38Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 31.07.19 um 05:27 schrieb Jeff King:\n> One thing that makes it all a bit funky is that the \"put\" lines also\n> output the old value (which is what all those NULLs) are. And I think\n> that solves my \"value3\" puzzlement from earlier. It is not part of the\n> iteration at all, but rather the result of the duplicate \"put\".\n>\n> That would perhaps be clearer if the \"hashmap\" tool actually did the\n> sorting itself (so we'd sort _just_ the iteration, not the whole\n> output). Something like this, though I'm on the fence about whether it\n> is worth it:\n\nWe already have a few other tests that sort and compare.  Perhaps it's\ntime for a test_cmp_ignore_order?\n\nAnd perhaps something like this might even be worth implementing as a\ndiff option?  https://github.com/l0b0/diff-ignore-moved-lines has\npost-processing script for that..\n\nRené\n"},{"id":"379639","messageId":"20190731043039.GB26237@sigill.intra.peff.net","threadId":"51556","inReplyTo":"b32a178d-0255-6c82-a3fd-ced2ec828fc8@web.de","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-31T04:30:39Z","receivedAt":"2019-07-31T04:30:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 31, 2019 at 06:06:02AM +0200, René Scharfe wrote:\n\n> Am 31.07.19 um 05:27 schrieb Jeff King:\n> > One thing that makes it all a bit funky is that the \"put\" lines also\n> > output the old value (which is what all those NULLs) are. And I think\n> > that solves my \"value3\" puzzlement from earlier. It is not part of the\n> > iteration at all, but rather the result of the duplicate \"put\".\n> >\n> > That would perhaps be clearer if the \"hashmap\" tool actually did the\n> > sorting itself (so we'd sort _just_ the iteration, not the whole\n> > output). Something like this, though I'm on the fence about whether it\n> > is worth it:\n> \n> We already have a few other tests that sort and compare.  Perhaps it's\n> time for a test_cmp_ignore_order?\n\nThat would be OK with me, but I think it would just replace the calls to\n\"sort\". To do the \"just sort the iteration, but not the rest of the\noutput that I'm talking about above\", the change _has_ to go into\ntest-hashmap.c (but I think I still favor just sorting the whole thing\nfor simplicity).\n\n> And perhaps something like this might even be worth implementing as a\n> diff option?  https://github.com/l0b0/diff-ignore-moved-lines has\n> post-processing script for that..\n\nGiven that we don't actually care that much about looking at the\nresulting diff (and so we don't care about preserving order), I think\njust sorting both sides gives equivalent results and is much simpler.\n\n-Peff\n"},{"id":"379652","messageId":"20190731060449.GE4545@pobox.com","threadId":"51556","inReplyTo":"20190731032735.GA14684@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-07-31T06:04:49Z","receivedAt":"2019-07-31T06:04:55Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Jeff King wrote:\n> On Tue, Jul 30, 2019 at 09:59:17PM -0400, Todd Zullinger wrote:\n>> At the risk of showing my complete lack of knowledge about\n>> these tests, I was wondering if the order mattered for the\n>> other tests in t0011 and t0016.\n[...]\n>> You've got a more comprehensive patch and a proper commit\n>> message, so this is really just a matter of curiosity.\n> \n> I think the order does matter for those ones. E.g., the ones that run\n> \"get\" want to make sure they're seeing the values in the same order in\n> which they were requested.\n\nAhh, thanks for clarifying that (and saving me from sending\nthe version I had which would have incorrectly sorted all\nthe test_{hashmap,oidmap} output. :)\n\nFWIW, I applied your patch for sorting hashmap iterations\n(<20190731012336.GA13880@sigill.intra.peff.net>¹) and ran it\nthrough the Fedora build system.  All architectures passed,\nas expected.\n\n¹ https://public-inbox.org/git/20190731012336.GA13880@sigill.intra.peff.net/\n\n-- \nTodd\n"},{"id":"379692","messageId":"xmqqh872qgz6.fsf@gitster-ct.c.googlers.com","threadId":"51556","inReplyTo":"20190731005933.GA9610@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-31T16:57:49Z","receivedAt":"2019-07-31T16:57:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So regardless of the endian thing above, it probably does make sense for\n> any hashmap iteration output to be sorted before comparing. That goes\n> for t0011, too; it doesn't have this endian thing, but it looks to be\n> relying on hash order that could change if we swapped out hash\n> functions.\n\nSounds sensible.\n"},{"id":"379694","messageId":"xmqq5zniqg36.fsf@gitster-ct.c.googlers.com","threadId":"51556","inReplyTo":"20190731035344.GA26019@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-31T17:17:01Z","receivedAt":"2019-07-31T17:17:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> And here it is for reference with the matching change in test-oidmap,\n> and the adjustments necessary for the test scripts (from master, not\n> from my earlier patch). I think I prefer the simpler \"just sort it all\"\n> version I posted with the commit message.\n\nYeah, let's go with that version.\n\nI am wondering if we should follow suit to certain language's hash\nimplementation to make sure the iteration order is unpredictable to\ncatch bad scripts ;-)  Perhaps that is not worth it, either.\n"},{"id":"379716","messageId":"20190731212258.GB1207@sigill.intra.peff.net","threadId":"51556","inReplyTo":"xmqq5zniqg36.fsf@gitster-ct.c.googlers.com","subject":"non-cryptographic hash algorithms in git","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-31T21:22:58Z","receivedAt":"2019-07-31T21:23:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[breaking the thread since we're way off the original topic, and this\n subject might get more interesting attention]\n\nOn Wed, Jul 31, 2019 at 10:17:01AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > And here it is for reference with the matching change in test-oidmap,\n> > and the adjustments necessary for the test scripts (from master, not\n> > from my earlier patch). I think I prefer the simpler \"just sort it all\"\n> > version I posted with the commit message.\n> \n> Yeah, let's go with that version.\n> \n> I am wondering if we should follow suit to certain language's hash\n> implementation to make sure the iteration order is unpredictable to\n> catch bad scripts ;-)  Perhaps that is not worth it, either.\n\nThat would be a nice side effect, but the real benefit is that it makes\nhash-collision denial-of-service attacks harder. I experimented with\nthis some when I looked at swapping out the xdiff hash algorithm[1] for\nmurmur, siphash, or similar, but I could never get the performance quite\non par with what we have now.\n\nI haven't pursued randomization that much because git's diff engine is\na ready-made DoS machine in the first place. If you're diffing untrusted\ninput, you have to be ready to cut it off after using too much CPU and\nsay \"nope, this one is too big\". That may be less true for our general\npurpose hashmap, though.\n\n-Peff\n\n[1] I didn't dig up the emails, but this was several years ago, when we\n    realized that XDL_FAST_HASH didn't actually work well. I recently\n    found out about xxhash:\n\n      https://cyan4973.github.io/xxHash/\n\n    which looks promising. But I haven't gotten around to plugging it in\n    and timing the result. Anybody else is welcome to beat me to it. :)\n\n    IIRC, one really tricky thing about our diff code is that it finds\n    the newline for each line while it's hashing. That makes it hard to\n    plug in an existing hash implementation without going over each line\n    twice, and many of them perform poorly if you hand them a byte at a\n    time.\n"}]}