Re: [PATCH v5 8/9] t/unit-tests: convert strvec tests to use clar

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Patrick
>
> On 16/08/2024 08:04, Patrick Steinhardt wrote:
>> diff --git a/t/unit-tests/unit-test.c b/t/unit-tests/unit-test.c
>> index 32a81299e9..82b7635e6a 100644
>> --- a/t/unit-tests/unit-test.c
>> +++ b/t/unit-tests/unit-test.c
>> @@ -6,10 +6,9 @@ int cmd_main(int argc, const char **argv)
>>   	int ret;
>>     	/* Append the "-t" flag such that the tests generate TAP
>> output. */
>> -	ALLOC_ARRAY(argv_copy, argc + 2);
>> +	ALLOC_ARRAY(argv_copy, argc + 1);
>>   	COPY_ARRAY(argv_copy, argv, argc);
>>   	argv_copy[argc++] = "-t";
>> -	argv_copy[argc] = NULL;
>
> I'm confused by this - it looks like it belongs in the previous patch
> but why are we deleting the line that adds the terminating NULL to
> argv in the first place?

I agree with you that it belongs to the previous step.  clar_test(),
unlike the usual convention used by main(), works solely with argc
to tell where the list of argument ends, without requiring argv[] to
be an NULL terminated array.  I do not know if clar _promises_ that
as the calling convention to its users, but if it does so, and if we
want to take advantage of that promise and leave the terminating
NULL out of argv[], we should do so from the beginning.

A NULL-terminated argv[], as long as we make sure argc only counts
the real arguments without counting that terminating NULL, would not
harm the clar_test(argc, argv) call, so I actually do not mind the
way this caller was originally written, with terminating NULL.  It
looks a lot more familiar, even though we may be wasting memory at
the end of argv[] array.