[PATCH] test-tool run-command: remove dead "testsuite" code

[PATCH] test-tool run-command: remove dead "testsuite" code

[Ævar Arnfjörð Bjarmason]

Remove the "test-tool run-command testsuite" sub-sub-command, it has
not been used since 4c2c38e800f (ci: modification of main.yml to use
cmake for vs-build job, 2020-06-26), see also the earlier
6081d3898fe (ci: retire the Azure Pipelines definition, 2020-04-11)
for another phasing out of the command.

This the "testsuite" functionality here was added in
be5d88e1128 (test-tool run-command: learn to run (parts of) the
testsuite, 2019-10-04), and then first used in 46689317ac0 (ci: also
build and test with MS Visual Studio on Azure Pipelines, 2019-10-04).

I'd started out fixing a bug in the "init" pattern here. We set "next"
to "-1" and then proceed to memset() the whole struct to "0", so the
two "STRING_LIST_INIT_DUP" here are also redundant to our setting of
the "strdup_strings" members later on.

But rather than fix that let's just remove this whole thing, as
nothing is using it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

There's a trivial conflict here with ab/config-based-hooks-base. This
removes code, that series adds a couple of new (but unrelated)
functions in-between some of these removed functions.

 t/helper/test-run-command.c | 151 ------------------------------------
 1 file changed, 151 deletions(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index 7ae03dc7123..1d41c0eda81 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -10,14 +10,10 @@
 
 #include "test-tool.h"
 #include "git-compat-util.h"
-#include "cache.h"
 #include "run-command.h"
 #include "strvec.h"
 #include "strbuf.h"
 #include "parse-options.h"
-#include "string-list.h"
-#include "thread-utils.h"
-#include "wildmatch.h"
 #include "gettext.h"
 #include "parse-options.h"
 
@@ -55,151 +51,6 @@ static int task_finished(int result,
 	return 1;
 }
 
-struct testsuite {
-	struct string_list tests, failed;
-	int next;
-	int quiet, immediate, verbose, verbose_log, trace, write_junit_xml;
-};
-#define TESTSUITE_INIT \
-	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }
-
-static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
-		     void **task_cb)
-{
-	struct testsuite *suite = cb;
-	const char *test;
-	if (suite->next >= suite->tests.nr)
-		return 0;
-
-	test = suite->tests.items[suite->next++].string;
-	strvec_pushl(&cp->args, "sh", test, NULL);
-	if (suite->quiet)
-		strvec_push(&cp->args, "--quiet");
-	if (suite->immediate)
-		strvec_push(&cp->args, "-i");
-	if (suite->verbose)
-		strvec_push(&cp->args, "-v");
-	if (suite->verbose_log)
-		strvec_push(&cp->args, "-V");
-	if (suite->trace)
-		strvec_push(&cp->args, "-x");
-	if (suite->write_junit_xml)
-		strvec_push(&cp->args, "--write-junit-xml");
-
-	strbuf_addf(err, "Output of '%s':\n", test);
-	*task_cb = (void *)test;
-
-	return 1;
-}
-
-static int test_finished(int result, struct strbuf *err, void *cb,
-			 void *task_cb)
-{
-	struct testsuite *suite = cb;
-	const char *name = (const char *)task_cb;
-
-	if (result)
-		string_list_append(&suite->failed, name);
-
-	strbuf_addf(err, "%s: '%s'\n", result ? "FAIL" : "SUCCESS", name);
-
-	return 0;
-}
-
-static int test_failed(struct strbuf *out, void *cb, void *task_cb)
-{
-	struct testsuite *suite = cb;
-	const char *name = (const char *)task_cb;
-
-	string_list_append(&suite->failed, name);
-	strbuf_addf(out, "FAILED TO START: '%s'\n", name);
-
-	return 0;
-}
-
-static const char * const testsuite_usage[] = {
-	"test-run-command testsuite [<options>] [<pattern>...]",
-	NULL
-};
-
-static int testsuite(int argc, const char **argv)
-{
-	struct testsuite suite = TESTSUITE_INIT;
-	int max_jobs = 1, i, ret;
-	DIR *dir;
-	struct dirent *d;
-	struct option options[] = {
-		OPT_BOOL('i', "immediate", &suite.immediate,
-			 "stop at first failed test case(s)"),
-		OPT_INTEGER('j', "jobs", &max_jobs, "run <N> jobs in parallel"),
-		OPT_BOOL('q', "quiet", &suite.quiet, "be terse"),
-		OPT_BOOL('v', "verbose", &suite.verbose, "be verbose"),
-		OPT_BOOL('V', "verbose-log", &suite.verbose_log,
-			 "be verbose, redirected to a file"),
-		OPT_BOOL('x', "trace", &suite.trace, "trace shell commands"),
-		OPT_BOOL(0, "write-junit-xml", &suite.write_junit_xml,
-			 "write JUnit-style XML files"),
-		OPT_END()
-	};
-
-	memset(&suite, 0, sizeof(suite));
-	suite.tests.strdup_strings = suite.failed.strdup_strings = 1;
-
-	argc = parse_options(argc, argv, NULL, options,
-			testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
-
-	if (max_jobs <= 0)
-		max_jobs = online_cpus();
-
-	dir = opendir(".");
-	if (!dir)
-		die("Could not open the current directory");
-	while ((d = readdir(dir))) {
-		const char *p = d->d_name;
-
-		if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) ||
-		    !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' ||
-		    !ends_with(p, ".sh"))
-			continue;
-
-		/* No pattern: match all */
-		if (!argc) {
-			string_list_append(&suite.tests, p);
-			continue;
-		}
-
-		for (i = 0; i < argc; i++)
-			if (!wildmatch(argv[i], p, 0)) {
-				string_list_append(&suite.tests, p);
-				break;
-			}
-	}
-	closedir(dir);
-
-	if (!suite.tests.nr)
-		die("No tests match!");
-	if (max_jobs > suite.tests.nr)
-		max_jobs = suite.tests.nr;
-
-	fprintf(stderr, "Running %d tests (%d at a time)\n",
-		suite.tests.nr, max_jobs);
-
-	ret = run_processes_parallel(max_jobs, next_test, test_failed,
-				     test_finished, &suite);
-
-	if (suite.failed.nr > 0) {
-		ret = 1;
-		fprintf(stderr, "%d tests failed:\n\n", suite.failed.nr);
-		for (i = 0; i < suite.failed.nr; i++)
-			fprintf(stderr, "\t%s\n", suite.failed.items[i].string);
-	}
-
-	string_list_clear(&suite.tests, 0);
-	string_list_clear(&suite.failed, 0);
-
-	return !!ret;
-}
-
 static uint64_t my_random_next = 1234;
 
 static uint64_t my_random(void)
