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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 10, 2021, 11:23 UTC
Message-ID: <nycvar.QRO.7.76.6.2109101319110.59@tvgsbejvaqbjf.bet>
In-Reply-To: <patch-1.1-0aa4523ab6e-20210909T130849Z-avarab@gmail.com>

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:

Show 21 quoted lines

> 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
>
>