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@xxxxxxxxxxxxxxxxx>[1].
>>
>> 1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@xxxxxxxxxxxxxxxxx/
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
>> ---
>>
>> This patch is the immediate reason for why I submitted
>> https://lore.kernel.org/git/patch-1.1-d1e464da0a9-20210906T002938Z-avarab@xxxxxxxxx/,
>> 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@xxxxxxxxxxxxxxxxx/
:)
> -- 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
>>
>>