@@ -373,8 +224,6 @@ int cmd__run_command(int argc, const char **argv)
 	struct child_process proc = CHILD_PROCESS_INIT;
 	int jobs;
 
-	if (argc > 1 && !strcmp(argv[1], "testsuite"))
-		exit(testsuite(argc - 1, argv + 1));
 	if (!strcmp(argv[1], "inherited-handle"))
 		exit(inherit_handle(argv[0]));
 	if (!strcmp(argv[1], "inherited-handle-child"))
-- 
2.33.0.821.g8b294b0dcf8

Re: [PATCH] test-tool run-command: remove dead "testsuite" code

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Remove the "test-tool run-command testsuite" sub-sub-command, it has
> not been used since 4c2c38e800f (ci: modification of main.yml to use
> cmake for vs-build job, 2020-06-26), see also the earlier
> 6081d3898fe (ci: retire the Azure Pipelines definition, 2020-04-11)
> for another phasing out of the command.

I'll leave this patch hanging in the list archive until I hear from
somebody from Azure camp say that they do not need it anymore and
they do not plan to use it in the future.  Of course, if somebody
else from outside the Windows circle is using it or plans to use it
in a near future, they can raise their voice heard while we are
waiting for such an Ack.

Thanks.

Re: [PATCH] test-tool run-command: remove dead "testsuite" code

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]

Hi Ævar,

On Mon, 6 Sep 2021, Ævar Arnfjörð Bjarmason wrote:

> Remove the "test-tool run-command testsuite" sub-sub-command, it has
> not been used since 4c2c38e800f (ci: modification of main.yml to use
> cmake for vs-build job, 2020-06-26), see also the earlier
> 6081d3898fe (ci: retire the Azure Pipelines definition, 2020-04-11)
> for another phasing out of the command.

Sure, the code is currently unused, but I had hoped to use it when
BusyBox-w32 support in Git for Windows would be more robust.

Ciao,
Johannes

Re: [PATCH] test-tool run-command: remove dead "testsuite" code

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]

Hi Junio,

