{"thread":{"id":"51562","subject":"[BUG]: Testsuite failures on big-endian targets","startedAt":"2019-07-31T06:37:18Z","lastAt":"2019-10-24T17:55:52Z","messageCount":14,"participants":["John Paul Adrian Glaubitz","Todd Zullinger","SZEDER Gábor","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"379653","messageId":"b0bec82e-ad0a-32f6-e2e6-e1f0e6920639@physik.fu-berlin.de","threadId":"51562","inReplyTo":null,"subject":"[BUG]: Testsuite failures on big-endian targets","fromName":"John Paul Adrian Glaubitz","fromEmail":"glaubitz@physik.fu-berlin.de","sentAt":"2019-07-31T06:37:13Z","receivedAt":"2019-07-31T06:37:18Z","isPatch":false,"sender":{"key":"glaubitz@physik.fu-berlin.de","avatar":"https://avatars.githubusercontent.com/u/1647645?v=4"},"body":"Hello!\n\nRecent versions of git are failing the testsuite on big-endian targets\nsuch as s390x in Debian.\n\nBuild logs are:\n\n> https://buildd.debian.org/status/fetch.php?pkg=git&arch=s390x&ver=1%3A2.23.0%7Erc0-1&stamp=1564449102&raw=0\n> https://buildd.debian.org/status/fetch.php?pkg=git&arch=s390x&ver=1%3A2.23.0%7Erc0%2Bnext.20190729-1&stamp=1564449397&raw=0\n\nUnfortunately, I cannot really read from the build logs which test in\nparticular is actually failing as I see a lot of lines starting with\nthe string \"error\".\n\nAccess to big-endian machines such as sparc64 can be retrieved through\nthe gcc compile farm [1].\n\nThanks,\nAdrian\n\n> [1] https://gcc.gnu.org/wiki/CompileFarm\n\n-- \n .''`. John Paul Adrian Glaubitz\n: :' : Debian Developer - glaubitz@debian.org\n`. `' Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de\n `- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913\n\n"},{"id":"379654","messageId":"20190731071755.GF4545@pobox.com","threadId":"51562","inReplyTo":"b0bec82e-ad0a-32f6-e2e6-e1f0e6920639@physik.fu-berlin.de","subject":"Re: [BUG]: Testsuite failures on big-endian targets","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-07-31T07:17:55Z","receivedAt":"2019-07-31T07:18:05Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi,\n\nJohn Paul Adrian Glaubitz wrote:\n> Recent versions of git are failing the testsuite on big-endian targets\n> such as s390x in Debian.\n> \n> Build logs are:\n> \n>> https://buildd.debian.org/status/fetch.php?pkg=git&arch=s390x&ver=1%3A2.23.0%7Erc0-1&stamp=1564449102&raw=0\n>> https://buildd.debian.org/status/fetch.php?pkg=git&arch=s390x&ver=1%3A2.23.0%7Erc0%2Bnext.20190729-1&stamp=1564449397&raw=0\n> \n> Unfortunately, I cannot really read from the build logs which test in\n> particular is actually failing as I see a lot of lines starting with\n> the string \"error\".\n\nThe test I see failing is test 6 in t0016-oidmap. Grepping\nfor '^not ok ' is helpful in this case, though it's even\nbetter when the test summary is provided, as it points to\nthe failing tests by name and number.\n\nThe t0016-oidmap failure is discussed in the thread starting\nhere:\n\n https://public-inbox.org/git/04b301d54715$3b371a90$b1a54fb0$@nexbridge.com/T/\n\nPeff posted a patch which resolves the test failure here:\n\n https://public-inbox.org/git/20190731012336.GA13880@sigill.intra.peff.net/\n\nCheers,\n\n-- \nTodd\n"},{"id":"384459","messageId":"f1ce445e-6954-8e7b-2dca-3a566ce689a5@physik.fu-berlin.de","threadId":"51562","inReplyTo":"20190731071755.GF4545@pobox.com","subject":"Re: [BUG]: Testsuite failures on big-endian targets","fromName":"John Paul Adrian Glaubitz","fromEmail":"glaubitz@physik.fu-berlin.de","sentAt":"2019-10-19T21:38:40Z","receivedAt":"2019-10-19T21:38:47Z","isPatch":false,"sender":{"key":"glaubitz@physik.fu-berlin.de","avatar":"https://avatars.githubusercontent.com/u/1647645?v=4"},"body":"Hi!\n\nOn 7/31/19 9:17 AM, Todd Zullinger wrote:\n>> Build logs are:\n>>\n>>> https://buildd.debian.org/status/fetch.php?pkg=git&arch=s390x&ver=1%3A2.23.0%7Erc0-1&stamp=1564449102&raw=0\n>>> https://buildd.debian.org/status/fetch.php?pkg=git&arch=s390x&ver=1%3A2.23.0%7Erc0%2Bnext.20190729-1&stamp=1564449397&raw=0\n>>\n>> Unfortunately, I cannot really read from the build logs which test in\n>> particular is actually failing as I see a lot of lines starting with\n>> the string \"error\".\n> \n> The test I see failing is test 6 in t0016-oidmap. Grepping\n> for '^not ok ' is helpful in this case, though it's even\n> better when the test summary is provided, as it points to\n> the failing tests by name and number.\n\nThe testsuite is failing again on s390x and all other big-endian targets in\nDebian. For a full build log on s390x see [1].\n\nAdrian\n\n> [1] https://buildd.debian.org/status/fetch.php?pkg=git&arch=s390x&ver=1%3A2.24.0%7Erc0-1&stamp=1571440098&raw=0\n\n-- \n .''`. John Paul Adrian Glaubitz\n: :' : Debian Developer - glaubitz@debian.org\n`. `' Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de\n `- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913\n"},{"id":"384462","messageId":"20191019233706.GM29845@szeder.dev","threadId":"51562","inReplyTo":"f1ce445e-6954-8e7b-2dca-3a566ce689a5@physik.fu-berlin.de","subject":"[PATCH] test-progress: fix test failures on big-endian systems","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-10-19T23:37:06Z","receivedAt":"2019-10-19T23:37:12Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sat, Oct 19, 2019 at 11:38:40PM +0200, John Paul Adrian Glaubitz wrote:\n> The testsuite is failing again on s390x and all other big-endian targets in\n> Debian. For a full build log on s390x see [1].\n\nGah, my progress display fixes strike again...\n\nI think the patch below should fix it, but I could only test it on\nlittle-endian systems. Could you please confirm that it indeed works\non big-endian as well?\n\n\n --- >8 ---\n\nSubject: [PATCH] test-progress: fix test failures on big-endian systems\n\nIn 't0500-progress-display.sh' all tests running 'test-tool progress\n--total=' fail on big-endian systems, e.g. like this:\n\n + test-tool progress --total=3 Working hard\n [...]\n + test_i18ncmp expect out\n --- expect\t2019-10-18 23:07:54.765523916 +0000\n +++ out\t2019-10-18 23:07:54.773523916 +0000\n @@ -1,4 +1,2 @@\n -Working hard: 33% (1/3)\n -Working hard: 66% (2/3)\n -Working hard: 100% (3/3)\n -Working hard: 100% (3/3), done.\n +Working hard: 0% (1/12884901888)\n +Working hard: 0% (3/12884901888), done.\n\nThe reason for that bogus value is that '--total's parameter is parsed\nvia parse-options's OPT_INTEGER into a uint64_t variable [1], so the\ntwo bits of 3 end up in the \"wrong\" bytes on big-endian systems\n(12884901888 = 0x300000000).\n\nChange the type of that variable from uint64_t to int, to match what\nparse-options expects; in the tests of the progress output we won't\nuse values that don't fit into an int anyway.\n\n[1] start_progress() expects the total number as an uint64_t, that's\n why I chose the same type when declaring the variable holding the\n value given on the command line.\n\nSigned-off-by: SZEDER Gábor \n---\n t/helper/test-progress.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/helper/test-progress.c b/t/helper/test-progress.c\nindex 4e9f7fafdf..42b96cb103 100644\n--- a/t/helper/test-progress.c\n+++ b/t/helper/test-progress.c\n@@ -29,7 +29,7 @@ void progress_test_force_update(void);\n \n int cmd__progress(int argc, const char **argv)\n {\n-\tuint64_t total = 0;\n+\tint total = 0;\n \tconst char *title;\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct progress *progress;\n-- \n2.24.0.rc0.472.ga6f06c86b4\n\n\n"},{"id":"384463","messageId":"f0d216a8-95ee-bdec-4116-012906117aad@physik.fu-berlin.de","threadId":"51562","inReplyTo":"20191019233706.GM29845@szeder.dev","subject":"Re: [PATCH] test-progress: fix test failures on big-endian systems","fromName":"John Paul Adrian Glaubitz","fromEmail":"glaubitz@physik.fu-berlin.de","sentAt":"2019-10-19T23:55:52Z","receivedAt":"2019-10-19T23:57:13Z","isPatch":true,"sender":{"key":"glaubitz@physik.fu-berlin.de","avatar":"https://avatars.githubusercontent.com/u/1647645?v=4"},"body":"Hi Gábor!\n\nOn 10/20/19 1:37 AM, SZEDER Gábor wrote:\n> On Sat, Oct 19, 2019 at 11:38:40PM +0200, John Paul Adrian Glaubitz wrote:\n>> The testsuite is failing again on s390x and all other big-endian targets in\n>> Debian. For a full build log on s390x see [1].\n> \n> Gah, my progress display fixes strike again...\n> \n> I think the patch below should fix it, but I could only test it on\n> little-endian systems. Could you please confirm that it indeed works\n> on big-endian as well?\n> \n> \n> --- >8 ---\n> \n> Subject: [PATCH] test-progress: fix test failures on big-endian systems\n> \n> In 't0500-progress-display.sh' all tests running 'test-tool progress\n> --total=' fail on big-endian systems, e.g. like this:\n> \n> + test-tool progress --total=3 Working hard\n> [...]\n> + test_i18ncmp expect out\n> --- expect\t2019-10-18 23:07:54.765523916 +0000\n> +++ out\t2019-10-18 23:07:54.773523916 +0000\n> @@ -1,4 +1,2 @@\n> -Working hard: 33% (1/3)\n> -Working hard: 66% (2/3)\n> -Working hard: 100% (3/3)\n> -Working hard: 100% (3/3), done.\n> +Working hard: 0% (1/12884901888)\n> +Working hard: 0% (3/12884901888), done.\n> \n> The reason for that bogus value is that '--total's parameter is parsed\n> via parse-options's OPT_INTEGER into a uint64_t variable [1], so the\n> two bits of 3 end up in the \"wrong\" bytes on big-endian systems\n> (12884901888 = 0x300000000).\n> \n> Change the type of that variable from uint64_t to int, to match what\n> parse-options expects; in the tests of the progress output we won't\n> use values that don't fit into an int anyway.\n> \n> [1] start_progress() expects the total number as an uint64_t, that's\n> why I chose the same type when declaring the variable holding the\n> value given on the command line.\n> \n> Signed-off-by: SZEDER Gábor \n> ---\n> t/helper/test-progress.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c\n> index 4e9f7fafdf..42b96cb103 100644\n> --- a/t/helper/test-progress.c\n> +++ b/t/helper/test-progress.c\n> @@ -29,7 +29,7 @@ void progress_test_force_update(void);\n> \n> int cmd__progress(int argc, const char **argv)\n> {\n> -\tuint64_t total = 0;\n> +\tint total = 0;\n> \tconst char *title;\n> \tstruct strbuf line = STRBUF_INIT;\n> \tstruct progress *progress;\n> \n\nI can confirm that your patch fixes the testsuite for me on Debian\nunstable/ppc64 (big-endian).\n\nTested-By: John Paul Adrian Glaubitz \n\nThanks,\nAdrian\n\n-- \n .''`. John Paul Adrian Glaubitz\n: :' : Debian Developer - glaubitz@debian.org\n`. `' Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de\n `- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913\n"},{"id":"384464","messageId":"20191020001959.GY10893@pobox.com","threadId":"51562","inReplyTo":"f0d216a8-95ee-bdec-4116-012906117aad@physik.fu-berlin.de","subject":"Re: [PATCH] test-progress: fix test failures on big-endian systems","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-10-20T00:19:59Z","receivedAt":"2019-10-20T00:20:09Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi,\n\nJohn Paul Adrian Glaubitz wrote:\n> Hi Gábor!\n> \n> On 10/20/19 1:37 AM, SZEDER Gábor wrote:\n>> On Sat, Oct 19, 2019 at 11:38:40PM +0200, John Paul Adrian Glaubitz wrote:\n>>> The testsuite is failing again on s390x and all other big-endian targets in\n>>> Debian. For a full build log on s390x see [1].\n>> \n>> Gah, my progress display fixes strike again...\n>> \n>> I think the patch below should fix it, but I could only test it on\n>> little-endian systems. Could you please confirm that it indeed works\n>> on big-endian as well?\n[...]\n> I can confirm that your patch fixes the testsuite for me on Debian\n> unstable/ppc64 (big-endian).\n> \n> Tested-By: John Paul Adrian Glaubitz \n\nYep, that worked well on the Fedora s390x builders as well\n(unsurprisingly).\n\nThanks!\n\n-- \nTodd\n"},{"id":"384465","messageId":"20191020002648.GZ10893@pobox.com","threadId":"51562","inReplyTo":"f1ce445e-6954-8e7b-2dca-3a566ce689a5@physik.fu-berlin.de","subject":"Re: [BUG]: Testsuite failures on big-endian targets","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-10-20T00:26:48Z","receivedAt":"2019-10-20T00:26:57Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hello,\n\n[+cc: Ævar]\n\nJohn Paul Adrian Glaubitz wrote:\n> The testsuite is failing again on s390x and all other big-endian targets in\n> Debian. For a full build log on s390x see [1].\n> \n> Adrian\n> \n>> [1] https://buildd.debian.org/status/fetch.php?pkg=git&arch=s390x&ver=1%3A2.24.0%7Erc0-1&stamp=1571440098&raw=0\n\nWith t0500 resolved by <20191019233706.GM29845@szeder.dev>,\nthat just leaves the one failure in t7812.\n\n Test Summary Report\n -------------------\n t7812-grep-icase-non-ascii.sh (Wstat: 256 Tests: 11 Failed: 1)\n Failed test: 11\n Non-zero exit status: 1\n Files=879, Tests=21880, 404 wallclock secs ( 3.38 usr 1.15 sys + 440.87 cusr 729.29 csys = 1174.69 CPU)\n Result: FAIL\n\nThe failing test output:\n\n expecting success of 7812.11 'PCRE v2: grep non-ASCII from invalid UTF-8 data with -i': \n test_might_fail git grep -hi \"Æ\" invalid-0x80 >actual &&\n test_cmp expected actual &&\n test_must_fail git grep -hi \"(*NO_JIT)Æ\" invalid-0x80 &&\n test_cmp expected actual\n ++ test_might_fail git grep -hi Æ invalid-0x80\n ++ test_must_fail ok=success git grep -hi Æ invalid-0x80\n ++ case \"$1\" in\n ++ _test_ok=success\n ++ shift\n ++ git grep -hi Æ invalid-0x80\n fatal: pcre2_match failed with error code -22: UTF-8 error: isolated byte with 0x80 bit set\n ++ exit_code=128\n ++ test 128 -eq 0\n ++ test_match_signal 13 128\n ++ test 128 = 141\n ++ test 128 = 269\n ++ return 1\n ++ test 128 -gt 129\n ++ test 128 -eq 127\n ++ test 128 -eq 126\n ++ return 0\n ++ test_cmp expected actual\n ++ diff -u expected actual\n --- expected 2019-10-19 21:56:08.634252012 +0000\n +++ actual 2019-10-19 21:56:08.714252012 +0000\n @@ -1 +0,0 @@\n -ævar\n error: last command exited with $?=1\n not ok 11 - PCRE v2: grep non-ASCII from invalid UTF-8 data with -i\n # \n # test_might_fail git grep -hi \"Æ\" invalid-0x80 >actual &&\n # test_cmp expected actual &&\n # test_must_fail git grep -hi \"(*NO_JIT)Æ\" invalid-0x80 &&\n # test_cmp expected actual\n # \n # failed 1 among 11 test(s)\n\nI'm not flush on time to even try to look much; but I'd be\nkidding myself if I said I was likely to find the issue\nquickly. ;)\n\nBut I'm pretty sure it will be obvious to someone here.\n\n-- \nTodd\n"},{"id":"384482","messageId":"xmqq36fmor7o.fsf@gitster-ct.c.googlers.com","threadId":"51562","inReplyTo":"20191019233706.GM29845@szeder.dev","subject":"Re: [PATCH] test-progress: fix test failures on big-endian systems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-21T00:52:11Z","receivedAt":"2019-10-21T00:52:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> The reason for that bogus value is that '--total's parameter is parsed\n> via parse-options's OPT_INTEGER into a uint64_t variable [1]...\n>\n> Change the type of that variable from uint64_t to int, to match what\n> parse-options expects; in the tests of the progress output we won't\n> use values that don't fit into an int anyway.\n\nOK, so when the call to start_progress() is made, the second\nargument (i.e. \"total\" which now is int) is promoted to what the\ncallee expects, so there needs no other change. Makes sense.\n\n> [1] start_progress() expects the total number as an uint64_t, that's\n> why I chose the same type when declaring the variable holding the\n> value given on the command line.\n\nI can sympathize, but I do not think it is worth inventing OPT_U64()\nor adding \"int total_i\" whose value is assigned to \"u64 total\" after\nparsing a command line arg with OPT_INTEGER() into the former.\n\nCatching a pointer whose type is not \"int*\" passed at the third\nposition of OPT_INTGER() mechanically may be worth it, though.\nWould Coccinelle be a suitable tool for that kind of thing?\n\n> int cmd__progress(int argc, const char **argv)\n> {\n> -\tuint64_t total = 0;\n> +\tint total = 0;\n> \tconst char *title;\n> \tstruct strbuf line = STRBUF_INIT;\n> \tstruct progress *progress;\n"},{"id":"384490","messageId":"20191021032144.GB13083@sigill.intra.peff.net","threadId":"51562","inReplyTo":"xmqq36fmor7o.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] test-progress: fix test failures on big-endian systems","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-21T03:21:44Z","receivedAt":"2019-10-21T03:21:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 21, 2019 at 09:52:11AM +0900, Junio C Hamano wrote:\n\n> I can sympathize, but I do not think it is worth inventing OPT_U64()\n> or adding \"int total_i\" whose value is assigned to \"u64 total\" after\n> parsing a command line arg with OPT_INTEGER() into the former.\n> \n> Catching a pointer whose type is not \"int*\" passed at the third\n> position of OPT_INTGER() mechanically may be worth it, though.\n> Would Coccinelle be a suitable tool for that kind of thing?\n\nI wondered if we could be a bit more clever with the definition of\n\"struct option\". Something like:\n\ndiff --git a/parse-options.h b/parse-options.h\nindex 38a33a087e..99c7ff466d 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -126,7 +126,10 @@ struct option {\n \tenum parse_opt_type type;\n \tint short_name;\n \tconst char *long_name;\n-\tvoid *value;\n+\tunion {\n+\t\tint *intp;\n+\t\tconst char *strp;\n+\t} value;\n \tconst char *argh;\n \tconst char *help;\n \n\nwhich would let the compiler complain about the type mismatch (of course\nit can't help you if you assign to \"intp\" while trying to parse a\nstring).\n\nInitializing the union from a compound literal becomes more painful,\nbut:\n\n 1. That's mostly hidden behind OPT_INTEGER(), etc.\n\n 2. I think we're OK with named initializers these days. I.e., I think:\n\n { OPTION_INTEGER, 'f', \"--foo\", { .intp = &foo } }\n\n would work OK.\n\nI didn't even try compiling to see how painful the fallout might be,\nthough.\n\n-Peff\n"},{"id":"384494","messageId":"xmqqftjmlbvb.fsf@gitster-ct.c.googlers.com","threadId":"51562","inReplyTo":"20191021032144.GB13083@sigill.intra.peff.net","subject":"Re: [PATCH] test-progress: fix test failures on big-endian systems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-21T08:51:52Z","receivedAt":"2019-10-21T08:51:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I wondered if we could be a bit more clever with the definition of\n> \"struct option\". Something like:\n>\n> diff --git a/parse-options.h b/parse-options.h\n> index 38a33a087e..99c7ff466d 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -126,7 +126,10 @@ struct option {\n> \tenum parse_opt_type type;\n> \tint short_name;\n> \tconst char *long_name;\n> -\tvoid *value;\n> +\tunion {\n> +\t\tint *intp;\n> +\t\tconst char *strp;\n> +\t} value;\n> \tconst char *argh;\n> \tconst char *help;\n> \n>\n> which would let the compiler complain about the type mismatch (of course\n> it can't help you if you assign to \"intp\" while trying to parse a\n> string).\n>\n> Initializing the union from a compound literal becomes more painful,\n> but:\n>\n> 1. That's mostly hidden behind OPT_INTEGER(), etc.\n>\n> 2. I think we're OK with named initializers these days. I.e., I think:\n>\n> { OPTION_INTEGER, 'f', \"--foo\", { .intp = &foo } }\n>\n> would work OK.\n\nThe side that actually use .vale would need to change for obvious\nreasons, which may be painful, but I agree it would have easily\nprevented the regression from happening in the first place.\n\nThanks for a food for thought.\n"},{"id":"384549","messageId":"20191021184954.GA2526@sigill.intra.peff.net","threadId":"51562","inReplyTo":"xmqqftjmlbvb.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] test-progress: fix test failures on big-endian systems","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-21T18:49:55Z","receivedAt":"2019-10-21T18:49:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 21, 2019 at 05:51:52PM +0900, Junio C Hamano wrote:\n\n> > -\tvoid *value;\n> > +\tunion {\n> > +\t\tint *intp;\n> > +\t\tconst char *strp;\n> > +\t} value;\n> [...]\n> The side that actually use .vale would need to change for obvious\n> reasons, which may be painful, but I agree it would have easily\n> prevented the regression from happening in the first place.\n\nI was curious just how painful, so here's what I found.\n\nThe conversion is indeed annoying. There are 330 sites that need\ntouched to handle the switch to a union (both declarations and places\nthat access the variables).\n\nMost of the declarations are hidden by the OPT_*() macros, but there's a\nfair bit of changes like this sprinkled around:\n\n@@ -4952,13 +4952,13 @@ int apply_parse_options(int argc, const char **argv,\n const char * const *apply_usage)\n {\n struct option builtin_apply_options[] = {\n- { OPTION_CALLBACK, 0, \"exclude\", state, N_(\"path\"),\n+ { OPTION_CALLBACK, 0, \"exclude\", { .voidp = state }, N_(\"path\"),\n N_(\"don't apply changes matching the given path\"),\n PARSE_OPT_NONEG, apply_option_parse_exclude },\n- { OPTION_CALLBACK, 0, \"include\", state, N_(\"path\"),\n+ { OPTION_CALLBACK, 0, \"include\", { .voidp = state }, N_(\"path\"),\n N_(\"apply changes matching the given path\"),\n PARSE_OPT_NONEG, apply_option_parse_include },\n- { OPTION_CALLBACK, 'p', NULL, state, N_(\"num\"),\n+ { OPTION_CALLBACK, 'p', NULL, { .voidp = state }, N_(\"num\"),\n N_(\"remove leading slashes from traditional diff paths\"),\n 0, apply_option_parse_p },\n OPT_BOOL(0, \"no-add\", &state->no_add,\n\nwhich is strictly worse syntactically, and doesn't give us any type\nsafety (and won't ever, because parse-options is never going to learn\nabout \"struct apply_state\").\n\nLikewise the access side gets slightly uglier, but not too bad:\n\n@@ -4768,7 +4768,7 @@ static int apply_patch(struct apply_state *state,\n static int apply_option_parse_exclude(const struct option *opt,\n const char *arg, int unset)\n {\n- struct apply_state *state = opt->value;\n+ struct apply_state *state = opt->value.voidp;\n \n BUG_ON_OPT_NEG(unset);\n \n\nFor things that actually use intp, I think the access side is fine (and\npossibly even slightly nicer):\n\n@@ -101,65 +101,65 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \n case OPTION_BIT:\n if (unset)\n- *(int *)opt->value &= ~opt->defval;\n+ *opt->value.intp &= ~opt->defval;\n else\n- *(int *)opt->value |= opt->defval;\n+ *opt->value.intp |= opt->defval;\n return 0;\n \n\nThe declaration side is mostly handled by OPT_INTEGER(), etc, but\nhand-written ones still need to adjust as you'd expect:\n\n@@ -298,7 +298,7 @@ static struct option builtin_add_options[] = {\n OPT_BOOL(0, \"renormalize\", &add_renormalize, N_(\"renormalize EOL of tracked files (implies -u)\")),\n OPT_BOOL('N', \"intent-to-add\", &intent_to_add, N_(\"record only the fact that the path will be added later\")),\n OPT_BOOL('A', \"all\", &addremove_explicit, N_(\"add changes from all tracked and untracked files\")),\n- { OPTION_CALLBACK, 0, \"ignore-removal\", &addremove_explicit,\n+ { OPTION_CALLBACK, 0, \"ignore-removal\", { .intp = &addremove_explicit },\n NULL /* takes no arguments */,\n N_(\"ignore paths removed in the working tree (same as --no-all)\"),\n PARSE_OPT_NOARG, ignore_removal_cb },\n\nThat's ugly, but at least we're getting some type safety out of it.\n\nBut here's where it gets tricky. In addition to catching any size\nmismatches, this will also catch signedness problems. I.e., if we make\nOPT_INTEGER() use \"intp\", then everybody passing in &unsigned_var now\ngets a compiler warning. Which maybe is a good thing, I dunno. But it\ntriggers a lot of warnings. We probably ought to be using a \"uintp\" for\nOPT_BIT(), etc, but that just complains about callers passing in signed\nintegers. ;)\n\nSo that's where I gave up. Converting between signed and unsigned\nvariables needs to be done very carefully, as there are often subtle\nimpacts (e.g., loop terminations). And because we have so many sign\nissues already, compiling with \"-Wsign-compare\", etc, isn't likely to\nhelp.\n\nBut if anybody wants to take a stab at it, the work I've done so far is\ncan be fetched from:\n\n https://github.com/peff/git jk/parseopt-intp-wip\n\n-Peff\n"},{"id":"384659","messageId":"xmqqpniojk8z.fsf@gitster-ct.c.googlers.com","threadId":"51562","inReplyTo":"20191021184954.GA2526@sigill.intra.peff.net","subject":"Re: [PATCH] test-progress: fix test failures on big-endian systems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-23T01:58:20Z","receivedAt":"2019-10-23T01:58:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But here's where it gets tricky. In addition to catching any size\n> mismatches, this will also catch signedness problems. I.e., if we make\n> OPT_INTEGER() use \"intp\", then everybody passing in &unsigned_var now\n> gets a compiler warning. Which maybe is a good thing, I dunno.\n\nHmph, true. I'd agree with back-burnering it for now. \n\nPerhaps we'd fix the signedness issue one by one in a preparatory\nseries before converting the value field to a union, if we want to\npursue this idea further (in which I am mildly interested, by the\nway), but it does sound like it should be given lower priority.\n\n> So that's where I gave up. Converting between signed and unsigned\n> variables needs to be done very carefully, as there are often subtle\n> impacts (e.g., loop terminations). And because we have so many sign\n> issues already, compiling with \"-Wsign-compare\", etc, isn't likely to\n> help.\n\nTrue.\n\nThanks.\n"},{"id":"384778","messageId":"20191024172442.GM4348@szeder.dev","threadId":"51562","inReplyTo":"xmqq36fmor7o.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] test-progress: fix test failures on big-endian systems","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-10-24T17:24:42Z","receivedAt":"2019-10-24T17:24:50Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Oct 21, 2019 at 09:52:11AM +0900, Junio C Hamano wrote:\n> I can sympathize, but I do not think it is worth inventing OPT_U64()\n> or adding \"int total_i\" whose value is assigned to \"u64 total\" after\n> parsing a command line arg with OPT_INTEGER() into the former.\n\nI agree, we should wait for the first real use case where specifying a\nlarger-than-32bit integer actually makes sense in practice.\n\n> Catching a pointer whose type is not \"int*\" passed at the third\n> position of OPT_INTGER() mechanically may be worth it, though.\n> Would Coccinelle be a suitable tool for that kind of thing?\n\nThe semantic patch below will do that, but this is one of those \"I\ndon't have the slightest idea what I am doing\" patches...\n\nIt's output looks like this when applied to an older version without\nthe big-endian fix upthread:\n\n potential error at apply.c:4982:26:\n passing variable 'state -> p_context' of type 'unsigned int' to OPT_INTEGER\n OPT_INTEGER expects an int\n potential error at builtin/column.c:29:30:\n passing variable 'colopts' of type 'unsigned int' to OPT_INTEGER\n OPT_INTEGER expects an int\n potential error at builtin/column.c:32:24:\n passing variable 'copts . nl' of type 'const char *' to OPT_INTEGER\n OPT_INTEGER expects an int\n potential error at builtin/grep.c:884:38:\n passing variable 'opt . pre_context' of type 'unsigned' to OPT_INTEGER\n OPT_INTEGER expects an int\n potential error at builtin/grep.c:886:37:\n passing variable 'opt . post_context' of type 'unsigned' to OPT_INTEGER\n OPT_INTEGER expects an int\n potential error at builtin/upload-pack.c:28:29:\n passing variable 'opts . timeout' of type 'unsigned int' to OPT_INTEGER\n OPT_INTEGER expects an int\n potential error at t/helper/test-progress.c:42:27:\n passing variable 'total' of type 'uint64_t' to OPT_INTEGER\n OPT_INTEGER expects an int\n\n https://travis-ci.org/szeder/git/jobs/602423358#L436\n\nI think most of them are harmless, like the number of context lines in\napply and grep, or the timeout in seconds in upload-pack. So I think\nthe semantic patch should allow 'unsigned' and 'unsigned int' as well.\n\nBut note the one in 'builtin/column.c', where we pass a 'const char\n*' to OPT_INTEGER. That can't possibly be good; I suspect copy-paste\nerror and it should have been OPT_STRING.\n\n\n --- >8 ---\n\nSubject: [PATCH] coccinelle: warn about passing a non-int to parse-options'\n OPT_INTEGER\n\nparse-options' OPT_INTEGER wants to parse an integer argument into a\nvariable of type 'int', and passing e.g. an 'uint64_t' causes troubles\n[1].\n\nAdd a Coccinelle semantic patch that checks the type of the variable\nwhere the integer argument should be parsed into, and print an error\nif that variable is not of type 'int'.\n\nNote that this semantic patch won't result in a proper and applicable\npatch, because who knows where that variable of the inappropriate type\nis defined. However, the printed error message will still cause our\nstatic analysis CI jobs to fail, drawing our attention to the issue.\n\nTODO: refusing an 'unsigned int' might be unnecessarily harsh...\n\n[1] 11a803d861 (test-progress: fix test failures on big-endian\n systems, 2019-10-20)\n\nSigned-off-by: SZEDER Gábor \n---\n contrib/coccinelle/parse-options.cocci | 18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n create mode 100644 contrib/coccinelle/parse-options.cocci\n\ndiff --git a/contrib/coccinelle/parse-options.cocci b/contrib/coccinelle/parse-options.cocci\nnew file mode 100644\nindex 0000000000..e0cddef421\n--- /dev/null\n+++ b/contrib/coccinelle/parse-options.cocci\n@@ -0,0 +1,18 @@\n+@ optint @\n+identifier opts;\n+type T;\n+T var;\n+expression SHORT, LONG, HELP;\n+position p;\n+@@\n+struct option opts[] = { ..., OPT_INTEGER(SHORT, LONG, &var@p, HELP), ...};\n+\n+@ script:python @\n+p << optint.p;\n+var << optint.var;\n+vartype << optint.T;\n+@@\n+if vartype != \"int\":\n+\tprint \"potential error at %s:%s:%s:\" % (p[0].file, p[0].line, p[0].column)\n+\tprint \" passing variable '%s' of type '%s' to OPT_INTEGER\" % (var, vartype)\n+\tprint \" OPT_INTEGER expects an int\"\n-- \n2.24.0.rc0.502.g7008375535\n\n\n"},{"id":"384779","messageId":"20191024175549.GA12892@sigill.intra.peff.net","threadId":"51562","inReplyTo":"20191024172442.GM4348@szeder.dev","subject":"Re: [PATCH] test-progress: fix test failures on big-endian systems","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-24T17:55:49Z","receivedAt":"2019-10-24T17:55:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 24, 2019 at 07:24:42PM +0200, SZEDER Gábor wrote:\n\n> On Mon, Oct 21, 2019 at 09:52:11AM +0900, Junio C Hamano wrote:\n> > I can sympathize, but I do not think it is worth inventing OPT_U64()\n> > or adding \"int total_i\" whose value is assigned to \"u64 total\" after\n> > parsing a command line arg with OPT_INTEGER() into the former.\n> \n> I agree, we should wait for the first real use case where specifying a\n> larger-than-32bit integer actually makes sense in practice.\n\nAnybody who wants to refer to a file or memory size would want that. We\nhave a few cases in pack-objects already, but they use OPT_MAGNITUDE()\ninstead. Which makes sense. Anything we expect to be that gigantic would\nwant to have the shorthand to say \"4G\" or whatever.\n\n(As a side note, I notice that OPT_MAGNITUDE uses \"unsigned long\", which\nprobably needs to be adjusted for Windows. Dealing with all of the\nvoid-pointer type-punning fallout from that will be fun. :) ).\n\n> It's output looks like this when applied to an older version without\n> the big-endian fix upthread:\n> \n> potential error at apply.c:4982:26:\n> passing variable 'state -> p_context' of type 'unsigned int' to OPT_INTEGER\n> OPT_INTEGER expects an int\n\nLooks like this found a lot of the same issues my \"intp\" conversion\nfound. My recollection is that mine found more, but it might just have\nbeen that the compiler's warning output is rather verbose. TBH, I didn't\ncarefully catalog them; as soon as I saw the problem, I went to bed in\ndisgust. ;)\n\n> diff --git a/contrib/coccinelle/parse-options.cocci b/contrib/coccinelle/parse-options.cocci\n> new file mode 100644\n> index 0000000000..e0cddef421\n> --- /dev/null\n> +++ b/contrib/coccinelle/parse-options.cocci\n> @@ -0,0 +1,18 @@\n> +@ optint @\n> +identifier opts;\n> +type T;\n> +T var;\n> +expression SHORT, LONG, HELP;\n> +position p;\n> +@@\n> +struct option opts[] = { ..., OPT_INTEGER(SHORT, LONG, &var@p, HELP), ...};\n> +\n> +@ script:python @\n> +p << optint.p;\n> +var << optint.var;\n> +vartype << optint.T;\n> +@@\n\nI avoided relying on the python interpreter for previous cocci patches,\nsince some builds don't have it (IIRC, the one in Debian experimental\ndoesn't, but that one has not graduated even to \"unstable\" after several\nyears, so maybe it's not worth worrying about).\n\nIf we do start using python, we might want to revisit 4d168e742a\n(coccinelle: use <...> for function exclusion, 2018-08-28), which\ndescribes a faster python alternative in the commit message.\n\n-Peff\n"}]}