{"thread":{"id":"61737","subject":"[PATCH] t-strvec: use test_msg()","startedAt":"2024-07-04T18:04:27Z","lastAt":"2024-07-16T16:43:22Z","messageCount":20,"participants":["René Scharfe","Eric Sunshine","Phillip Wood","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"498091","messageId":"35b0ba6b-d485-44f2-a19f-3ce816f8b435@web.de","threadId":"61737","inReplyTo":null,"subject":"[PATCH] t-strvec: use test_msg()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-07-04T18:04:14Z","receivedAt":"2024-07-04T18:04:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"check_strvec_loc() checks each the strvec item by looping through them\nand comparing them with expected values.  If a check fails then we'd\nlike to know which item is affected.  It reports that information by\nbuilding a strbuf and delivering its contents using a failing assertion.\n\nHere's an example in which there are less items in the strvec than\nexpected; the index of the missing item is reported in the last line:\n\n   # check \"vec->nr > nr\" failed at t/unit-tests/t-strvec.c:19\n   #    left: 1\n   #   right: 1\n   # check \"strvec index 1\" failed at t/unit-tests/t-strvec.c:71\n\nNote that it is also reported in the third line, i.e. the variable\n\"nr\" contains that index.\n\nStop printing the index explicitly for checks that already report it.\nThe message for the same condition as above becomes:\n\n   # check \"vec->nr > nr\" failed at t/unit-tests/t-strvec.c:19\n   #    left: 1\n   #   right: 1\n\nFor the string comparison, whose error message doesn't include the\nindex, report it using the simpler and more appropriate test_msg()\ninstead.  Report the index using its actual name and format the line\nlike the preceding ones.  The message for an unexpected string value\nbecomes:\n\n   # check \"!strcmp(vec->v[nr], str)\" failed at t/unit-tests/t-strvec.c:24\n   #    left: \"foo\"\n   #   right: \"bar\"\n   #      nr: 0\n\nReported-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n t/unit-tests/t-strvec.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/unit-tests/t-strvec.c b/t/unit-tests/t-strvec.c\nindex d4615ab06d..236203af61 100644\n--- a/t/unit-tests/t-strvec.c\n+++ b/t/unit-tests/t-strvec.c\n@@ -17,12 +17,12 @@ static void check_strvec_loc(const char *loc, struct strvec *vec, ...)\n \t\t\tbreak;\n\n \t\tif (!check_uint(vec->nr, >, nr) ||\n-\t\t    !check_uint(vec->alloc, >, nr) ||\n-\t\t    !check_str(vec->v[nr], str)) {\n-\t\t\tstruct strbuf msg = STRBUF_INIT;\n-\t\t\tstrbuf_addf(&msg, \"strvec index %\"PRIuMAX, (uintmax_t) nr);\n-\t\t\ttest_assert(loc, msg.buf, 0);\n-\t\t\tstrbuf_release(&msg);\n+\t\t    !check_uint(vec->alloc, >, nr)) {\n+\t\t\tva_end(ap);\n+\t\t\treturn;\n+\t\t}\n+\t\tif (!check_str(vec->v[nr], str)) {\n+\t\t\ttest_msg(\"     nr: %\"PRIuMAX, (uintmax_t)nr);\n \t\t\tva_end(ap);\n \t\t\treturn;\n \t\t}\n--\n2.45.2\n"},{"id":"498092","messageId":"CAPig+cSHoHVUGdEDk2h5updMqaPqQMkAvypFLK+S2qRYhw=CWw@mail.gmail.com","threadId":"61737","inReplyTo":"35b0ba6b-d485-44f2-a19f-3ce816f8b435@web.de","subject":"Re: [PATCH] t-strvec: use test_msg()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-07-04T18:31:33Z","receivedAt":"2024-07-04T18:31:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 4, 2024 at 2:04 PM René Scharfe <l.s.r@web.de> wrote:\n> check_strvec_loc() checks each the strvec item by looping through them\n\ns/each the/each/\n\n> and comparing them with expected values.  If a check fails then we'd\n> like to know which item is affected.  It reports that information by\n> building a strbuf and delivering its contents using a failing assertion.\n>\n> Here's an example in which there are less items in the strvec than\n\ns/less/fewer/\n\n> expected; the index of the missing item is reported in the last line:\n>\n>    # check \"vec->nr > nr\" failed at t/unit-tests/t-strvec.c:19\n>    #    left: 1\n>    #   right: 1\n>    # check \"strvec index 1\" failed at t/unit-tests/t-strvec.c:71\n>\n> Note that it is also reported in the third line, i.e. the variable\n> \"nr\" contains that index.\n>\n> Stop printing the index explicitly for checks that already report it.\n> The message for the same condition as above becomes:\n>\n>    # check \"vec->nr > nr\" failed at t/unit-tests/t-strvec.c:19\n>    #    left: 1\n>    #   right: 1\n>\n> For the string comparison, whose error message doesn't include the\n> index, report it using the simpler and more appropriate test_msg()\n> instead.  Report the index using its actual name and format the line\n> like the preceding ones.  The message for an unexpected string value\n> becomes:\n>\n>    # check \"!strcmp(vec->v[nr], str)\" failed at t/unit-tests/t-strvec.c:24\n>    #    left: \"foo\"\n>    #   right: \"bar\"\n>    #      nr: 0\n>\n> Reported-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n\nMissing sign-off.\n"},{"id":"498105","messageId":"3add9624-4f3a-4f70-bc15-2357920dbc9f@gmail.com","threadId":"61737","inReplyTo":"35b0ba6b-d485-44f2-a19f-3ce816f8b435@web.de","subject":"Re: [PATCH] t-strvec: use test_msg()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-05T09:44:40Z","receivedAt":"2024-07-05T09:44:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi René\n\nThanks for working on this, this looks good modulo Eric's suggestions\n\nBest Wishes\n\nPhillip\n\nOn 04/07/2024 19:04, René Scharfe wrote:\n> check_strvec_loc() checks each the strvec item by looping through them\n> and comparing them with expected values.  If a check fails then we'd\n> like to know which item is affected.  It reports that information by\n> building a strbuf and delivering its contents using a failing assertion.\n> \n> Here's an example in which there are less items in the strvec than\n> expected; the index of the missing item is reported in the last line:\n> \n>     # check \"vec->nr > nr\" failed at t/unit-tests/t-strvec.c:19\n>     #    left: 1\n>     #   right: 1\n>     # check \"strvec index 1\" failed at t/unit-tests/t-strvec.c:71\n> \n> Note that it is also reported in the third line, i.e. the variable\n> \"nr\" contains that index.\n> \n> Stop printing the index explicitly for checks that already report it.\n> The message for the same condition as above becomes:\n> \n>     # check \"vec->nr > nr\" failed at t/unit-tests/t-strvec.c:19\n>     #    left: 1\n>     #   right: 1\n> \n> For the string comparison, whose error message doesn't include the\n> index, report it using the simpler and more appropriate test_msg()\n> instead.  Report the index using its actual name and format the line\n> like the preceding ones.  The message for an unexpected string value\n> becomes:\n> \n>     # check \"!strcmp(vec->v[nr], str)\" failed at t/unit-tests/t-strvec.c:24\n>     #    left: \"foo\"\n>     #   right: \"bar\"\n>     #      nr: 0\n> \n> Reported-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>   t/unit-tests/t-strvec.c | 12 ++++++------\n>   1 file changed, 6 insertions(+), 6 deletions(-)\n> \n> diff --git a/t/unit-tests/t-strvec.c b/t/unit-tests/t-strvec.c\n> index d4615ab06d..236203af61 100644\n> --- a/t/unit-tests/t-strvec.c\n> +++ b/t/unit-tests/t-strvec.c\n> @@ -17,12 +17,12 @@ static void check_strvec_loc(const char *loc, struct strvec *vec, ...)\n>   \t\t\tbreak;\n> \n>   \t\tif (!check_uint(vec->nr, >, nr) ||\n> -\t\t    !check_uint(vec->alloc, >, nr) ||\n> -\t\t    !check_str(vec->v[nr], str)) {\n> -\t\t\tstruct strbuf msg = STRBUF_INIT;\n> -\t\t\tstrbuf_addf(&msg, \"strvec index %\"PRIuMAX, (uintmax_t) nr);\n> -\t\t\ttest_assert(loc, msg.buf, 0);\n> -\t\t\tstrbuf_release(&msg);\n> +\t\t    !check_uint(vec->alloc, >, nr)) {\n> +\t\t\tva_end(ap);\n> +\t\t\treturn;\n> +\t\t}\n> +\t\tif (!check_str(vec->v[nr], str)) {\n> +\t\t\ttest_msg(\"     nr: %\"PRIuMAX, (uintmax_t)nr);\n>   \t\t\tva_end(ap);\n>   \t\t\treturn;\n>   \t\t}\n> --\n> 2.45.2\n"},{"id":"498109","messageId":"983be396-f47c-4573-8c33-af8367f8ddbe@web.de","threadId":"61737","inReplyTo":"35b0ba6b-d485-44f2-a19f-3ce816f8b435@web.de","subject":"[PATCH v2] t-strvec: use test_msg()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-07-05T17:03:36Z","receivedAt":"2024-07-05T17:03:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"check_strvec_loc() checks each strvec item by looping through them and\ncomparing them with expected values.  If a check fails then we'd like\nto know which item is affected.  It reports that information by building\na strbuf and delivering its contents using a failing assertion, e.g.\nif there are fewer items in the strvec than expected:\n\n   # check \"vec->nr > nr\" failed at t/unit-tests/t-strvec.c:19\n   #    left: 1\n   #   right: 1\n   # check \"strvec index 1\" failed at t/unit-tests/t-strvec.c:71\n\nNote that the index variable is \"nr\" and thus the interesting value is\nreported twice in that example (in lines three and four).\n\nStop printing the index explicitly for checks that already report it.\nThe message for the same condition as above becomes:\n\n   # check \"vec->nr > nr\" failed at t/unit-tests/t-strvec.c:19\n   #    left: 1\n   #   right: 1\n\nFor the string comparison, whose error message doesn't include the\nindex, report it using the simpler and more appropriate test_msg()\ninstead.  Report the index using its actual variable name and format the\nline like the preceding ones.  The message for an unexpected string\nvalue becomes:\n\n   # check \"!strcmp(vec->v[nr], str)\" failed at t/unit-tests/t-strvec.c:24\n   #    left: \"foo\"\n   #   right: \"bar\"\n   #      nr: 0\n\nReported-by: Phillip Wood <phillip.wood@dunelm.org.uk>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nChanges since v1:\n- Typo fix.\n- Grammar fix.\n- Reworded problem description for brevity.\n- Qualify \"name\" in the last paragraph for clarity.\n- Add sign-off.\n- No code changes.\n\nThank you, Eric!\n\n t/unit-tests/t-strvec.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/unit-tests/t-strvec.c b/t/unit-tests/t-strvec.c\nindex d4615ab06d..236203af61 100644\n--- a/t/unit-tests/t-strvec.c\n+++ b/t/unit-tests/t-strvec.c\n@@ -17,12 +17,12 @@ static void check_strvec_loc(const char *loc, struct strvec *vec, ...)\n \t\t\tbreak;\n\n \t\tif (!check_uint(vec->nr, >, nr) ||\n-\t\t    !check_uint(vec->alloc, >, nr) ||\n-\t\t    !check_str(vec->v[nr], str)) {\n-\t\t\tstruct strbuf msg = STRBUF_INIT;\n-\t\t\tstrbuf_addf(&msg, \"strvec index %\"PRIuMAX, (uintmax_t) nr);\n-\t\t\ttest_assert(loc, msg.buf, 0);\n-\t\t\tstrbuf_release(&msg);\n+\t\t    !check_uint(vec->alloc, >, nr)) {\n+\t\t\tva_end(ap);\n+\t\t\treturn;\n+\t\t}\n+\t\tif (!check_str(vec->v[nr], str)) {\n+\t\t\ttest_msg(\"     nr: %\"PRIuMAX, (uintmax_t)nr);\n \t\t\tva_end(ap);\n \t\t\treturn;\n \t\t}\n--\n2.45.2\n"},{"id":"498354","messageId":"20240709113201.GA994226@coredump.intra.peff.net","threadId":"61737","inReplyTo":"983be396-f47c-4573-8c33-af8367f8ddbe@web.de","subject":"Re: [PATCH v2] t-strvec: use test_msg()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-09T11:32:01Z","receivedAt":"2024-07-09T11:32:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 05, 2024 at 07:03:36PM +0200, René Scharfe wrote:\n\n> diff --git a/t/unit-tests/t-strvec.c b/t/unit-tests/t-strvec.c\n> index d4615ab06d..236203af61 100644\n> --- a/t/unit-tests/t-strvec.c\n> +++ b/t/unit-tests/t-strvec.c\n> @@ -17,12 +17,12 @@ static void check_strvec_loc(const char *loc, struct strvec *vec, ...)\n>  \t\t\tbreak;\n> \n>  \t\tif (!check_uint(vec->nr, >, nr) ||\n> -\t\t    !check_uint(vec->alloc, >, nr) ||\n> -\t\t    !check_str(vec->v[nr], str)) {\n> -\t\t\tstruct strbuf msg = STRBUF_INIT;\n> -\t\t\tstrbuf_addf(&msg, \"strvec index %\"PRIuMAX, (uintmax_t) nr);\n> -\t\t\ttest_assert(loc, msg.buf, 0);\n> -\t\t\tstrbuf_release(&msg);\n> +\t\t    !check_uint(vec->alloc, >, nr)) {\n> +\t\t\tva_end(ap);\n> +\t\t\treturn;\n> +\t\t}\n> +\t\tif (!check_str(vec->v[nr], str)) {\n> +\t\t\ttest_msg(\"     nr: %\"PRIuMAX, (uintmax_t)nr);\n>  \t\t\tva_end(ap);\n>  \t\t\treturn;\n>  \t\t}\n\nThe \"loc\" parameter to the function is now unused. Should it be removed,\nor is it a bug that we are no longer reporting the caller's location?\nShould we be using check_str_loc() in the post-image?\n\n-Peff\n"},{"id":"498658","messageId":"442b692d-a51b-4f45-a131-a44ac08c431b@web.de","threadId":"61737","inReplyTo":"20240709113201.GA994226@coredump.intra.peff.net","subject":"Re: [PATCH v2] t-strvec: use test_msg()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-07-14T10:17:09Z","receivedAt":"2024-07-14T10:17:22Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.07.24 um 13:32 schrieb Jeff King:\n> On Fri, Jul 05, 2024 at 07:03:36PM +0200, René Scharfe wrote:\n>\n>> diff --git a/t/unit-tests/t-strvec.c b/t/unit-tests/t-strvec.c\n>> index d4615ab06d..236203af61 100644\n>> --- a/t/unit-tests/t-strvec.c\n>> +++ b/t/unit-tests/t-strvec.c\n>> @@ -17,12 +17,12 @@ static void check_strvec_loc(const char *loc, struct strvec *vec, ...)\n>>  \t\t\tbreak;\n>>\n>>  \t\tif (!check_uint(vec->nr, >, nr) ||\n>> -\t\t    !check_uint(vec->alloc, >, nr) ||\n>> -\t\t    !check_str(vec->v[nr], str)) {\n>> -\t\t\tstruct strbuf msg = STRBUF_INIT;\n>> -\t\t\tstrbuf_addf(&msg, \"strvec index %\"PRIuMAX, (uintmax_t) nr);\n>> -\t\t\ttest_assert(loc, msg.buf, 0);\n>> -\t\t\tstrbuf_release(&msg);\n>> +\t\t    !check_uint(vec->alloc, >, nr)) {\n>> +\t\t\tva_end(ap);\n>> +\t\t\treturn;\n>> +\t\t}\n>> +\t\tif (!check_str(vec->v[nr], str)) {\n>> +\t\t\ttest_msg(\"     nr: %\"PRIuMAX, (uintmax_t)nr);\n>>  \t\t\tva_end(ap);\n>>  \t\t\treturn;\n>>  \t\t}\n>\n> The \"loc\" parameter to the function is now unused. Should it be removed,\n> or is it a bug that we are no longer reporting the caller's location?\n\nIt's a bug.  If only there was a way to detect such an unused parameter\nautomatically.. ;->\n\n> Should we be using check_str_loc() in the post-image?\n\nYes, and check_uint_loc() and check_pointer_eq_loc() as well.  Which\nwould be a pain.  Or we drag everything into the macro check_strvec and\nget the caller's line number for free.\n\nRené\n"},{"id":"498659","messageId":"5bbef273-382e-4096-9ca6-d74781223e55@web.de","threadId":"61737","inReplyTo":"35b0ba6b-d485-44f2-a19f-3ce816f8b435@web.de","subject":"[PATCH v3] t-strvec: improve check_strvec() output","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-07-14T10:17:53Z","receivedAt":"2024-07-14T10:18:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The macro check_strvec calls the function check_strvec_loc(), which\nperforms the actual checks.  They report the line number inside that\nfunction on error, which is not very helpful.  Half of them trigger\nan assertion with a custom message that reports the line of the\ncheck_strvec call, which is more useful, but a bit awkward.\n\nImprove the output by getting rid of check_strvec_loc() and performing\nall checks within check_strvec, as they then report the line number of\nthe call site, aiding in finding the broken test.  Determine the number\nof items and check it up front to avoid having to do them both in the\nloop and at the end.\n\nSanity check the expected items to make sure there are any and that the\nlast one is NULL, as the compiler no longer does that for us with the\nremoval of the function attribute LAST_ARG_MUST_BE_NULL.\n\nUse only the actual strvec name passed to the macro, the internal\n\"expect\" array name and an index \"i\" in the output, for clarity.  While\n\"expect\" does not exist at the call site, it's reasonably easy to infer\nthat its referring to the NULL-terminated list of expected strings,\nconverted to an array.  Align the \"i\" with previous lines in the output.\n\nHere's the output with less items than expected in the strvec before:\n\n # check \"vec->nr > nr\" failed at t/unit-tests/t-strvec.c:19\n #    left: 1\n #   right: 1\n # check \"strvec index 1\" failed at t/unit-tests/t-strvec.c:71\n\n... and with the patch we get the more concise and still comprehensive:\n\n # check \"(&vec)->nr == ARRAY_SIZE(expect) - 1\" failed at t/unit-tests/t-strvec.c:53\n #    left: 1\n #   right: 2\n\nWith too many items in the strvec we got before:\n\n # check \"vec->nr == nr\" failed at t/unit-tests/t-strvec.c:34\n #    left: 1\n #   right: 0\n # check \"vec->v[nr] == NULL\" failed at t/unit-tests/t-strvec.c:36\n #    left: 0x6000019a40b0\n #   right: 0x0\n\n... and with the patch we get the more concise and informative output\nthat includes the caller's line number:\n\n # check \"(&vec)->nr == ARRAY_SIZE(expect) - 1\" failed at t/unit-tests/t-strvec.c:53\n #    left: 1\n #   right: 0\n\nA broken alloc value was reported like this:\n\n # check \"vec->alloc > nr\" failed at t/unit-tests/t-strvec.c:20\n #    left: 0\n #   right: 0\n # check \"strvec index 0\" failed at t/unit-tests/t-strvec.c:74\n\n ... and with the patch:\n\n # check \"(&vec)->nr <= (&vec)->alloc\" failed at t/unit-tests/t-strvec.c:56\n #    left: 2\n #   right: 0\n\nNote that .alloc == .nr is only valid if the strvec is empty, but\ncheck_strvec doesn't detect this error with or without this patch.\nLeave that for later.\n\nAn unexpected string value was reported like this:\n\n # check \"!strcmp(vec->v[nr], str)\" failed at t/unit-tests/t-strvec.c:21\n #    left: \"foo\"\n #   right: \"bar\"\n # check \"strvec index 0\" failed at t/unit-tests/t-strvec.c:71\n\n... and with the patch we get:\n\n # check \"!strcmp((&vec)->v[i], expect[i])\" failed at t/unit-tests/t-strvec.c:53\n #    left: \"foo\"\n #   right: \"bar\"\n #       i: 0\n\nIf the strvec is not NULL terminated, we got:\n\n # check \"vec->v[nr] == NULL\" failed at t/unit-tests/t-strvec.c:36\n #    left: 0x10444e9e4\n #   right: 0x0\n\n... and with the patch we get the line number of the caller:\n\n # check \"!strcmp((&vec)->v[i], expect[i])\" failed at t/unit-tests/t-strvec.c:53\n #    left: \"bar\"\n #   right: NULL\n #       i: 1\n\ncheck_strvec calls without a trailing NULL were detected at compile time\nbefore:\n\n t/unit-tests/t-strvec.c:71:2: error: missing sentinel in function call [-Werror,-Wsentinel]\n\n... and with the patch it's only found at runtime:\n\n # check \"expect[ARRAY_SIZE(expect) - 1] == NULL\" failed at t/unit-tests/t-strvec.c:53\n #    left: 0x100e5a663\n #   right: 0x0\n\nWe can let check_strvec add the terminating NULL for us and remove it\nfrom callers, making it impossible to forget.  Leave that conversion for\na future patch, though, since this reimplementation is already intrusive\nenough.\n\nReported-by: Phillip Wood <phillip.wood@dunelm.org.uk>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nChanges since v2:\n- Replace check_strvec_loc() with macro code.\n- Rewrite commit message and subject accordingly.\n\n t/unit-tests/t-strvec.c | 46 +++++++++++++----------------------------\n 1 file changed, 14 insertions(+), 32 deletions(-)\n\ndiff --git a/t/unit-tests/t-strvec.c b/t/unit-tests/t-strvec.c\nindex d4615ab06d..fdb28ba220 100644\n--- a/t/unit-tests/t-strvec.c\n+++ b/t/unit-tests/t-strvec.c\n@@ -3,38 +3,20 @@\n #include \"strvec.h\"\n\n #define check_strvec(vec, ...) \\\n-\tcheck_strvec_loc(TEST_LOCATION(), vec, __VA_ARGS__)\n-LAST_ARG_MUST_BE_NULL\n-static void check_strvec_loc(const char *loc, struct strvec *vec, ...)\n-{\n-\tva_list ap;\n-\tsize_t nr = 0;\n-\n-\tva_start(ap, vec);\n-\twhile (1) {\n-\t\tconst char *str = va_arg(ap, const char *);\n-\t\tif (!str)\n-\t\t\tbreak;\n-\n-\t\tif (!check_uint(vec->nr, >, nr) ||\n-\t\t    !check_uint(vec->alloc, >, nr) ||\n-\t\t    !check_str(vec->v[nr], str)) {\n-\t\t\tstruct strbuf msg = STRBUF_INIT;\n-\t\t\tstrbuf_addf(&msg, \"strvec index %\"PRIuMAX, (uintmax_t) nr);\n-\t\t\ttest_assert(loc, msg.buf, 0);\n-\t\t\tstrbuf_release(&msg);\n-\t\t\tva_end(ap);\n-\t\t\treturn;\n-\t\t}\n-\n-\t\tnr++;\n-\t}\n-\tva_end(ap);\n-\n-\tcheck_uint(vec->nr, ==, nr);\n-\tcheck_uint(vec->alloc, >=, nr);\n-\tcheck_pointer_eq(vec->v[nr], NULL);\n-}\n+\tdo { \\\n+\t\tconst char *expect[] = { __VA_ARGS__ }; \\\n+\t\tif (check_uint(ARRAY_SIZE(expect), >, 0) && \\\n+\t\t    check_pointer_eq(expect[ARRAY_SIZE(expect) - 1], NULL) && \\\n+\t\t    check_uint((vec)->nr, ==, ARRAY_SIZE(expect) - 1) && \\\n+\t\t    check_uint((vec)->nr, <=, (vec)->alloc)) { \\\n+\t\t\tfor (size_t i = 0; i < ARRAY_SIZE(expect); i++) { \\\n+\t\t\t\tif (!check_str((vec)->v[i], expect[i])) { \\\n+\t\t\t\t\ttest_msg(\"      i: %\"PRIuMAX, i); \\\n+\t\t\t\t\tbreak; \\\n+\t\t\t\t} \\\n+\t\t\t} \\\n+\t\t} \\\n+\t} while (0)\n\n static void t_static_init(void)\n {\n--\n2.45.2\n"},{"id":"498684","messageId":"d91c794a-296f-469d-9f1d-0b45351e8ba7@web.de","threadId":"61737","inReplyTo":"5bbef273-382e-4096-9ca6-d74781223e55@web.de","subject":"Re: [PATCH v3] t-strvec: improve check_strvec() output","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-07-14T17:06:30Z","receivedAt":"2024-07-14T17:06:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Oh, v2 is already on next.  Will send the equivalent of v3 as an update\non top of next as a replacement.\n\nRené\n"},{"id":"498685","messageId":"f2bcee2b-88fa-4dcd-8247-f68d842c6a4c@web.de","threadId":"61737","inReplyTo":"983be396-f47c-4573-8c33-af8367f8ddbe@web.de","subject":"[PATCH v2 2/1] t-strvec: improve check_strvec() output","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-07-14T17:06:50Z","receivedAt":"2024-07-14T17:07:02Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The macro check_strvec calls the function check_strvec_loc(), which\nperforms the actual checks.  They report the line number inside that\nfunction on error, which is not very helpful.  Before the previous\npatch half of them triggered an assertion that reported the caller's\nline number using a custom message, which was more useful, but a bit\nawkward.\n\nImprove the output by getting rid of check_strvec_loc() and performing\nall checks within check_strvec, as they then report the line number of\nthe call site, aiding in finding the broken test.  Determine the number\nof items and check it up front to avoid having to do them both in the\nloop and at the end.\n\nSanity check the expected items to make sure there are any and that the\nlast one is NULL, as the compiler no longer does that for us with the\nremoval of the function attribute LAST_ARG_MUST_BE_NULL.\n\nUse only the actual strvec name passed to the macro, the internal\n\"expect\" array name and an index \"i\" in the output, for clarity.  While\n\"expect\" does not exist at the call site, it's reasonably easy to infer\nthat it's referring to the NULL-terminated list of expected strings,\nconverted to an array.\n\nHere's the output with less items than expected in the strvec before:\n\n # check \"vec->nr > nr\" failed at t/unit-tests/t-strvec.c:19\n #    left: 1\n #   right: 1\n\n... and with the patch:\n\n # check \"(&vec)->nr == ARRAY_SIZE(expect) - 1\" failed at t/unit-tests/t-strvec.c:53\n #    left: 1\n #   right: 2\n\nWith too many items in the strvec we got before:\n\n # check \"vec->nr == nr\" failed at t/unit-tests/t-strvec.c:34\n #    left: 1\n #   right: 0\n # check \"vec->v[nr] == NULL\" failed at t/unit-tests/t-strvec.c:36\n #    left: 0x6000004b8010\n #   right: 0x0\n\n... and with the patch:\n\n # check \"(&vec)->nr == ARRAY_SIZE(expect) - 1\" failed at t/unit-tests/t-strvec.c:53\n #    left: 1\n #   right: 0\n\nA broken alloc value was reported like this:\n\n # check \"vec->alloc > nr\" failed at t/unit-tests/t-strvec.c:20\n #    left: 0\n #   right: 0\n\n ... and with the patch:\n\n # check \"(&vec)->nr <= (&vec)->alloc\" failed at t/unit-tests/t-strvec.c:56\n #    left: 2\n #   right: 0\n\nAn unexpected string value was reported like this:\n\n # check \"!strcmp(vec->v[nr], str)\" failed at t/unit-tests/t-strvec.c:24\n #    left: \"foo\"\n #   right: \"bar\"\n #      nr: 0\n\n... and with the patch:\n\n # check \"!strcmp((&vec)->v[i], expect[i])\" failed at t/unit-tests/t-strvec.c:53\n #    left: \"foo\"\n #   right: \"bar\"\n #       i: 0\n\nIf the strvec is not NULL terminated, we got:\n\n # check \"vec->v[nr] == NULL\" failed at t/unit-tests/t-strvec.c:36\n #    left: 0x102c3abc8\n #   right: 0x0\n\n... and with the patch we get the line number of the caller:\n\n # check \"!strcmp((&vec)->v[i], expect[i])\" failed at t/unit-tests/t-strvec.c:53\n #    left: \"bar\"\n #   right: NULL\n #       i: 1\n\ncheck_strvec calls without a trailing NULL were detected at compile time\nbefore:\n\n t/unit-tests/t-strvec.c:71:2: error: missing sentinel in function call [-Werror,-Wsentinel]\n\n... and with the patch it's only found at runtime:\n\n # check \"expect[ARRAY_SIZE(expect) - 1] == NULL\" failed at t/unit-tests/t-strvec.c:53\n #    left: 0x100e5a663\n #   right: 0x0\n\nWe can let check_strvec add the terminating NULL for us and remove it\nfrom callers, making it impossible to forget.  Leave that conversion for\na future patch, though, since this reimplementation is already intrusive\nenough.\n\nReported-by: Jeff King <peff@peff.net>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nFor next, on top of c6f35e529e t-strvec: use test_msg().\n\n t/unit-tests/t-strvec.c | 46 +++++++++++++----------------------------\n 1 file changed, 14 insertions(+), 32 deletions(-)\n\ndiff --git a/t/unit-tests/t-strvec.c b/t/unit-tests/t-strvec.c\nindex 236203af61..fdb28ba220 100644\n--- a/t/unit-tests/t-strvec.c\n+++ b/t/unit-tests/t-strvec.c\n@@ -3,38 +3,20 @@\n #include \"strvec.h\"\n\n #define check_strvec(vec, ...) \\\n-\tcheck_strvec_loc(TEST_LOCATION(), vec, __VA_ARGS__)\n-LAST_ARG_MUST_BE_NULL\n-static void check_strvec_loc(const char *loc, struct strvec *vec, ...)\n-{\n-\tva_list ap;\n-\tsize_t nr = 0;\n-\n-\tva_start(ap, vec);\n-\twhile (1) {\n-\t\tconst char *str = va_arg(ap, const char *);\n-\t\tif (!str)\n-\t\t\tbreak;\n-\n-\t\tif (!check_uint(vec->nr, >, nr) ||\n-\t\t    !check_uint(vec->alloc, >, nr)) {\n-\t\t\tva_end(ap);\n-\t\t\treturn;\n-\t\t}\n-\t\tif (!check_str(vec->v[nr], str)) {\n-\t\t\ttest_msg(\"     nr: %\"PRIuMAX, (uintmax_t)nr);\n-\t\t\tva_end(ap);\n-\t\t\treturn;\n-\t\t}\n-\n-\t\tnr++;\n-\t}\n-\tva_end(ap);\n-\n-\tcheck_uint(vec->nr, ==, nr);\n-\tcheck_uint(vec->alloc, >=, nr);\n-\tcheck_pointer_eq(vec->v[nr], NULL);\n-}\n+\tdo { \\\n+\t\tconst char *expect[] = { __VA_ARGS__ }; \\\n+\t\tif (check_uint(ARRAY_SIZE(expect), >, 0) && \\\n+\t\t    check_pointer_eq(expect[ARRAY_SIZE(expect) - 1], NULL) && \\\n+\t\t    check_uint((vec)->nr, ==, ARRAY_SIZE(expect) - 1) && \\\n+\t\t    check_uint((vec)->nr, <=, (vec)->alloc)) { \\\n+\t\t\tfor (size_t i = 0; i < ARRAY_SIZE(expect); i++) { \\\n+\t\t\t\tif (!check_str((vec)->v[i], expect[i])) { \\\n+\t\t\t\t\ttest_msg(\"      i: %\"PRIuMAX, i); \\\n+\t\t\t\t\tbreak; \\\n+\t\t\t\t} \\\n+\t\t\t} \\\n+\t\t} \\\n+\t} while (0)\n\n static void t_static_init(void)\n {\n--\n2.45.2\n"},{"id":"498686","messageId":"075b3d08-4270-4064-8103-1fece055e197@web.de","threadId":"61737","inReplyTo":"983be396-f47c-4573-8c33-af8367f8ddbe@web.de","subject":"[PATCH v2 3/1] t-strvec: tighten .alloc check in check_strvec","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-07-14T17:06:58Z","receivedAt":"2024-07-14T17:07:10Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The members .nr and .alloc of an empty strvec are both zero and the\ntrailing NULL is provided by the shared global variable empty_strvec.\nOnce at least one item is added, .alloc needs to be bigger than .nr to\nleave room for the terminating NULL.  The current .alloc check only\ntests whether it's bigger or equal to .nr in all cases.\n\nTighten it depending on whether the strvec is backed by empty_strvec or\nnot.  A report for a non-zero .alloc in an empty strvec looks like this:\n\n # check \"(&vec)->nr == (&vec)->alloc\" failed at t/unit-tests/t-strvec.c:55\n #    left: 0\n #   right: 1\n\nAnd .alloc being .equal to .nr in a non-empty strvec results in:\n\n # check \"(&vec)->nr < (&vec)->alloc\" failed at t/unit-tests/t-strvec.c:55\n #    left: 1\n #   right: 1\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nBonus patch.\n\n t/unit-tests/t-strvec.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/unit-tests/t-strvec.c b/t/unit-tests/t-strvec.c\nindex fdb28ba220..6a4d425840 100644\n--- a/t/unit-tests/t-strvec.c\n+++ b/t/unit-tests/t-strvec.c\n@@ -8,7 +8,9 @@\n \t\tif (check_uint(ARRAY_SIZE(expect), >, 0) && \\\n \t\t    check_pointer_eq(expect[ARRAY_SIZE(expect) - 1], NULL) && \\\n \t\t    check_uint((vec)->nr, ==, ARRAY_SIZE(expect) - 1) && \\\n-\t\t    check_uint((vec)->nr, <=, (vec)->alloc)) { \\\n+\t\t    ((vec)->v == empty_strvec ? \\\n+\t\t     check_uint((vec)->nr, ==, (vec)->alloc) : \\\n+\t\t     check_uint((vec)->nr, <, (vec)->alloc))) { \\\n \t\t\tfor (size_t i = 0; i < ARRAY_SIZE(expect); i++) { \\\n \t\t\t\tif (!check_str((vec)->v[i], expect[i])) { \\\n \t\t\t\t\ttest_msg(\"      i: %\"PRIuMAX, i); \\\n--\n2.45.2\n"},{"id":"498725","messageId":"xmqqcyne90lo.fsf@gitster.g","threadId":"61737","inReplyTo":"f2bcee2b-88fa-4dcd-8247-f68d842c6a4c@web.de","subject":"Re: [PATCH v2 2/1] t-strvec: improve check_strvec() output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-15T14:44:03Z","receivedAt":"2024-07-15T14:44:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> The macro check_strvec calls the function check_strvec_loc(), which\n> performs the actual checks.  They report the line number inside that\n> function on error, which is not very helpful.  Before the previous\n> patch half of them triggered an assertion that reported the caller's\n> line number using a custom message, which was more useful, but a bit\n> awkward.\n> ...\n> We can let check_strvec add the terminating NULL for us and remove it\n> from callers, making it impossible to forget.  Leave that conversion for\n> a future patch, though, since this reimplementation is already intrusive\n> enough.\n>\n> Reported-by: Jeff King <peff@peff.net>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> For next, on top of c6f35e529e t-strvec: use test_msg().\n\nThanks.\n"},{"id":"498726","messageId":"xmqqjzhm90mb.fsf@gitster.g","threadId":"61737","inReplyTo":"075b3d08-4270-4064-8103-1fece055e197@web.de","subject":"Re: [PATCH v2 3/1] t-strvec: tighten .alloc check in check_strvec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-15T14:43:40Z","receivedAt":"2024-07-15T14:45:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> diff --git a/t/unit-tests/t-strvec.c b/t/unit-tests/t-strvec.c\n> index fdb28ba220..6a4d425840 100644\n> --- a/t/unit-tests/t-strvec.c\n> +++ b/t/unit-tests/t-strvec.c\n> @@ -8,7 +8,9 @@\n>  \t\tif (check_uint(ARRAY_SIZE(expect), >, 0) && \\\n>  \t\t    check_pointer_eq(expect[ARRAY_SIZE(expect) - 1], NULL) && \\\n>  \t\t    check_uint((vec)->nr, ==, ARRAY_SIZE(expect) - 1) && \\\n> -\t\t    check_uint((vec)->nr, <=, (vec)->alloc)) { \\\n> +\t\t    ((vec)->v == empty_strvec ? \\\n> +\t\t     check_uint((vec)->nr, ==, (vec)->alloc) : \\\n> +\t\t     check_uint((vec)->nr, <, (vec)->alloc))) { \\\n\nNot a huge deal but with empty_strvec, don't we want to barf if\nnr==alloc==1?\n\n>  \t\t\tfor (size_t i = 0; i < ARRAY_SIZE(expect); i++) { \\\n>  \t\t\t\tif (!check_str((vec)->v[i], expect[i])) { \\\n>  \t\t\t\t\ttest_msg(\"      i: %\"PRIuMAX, i); \\\n> --\n> 2.45.2\n"},{"id":"498740","messageId":"bf7fd471-18fb-41df-a306-2413c9d441bd@web.de","threadId":"61737","inReplyTo":"xmqqjzhm90mb.fsf@gitster.g","subject":"Re: [PATCH v2 3/1] t-strvec: tighten .alloc check in check_strvec","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-07-15T16:47:49Z","receivedAt":"2024-07-15T16:48:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.07.24 um 16:43 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> diff --git a/t/unit-tests/t-strvec.c b/t/unit-tests/t-strvec.c\n>> index fdb28ba220..6a4d425840 100644\n>> --- a/t/unit-tests/t-strvec.c\n>> +++ b/t/unit-tests/t-strvec.c\n>> @@ -8,7 +8,9 @@\n>>  \t\tif (check_uint(ARRAY_SIZE(expect), >, 0) && \\\n>>  \t\t    check_pointer_eq(expect[ARRAY_SIZE(expect) - 1], NULL) && \\\n>>  \t\t    check_uint((vec)->nr, ==, ARRAY_SIZE(expect) - 1) && \\\n>> -\t\t    check_uint((vec)->nr, <=, (vec)->alloc)) { \\\n>> +\t\t    ((vec)->v == empty_strvec ? \\\n>> +\t\t     check_uint((vec)->nr, ==, (vec)->alloc) : \\\n>> +\t\t     check_uint((vec)->nr, <, (vec)->alloc))) { \\\n>\n> Not a huge deal but with empty_strvec, don't we want to barf if\n> nr==alloc==1?\n\nYes, and that's handled by the comparison with ARRAY_SIZE(expect) - 1\nabove.  Perhaps comparing to 0 explicitly would be clearer.  My thinking\nwas just to split up the old <= check into its < and == cases, without\nchanging anything else..\n\n>\n>>  \t\t\tfor (size_t i = 0; i < ARRAY_SIZE(expect); i++) { \\\n>>  \t\t\t\tif (!check_str((vec)->v[i], expect[i])) { \\\n>>  \t\t\t\t\ttest_msg(\"      i: %\"PRIuMAX, i); \\\n>> --\n>> 2.45.2\n"},{"id":"498742","messageId":"xmqq4j8q4lcb.fsf@gitster.g","threadId":"61737","inReplyTo":"bf7fd471-18fb-41df-a306-2413c9d441bd@web.de","subject":"Re: [PATCH v2 3/1] t-strvec: tighten .alloc check in check_strvec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-15T17:27:16Z","receivedAt":"2024-07-15T17:27:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n>>>  \t\t    check_uint((vec)->nr, ==, ARRAY_SIZE(expect) - 1) && \\\n>>> -\t\t    check_uint((vec)->nr, <=, (vec)->alloc)) { \\\n>>> +\t\t    ((vec)->v == empty_strvec ? \\\n>>> +\t\t     check_uint((vec)->nr, ==, (vec)->alloc) : \\\n>>> +\t\t     check_uint((vec)->nr, <, (vec)->alloc))) { \\\n>>\n>> Not a huge deal but with empty_strvec, don't we want to barf if\n>> nr==alloc==1?\n>\n> Yes, and that's handled by the comparison with ARRAY_SIZE(expect) - 1\n> above.\n\nAh, OK, thanks.\n\n"},{"id":"498769","messageId":"20240716014330.GA639947@coredump.intra.peff.net","threadId":"61737","inReplyTo":"442b692d-a51b-4f45-a131-a44ac08c431b@web.de","subject":"Re: [PATCH v2] t-strvec: use test_msg()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-16T01:43:30Z","receivedAt":"2024-07-16T01:43:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 14, 2024 at 12:17:09PM +0200, René Scharfe wrote:\n\n> > Should we be using check_str_loc() in the post-image?\n> \n> Yes, and check_uint_loc() and check_pointer_eq_loc() as well.  Which\n> would be a pain.  Or we drag everything into the macro check_strvec and\n> get the caller's line number for free.\n\nIs it that big of a pain? It's mostly just passing \"loc\" along to the\nrelative functions. To me it is more a problem that it is super easy to\nforget.\n\nAre the unit tests themselves multi-threaded within a single program?\nI'd think not. In which case, I kind of wonder if a simpler pattern\nwould be to just set a global \"location\" variable (probably as a stack\nof strings) that all of the individual check functions used. And then we\ncould set it once at the top-level of the test which wants its location\nreported, and any helper functions that get called in the middle would\nnot have to care. And existing check_foo() functions could use the\ncurrent file/line location if the stack is empty (so code that isn't\nusing helper functions can remain unaffected).\n\n-Peff\n"},{"id":"498772","messageId":"20240716043727.GA1429885@coredump.intra.peff.net","threadId":"61737","inReplyTo":"5bbef273-382e-4096-9ca6-d74781223e55@web.de","subject":"Re: [PATCH v3] t-strvec: improve check_strvec() output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-16T04:37:27Z","receivedAt":"2024-07-16T04:37:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 14, 2024 at 12:17:53PM +0200, René Scharfe wrote:\n\n> +\tdo { \\\n> +\t\tconst char *expect[] = { __VA_ARGS__ }; \\\n> +\t\tif (check_uint(ARRAY_SIZE(expect), >, 0) && \\\n> +\t\t    check_pointer_eq(expect[ARRAY_SIZE(expect) - 1], NULL) && \\\n> +\t\t    check_uint((vec)->nr, ==, ARRAY_SIZE(expect) - 1) && \\\n> +\t\t    check_uint((vec)->nr, <=, (vec)->alloc)) { \\\n> +\t\t\tfor (size_t i = 0; i < ARRAY_SIZE(expect); i++) { \\\n> +\t\t\t\tif (!check_str((vec)->v[i], expect[i])) { \\\n> +\t\t\t\t\ttest_msg(\"      i: %\"PRIuMAX, i); \\\n> +\t\t\t\t\tbreak; \\\n> +\t\t\t\t} \\\n> +\t\t\t} \\\n> +\t\t} \\\n> +\t} while (0)\n\nThe linux32 CI job seems to complain about this, since the concrete type\nof \"i\" (a size_t) is \"unsigned int\" there, but PRIuMAX is %llu.\nPresumably you just need to cast to uintmax_t.\n\n-Peff\n"},{"id":"498788","messageId":"xmqq4j8pcqji.fsf@gitster.g","threadId":"61737","inReplyTo":"20240716043727.GA1429885@coredump.intra.peff.net","subject":"Re: [PATCH v3] t-strvec: improve check_strvec() output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-16T15:20:01Z","receivedAt":"2024-07-16T15:20:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sun, Jul 14, 2024 at 12:17:53PM +0200, René Scharfe wrote:\n>\n>> +\tdo { \\\n>> +\t\tconst char *expect[] = { __VA_ARGS__ }; \\\n>> +\t\tif (check_uint(ARRAY_SIZE(expect), >, 0) && \\\n>> +\t\t    check_pointer_eq(expect[ARRAY_SIZE(expect) - 1], NULL) && \\\n>> +\t\t    check_uint((vec)->nr, ==, ARRAY_SIZE(expect) - 1) && \\\n>> +\t\t    check_uint((vec)->nr, <=, (vec)->alloc)) { \\\n>> +\t\t\tfor (size_t i = 0; i < ARRAY_SIZE(expect); i++) { \\\n>> +\t\t\t\tif (!check_str((vec)->v[i], expect[i])) { \\\n>> +\t\t\t\t\ttest_msg(\"      i: %\"PRIuMAX, i); \\\n>> +\t\t\t\t\tbreak; \\\n>> +\t\t\t\t} \\\n>> +\t\t\t} \\\n>> +\t\t} \\\n>> +\t} while (0)\n>\n> The linux32 CI job seems to complain about this, since the concrete type\n> of \"i\" (a size_t) is \"unsigned int\" there, but PRIuMAX is %llu.\n> Presumably you just need to cast to uintmax_t.\n\nYeah, I noticed it, too, but unfortunately it was after pushing it\nout and seeing it break there X-<.\n\n\n"},{"id":"498792","messageId":"1521ed89-989e-452b-b7fc-9e73672e0764@web.de","threadId":"61737","inReplyTo":"xmqq4j8pcqji.fsf@gitster.g","subject":"[PATCH] t-strvec: fix type mismatch in check_strvec","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-07-16T16:14:35Z","receivedAt":"2024-07-16T16:14:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Cast i from size_t to uintmax_t to match the format string.\n\nReported-by: Jeff King <peff@peff.net>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nOops, sorry about that. :-(\n\nPRIuMAX is \"lu\" on my system, so the compiler didn't flag the mistake.\n\n t/unit-tests/t-strvec.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/unit-tests/t-strvec.c b/t/unit-tests/t-strvec.c\nindex fdb28ba220..fa1a041469 100644\n--- a/t/unit-tests/t-strvec.c\n+++ b/t/unit-tests/t-strvec.c\n@@ -11,7 +11,8 @@\n \t\t    check_uint((vec)->nr, <=, (vec)->alloc)) { \\\n \t\t\tfor (size_t i = 0; i < ARRAY_SIZE(expect); i++) { \\\n \t\t\t\tif (!check_str((vec)->v[i], expect[i])) { \\\n-\t\t\t\t\ttest_msg(\"      i: %\"PRIuMAX, i); \\\n+\t\t\t\t\ttest_msg(\"      i: %\"PRIuMAX, \\\n+\t\t\t\t\t\t (uintmax_t)i); \\\n \t\t\t\t\tbreak; \\\n \t\t\t\t} \\\n \t\t\t} \\\n--\n2.45.2\n"},{"id":"498794","messageId":"xmqqbk2x9u0a.fsf@gitster.g","threadId":"61737","inReplyTo":"1521ed89-989e-452b-b7fc-9e73672e0764@web.de","subject":"Re: [PATCH] t-strvec: fix type mismatch in check_strvec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-16T16:33:25Z","receivedAt":"2024-07-16T16:33:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Cast i from size_t to uintmax_t to match the format string.\n>\n> Reported-by: Jeff King <peff@peff.net>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> Oops, sorry about that. :-(\n>\n> PRIuMAX is \"lu\" on my system, so the compiler didn't flag the mistake.\n>\n>  t/unit-tests/t-strvec.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n\nThe fix is of course identical to what I added to the integration I\nprepared, but the line-wrapping is better in this version, so I'll\nreplace ;-)\n\n> diff --git a/t/unit-tests/t-strvec.c b/t/unit-tests/t-strvec.c\n> index fdb28ba220..fa1a041469 100644\n> --- a/t/unit-tests/t-strvec.c\n> +++ b/t/unit-tests/t-strvec.c\n> @@ -11,7 +11,8 @@\n>  \t\t    check_uint((vec)->nr, <=, (vec)->alloc)) { \\\n>  \t\t\tfor (size_t i = 0; i < ARRAY_SIZE(expect); i++) { \\\n>  \t\t\t\tif (!check_str((vec)->v[i], expect[i])) { \\\n> -\t\t\t\t\ttest_msg(\"      i: %\"PRIuMAX, i); \\\n> +\t\t\t\t\ttest_msg(\"      i: %\"PRIuMAX, \\\n> +\t\t\t\t\t\t (uintmax_t)i); \\\n>  \t\t\t\t\tbreak; \\\n>  \t\t\t\t} \\\n>  \t\t\t} \\\n> --\n> 2.45.2\n"},{"id":"498795","messageId":"824ecb8b-b786-4220-a6b4-598a818bee52@web.de","threadId":"61737","inReplyTo":"20240716014330.GA639947@coredump.intra.peff.net","subject":"Re: [PATCH v2] t-strvec: use test_msg()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-07-16T16:43:06Z","receivedAt":"2024-07-16T16:43:22Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.07.24 um 03:43 schrieb Jeff King:\n> On Sun, Jul 14, 2024 at 12:17:09PM +0200, René Scharfe wrote:\n>\n>>> Should we be using check_str_loc() in the post-image?\n>>\n>> Yes, and check_uint_loc() and check_pointer_eq_loc() as well.  Which\n>> would be a pain.  Or we drag everything into the macro check_strvec and\n>> get the caller's line number for free.\n>\n> Is it that big of a pain? It's mostly just passing \"loc\" along to the\n> relative functions.\n\nThat part is bearable.  The pain comes from the need to pass in\narguments multiple times, for the stringified check description, as part\nof the check result and as separate values.\n\nIt could be mitigated by adding a new macro that takes loc, a, op,\nand b, perhaps called CHECK_UINT_LOC.  That additional evaluation step\ndoesn't work nicely with arguments that are themselves macros, though,\nlike NULL for string or pointer checks, as those will be expanded,\nchanging the message.\n\n> Are the unit tests themselves multi-threaded within a single program?\n\nNo, and check_pointer_eq, check_int, check_uint, and check_char use\nshared global variables to store temporary values (comments rightly\nwarn that \"this is not thread safe\").\n\n> I'd think not. In which case, I kind of wonder if a simpler pattern\n> would be to just set a global \"location\" variable (probably as a stack\n> of strings) that all of the individual check functions used. And then we\n> could set it once at the top-level of the test which wants its location\n> reported, and any helper functions that get called in the middle would\n> not have to care. And existing check_foo() functions could use the\n> current file/line location if the stack is empty (so code that isn't\n> using helper functions can remain unaffected).\n\nThat doesn't sound simpler to me, quite the opposite actually.  To the\npoint that I don't dare to ask for a demo. o_O\n\nOr perhaps I need to revisit it when I'm no longer tired and hungry.\n\nRené\n\n"}]}