On Tue, 7 Sep 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
> > Remove the "test-tool run-command testsuite" sub-sub-command, it has
> > not been used since 4c2c38e800f (ci: modification of main.yml to use
> > cmake for vs-build job, 2020-06-26), see also the earlier
> > 6081d3898fe (ci: retire the Azure Pipelines definition, 2020-04-11)
> > for another phasing out of the command.
>
> I'll leave this patch hanging in the list archive until I hear from
> somebody from Azure camp say that they do not need it anymore and
> they do not plan to use it in the future.  Of course, if somebody
> else from outside the Windows circle is using it or plans to use it
> in a near future, they can raise their voice heard while we are
> waiting for such an Ack.

As I mentioned, I would love for this to stay.

The reason is that there is a long-running ticket about polishing
BusyBox-w32, polishing Git for Windows' support for BusyBox, and then
shipping a MinGit [*1*] version _without_ Bash and _without_ Perl.
Obviously, I would want to verify that it works as intended, and that's
where this `testsuite` command would come in (we already bundle the test
artifacts in our CI runs, so `test-tool.exe` would be available).

Ciao,
Dscho

Footnote *1*: MinGit is a subset of Git for Windows, intended to be
bundled by 3rd-party applications that do not require interactive Git
usage, optimized for a small disk foot print. For more details, see
https://github.com/git-for-windows/git/wiki/MinGit

Re: [PATCH] test-tool run-command: remove dead "testsuite" code

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 09 2021, Johannes Schindelin wrote:

> Hi Junio,
>
> On Tue, 7 Sep 2021, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>> > Remove the "test-tool run-command testsuite" sub-sub-command, it has
>> > not been used since 4c2c38e800f (ci: modification of main.yml to use
>> > cmake for vs-build job, 2020-06-26), see also the earlier
>> > 6081d3898fe (ci: retire the Azure Pipelines definition, 2020-04-11)
>> > for another phasing out of the command.
>>
>> I'll leave this patch hanging in the list archive until I hear from
>> somebody from Azure camp say that they do not need it anymore and
>> they do not plan to use it in the future.  Of course, if somebody
>> else from outside the Windows circle is using it or plans to use it
>> in a near future, they can raise their voice heard while we are
>> waiting for such an Ack.
>
> As I mentioned, I would love for this to stay.
>
> The reason is that there is a long-running ticket about polishing
> BusyBox-w32, polishing Git for Windows' support for BusyBox, and then
> shipping a MinGit [*1*] version _without_ Bash and _without_ Perl.
> Obviously, I would want to verify that it works as intended, and that's
> where this `testsuite` command would come in (we already bundle the test
> artifacts in our CI runs, so `test-tool.exe` would be available).
>
> Ciao,
> Dscho
>
> Footnote *1*: MinGit is a subset of Git for Windows, intended to be
> bundled by 3rd-party applications that do not require interactive Git
> usage, optimized for a small disk foot print. For more details, see
> https://github.com/git-for-windows/git/wiki/MinGit

I'm fine with it staying, I have some cooking serieses where I'll submit
some tree-wide fixes that might touch this code then (e.g. general
*_INIT pattern improvements).

I did find the "without bash and without Perl" part of your above
confusing, most of what this program does is a re-implementation of GNU
make and t/aggregate-results.sh, and indeed your be5d88e1128 (test-tool
run-command: learn to run (parts of) the testsuite, 2019-10-04) says as
much, and that makes sense for the reasons stated in your be5d88e1128.

[PATCH] test-tool run-command: fix confusing init pattern

[Ævar Arnfjörð Bjarmason]

In be5d88e1128 (test-tool run-command: learn to run (parts of) the
testsuite, 2019-10-04) an init pattern was added that would use
TESTSUITE_INIT, but then promptly memset() everything back to 0. We'd
then set the "dup" on the two string lists. Our setting of "next" to
"-1" thus did nothing, we'd reset it to "0" before using it.

Let's just use the init macro for the STRING_LIST members, we can then
remove the already redundant memset().

Note that while we compile this code, there's no in-tree user for the
"testsuite" target being modified here anymore, see the discussion at
and around <nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet>[1].

1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

This patch is the immediate reason for why I submitted
https://lore.kernel.org/git/patch-1.1-d1e464da0a9-20210906T002938Z-avarab@gmail.com/,
since Johannes would prefer to keep it let's fix this init pattern.

 t/helper/test-run-command.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index 7ae03dc7123..8e42516bdc1 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -56,12 +56,15 @@ static int task_finished(int result,
 }
 
 struct testsuite {
-	struct string_list tests, failed;
+	struct string_list tests;
+	struct string_list failed;
 	int next;
 	int quiet, immediate, verbose, verbose_log, trace, write_junit_xml;
 };
