Re: [PATCH 2/5] u-string-list: move "test_split" into "u-string-list.c"

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 22, 2025, 21:27 UTC
Message-ID: <xmqqr01j3n15.fsf@gitster.g>
In-Reply-To: <aAetv8l8jrxvEywB@ArchLinux>

shejialuo <shejialuo@gmail.com> writes:

Show 19 quoted lines

> We rely on "test-tool string-list" command to test the functionality of
> the "string-list". However, as we have introduced clar test framework,
> we'd better move the shell script into C program to improve speed and
> readability.
>
> Create a new file "u-string-list.c" under "t/unit-tests", then update
> the Makefile and "meson.build" to build the file. And let's first move
> "test_split" into unit test and gradually convert the shell script into
> C program.
>
> In order to create `string_list` easily by simply specifying strings in
> the function call, create "t_vcreate_string_list_dup" and
> "t_create_string_list_dup" functions to do above.
>
> Then port the shell script tests to C program and remove unused
> "test-tool" code and tests.
>
> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---

This is the most interesting in the u-string-list patches, as it adds not just a moved test but adds supporting functions that are shared with test functions added in later steps.

Show 19 quoted lines

> diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c
> new file mode 100644
> index 0000000000..0c148684ea
> --- /dev/null
> +++ b/t/unit-tests/u-string-list.c
> @@ -0,0 +1,86 @@
> +#include "unit-test.h"
> +#include "string-list.h"
> +
> +static void t_check_string_list(struct string_list *list,
> +				struct string_list *expected_strings)
> +{
> +	size_t expect_len = expected_strings->nr;
> +	cl_assert_equal_i(list->nr, expect_len);
> +	cl_assert(list->nr <= list->alloc);
> +	for (size_t i = 0; i < expect_len; i++)
> +		cl_assert_equal_s(list->items[i].string,
> +				  expected_strings->items[i].string);
> +}

Perhaps call it "string_list_equal()" or something? "check" is a convenient name that can mean different kind of validation that is not limited to "is the actual answer identical to the expected one?"

Wouldn't it be cleaner to read if you wrote it without an extra variable expect_len? The compiler would notice repeated reference of expected_strings->nr and optimize them away anyway, I would imagine.

Show 7 quoted lines

> +static void t_string_list_clear(struct string_list *list, int free_util)
> +{
> +	string_list_clear(list, free_util);
> +	cl_assert_equal_p(list->items, NULL);
> +	cl_assert_equal_i(list->nr, 0);
> +	cl_assert_equal_i(list->alloc, 0);
> +}

Validating the result of clearing a list may be a good thing to do at least once in the test suite, but this is called from many places in other tests. Conceptually it feels kludgy to call this from other places where they should all just call string_list_clear(), like ...

Show 8 quoted lines

> +static void t_vcreate_string_list_dup(struct string_list *list,
> +				      int free_util, va_list ap)
> +{
> +	const char *arg;
> +
> +	cl_assert(list->strdup_strings);
> +
> +	t_string_list_clear(list, free_util);

... this place.

> +	while ((arg = va_arg(ap, const char *)))
> +		string_list_append(list, arg);
> +}

To put it differently, you could be calling t_string_list_append() in this loop, which would

 - remember list->nr
 - call string_list_append()
 - cl_assert_equal() to ensure that list->nr is one larger than
   the value we remembered upon entry to the function.

which is not wrong per-se, but hopefully you'd agree that it is overkill. t_stirng_list_clear() is overkill in the same way.

Show 11 quoted lines

> +void test_string_list__split(void)
> +{
> +	struct string_list expected_strings = STRING_LIST_INIT_DUP;
> +
> +	t_create_string_list_dup(&expected_strings, 0, "foo", "bar", "baz", NULL);
> +...
> +	t_create_string_list_dup(&expected_strings, 0, "", "", NULL);
> +	t_string_list_split(":", ':', -1, &expected_strings);
> +
> +	t_string_list_clear(&expected_strings, 0);
> +}

This is a good place to call t_string_list_clear(), just once in this script. All other callers are conceptually simpler to call string_list_clear(), as the point at their callsites is to clear after themselves, not about testing string_list_clear() works correctly.

Thanks.