{"thread":{"id":"56449","subject":"[PATCH] test-tool run-command: remove dead \"testsuite\" code","startedAt":"2021-09-06T00:45:17Z","lastAt":"2021-09-11T18:26:47Z","messageCount":10,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"434747","messageId":"patch-1.1-d1e464da0a9-20210906T002938Z-avarab@gmail.com","threadId":"56449","inReplyTo":null,"subject":"[PATCH] test-tool run-command: remove dead \"testsuite\" code","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-06T00:37:37Z","receivedAt":"2021-09-06T00:45:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the \"test-tool run-command testsuite\" sub-sub-command, it has\nnot been used since 4c2c38e800f (ci: modification of main.yml to use\ncmake for vs-build job, 2020-06-26), see also the earlier\n6081d3898fe (ci: retire the Azure Pipelines definition, 2020-04-11)\nfor another phasing out of the command.\n\nThis the \"testsuite\" functionality here was added in\nbe5d88e1128 (test-tool run-command: learn to run (parts of) the\ntestsuite, 2019-10-04), and then first used in 46689317ac0 (ci: also\nbuild and test with MS Visual Studio on Azure Pipelines, 2019-10-04).\n\nI'd started out fixing a bug in the \"init\" pattern here. We set \"next\"\nto \"-1\" and then proceed to memset() the whole struct to \"0\", so the\ntwo \"STRING_LIST_INIT_DUP\" here are also redundant to our setting of\nthe \"strdup_strings\" members later on.\n\nBut rather than fix that let's just remove this whole thing, as\nnothing is using it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nThere's a trivial conflict here with ab/config-based-hooks-base. This\nremoves code, that series adds a couple of new (but unrelated)\nfunctions in-between some of these removed functions.\n\n t/helper/test-run-command.c | 151 ------------------------------------\n 1 file changed, 151 deletions(-)\n\ndiff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\nindex 7ae03dc7123..1d41c0eda81 100644\n--- a/t/helper/test-run-command.c\n+++ b/t/helper/test-run-command.c\n@@ -10,14 +10,10 @@\n \n #include \"test-tool.h\"\n #include \"git-compat-util.h\"\n-#include \"cache.h\"\n #include \"run-command.h\"\n #include \"strvec.h\"\n #include \"strbuf.h\"\n #include \"parse-options.h\"\n-#include \"string-list.h\"\n-#include \"thread-utils.h\"\n-#include \"wildmatch.h\"\n #include \"gettext.h\"\n #include \"parse-options.h\"\n \n@@ -55,151 +51,6 @@ static int task_finished(int result,\n \treturn 1;\n }\n \n-struct testsuite {\n-\tstruct string_list tests, failed;\n-\tint next;\n-\tint quiet, immediate, verbose, verbose_log, trace, write_junit_xml;\n-};\n-#define TESTSUITE_INIT \\\n-\t{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }\n-\n-static int next_test(struct child_process *cp, struct strbuf *err, void *cb,\n-\t\t     void **task_cb)\n-{\n-\tstruct testsuite *suite = cb;\n-\tconst char *test;\n-\tif (suite->next >= suite->tests.nr)\n-\t\treturn 0;\n-\n-\ttest = suite->tests.items[suite->next++].string;\n-\tstrvec_pushl(&cp->args, \"sh\", test, NULL);\n-\tif (suite->quiet)\n-\t\tstrvec_push(&cp->args, \"--quiet\");\n-\tif (suite->immediate)\n-\t\tstrvec_push(&cp->args, \"-i\");\n-\tif (suite->verbose)\n-\t\tstrvec_push(&cp->args, \"-v\");\n-\tif (suite->verbose_log)\n-\t\tstrvec_push(&cp->args, \"-V\");\n-\tif (suite->trace)\n-\t\tstrvec_push(&cp->args, \"-x\");\n-\tif (suite->write_junit_xml)\n-\t\tstrvec_push(&cp->args, \"--write-junit-xml\");\n-\n-\tstrbuf_addf(err, \"Output of '%s':\\n\", test);\n-\t*task_cb = (void *)test;\n-\n-\treturn 1;\n-}\n-\n-static int test_finished(int result, struct strbuf *err, void *cb,\n-\t\t\t void *task_cb)\n-{\n-\tstruct testsuite *suite = cb;\n-\tconst char *name = (const char *)task_cb;\n-\n-\tif (result)\n-\t\tstring_list_append(&suite->failed, name);\n-\n-\tstrbuf_addf(err, \"%s: '%s'\\n\", result ? \"FAIL\" : \"SUCCESS\", name);\n-\n-\treturn 0;\n-}\n-\n-static int test_failed(struct strbuf *out, void *cb, void *task_cb)\n-{\n-\tstruct testsuite *suite = cb;\n-\tconst char *name = (const char *)task_cb;\n-\n-\tstring_list_append(&suite->failed, name);\n-\tstrbuf_addf(out, \"FAILED TO START: '%s'\\n\", name);\n-\n-\treturn 0;\n-}\n-\n-static const char * const testsuite_usage[] = {\n-\t\"test-run-command testsuite [<options>] [<pattern>...]\",\n-\tNULL\n-};\n-\n-static int testsuite(int argc, const char **argv)\n-{\n-\tstruct testsuite suite = TESTSUITE_INIT;\n-\tint max_jobs = 1, i, ret;\n-\tDIR *dir;\n-\tstruct dirent *d;\n-\tstruct option options[] = {\n-\t\tOPT_BOOL('i', \"immediate\", &suite.immediate,\n-\t\t\t \"stop at first failed test case(s)\"),\n-\t\tOPT_INTEGER('j', \"jobs\", &max_jobs, \"run <N> jobs in parallel\"),\n-\t\tOPT_BOOL('q', \"quiet\", &suite.quiet, \"be terse\"),\n-\t\tOPT_BOOL('v', \"verbose\", &suite.verbose, \"be verbose\"),\n-\t\tOPT_BOOL('V', \"verbose-log\", &suite.verbose_log,\n-\t\t\t \"be verbose, redirected to a file\"),\n-\t\tOPT_BOOL('x', \"trace\", &suite.trace, \"trace shell commands\"),\n-\t\tOPT_BOOL(0, \"write-junit-xml\", &suite.write_junit_xml,\n-\t\t\t \"write JUnit-style XML files\"),\n-\t\tOPT_END()\n-\t};\n-\n-\tmemset(&suite, 0, sizeof(suite));\n-\tsuite.tests.strdup_strings = suite.failed.strdup_strings = 1;\n-\n-\targc = parse_options(argc, argv, NULL, options,\n-\t\t\ttestsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);\n-\n-\tif (max_jobs <= 0)\n-\t\tmax_jobs = online_cpus();\n-\n-\tdir = opendir(\".\");\n-\tif (!dir)\n-\t\tdie(\"Could not open the current directory\");\n-\twhile ((d = readdir(dir))) {\n-\t\tconst char *p = d->d_name;\n-\n-\t\tif (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) ||\n-\t\t    !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' ||\n-\t\t    !ends_with(p, \".sh\"))\n-\t\t\tcontinue;\n-\n-\t\t/* No pattern: match all */\n-\t\tif (!argc) {\n-\t\t\tstring_list_append(&suite.tests, p);\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tfor (i = 0; i < argc; i++)\n-\t\t\tif (!wildmatch(argv[i], p, 0)) {\n-\t\t\t\tstring_list_append(&suite.tests, p);\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t}\n-\tclosedir(dir);\n-\n-\tif (!suite.tests.nr)\n-\t\tdie(\"No tests match!\");\n-\tif (max_jobs > suite.tests.nr)\n-\t\tmax_jobs = suite.tests.nr;\n-\n-\tfprintf(stderr, \"Running %d tests (%d at a time)\\n\",\n-\t\tsuite.tests.nr, max_jobs);\n-\n-\tret = run_processes_parallel(max_jobs, next_test, test_failed,\n-\t\t\t\t     test_finished, &suite);\n-\n-\tif (suite.failed.nr > 0) {\n-\t\tret = 1;\n-\t\tfprintf(stderr, \"%d tests failed:\\n\\n\", suite.failed.nr);\n-\t\tfor (i = 0; i < suite.failed.nr; i++)\n-\t\t\tfprintf(stderr, \"\\t%s\\n\", suite.failed.items[i].string);\n-\t}\n-\n-\tstring_list_clear(&suite.tests, 0);\n-\tstring_list_clear(&suite.failed, 0);\n-\n-\treturn !!ret;\n-}\n-\n static uint64_t my_random_next = 1234;\n \n static uint64_t my_random(void)\n@@ -373,8 +224,6 @@ int cmd__run_command(int argc, const char **argv)\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n \tint jobs;\n \n-\tif (argc > 1 && !strcmp(argv[1], \"testsuite\"))\n-\t\texit(testsuite(argc - 1, argv + 1));\n \tif (!strcmp(argv[1], \"inherited-handle\"))\n \t\texit(inherit_handle(argv[0]));\n \tif (!strcmp(argv[1], \"inherited-handle-child\"))\n-- \n2.33.0.821.g8b294b0dcf8\n\n"},{"id":"434929","messageId":"xmqqtuiw9loh.fsf@gitster.g","threadId":"56449","inReplyTo":"patch-1.1-d1e464da0a9-20210906T002938Z-avarab@gmail.com","subject":"Re: [PATCH] test-tool run-command: remove dead \"testsuite\" code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-07T20:26:38Z","receivedAt":"2021-09-07T20:26:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Remove the \"test-tool run-command testsuite\" sub-sub-command, it has\n> not been used since 4c2c38e800f (ci: modification of main.yml to use\n> cmake for vs-build job, 2020-06-26), see also the earlier\n> 6081d3898fe (ci: retire the Azure Pipelines definition, 2020-04-11)\n> for another phasing out of the command.\n\nI'll leave this patch hanging in the list archive until I hear from\nsomebody from Azure camp say that they do not need it anymore and\nthey do not plan to use it in the future.  Of course, if somebody\nelse from outside the Windows circle is using it or plans to use it\nin a near future, they can raise their voice heard while we are\nwaiting for such an Ack.\n\nThanks.\n"},{"id":"434960","messageId":"nycvar.QRO.7.76.6.2109072339550.55@tvgsbejvaqbjf.bet","threadId":"56449","inReplyTo":"patch-1.1-d1e464da0a9-20210906T002938Z-avarab@gmail.com","subject":"Re: [PATCH] test-tool run-command: remove dead \"testsuite\" code","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-09-07T21:41:48Z","receivedAt":"2021-09-07T21:41:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Mon, 6 Sep 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> Remove the \"test-tool run-command testsuite\" sub-sub-command, it has\n> not been used since 4c2c38e800f (ci: modification of main.yml to use\n> cmake for vs-build job, 2020-06-26), see also the earlier\n> 6081d3898fe (ci: retire the Azure Pipelines definition, 2020-04-11)\n> for another phasing out of the command.\n\nSure, the code is currently unused, but I had hoped to use it when\nBusyBox-w32 support in Git for Windows would be more robust.\n\nCiao,\nJohannes\n"},{"id":"435247","messageId":"nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet","threadId":"56449","inReplyTo":"xmqqtuiw9loh.fsf@gitster.g","subject":"Re: [PATCH] test-tool run-command: remove dead \"testsuite\" code","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-09-09T11:26:55Z","receivedAt":"2021-09-09T11:27:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 7 Sep 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n> > Remove the \"test-tool run-command testsuite\" sub-sub-command, it has\n> > not been used since 4c2c38e800f (ci: modification of main.yml to use\n> > cmake for vs-build job, 2020-06-26), see also the earlier\n> > 6081d3898fe (ci: retire the Azure Pipelines definition, 2020-04-11)\n> > for another phasing out of the command.\n>\n> I'll leave this patch hanging in the list archive until I hear from\n> somebody from Azure camp say that they do not need it anymore and\n> they do not plan to use it in the future.  Of course, if somebody\n> else from outside the Windows circle is using it or plans to use it\n> in a near future, they can raise their voice heard while we are\n> waiting for such an Ack.\n\nAs I mentioned, I would love for this to stay.\n\nThe reason is that there is a long-running ticket about polishing\nBusyBox-w32, polishing Git for Windows' support for BusyBox, and then\nshipping a MinGit [*1*] version _without_ Bash and _without_ Perl.\nObviously, I would want to verify that it works as intended, and that's\nwhere this `testsuite` command would come in (we already bundle the test\nartifacts in our CI runs, so `test-tool.exe` would be available).\n\nCiao,\nDscho\n\nFootnote *1*: MinGit is a subset of Git for Windows, intended to be\nbundled by 3rd-party applications that do not require interactive Git\nusage, optimized for a small disk foot print. For more details, see\nhttps://github.com/git-for-windows/git/wiki/MinGit\n"},{"id":"435251","messageId":"877dfqdjvz.fsf@evledraar.gmail.com","threadId":"56449","inReplyTo":"nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] test-tool run-command: remove dead \"testsuite\" code","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-09T12:05:32Z","receivedAt":"2021-09-09T14:10:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 09 2021, Johannes Schindelin wrote:\n\n> Hi Junio,\n>\n> On Tue, 7 Sep 2021, Junio C Hamano wrote:\n>\n>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>>\n>> > Remove the \"test-tool run-command testsuite\" sub-sub-command, it has\n>> > not been used since 4c2c38e800f (ci: modification of main.yml to use\n>> > cmake for vs-build job, 2020-06-26), see also the earlier\n>> > 6081d3898fe (ci: retire the Azure Pipelines definition, 2020-04-11)\n>> > for another phasing out of the command.\n>>\n>> I'll leave this patch hanging in the list archive until I hear from\n>> somebody from Azure camp say that they do not need it anymore and\n>> they do not plan to use it in the future.  Of course, if somebody\n>> else from outside the Windows circle is using it or plans to use it\n>> in a near future, they can raise their voice heard while we are\n>> waiting for such an Ack.\n>\n> As I mentioned, I would love for this to stay.\n>\n> The reason is that there is a long-running ticket about polishing\n> BusyBox-w32, polishing Git for Windows' support for BusyBox, and then\n> shipping a MinGit [*1*] version _without_ Bash and _without_ Perl.\n> Obviously, I would want to verify that it works as intended, and that's\n> where this `testsuite` command would come in (we already bundle the test\n> artifacts in our CI runs, so `test-tool.exe` would be available).\n>\n> Ciao,\n> Dscho\n>\n> Footnote *1*: MinGit is a subset of Git for Windows, intended to be\n> bundled by 3rd-party applications that do not require interactive Git\n> usage, optimized for a small disk foot print. For more details, see\n> https://github.com/git-for-windows/git/wiki/MinGit\n\nI'm fine with it staying, I have some cooking serieses where I'll submit\nsome tree-wide fixes that might touch this code then (e.g. general\n*_INIT pattern improvements).\n\nI did find the \"without bash and without Perl\" part of your above\nconfusing, most of what this program does is a re-implementation of GNU\nmake and t/aggregate-results.sh, and indeed your be5d88e1128 (test-tool\nrun-command: learn to run (parts of) the testsuite, 2019-10-04) says as\nmuch, and that makes sense for the reasons stated in your be5d88e1128.\n\n\n"},{"id":"435261","messageId":"patch-1.1-0aa4523ab6e-20210909T130849Z-avarab@gmail.com","threadId":"56449","inReplyTo":"nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet","subject":"[PATCH] test-tool run-command: fix confusing init pattern","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-09T13:09:51Z","receivedAt":"2021-09-09T14:27:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In be5d88e1128 (test-tool run-command: learn to run (parts of) the\ntestsuite, 2019-10-04) an init pattern was added that would use\nTESTSUITE_INIT, but then promptly memset() everything back to 0. We'd\nthen set the \"dup\" on the two string lists. Our setting of \"next\" to\n\"-1\" thus did nothing, we'd reset it to \"0\" before using it.\n\nLet's just use the init macro for the STRING_LIST members, we can then\nremove the already redundant memset().\n\nNote that while we compile this code, there's no in-tree user for the\n\"testsuite\" target being modified here anymore, see the discussion at\nand around <nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet>[1].\n\n1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nThis patch is the immediate reason for why I submitted\nhttps://lore.kernel.org/git/patch-1.1-d1e464da0a9-20210906T002938Z-avarab@gmail.com/,\nsince Johannes would prefer to keep it let's fix this init pattern.\n\n t/helper/test-run-command.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\nindex 7ae03dc7123..8e42516bdc1 100644\n--- a/t/helper/test-run-command.c\n+++ b/t/helper/test-run-command.c\n@@ -56,12 +56,15 @@ static int task_finished(int result,\n }\n \n struct testsuite {\n-\tstruct string_list tests, failed;\n+\tstruct string_list tests;\n+\tstruct string_list failed;\n \tint next;\n \tint quiet, immediate, verbose, verbose_log, trace, write_junit_xml;\n };\n-#define TESTSUITE_INIT \\\n-\t{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }\n+#define TESTSUITE_INIT { \\\n+\t.tests = STRING_LIST_INIT_DUP, \\\n+\t.failed = STRING_LIST_INIT_DUP, \\\n+}\n \n static int next_test(struct child_process *cp, struct strbuf *err, void *cb,\n \t\t     void **task_cb)\n@@ -142,9 +145,6 @@ static int testsuite(int argc, const char **argv)\n \t\tOPT_END()\n \t};\n \n-\tmemset(&suite, 0, sizeof(suite));\n-\tsuite.tests.strdup_strings = suite.failed.strdup_strings = 1;\n-\n \targc = parse_options(argc, argv, NULL, options,\n \t\t\ttestsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);\n \n-- \n2.33.0.867.g88ec4638586\n\n"},{"id":"435395","messageId":"nycvar.QRO.7.76.6.2109101319110.59@tvgsbejvaqbjf.bet","threadId":"56449","inReplyTo":"patch-1.1-0aa4523ab6e-20210909T130849Z-avarab@gmail.com","subject":"Re: [PATCH] test-tool run-command: fix confusing init pattern","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-09-10T11:23:41Z","receivedAt":"2021-09-10T11:23:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nthe commit title is misleading: it suggests that there is a bug that needs\nto be fixed.\n\nThe idea of the patch, however, is to avoid redundant code, and if\ndescribed that way, the patch is a lot better for it.\n\nOn Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> In be5d88e1128 (test-tool run-command: learn to run (parts of) the\n> testsuite, 2019-10-04) an init pattern was added that would use\n> TESTSUITE_INIT, but then promptly memset() everything back to 0. We'd\n> then set the \"dup\" on the two string lists. Our setting of \"next\" to\n> \"-1\" thus did nothing, we'd reset it to \"0\" before using it.\n>\n> Let's just use the init macro for the STRING_LIST members, we can then\n> remove the already redundant memset().\n>\n> Note that while we compile this code, there's no in-tree user for the\n> \"testsuite\" target being modified here anymore, see the discussion at\n> and around <nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet>[1].\n>\n> 1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>\n> This patch is the immediate reason for why I submitted\n> https://lore.kernel.org/git/patch-1.1-d1e464da0a9-20210906T002938Z-avarab@gmail.com/,\n> since Johannes would prefer to keep it let's fix this init pattern.\n\nThe diff does too many things, some of which are your purely personal\npreferences and do not actually need to be changed. This is a much more\nto-the-point diff:\n\n-- snip --\ndiff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\nindex 7ae03dc7123..14c57365e76 100644\n--- a/t/helper/test-run-command.c\n+++ b/t/helper/test-run-command.c\n@@ -61,7 +61,7 @@ struct testsuite {\n \tint quiet, immediate, verbose, verbose_log, trace, write_junit_xml;\n };\n #define TESTSUITE_INIT \\\n-\t{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }\n+\t{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, 0, 0, 0, 0, 0, 0, 0 }\n\n static int next_test(struct child_process *cp, struct strbuf *err, void *cb,\n \t\t     void **task_cb)\n@@ -142,9 +142,6 @@ static int testsuite(int argc, const char **argv)\n \t\tOPT_END()\n \t};\n\n-\tmemset(&suite, 0, sizeof(suite));\n-\tsuite.tests.strdup_strings = suite.failed.strdup_strings = 1;\n-\n \targc = parse_options(argc, argv, NULL, options,\n \t\t\ttestsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);\n-- snap --\n\nI would strongly suggest to use this diff instead.\n\nCiao,\nJohannes\n\n\n>\n>  t/helper/test-run-command.c | 12 ++++++------\n>  1 file changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\n> index 7ae03dc7123..8e42516bdc1 100644\n> --- a/t/helper/test-run-command.c\n> +++ b/t/helper/test-run-command.c\n> @@ -56,12 +56,15 @@ static int task_finished(int result,\n>  }\n>\n>  struct testsuite {\n> -\tstruct string_list tests, failed;\n> +\tstruct string_list tests;\n> +\tstruct string_list failed;\n>  \tint next;\n>  \tint quiet, immediate, verbose, verbose_log, trace, write_junit_xml;\n>  };\n> -#define TESTSUITE_INIT \\\n> -\t{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }\n> +#define TESTSUITE_INIT { \\\n> +\t.tests = STRING_LIST_INIT_DUP, \\\n> +\t.failed = STRING_LIST_INIT_DUP, \\\n> +}\n>\n>  static int next_test(struct child_process *cp, struct strbuf *err, void *cb,\n>  \t\t     void **task_cb)\n> @@ -142,9 +145,6 @@ static int testsuite(int argc, const char **argv)\n>  \t\tOPT_END()\n>  \t};\n>\n> -\tmemset(&suite, 0, sizeof(suite));\n> -\tsuite.tests.strdup_strings = suite.failed.strdup_strings = 1;\n> -\n>  \targc = parse_options(argc, argv, NULL, options,\n>  \t\t\ttestsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);\n>\n> --\n> 2.33.0.867.g88ec4638586\n>\n>\n"},{"id":"435461","messageId":"87pmtgb4m1.fsf@evledraar.gmail.com","threadId":"56449","inReplyTo":"nycvar.QRO.7.76.6.2109101319110.59@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] test-tool run-command: fix confusing init pattern","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-10T19:32:03Z","receivedAt":"2021-09-10T19:41:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 10 2021, Johannes Schindelin wrote:\n\n> Hi Ævar,\n>\n> the commit title is misleading: it suggests that there is a bug that needs\n> to be fixed.\n\nI picked \"confusing\" because this doesn't impact the end-state of the\nprogram, it's just confusing to do:\n\n    x = -1\n\nFollowed by:\n\n    x = 0\n\nWhich the reader might wonder about, only to find that the initial\nassignment wasn't needed or used for anything.\n\n> The idea of the patch, however, is to avoid redundant code, and if\n> described that way, the patch is a lot better for it.\n>\n> On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote:\n>\n>> In be5d88e1128 (test-tool run-command: learn to run (parts of) the\n>> testsuite, 2019-10-04) an init pattern was added that would use\n>> TESTSUITE_INIT, but then promptly memset() everything back to 0. We'd\n>> then set the \"dup\" on the two string lists. Our setting of \"next\" to\n>> \"-1\" thus did nothing, we'd reset it to \"0\" before using it.\n>>\n>> Let's just use the init macro for the STRING_LIST members, we can then\n>> remove the already redundant memset().\n>>\n>> Note that while we compile this code, there's no in-tree user for the\n>> \"testsuite\" target being modified here anymore, see the discussion at\n>> and around <nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet>[1].\n>>\n>> 1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> ---\n>>\n>> This patch is the immediate reason for why I submitted\n>> https://lore.kernel.org/git/patch-1.1-d1e464da0a9-20210906T002938Z-avarab@gmail.com/,\n>> since Johannes would prefer to keep it let's fix this init pattern.\n>\n> The diff does too many things, some of which are your purely personal\n> preferences and do not actually need to be changed. This is a much more\n> to-the-point diff:\n\nWe've been slowly converting everything to designated initializers. It\nseems to make sense to just do that if the line is being touched anyway.\n\nFor instance my d385784f89b (fsck.h: use designed initializers for\nFSCK_OPTIONS_{DEFAULT,STRICT}, 2021-03-28) was part of a series whose\ninitial version just changed one field at a time as you're doing below,\nbut during early review I was asked just to use designated initializers\nalready.\n\nBut yes, it is strictly unrelated. It's a judgement call when to do\ncleanups on lines you touch while you're at it, v.s. turning one patch\ninto a potential series of really tiny changes.\n\nThen there's the change of \"struct foo x, y\" to \"struct foo x;\\nstruct\nfoo y;\\n\" above. I changed that because that tends to be the usual\nstyle, it also preempts feedback / reviewers having to look at the code\nout-of-bounds to see what the struct looks like in the default diff\n-U<n>.\n\nSo, in advance addressing another type of question, or needing to\ndescribe the context, as you brought up in\nhttps://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091222260.59@tvgsbejvaqbjf.bet/\n:)\n\n\n> -- snip --\n> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\n> index 7ae03dc7123..14c57365e76 100644\n> --- a/t/helper/test-run-command.c\n> +++ b/t/helper/test-run-command.c\n> @@ -61,7 +61,7 @@ struct testsuite {\n>  \tint quiet, immediate, verbose, verbose_log, trace, write_junit_xml;\n>  };\n>  #define TESTSUITE_INIT \\\n> -\t{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }\n> +\t{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, 0, 0, 0, 0, 0, 0, 0 }\n>\n>  static int next_test(struct child_process *cp, struct strbuf *err, void *cb,\n>  \t\t     void **task_cb)\n> @@ -142,9 +142,6 @@ static int testsuite(int argc, const char **argv)\n>  \t\tOPT_END()\n>  \t};\n>\n> -\tmemset(&suite, 0, sizeof(suite));\n> -\tsuite.tests.strdup_strings = suite.failed.strdup_strings = 1;\n> -\n>  \targc = parse_options(argc, argv, NULL, options,\n>  \t\t\ttestsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);\n> -- snap --\n>\n> I would strongly suggest to use this diff instead.\n>\n> Ciao,\n> Johannes\n>\n>\n>>\n>>  t/helper/test-run-command.c | 12 ++++++------\n>>  1 file changed, 6 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\n>> index 7ae03dc7123..8e42516bdc1 100644\n>> --- a/t/helper/test-run-command.c\n>> +++ b/t/helper/test-run-command.c\n>> @@ -56,12 +56,15 @@ static int task_finished(int result,\n>>  }\n>>\n>>  struct testsuite {\n>> -\tstruct string_list tests, failed;\n>> +\tstruct string_list tests;\n>> +\tstruct string_list failed;\n>>  \tint next;\n>>  \tint quiet, immediate, verbose, verbose_log, trace, write_junit_xml;\n>>  };\n>> -#define TESTSUITE_INIT \\\n>> -\t{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }\n>> +#define TESTSUITE_INIT { \\\n>> +\t.tests = STRING_LIST_INIT_DUP, \\\n>> +\t.failed = STRING_LIST_INIT_DUP, \\\n>> +}\n>>\n>>  static int next_test(struct child_process *cp, struct strbuf *err, void *cb,\n>>  \t\t     void **task_cb)\n>> @@ -142,9 +145,6 @@ static int testsuite(int argc, const char **argv)\n>>  \t\tOPT_END()\n>>  \t};\n>>\n>> -\tmemset(&suite, 0, sizeof(suite));\n>> -\tsuite.tests.strdup_strings = suite.failed.strdup_strings = 1;\n>> -\n>>  \targc = parse_options(argc, argv, NULL, options,\n>>  \t\t\ttestsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);\n>>\n>> --\n>> 2.33.0.867.g88ec4638586\n>>\n>>\n\n"},{"id":"435484","messageId":"xmqq1r5wrsrh.fsf@gitster.g","threadId":"56449","inReplyTo":"87pmtgb4m1.fsf@evledraar.gmail.com","subject":"Re: [PATCH] test-tool run-command: fix confusing init pattern","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-10T22:05:22Z","receivedAt":"2021-09-10T22:05:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> The diff does too many things, some of which are your purely personal\n>> preferences and do not actually need to be changed. This is a much more\n>> to-the-point diff:\n>\n> We've been slowly converting everything to designated initializers. It\n> seems to make sense to just do that if the line is being touched anyway.\n\nPerhaps a preliminary clean-up patch is called for in such a case?\n\nI do not think anybody can immediately see what the difference\nbetween the old -1 and the new 0 in TESTSUITE_INIT macro means in\nDscho's alternative, but if we had a preliminary clean-up whose sole\nchange is to use designated initializers, the real \"to-the-point\"\nstep would become much easier to see which member that used to be\ninitialized to -1 is now getting zero-initialized.\n\nAnd yes, changing the initializer style *and* the values the members\nare initialized to in a same patch is much worse than sticking to\nthe style of the unreadable original.  It buries the real change in\nthe noise.\n\n>>  #define TESTSUITE_INIT \\\n>> -\t{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }\n>> +\t{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, 0, 0, 0, 0, 0, 0, 0 }\n"},{"id":"435582","messageId":"patch-v2-1.1-0ddf38b47ac-20210911T182631Z-avarab@gmail.com","threadId":"56449","inReplyTo":"patch-1.1-0aa4523ab6e-20210909T130849Z-avarab@gmail.com","subject":"[PATCH v2] test-tool run-command: fix flip-flop init pattern","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-11T18:26:41Z","receivedAt":"2021-09-11T18:26:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In be5d88e1128 (test-tool run-command: learn to run (parts of) the\ntestsuite, 2019-10-04) an init pattern was added that would use\nTESTSUITE_INIT, but then promptly memset() everything back to 0. We'd\nthen set the \"dup\" on the two string lists.\n\nOur setting of \"next\" to \"-1\" thus did nothing, we'd reset it to \"0\"\nbefore using it. Let's set it to \"0\" instead, and trust the\n\"STRING_LIST_INIT_DUP\" to set \"strdup_strings\" appropriately for us.\n\nNote that while we compile this code, there's no in-tree user for the\n\"testsuite\" target being modified here anymore, see the discussion at\nand around <nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet>[1].\n\n1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\nRange-diff against v1:\n1:  0aa4523ab6e ! 1:  0ddf38b47ac test-tool run-command: fix confusing init pattern\n    @@ Metadata\n     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## Commit message ##\n    -    test-tool run-command: fix confusing init pattern\n    +    test-tool run-command: fix flip-flop init pattern\n     \n         In be5d88e1128 (test-tool run-command: learn to run (parts of) the\n         testsuite, 2019-10-04) an init pattern was added that would use\n         TESTSUITE_INIT, but then promptly memset() everything back to 0. We'd\n    -    then set the \"dup\" on the two string lists. Our setting of \"next\" to\n    -    \"-1\" thus did nothing, we'd reset it to \"0\" before using it.\n    +    then set the \"dup\" on the two string lists.\n     \n    -    Let's just use the init macro for the STRING_LIST members, we can then\n    -    remove the already redundant memset().\n    +    Our setting of \"next\" to \"-1\" thus did nothing, we'd reset it to \"0\"\n    +    before using it. Let's set it to \"0\" instead, and trust the\n    +    \"STRING_LIST_INIT_DUP\" to set \"strdup_strings\" appropriately for us.\n     \n         Note that while we compile this code, there's no in-tree user for the\n         \"testsuite\" target being modified here anymore, see the discussion at\n    @@ Commit message\n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## t/helper/test-run-command.c ##\n    -@@ t/helper/test-run-command.c: static int task_finished(int result,\n    - }\n    - \n    - struct testsuite {\n    --\tstruct string_list tests, failed;\n    -+\tstruct string_list tests;\n    -+\tstruct string_list failed;\n    - \tint next;\n    +@@ t/helper/test-run-command.c: struct testsuite {\n      \tint quiet, immediate, verbose, verbose_log, trace, write_junit_xml;\n      };\n    --#define TESTSUITE_INIT \\\n    + #define TESTSUITE_INIT \\\n     -\t{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }\n    -+#define TESTSUITE_INIT { \\\n    -+\t.tests = STRING_LIST_INIT_DUP, \\\n    -+\t.failed = STRING_LIST_INIT_DUP, \\\n    -+}\n    ++\t{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, 0, 0, 0, 0, 0, 0, 0 }\n      \n      static int next_test(struct child_process *cp, struct strbuf *err, void *cb,\n      \t\t     void **task_cb)\n\n t/helper/test-run-command.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\nindex 7ae03dc7123..14c57365e76 100644\n--- a/t/helper/test-run-command.c\n+++ b/t/helper/test-run-command.c\n@@ -61,7 +61,7 @@ struct testsuite {\n \tint quiet, immediate, verbose, verbose_log, trace, write_junit_xml;\n };\n #define TESTSUITE_INIT \\\n-\t{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }\n+\t{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, 0, 0, 0, 0, 0, 0, 0 }\n \n static int next_test(struct child_process *cp, struct strbuf *err, void *cb,\n \t\t     void **task_cb)\n@@ -142,9 +142,6 @@ static int testsuite(int argc, const char **argv)\n \t\tOPT_END()\n \t};\n \n-\tmemset(&suite, 0, sizeof(suite));\n-\tsuite.tests.strdup_strings = suite.failed.strdup_strings = 1;\n-\n \targc = parse_options(argc, argv, NULL, options,\n \t\t\ttestsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);\n \n-- \n2.33.0.995.ga5ea46173a2\n\n"}]}