-#define TESTSUITE_INIT \
-	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }
+#define TESTSUITE_INIT { \
+	.tests = STRING_LIST_INIT_DUP, \
+	.failed = STRING_LIST_INIT_DUP, \
+}
 
 static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
 		     void **task_cb)
@@ -142,9 +145,6 @@ static int testsuite(int argc, const char **argv)
 		OPT_END()
 	};
 
-	memset(&suite, 0, sizeof(suite));
-	suite.tests.strdup_strings = suite.failed.strdup_strings = 1;
-
 	argc = parse_options(argc, argv, NULL, options,
 			testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
 
-- 
2.33.0.867.g88ec4638586

Re: [PATCH] test-tool run-command: fix confusing init pattern

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3777 bytes --]

Hi Ævar,

the commit title is misleading: it suggests that there is a bug that needs
to be fixed.

The idea of the patch, however, is to avoid redundant code, and if
described that way, the patch is a lot better for it.

On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote:

> In be5d88e1128 (test-tool run-command: learn to run (parts of) the
> testsuite, 2019-10-04) an init pattern was added that would use
> TESTSUITE_INIT, but then promptly memset() everything back to 0. We'd
> then set the "dup" on the two string lists. Our setting of "next" to
> "-1" thus did nothing, we'd reset it to "0" before using it.
>
> Let's just use the init macro for the STRING_LIST members, we can then
> remove the already redundant memset().
>
> Note that while we compile this code, there's no in-tree user for the
> "testsuite" target being modified here anymore, see the discussion at
> and around <nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet>[1].
>
> 1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> This patch is the immediate reason for why I submitted
> https://lore.kernel.org/git/patch-1.1-d1e464da0a9-20210906T002938Z-avarab@gmail.com/,
> since Johannes would prefer to keep it let's fix this init pattern.

The diff does too many things, some of which are your purely personal
preferences and do not actually need to be changed. This is a much more
to-the-point diff:

-- snip --
diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index 7ae03dc7123..14c57365e76 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -61,7 +61,7 @@ struct testsuite {
 	int quiet, immediate, verbose, verbose_log, trace, write_junit_xml;
 };
 #define TESTSUITE_INIT \
-	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }
+	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, 0, 0, 0, 0, 0, 0, 0 }

 static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
 		     void **task_cb)
@@ -142,9 +142,6 @@ static int testsuite(int argc, const char **argv)
 		OPT_END()
 	};

