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

[Patrick Steinhardt <ps@pks.im>]

On Fri, Aug 16, 2024 at 09:11:36AM -0700, Junio C Hamano wrote:
> 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.

Agreed.

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

Okay. It's not necessary, but I don't mind it much, either.

Patrick