Re: [PATCH v2 1/2] u-string-list: add unit tests for string-list methods

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 26, 2026, 19:50 UTC
Message-ID: <xmqqa4y0i25m.fsf@gitster.g>
In-Reply-To: <20260126185604.90089-1-amishhhaaaa@gmail.com>

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

Show 12 quoted lines

> +void test_string_list__remove(void)
> +{
> +	struct string_list expected_strings = STRING_LIST_INIT_DUP;
> +	struct string_list list = STRING_LIST_INIT_DUP;
> +
> +	t_create_string_list_dup(&expected_strings, 0, NULL);
> +	t_create_string_list_dup(&list, 0, NULL);
> +	t_string_list_remove(&expected_strings, &list, "");
> +
> +	t_create_string_list_dup(&expected_strings, 0, "a", NULL);
> +	t_create_string_list_dup(&list, 0, "a", "a", NULL);
> +	t_string_list_remove(&expected_strings, &list, "a");

Not a complaint, not a suggestion to change anything, but just an observation. While "remove" requires the string-list to be sorted, its implementation does not seem to care if you by mistake fed an unsorted string list.

After seeing this particular test that feeds a list with two "a" and expects in the resulting list a single "a", I naturally wondered which one of these two "a" survives and which one is dropped.

"remove" removes only one matching element that is picked at random among the duplicates, but because the input is expected to be sorted, these duplicate elements sit next to each other forming a single strand of identical pearls. The end result of picking one of these pearls out would not be different no matter which one of them you pick. So the answer to my "which one of these 'a'?" question turns out to be "you cannot tell, but it does not matter" ;-)

Show 14 quoted lines

> +static void t_string_list_remove_empty_items(struct string_list *expected_strings, struct string_list *list)
> +{
> +	string_list_remove_empty_items(list, 0);
> +	t_string_list_equal(list, expected_strings);
> +}
> +
> +void test_string_list__remove_empty_items(void)
> +{
> +	struct string_list expected_strings = STRING_LIST_INIT_DUP;
> +	struct string_list list = STRING_LIST_INIT_DUP;
> +
> +	t_create_string_list_dup(&expected_strings, 0, NULL);
> +	t_create_string_list_dup(&list, 0, "", "", "", NULL);
> +	t_string_list_remove_empty_items(&expected_strings, &list);

Again, not a complaint, not a suggestion to change anything, but just an observation. As we saw earlier, "remove" is "remove just one of many", but "remove_empty" is "remove all empties". Simply makes me wonder if the API looked more sane if we had "remove_all" whose signature is the same as string_list_remove().

> +static void t_string_list_unsorted_string_list_delete_item(struct string_list *expected_list, struct string_list *list, int i)

This is a way overlong line.

Show 14 quoted lines

> +{
> +	unsorted_string_list_delete_item(list, i, 0);
> +
> +	t_string_list_equal(list, expected_list);
> +}
> +
> +void test_string_list__unsorted_string_list_delete_item(void)
> +{
> +	struct string_list expected_strings = STRING_LIST_INIT_DUP;
> +	struct string_list list = STRING_LIST_INIT_DUP;
> +
> +	t_create_string_list_dup(&expected_strings, 0, "a", "c", "b", NULL);
> +	t_create_string_list_dup(&list, 0, "a", "d", "b", "c", NULL);
> +	t_string_list_unsorted_string_list_delete_item(&expected_strings, &list, 1);

This demonstrates one peculiar aspect of this "delete item from unsorted list" API function very well. If one is expected to name an element to delete by specifying its position in the list, it is natural to expect that the elements in the list to be ordered in some way that is meaningful to the application [*], and the API is not expected to shuffle the resulting list in such a way that makes further use of the list cumbersome. Yet, the function does exactly that by moving the element at the end of the list to the place the location of the deleted element.

	Side note: [*] The "unsorted" in the name of the function is
	a reference to the fact that the elements are not sorted by
	the natural order string-list API uses to allow it to binary
	search; it does not mean the elements are entirely randomly
	thrown in and it shouldn't mean that the application cannot
	rely on

The only caller of this function is git.c::list_cmds() that is asked to remove the helper binaries (i.e., those whose name contains "--"), so even though git.c::commands[] list is in sorted order, and the list_builtins() function slurps them into a working list with string_list_append(), processing "nohelpers" will splinkle command names from near the tail of the list into random places in the middle of the list.

> +	t_string_list_clear(&expected_strings, 0);
> +	t_string_list_clear(&list, 0);
> +}
> \ No newline at end of file

Don't.  Always end a text file with a complete line, please.