-	memset(&suite, 0, sizeof(suite));
-	suite.tests.strdup_strings = suite.failed.strdup_strings = 1;
-
 	argc = parse_options(argc, argv, NULL, options,
 			testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
-- snap --

I would strongly suggest to use this diff instead.

Ciao,
Johannes


>
>  t/helper/test-run-command.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
> index 7ae03dc7123..8e42516bdc1 100644
> --- a/t/helper/test-run-command.c
> +++ b/t/helper/test-run-command.c
> @@ -56,12 +56,15 @@ static int task_finished(int result,
>  }
>
>  struct testsuite {
> -	struct string_list tests, failed;
> +	struct string_list tests;
> +	struct string_list failed;
>  	int next;
>  	int quiet, immediate, verbose, verbose_log, trace, write_junit_xml;
>  };
> -#define TESTSUITE_INIT \
> -	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }
> +#define TESTSUITE_INIT { \
> +	.tests = STRING_LIST_INIT_DUP, \
> +	.failed = STRING_LIST_INIT_DUP, \
> +}
>
>  static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
>  		     void **task_cb)
> @@ -142,9 +145,6 @@ static int testsuite(int argc, const char **argv)
>  		OPT_END()
>  	};
>
> -	memset(&suite, 0, sizeof(suite));
> -	suite.tests.strdup_strings = suite.failed.strdup_strings = 1;
> -
>  	argc = parse_options(argc, argv, NULL, options,
>  			testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
>
> --
> 2.33.0.867.g88ec4638586
>
>

Re: [PATCH] test-tool run-command: fix confusing init pattern

[Ævar Arnfjörð Bjarmason]

On Fri, Sep 10 2021, Johannes Schindelin wrote:

> Hi Ævar,
>
> the commit title is misleading: it suggests that there is a bug that needs
> to be fixed.

I picked "confusing" because this doesn't impact the end-state of the
program, it's just confusing to do:

    x = -1

Followed by:

    x = 0

Which the reader might wonder about, only to find that the initial
assignment wasn't needed or used for anything.

> The idea of the patch, however, is to avoid redundant code, and if
> described that way, the patch is a lot better for it.
>
> On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote:
>
>> In be5d88e1128 (test-tool run-command: learn to run (parts of) the
>> testsuite, 2019-10-04) an init pattern was added that would use
>> TESTSUITE_INIT, but then promptly memset() everything back to 0. We'd
>> then set the "dup" on the two string lists. Our setting of "next" to
>> "-1" thus did nothing, we'd reset it to "0" before using it.
>>
>> Let's just use the init macro for the STRING_LIST members, we can then
>> remove the already redundant memset().
>>
>> Note that while we compile this code, there's no in-tree user for the
>> "testsuite" target being modified here anymore, see the discussion at
>> and around <nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet>[1].
>>
>> 1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>
>> This patch is the immediate reason for why I submitted
>> https://lore.kernel.org/git/patch-1.1-d1e464da0a9-20210906T002938Z-avarab@gmail.com/,
>> since Johannes would prefer to keep it let's fix this init pattern.
>
> The diff does too many things, some of which are your purely personal
> preferences and do not actually need to be changed. This is a much more
> to-the-point diff:

We've been slowly converting everything to designated initializers. It
seems to make sense to just do that if the line is being touched anyway.

For instance my d385784f89b (fsck.h: use designed initializers for
FSCK_OPTIONS_{DEFAULT,STRICT}, 2021-03-28) was part of a series whose
initial version just changed one field at a time as you're doing below,
but during early review I was asked just to use designated initializers
already.

But yes, it is strictly unrelated. It's a judgement call when to do
cleanups on lines you touch while you're at it, v.s. turning one patch
into a potential series of really tiny changes.

Then there's the change of "struct foo x, y" to "struct foo x;\nstruct
foo y;\n" above. I changed that because that tends to be the usual
style, it also preempts feedback / reviewers having to look at the code
out-of-bounds to see what the struct looks like in the default diff
-U<n>.

So, in advance addressing another type of question, or needing to
describe the context, as you brought up in
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091222260.59@tvgsbejvaqbjf.bet/
:)


> -- snip --
> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
> index 7ae03dc7123..14c57365e76 100644
> --- a/t/helper/test-run-command.c
> +++ b/t/helper/test-run-command.c
> @@ -61,7 +61,7 @@ struct testsuite {
>  	int quiet, immediate, verbose, verbose_log, trace, write_junit_xml;
>  };
>  #define TESTSUITE_INIT \
> -	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }
> +	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, 0, 0, 0, 0, 0, 0, 0 }
>
>  static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
>  		     void **task_cb)
> @@ -142,9 +142,6 @@ static int testsuite(int argc, const char **argv)
>  		OPT_END()
>  	};
>
> -	memset(&suite, 0, sizeof(suite));
> -	suite.tests.strdup_strings = suite.failed.strdup_strings = 1;
> -
>  	argc = parse_options(argc, argv, NULL, options,
>  			testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
> -- snap --
>
> I would strongly suggest to use this diff instead.
>
> Ciao,
> Johannes
>
>
>>
>>  t/helper/test-run-command.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
>> index 7ae03dc7123..8e42516bdc1 100644
>> --- a/t/helper/test-run-command.c
>> +++ b/t/helper/test-run-command.c
>> @@ -56,12 +56,15 @@ static int task_finished(int result,
>>  }
>>
>>  struct testsuite {
>> -	struct string_list tests, failed;
>> +	struct string_list tests;
>> +	struct string_list failed;
>>  	int next;
>>  	int quiet, immediate, verbose, verbose_log, trace, write_junit_xml;
>>  };
>> -#define TESTSUITE_INIT \
>> -	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }
>> +#define TESTSUITE_INIT { \
>> +	.tests = STRING_LIST_INIT_DUP, \
>> +	.failed = STRING_LIST_INIT_DUP, \
>> +}
>>
>>  static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
>>  		     void **task_cb)
>> @@ -142,9 +145,6 @@ static int testsuite(int argc, const char **argv)
>>  		OPT_END()
>>  	};
>>
>> -	memset(&suite, 0, sizeof(suite));
>> -	suite.tests.strdup_strings = suite.failed.strdup_strings = 1;
>> -
>>  	argc = parse_options(argc, argv, NULL, options,
>>  			testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
>>
>> --
>> 2.33.0.867.g88ec4638586
>>
>>

Re: [PATCH] test-tool run-command: fix confusing init pattern

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> The diff does too many things, some of which are your purely personal
>> preferences and do not actually need to be changed. This is a much more
>> to-the-point diff:
>
> We've been slowly converting everything to designated initializers. It
> seems to make sense to just do that if the line is being touched anyway.

Perhaps a preliminary clean-up patch is called for in such a case?

I do not think anybody can immediately see what the difference
between the old -1 and the new 0 in TESTSUITE_INIT macro means in
Dscho's alternative, but if we had a preliminary clean-up whose sole
change is to use designated initializers, the real "to-the-point"
step would become much easier to see which member that used to be
initialized to -1 is now getting zero-initialized.

And yes, changing the initializer style *and* the values the members
are initialized to in a same patch is much worse than sticking to
the style of the unreadable original.  It buries the real change in
the noise.

>>  #define TESTSUITE_INIT \
>> -	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }
>> +	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, 0, 0, 0, 0, 0, 0, 0 }

