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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 10, 2021, 19:32 UTC
Message-ID: <87pmtgb4m1.fsf@evledraar.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.2109101319110.59@tvgsbejvaqbjf.bet>

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.

Show 30 quoted lines

> 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/ :)

Show 72 quoted lines

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