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

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

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

> +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" ;-)

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

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