[PATCH v2] test-tool run-command: fix flip-flop init pattern

[Ævar Arnfjörð Bjarmason]

In be5d88e1128 (test-tool run-command: learn to run (parts of) the
testsuite, 2019-10-04) an init pattern was added that would use
TESTSUITE_INIT, but then promptly memset() everything back to 0. We'd
then set the "dup" on the two string lists.

Our setting of "next" to "-1" thus did nothing, we'd reset it to "0"
before using it. Let's set it to "0" instead, and trust the
"STRING_LIST_INIT_DUP" to set "strdup_strings" appropriately for us.

Note that while we compile this code, there's no in-tree user for the
"testsuite" target being modified here anymore, see the discussion at
and around <nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet>[1].

1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Range-diff against v1:
1:  0aa4523ab6e ! 1:  0ddf38b47ac test-tool run-command: fix confusing init pattern
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    test-tool run-command: fix confusing init pattern
    +    test-tool run-command: fix flip-flop init pattern
     
         In be5d88e1128 (test-tool run-command: learn to run (parts of) the
         testsuite, 2019-10-04) an init pattern was added that would use
         TESTSUITE_INIT, but then promptly memset() everything back to 0. We'd
    -    then set the "dup" on the two string lists. Our setting of "next" to
    -    "-1" thus did nothing, we'd reset it to "0" before using it.
    +    then set the "dup" on the two string lists.
     
    -    Let's just use the init macro for the STRING_LIST members, we can then
    -    remove the already redundant memset().
    +    Our setting of "next" to "-1" thus did nothing, we'd reset it to "0"
    +    before using it. Let's set it to "0" instead, and trust the
    +    "STRING_LIST_INIT_DUP" to set "strdup_strings" appropriately for us.
     
         Note that while we compile this code, there's no in-tree user for the
         "testsuite" target being modified here anymore, see the discussion at
    @@ Commit message
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## t/helper/test-run-command.c ##
    -@@ t/helper/test-run-command.c: static int task_finished(int result,
    - }
    - 
    - struct testsuite {
    --	struct string_list tests, failed;
    -+	struct string_list tests;
    -+	struct string_list failed;
    - 	int next;
    +@@ t/helper/test-run-command.c: struct testsuite {
      	int quiet, immediate, verbose, verbose_log, trace, write_junit_xml;
      };
    --#define TESTSUITE_INIT \
    + #define TESTSUITE_INIT \
     -	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }
    -+#define TESTSUITE_INIT { \
    -+	.tests = STRING_LIST_INIT_DUP, \
    -+	.failed = STRING_LIST_INIT_DUP, \
    -+}
    ++	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, 0, 0, 0, 0, 0, 0, 0 }
      
      static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
      		     void **task_cb)

 t/helper/test-run-command.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index 7ae03dc7123..14c57365e76 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -61,7 +61,7 @@ struct testsuite {
 	int quiet, immediate, verbose, verbose_log, trace, write_junit_xml;
 };
 #define TESTSUITE_INIT \
-	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }
+	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, 0, 0, 0, 0, 0, 0, 0 }
 
 static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
 		     void **task_cb)
@@ -142,9 +142,6 @@ static int testsuite(int argc, const char **argv)
 		OPT_END()
 	};
 
-	memset(&suite, 0, sizeof(suite));
-	suite.tests.strdup_strings = suite.failed.strdup_strings = 1;
-
 	argc = parse_options(argc, argv, NULL, options,
 			testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
 
-- 
2.33.0.995.ga5ea46173a2