[PATCH 0/5] enhance "string_list" code and test

[PATCH 0/5] enhance "string_list" code and test

[shejialuo]

Hi all:

During I study and learn the Git source code, I have found something
which could be improved for "string_list".

And this patch mainly enhances the "string_list" code and test.

    1. For code, I mainly fix sign compare warnings.
    2. For test, I move the shell script to clar based unit test.

Thanks,
Jialuo

shejialuo (5):
  string-list: fix sign compare warnings
  u-string-list: move "test_split" into "u-string-list.c"
  u-string-list: move "test_split_in_place" to "u-string-list.c"
  u-string-list: move "filter string" test to "u-string-list.c"
  u-string-list: move "remove duplicates" test to "u-string-list.c"

 Makefile                     |   1 +
 string-list.c                |  30 ++---
 t/helper/test-string-list.c  |  96 --------------
 t/meson.build                |   2 +-
 t/t0063-string-list.sh       | 142 ---------------------
 t/unit-tests/u-string-list.c | 238 +++++++++++++++++++++++++++++++++++
 6 files changed, 253 insertions(+), 256 deletions(-)
 delete mode 100755 t/t0063-string-list.sh
 create mode 100644 t/unit-tests/u-string-list.c

-- 
2.49.0

[PATCH 1/5] string-list: fix sign compare warnings

[shejialuo]

In "string-list.c", there are six warnings which are emitted by
"Wsign-compare". And five warnings are caused by the loop iterator type
mismatch, which could be simply fixed by changing the `int` type to
`size_t` type.

However, for "string-list.c::add_entry" function, we compare the `index`
of the `int` type with the `list->nr` of unsigned type. It seems that
we could just simply convert the type of `index` from `int` to
`size_t`. But actually this is a correct behavior.

We would set the `index` value by checking whether `insert_at` is -1.
If not, we would set `index` to be `insert_at`, otherwise we would use
"get_entry_index` to find the inserted position.

What if the caller passes a negative value except "-1", the compiler
would convert the `index` to be a positive value which would make the
`if` statement be false to avoid moving array. However, we would
definitely encounter trouble when setting the inserted item.

And we only call "add_entry" in "string_list_insert" function, and we
simply pass "-1" for "insert_at" parameter. So, we never use this
parameter to insert element in a user specified position. Let's delete
this parameter. If there is any requirement later, we may use a better
way to do this. And then we could safely convert the index to be
`size_t` when comparing.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 string-list.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/string-list.c b/string-list.c
index bf061fec56..a967421b60 100644
--- a/string-list.c
+++ b/string-list.c
@@ -1,5 +1,3 @@
-#define DISABLE_SIGN_COMPARE_WARNINGS
-
 #include "git-compat-util.h"
 #include "string-list.h"
 
@@ -41,16 +39,16 @@ static int get_entry_index(const struct string_list *list, const char *string,
 }
 
 /* returns -1-index if already exists */
-static int add_entry(int insert_at, struct string_list *list, const char *string)
+static int add_entry(struct string_list *list, const char *string)
 {
 	int exact_match = 0;
-	int index = insert_at != -1 ? insert_at : get_entry_index(list, string, &exact_match);
+	int index = get_entry_index(list, string, &exact_match);
 
 	if (exact_match)
 		return -1 - index;
 
 	ALLOC_GROW(list->items, list->nr+1, list->alloc);
-	if (index < list->nr)
+	if ((size_t)index < list->nr)
 		MOVE_ARRAY(list->items + index + 1, list->items + index,
 			   list->nr - index);
 	list->items[index].string = list->strdup_strings ?
@@ -63,7 +61,7 @@ static int add_entry(int insert_at, struct string_list *list, const char *string
 
 struct string_list_item *string_list_insert(struct string_list *list, const char *string)
 {
-	int index = add_entry(-1, list, string);
+	int index = add_entry(list, string);
 
 	if (index < 0)
 		index = -1 - index;
@@ -116,7 +114,7 @@ struct string_list_item *string_list_lookup(struct string_list *list, const char
 void string_list_remove_duplicates(struct string_list *list, int free_util)
 {
 	if (list->nr > 1) {
-		int src, dst;
+		size_t src, dst;
 		compare_strings_fn cmp = list->cmp ? list->cmp : strcmp;
 		for (src = dst = 1; src < list->nr; src++) {
 			if (!cmp(list->items[dst - 1].string, list->items[src].string)) {
@@ -134,8 +132,8 @@ void string_list_remove_duplicates(struct string_list *list, int free_util)
 int for_each_string_list(struct string_list *list,
 			 string_list_each_func_t fn, void *cb_data)
 {
-	int i, ret = 0;
-	for (i = 0; i < list->nr; i++)
+	int ret = 0;
+	for (size_t i = 0; i < list->nr; i++)
 		if ((ret = fn(&list->items[i], cb_data)))
 			break;
 	return ret;
@@ -144,8 +142,8 @@ int for_each_string_list(struct string_list *list,
 void filter_string_list(struct string_list *list, int free_util,
 			string_list_each_func_t want, void *cb_data)
 {
-	int src, dst = 0;
-	for (src = 0; src < list->nr; src++) {
+	size_t dst = 0;
+	for (size_t src = 0; src < list->nr; src++) {
 		if (want(&list->items[src], cb_data)) {
 			list->items[dst++] = list->items[src];
 		} else {
@@ -171,13 +169,12 @@ void string_list_remove_empty_items(struct string_list *list, int free_util)
 void string_list_clear(struct string_list *list, int free_util)
 {
 	if (list->items) {
-		int i;
 		if (list->strdup_strings) {
-			for (i = 0; i < list->nr; i++)
+			for (size_t i = 0; i < list->nr; i++)
 				free(list->items[i].string);
 		}
 		if (free_util) {
-			for (i = 0; i < list->nr; i++)
+			for (size_t i = 0; i < list->nr; i++)
 				free(list->items[i].util);
 		}
 		free(list->items);
@@ -189,13 +186,12 @@ void string_list_clear(struct string_list *list, int free_util)
 void string_list_clear_func(struct string_list *list, string_list_clear_func_t clearfunc)
 {
 	if (list->items) {
-		int i;
 		if (clearfunc) {
-			for (i = 0; i < list->nr; i++)
+			for (size_t i = 0; i < list->nr; i++)
 				clearfunc(list->items[i].util, list->items[i].string);
 		}
 		if (list->strdup_strings) {
-			for (i = 0; i < list->nr; i++)
+			for (size_t i = 0; i < list->nr; i++)
 				free(list->items[i].string);
 		}
 		free(list->items);
-- 
2.49.0

Re: [PATCH 1/5] string-list: fix sign compare warnings

[Junio C Hamano]

shejialuo <shejialuo@gmail.com> writes:

> However, for "string-list.c::add_entry" function, we compare the `index`
> of the `int` type with the `list->nr` of unsigned type. It seems that
> we could just simply convert the type of `index` from `int` to
> `size_t`. But actually this is a correct behavior.

Sorry, but I am lost by the last sentence.

"this" that is a correct behavior refers to...?  That the incoming
parameter insert_at and the local variable index are both of signed
integer?

> We would set the `index` value by checking whether `insert_at` is -1.
> If not, we would set `index` to be `insert_at`, otherwise we would use
> "get_entry_index` to find the inserted position.

To rephrase the above (simply because the above is literal English
translation from what C says), the caller either can pass -1 to mean
"find an appropriate location in the list to keep it sorted", or an
index into the list->items[] array to specify exactly where the item
should be inserted.

Naturally, insert_at must be either -1 (auto), or between 0
(i.e. the candidate is smaller than anything in the list) and
list->nr (i.e. the candidate is larger than everything in the list)
inclusive.  Any other value is invalid.  I think that is a more
appropriate thing to say than ...

> What if the caller passes a negative value except "-1", the compiler
> would convert the `index` to be a positive value which would make the
> `if` statement be false to avoid moving array. However, we would
> definitely encounter trouble when setting the inserted item.

... this paragraph.  Not moving is _not_ avoiding problem, so it is
immaterial.  The lack of valid range check before using the index
is.

> And we only call "add_entry" in "string_list_insert" function, and we
> simply pass "-1" for "insert_at" parameter. So, we never use this
> parameter to insert element in a user specified position. Let's delete
> this parameter. If there is any requirement later, we may use a better
> way to do this. And then we could safely convert the index to be
> `size_t` when comparing.

Good.  As we only use the "auto" setting with this code now, as long
as get_entry_index() returns a value between 0 and list->nr, the
lack of such range checking in the original code no longer is an
issue.

Having said that, in the longer run, get_entry_index() would want to
return size_t simply because it is returning a value between 0 and
list->nr, whose type is size_t.   left/mid/right variables also need
to become size_t and the loop initialization may have to be tweaked
(since the current code strangely starts left with -1 which would
never be the index into the array), but fixing that should probably
make the loop easier to read, which is a bonus.

And add_entry(), since it needs to do the usual -1-pos dance to
indicate where things would have been returned, would return
ssize_t---or better yet, it can just turned into returning size_t
with an extra out parameter (just like the exact_match out parameter
get_entry_index() has) to indicate if we already had the same item
in the list already.  It is perfectly fine to leave it outside the
scope of this series, but if you are tweaking all the callers of
add_entry() anyway in this step, you may want to bite the bullet and
just go all the way.

Thanks.

Re: [PATCH 1/5] string-list: fix sign compare warnings

[shejialuo]

On Tue, Apr 22, 2025 at 02:02:18PM -0700, Junio C Hamano wrote:
> shejialuo <shejialuo@gmail.com> writes:
> 
> > However, for "string-list.c::add_entry" function, we compare the `index`
> > of the `int` type with the `list->nr` of unsigned type. It seems that
> > we could just simply convert the type of `index` from `int` to
> > `size_t`. But actually this is a correct behavior.
> 
> Sorry, but I am lost by the last sentence.
> 
> "this" that is a correct behavior refers to...?  That the incoming
> parameter insert_at and the local variable index are both of signed
> integer?
> 

Well, please ignore. I think I made a mistake. I should not say this is
correct but say that this is not harmful.

> > We would set the `index` value by checking whether `insert_at` is -1.
> > If not, we would set `index` to be `insert_at`, otherwise we would use
> > "get_entry_index` to find the inserted position.
> 
> To rephrase the above (simply because the above is literal English
> translation from what C says), the caller either can pass -1 to mean
> "find an appropriate location in the list to keep it sorted", or an
> index into the list->items[] array to specify exactly where the item
> should be inserted.
> 
> Naturally, insert_at must be either -1 (auto), or between 0
> (i.e. the candidate is smaller than anything in the list) and
> list->nr (i.e. the candidate is larger than everything in the list)
> inclusive.  Any other value is invalid.  I think that is a more
> appropriate thing to say than ...
> 

Yes, thanks for the hint.

> > What if the caller passes a negative value except "-1", the compiler
> > would convert the `index` to be a positive value which would make the
> > `if` statement be false to avoid moving array. However, we would
> > definitely encounter trouble when setting the inserted item.
> 
> ... this paragraph.  Not moving is _not_ avoiding problem, so it is
> immaterial.  The lack of valid range check before using the index
> is.
> 

Yes, that's right. Ideally, we should check whether the index is OK. And
I somehow think that I should not make the commit message complicate. I
should just say we should remove "insert_at" function in a separate
commit and then fix sign comparing warnings.

> > And we only call "add_entry" in "string_list_insert" function, and we
> > simply pass "-1" for "insert_at" parameter. So, we never use this
> > parameter to insert element in a user specified position. Let's delete
> > this parameter. If there is any requirement later, we may use a better
> > way to do this. And then we could safely convert the index to be
> > `size_t` when comparing.
> 
> Good.  As we only use the "auto" setting with this code now, as long
> as get_entry_index() returns a value between 0 and list->nr, the
> lack of such range checking in the original code no longer is an
> issue.
> 
> Having said that, in the longer run, get_entry_index() would want to
> return size_t simply because it is returning a value between 0 and
> list->nr, whose type is size_t.   left/mid/right variables also need
> to become size_t and the loop initialization may have to be tweaked
> (since the current code strangely starts left with -1 which would
> never be the index into the array), but fixing that should probably
> make the loop easier to read, which is a bonus.
> 

That's right. And I would clean the code a bit more.

> And add_entry(), since it needs to do the usual -1-pos dance to
> indicate where things would have been returned, would return
> ssize_t---or better yet, it can just turned into returning size_t
> with an extra out parameter (just like the exact_match out parameter
> get_entry_index() has) to indicate if we already had the same item
> in the list already.  It is perfectly fine to leave it outside the
> scope of this series, but if you are tweaking all the callers of
> add_entry() anyway in this step, you may want to bite the bullet and
> just go all the way.
> 

I agree. I'll add more patches to clean.

> Thanks.

Thanks for the review.

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

[shejialuo]

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>
---
 Makefile                     |  1 +
 t/helper/test-string-list.c  | 14 ------
 t/meson.build                |  1 +
 t/t0063-string-list.sh       | 53 ----------------------
 t/unit-tests/u-string-list.c | 86 ++++++++++++++++++++++++++++++++++++
 5 files changed, 88 insertions(+), 67 deletions(-)
 create mode 100644 t/unit-tests/u-string-list.c

diff --git a/Makefile b/Makefile
index 13f9062a05..58df1f1150 100644
--- a/Makefile
+++ b/Makefile
@@ -1365,6 +1365,7 @@ CLAR_TEST_SUITES += u-prio-queue
 CLAR_TEST_SUITES += u-reftable-tree
 CLAR_TEST_SUITES += u-strbuf
 CLAR_TEST_SUITES += u-strcmp-offset
+CLAR_TEST_SUITES += u-string-list
 CLAR_TEST_SUITES += u-strvec
 CLAR_TEST_SUITES += u-trailer
 CLAR_TEST_SUITES += u-urlmatch-normalization
diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c
index 6f10c5a435..17c18c30f6 100644
--- a/t/helper/test-string-list.c
+++ b/t/helper/test-string-list.c
@@ -46,20 +46,6 @@ static int prefix_cb(struct string_list_item *item, void *cb_data)
 
 int cmd__string_list(int argc, const char **argv)
 {
-	if (argc == 5 && !strcmp(argv[1], "split")) {
-		struct string_list list = STRING_LIST_INIT_DUP;
-		int i;
-		const char *s = argv[2];
-		int delim = *argv[3];
-		int maxsplit = atoi(argv[4]);
-
-		i = string_list_split(&list, s, delim, maxsplit);
-		printf("%d\n", i);
-		write_list(&list);
-		string_list_clear(&list, 0);
-		return 0;
-	}
-
 	if (argc == 5 && !strcmp(argv[1], "split_in_place")) {
 		struct string_list list = STRING_LIST_INIT_NODUP;
 		int i;
diff --git a/t/meson.build b/t/meson.build
index bfb744e886..424e7e445f 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -11,6 +11,7 @@ clar_test_suites = [
   'unit-tests/u-reftable-tree.c',
   'unit-tests/u-strbuf.c',
   'unit-tests/u-strcmp-offset.c',
+  'unit-tests/u-string-list.c',
   'unit-tests/u-strvec.c',
   'unit-tests/u-trailer.c',
   'unit-tests/u-urlmatch-normalization.c',
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
index aac63ba506..6b20ffd206 100755
--- a/t/t0063-string-list.sh
+++ b/t/t0063-string-list.sh
@@ -7,16 +7,6 @@ test_description='Test string list functionality'
 
 . ./test-lib.sh
 
-test_split () {
-	cat >expected &&
-	test_expect_success "split $1 at $2, max $3" "
-		test-tool string-list split '$1' '$2' '$3' >actual &&
-		test_cmp expected actual &&
-		test-tool string-list split_in_place '$1' '$2' '$3' >actual &&
-		test_cmp expected actual
-	"
-}
-
 test_split_in_place() {
 	cat >expected &&
 	test_expect_success "split (in place) $1 at $2, max $3" "
@@ -25,49 +15,6 @@ test_split_in_place() {
 	"
 }
 
-test_split "foo:bar:baz" ":" "-1" <<EOF
-3
-[0]: "foo"
-[1]: "bar"
-[2]: "baz"
-EOF
-
-test_split "foo:bar:baz" ":" "0" <<EOF
-1
-[0]: "foo:bar:baz"
-EOF
-
-test_split "foo:bar:baz" ":" "1" <<EOF
-2
-[0]: "foo"
-[1]: "bar:baz"
-EOF
-
-test_split "foo:bar:baz" ":" "2" <<EOF
-3
-[0]: "foo"
-[1]: "bar"
-[2]: "baz"
-EOF
-
-test_split "foo:bar:" ":" "-1" <<EOF
-3
-[0]: "foo"
-[1]: "bar"
-[2]: ""
-EOF
-
-test_split "" ":" "-1" <<EOF
-1
-[0]: ""
-EOF
-
-test_split ":" ":" "-1" <<EOF
-2
-[0]: ""
-[1]: ""
-EOF
-
 test_split_in_place "foo:;:bar:;:baz:;:" ":;" "-1" <<EOF
 10
 [0]: "foo"
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);
+}
+
+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);
+}
+
+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);
+	while ((arg = va_arg(ap, const char *)))
+		string_list_append(list, arg);
+}
+
+static void t_create_string_list_dup(struct string_list *list, int free_util, ...)
+{
+	va_list ap;
+
+	cl_assert(list->strdup_strings);
+
+	t_string_list_clear(list, free_util);
+	va_start(ap, free_util);
+	t_vcreate_string_list_dup(list, free_util, ap);
+	va_end(ap);
+}
+
+static void t_string_list_split(const char *data, int delim, int maxsplit,
+				struct string_list *expected_strings)
+{
+	struct string_list list = STRING_LIST_INIT_DUP;
+	int len;
+
+	len = string_list_split(&list, data, delim, maxsplit);
+	cl_assert_equal_i(len, expected_strings->nr);
+	t_check_string_list(&list, expected_strings);
+
+	t_string_list_clear(&list, 0);
+}
+
+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_string_list_split("foo:bar:baz", ':', -1, &expected_strings);
+
+	t_create_string_list_dup(&expected_strings, 0, "foo:bar:baz", NULL);
+	t_string_list_split("foo:bar:baz", ':', 0, &expected_strings);
+
+	t_create_string_list_dup(&expected_strings, 0, "foo", "bar:baz", NULL);
+	t_string_list_split("foo:bar:baz", ':', 1, &expected_strings);
+
+	t_create_string_list_dup(&expected_strings, 0, "foo", "bar", "baz", NULL);
+	t_string_list_split("foo:bar:baz", ':', 2, &expected_strings);
+
+	t_create_string_list_dup(&expected_strings, 0, "foo", "bar", "", NULL);
+	t_string_list_split("foo:bar:", ':', -1, &expected_strings);
+
+	t_create_string_list_dup(&expected_strings, 0, "", NULL);
+	t_string_list_split("", ':', -1, &expected_strings);
+
+	t_create_string_list_dup(&expected_strings, 0, "", "", NULL);
+	t_string_list_split(":", ':', -1, &expected_strings);
+
+	t_string_list_clear(&expected_strings, 0);
+}
-- 
2.49.0

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

[Junio C Hamano]

shejialuo <shejialuo@gmail.com> writes:

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

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

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

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

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

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

[shejialuo]

On Tue, Apr 22, 2025 at 02:27:02PM -0700, Junio C Hamano wrote:
> shejialuo <shejialuo@gmail.com> writes:
> 
> > 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.
> 
> > 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?"
> 

Good idea.

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

Yeah, the compiler would definitely optimize this. Will update in the
next version.

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

I agree that we should not call `t_string_list_clear` in many places.
It's overkill.

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

You're right. I will improve this in the next version.

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

Thanks,
Jialuo
> 

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

[Patrick Steinhardt]

On Tue, Apr 22, 2025 at 10:54:55PM +0800, shejialuo wrote:
> 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 @@
> +static void t_string_list_split(const char *data, int delim, int maxsplit,
> +				struct string_list *expected_strings)
> +{
> +	struct string_list list = STRING_LIST_INIT_DUP;
> +	int len;
> +
> +	len = string_list_split(&list, data, delim, maxsplit);
> +	cl_assert_equal_i(len, expected_strings->nr);
> +	t_check_string_list(&list, expected_strings);
> +
> +	t_string_list_clear(&list, 0);
> +}
> +
> +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_string_list_split("foo:bar:baz", ':', -1, &expected_strings);

Could we adapt `t_string_list_split()` so that it accepts the expected
strings as varargs? If so we could simplify the logic in this function
here.

Patrick

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

[shejialuo]

On Wed, Apr 23, 2025 at 01:52:58PM +0200, Patrick Steinhardt wrote:
> On Tue, Apr 22, 2025 at 10:54:55PM +0800, shejialuo wrote:
> > 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 @@
> > +static void t_string_list_split(const char *data, int delim, int maxsplit,
> > +				struct string_list *expected_strings)
> > +{
> > +	struct string_list list = STRING_LIST_INIT_DUP;
> > +	int len;
> > +
> > +	len = string_list_split(&list, data, delim, maxsplit);
> > +	cl_assert_equal_i(len, expected_strings->nr);
> > +	t_check_string_list(&list, expected_strings);
> > +
> > +	t_string_list_clear(&list, 0);
> > +}
> > +
> > +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_string_list_split("foo:bar:baz", ':', -1, &expected_strings);
> 
> Could we adapt `t_string_list_split()` so that it accepts the expected
> strings as varargs? If so we could simplify the logic in this function
> here.
> 

That's a good suggestion. I will update in the next version.

> Patrick

[PATCH 3/5] u-string-list: move "test_split_in_place" to "u-string-list.c"

[shejialuo]

We use "test-tool string-list split_in_place" to test the
"string_list_split_in_place" function. As we have introduced the unit
test, we'd better remove the logic from shell script to C program to
improve test speed and readability.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 t/helper/test-string-list.c  | 22 ----------------
 t/t0063-string-list.sh       | 51 ------------------------------------
 t/unit-tests/u-string-list.c | 39 +++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 73 deletions(-)

diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c
index 17c18c30f6..8a344347ad 100644
--- a/t/helper/test-string-list.c
+++ b/t/helper/test-string-list.c
@@ -18,13 +18,6 @@ static void parse_string_list(struct string_list *list, const char *arg)
 	(void)string_list_split(list, arg, ':', -1);
 }
 
-static void write_list(const struct string_list *list)
-{
-	int i;
-	for (i = 0; i < list->nr; i++)
-		printf("[%d]: \"%s\"\n", i, list->items[i].string);
-}
-
 static void write_list_compact(const struct string_list *list)
 {
 	int i;
@@ -46,21 +39,6 @@ static int prefix_cb(struct string_list_item *item, void *cb_data)
 
 int cmd__string_list(int argc, const char **argv)
 {
-	if (argc == 5 && !strcmp(argv[1], "split_in_place")) {
-		struct string_list list = STRING_LIST_INIT_NODUP;
-		int i;
-		char *s = xstrdup(argv[2]);
-		const char *delim = argv[3];
-		int maxsplit = atoi(argv[4]);
-
-		i = string_list_split_in_place(&list, s, delim, maxsplit);
-		printf("%d\n", i);
-		write_list(&list);
-		string_list_clear(&list, 0);
-		free(s);
-		return 0;
-	}
-
 	if (argc == 4 && !strcmp(argv[1], "filter")) {
 		/*
 		 * Retain only the items that have the specified prefix.
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
index 6b20ffd206..1a9cf8bfcf 100755
--- a/t/t0063-string-list.sh
+++ b/t/t0063-string-list.sh
@@ -7,57 +7,6 @@ test_description='Test string list functionality'
 
 . ./test-lib.sh
 
-test_split_in_place() {
-	cat >expected &&
-	test_expect_success "split (in place) $1 at $2, max $3" "
-		test-tool string-list split_in_place '$1' '$2' '$3' >actual &&
-		test_cmp expected actual
-	"
-}
-
-test_split_in_place "foo:;:bar:;:baz:;:" ":;" "-1" <<EOF
-10
-[0]: "foo"
-[1]: ""
-[2]: ""
-[3]: "bar"
-[4]: ""
-[5]: ""
-[6]: "baz"
-[7]: ""
-[8]: ""
-[9]: ""
-EOF
-
-test_split_in_place "foo:;:bar:;:baz" ":;" "0" <<EOF
-1
-[0]: "foo:;:bar:;:baz"
-EOF
-
-test_split_in_place "foo:;:bar:;:baz" ":;" "1" <<EOF
-2
-[0]: "foo"
-[1]: ";:bar:;:baz"
-EOF
-
-test_split_in_place "foo:;:bar:;:baz" ":;" "2" <<EOF
-3
-[0]: "foo"
-[1]: ""
-[2]: ":bar:;:baz"
-EOF
-
-test_split_in_place "foo:;:bar:;:" ":;" "-1" <<EOF
-7
-[0]: "foo"
-[1]: ""
-[2]: ""
-[3]: "bar"
-[4]: ""
-[5]: ""
-[6]: ""
-EOF
-
 test_expect_success "test filter_string_list" '
 	test "x-" = "x$(test-tool string-list filter - y)" &&
 	test "x-" = "x$(test-tool string-list filter no y)" &&
diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c
index 0c148684ea..44ec8de3d0 100644
--- a/t/unit-tests/u-string-list.c
+++ b/t/unit-tests/u-string-list.c
@@ -84,3 +84,42 @@ void test_string_list__split(void)
 
 	t_string_list_clear(&expected_strings, 0);
 }
+
+static void t_string_list_split_in_place(const char *data, const char *delim, int maxsplit,
+					 struct string_list *expected_strings)
+{
+	struct string_list list = STRING_LIST_INIT_NODUP;
+
+	char *string = xstrdup(data);
+
+	int len = string_list_split_in_place(&list, string, delim, maxsplit);
+	cl_assert_equal_i(len, expected_strings->nr);
+	t_check_string_list(&list, expected_strings);
+
+	free(string);
+	t_string_list_clear(&list, 0);
+}
+
+void test_string_list__split_in_place(void)
+{
+	struct string_list expected_strings = STRING_LIST_INIT_DUP;
+
+	t_create_string_list_dup(&expected_strings, 0, "foo", "", "", "bar",
+				 "", "", "baz", "", "", "", NULL);
+	t_string_list_split_in_place("foo:;:bar:;:baz:;:", ":;", -1, &expected_strings);
+
+	t_create_string_list_dup(&expected_strings, 0, "foo:;:bar:;:baz", NULL);
+	t_string_list_split_in_place("foo:;:bar:;:baz", ":;", 0, &expected_strings);
+
+	t_create_string_list_dup(&expected_strings, 0, "foo", ";:bar:;:baz", NULL);
+	t_string_list_split_in_place("foo:;:bar:;:baz", ":;", 1, &expected_strings);
+
+	t_create_string_list_dup(&expected_strings, 0, "foo", "", ":bar:;:baz", NULL);
+	t_string_list_split_in_place("foo:;:bar:;:baz", ":;", 2, &expected_strings);
+
+	t_create_string_list_dup(&expected_strings, 0, "foo", "", "", "bar",
+				 "", "", "", NULL);
+	t_string_list_split_in_place("foo:;:bar:;:", ":;", -1, &expected_strings);
+
+	t_string_list_clear(&expected_strings, 0);
+}
-- 
2.49.0

Re: [PATCH 3/5] u-string-list: move "test_split_in_place" to "u-string-list.c"

[Patrick Steinhardt]

On Tue, Apr 22, 2025 at 10:55:06PM +0800, shejialuo wrote:
> diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c
> index 0c148684ea..44ec8de3d0 100644
> --- a/t/unit-tests/u-string-list.c
> +++ b/t/unit-tests/u-string-list.c
> @@ -84,3 +84,42 @@ void test_string_list__split(void)
>  
>  	t_string_list_clear(&expected_strings, 0);
>  }
> +
> +static void t_string_list_split_in_place(const char *data, const char *delim, int maxsplit,
> +					 struct string_list *expected_strings)
> +{
> +	struct string_list list = STRING_LIST_INIT_NODUP;
> +

Nit: this empty newline should be removed.

> +	char *string = xstrdup(data);
> +
> +	int len = string_list_split_in_place(&list, string, delim, maxsplit);
> +	cl_assert_equal_i(len, expected_strings->nr);
> +	t_check_string_list(&list, expected_strings);
> +
> +	free(string);
> +	t_string_list_clear(&list, 0);
> +}
> +
> +void test_string_list__split_in_place(void)
> +{
> +	struct string_list expected_strings = STRING_LIST_INIT_DUP;
> +
> +	t_create_string_list_dup(&expected_strings, 0, "foo", "", "", "bar",
> +				 "", "", "baz", "", "", "", NULL);
> +	t_string_list_split_in_place("foo:;:bar:;:baz:;:", ":;", -1, &expected_strings);

Same question here, can we handle expected strings via varargs to avoid
code duplication? Also for subsequent patches.

Patrick

[PATCH 4/5] u-string-list: move "filter string" test to "u-string-list.c"

[shejialuo]

We use "test-tool string-list filter" to test the "filter_string_list"
function. As we have introduced the unit test, we'd better remove the
logic from shell script to C program to improve test speed and
readability.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 t/helper/test-string-list.c  | 21 ---------------
 t/t0063-string-list.sh       | 11 --------
 t/unit-tests/u-string-list.c | 51 ++++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 32 deletions(-)

diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c
index 8a344347ad..262b28c599 100644
--- a/t/helper/test-string-list.c
+++ b/t/helper/test-string-list.c
@@ -31,29 +31,8 @@ static void write_list_compact(const struct string_list *list)
 	}
 }
 
-static int prefix_cb(struct string_list_item *item, void *cb_data)
-{
-	const char *prefix = (const char *)cb_data;
-	return starts_with(item->string, prefix);
-}
-
 int cmd__string_list(int argc, const char **argv)
 {
-	if (argc == 4 && !strcmp(argv[1], "filter")) {
-		/*
-		 * Retain only the items that have the specified prefix.
-		 * Arguments: list|- prefix
-		 */
-		struct string_list list = STRING_LIST_INIT_DUP;
-		const char *prefix = argv[3];
-
-		parse_string_list(&list, argv[2]);
-		filter_string_list(&list, 0, prefix_cb, (void *)prefix);
-		write_list_compact(&list);
-		string_list_clear(&list, 0);
-		return 0;
-	}
-
 	if (argc == 3 && !strcmp(argv[1], "remove_duplicates")) {
 		struct string_list list = STRING_LIST_INIT_DUP;
 
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
index 1a9cf8bfcf..31fd62bba8 100755
--- a/t/t0063-string-list.sh
+++ b/t/t0063-string-list.sh
@@ -7,17 +7,6 @@ test_description='Test string list functionality'
 
 . ./test-lib.sh
 
-test_expect_success "test filter_string_list" '
-	test "x-" = "x$(test-tool string-list filter - y)" &&
-	test "x-" = "x$(test-tool string-list filter no y)" &&
-	test yes = "$(test-tool string-list filter yes y)" &&
-	test yes = "$(test-tool string-list filter no:yes y)" &&
-	test yes = "$(test-tool string-list filter yes:no y)" &&
-	test y1:y2 = "$(test-tool string-list filter y1:y2 y)" &&
-	test y2:y1 = "$(test-tool string-list filter y2:y1 y)" &&
-	test "x-" = "x$(test-tool string-list filter x1:x2 y)"
-'
-
 test_expect_success "test remove_duplicates" '
 	test "x-" = "x$(test-tool string-list remove_duplicates -)" &&
 	test "x" = "x$(test-tool string-list remove_duplicates "")" &&
diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c
index 44ec8de3d0..e02a15ac04 100644
--- a/t/unit-tests/u-string-list.c
+++ b/t/unit-tests/u-string-list.c
@@ -123,3 +123,54 @@ void test_string_list__split_in_place(void)
 
 	t_string_list_clear(&expected_strings, 0);
 }
+
+static int prefix_cb(struct string_list_item *item, void *cb_data)
+{
+	const char *prefix = (const char *)cb_data;
+	return starts_with(item->string, prefix);
+}
+
+static void t_string_list_filter(struct string_list *list,
+				 string_list_each_func_t want, void *cb_data,
+				 struct string_list *expected_strings)
+{
+	filter_string_list(list, 0, want, cb_data);
+	t_check_string_list(list, expected_strings);
+}
+
+void test_string_list__filter(void)
+{
+	struct string_list expected_strings = STRING_LIST_INIT_DUP;
+	struct string_list list = STRING_LIST_INIT_DUP;
+	const char *prefix = "y";
+
+	t_string_list_filter(&list, prefix_cb, (void*)prefix, &expected_strings);
+
+	t_create_string_list_dup(&list, 0, "no", NULL);
+	t_string_list_filter(&list, prefix_cb, (void*)prefix, &expected_strings);
+
+	t_create_string_list_dup(&list, 0, "yes", NULL);
+	t_create_string_list_dup(&expected_strings, 0, "yes", NULL);
+	t_string_list_filter(&list, prefix_cb, (void*)prefix, &expected_strings);
+
+	t_create_string_list_dup(&list, 0, "no", "yes", NULL);
+	t_string_list_filter(&list, prefix_cb, (void*)prefix, &expected_strings);
+
+	t_create_string_list_dup(&list, 0, "yes", "no", NULL);
+	t_string_list_filter(&list, prefix_cb, (void*)prefix, &expected_strings);
+
+	t_create_string_list_dup(&list, 0, "y1", "y2", NULL);
+	t_create_string_list_dup(&expected_strings, 0, "y1", "y2", NULL);
+	t_string_list_filter(&list, prefix_cb, (void*)prefix, &expected_strings);
+
+	t_create_string_list_dup(&list, 0, "y2", "y1", NULL);
+	t_create_string_list_dup(&expected_strings, 0, "y2", "y1", NULL);
+	t_string_list_filter(&list, prefix_cb, (void*)prefix, &expected_strings);
+
+	t_create_string_list_dup(&list, 0, "x1", "x2", NULL);
+	t_create_string_list_dup(&expected_strings, 0, NULL);
+	t_string_list_filter(&list, prefix_cb, (void*)prefix, &expected_strings);
+
+	t_string_list_clear(&list, 0);
+	t_string_list_clear(&expected_strings, 0);
+}
-- 
2.49.0

[PATCH 5/5] u-string-list: move "remove duplicates" test to "u-string-list.c"

[shejialuo]

We use "test-tool string-list remove_duplicates" to test the
"string_list_remove_duplicates" function. As we have introduced the unit
test, we'd better remove the logic from shell script to C program to
improve test speed and readability.

As all the tests in shell script are removed, let's just delete the
"t0063-string-list.sh" and update the "meson.build" file to align with
this change.

Also we could simply remove "DISABLE_SIGN_COMPARE_WARNINGS" due to we
have already deleted related code.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 t/helper/test-string-list.c  | 39 -----------------------
 t/meson.build                |  1 -
 t/t0063-string-list.sh       | 27 ----------------
 t/unit-tests/u-string-list.c | 62 ++++++++++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+), 67 deletions(-)
 delete mode 100755 t/t0063-string-list.sh

diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c
index 262b28c599..6be0cdb8e2 100644
--- a/t/helper/test-string-list.c
+++ b/t/helper/test-string-list.c
@@ -1,48 +1,9 @@
-#define DISABLE_SIGN_COMPARE_WARNINGS
-
 #include "test-tool.h"
 #include "strbuf.h"
 #include "string-list.h"
 
-/*
- * Parse an argument into a string list.  arg should either be a
- * ':'-separated list of strings, or "-" to indicate an empty string
- * list (as opposed to "", which indicates a string list containing a
- * single empty string).  list->strdup_strings must be set.
- */
-static void parse_string_list(struct string_list *list, const char *arg)
-{
-	if (!strcmp(arg, "-"))
-		return;
-
-	(void)string_list_split(list, arg, ':', -1);
-}
-
-static void write_list_compact(const struct string_list *list)
-{
-	int i;
-	if (!list->nr)
-		printf("-\n");
-	else {
-		printf("%s", list->items[0].string);
-		for (i = 1; i < list->nr; i++)
-			printf(":%s", list->items[i].string);
-		printf("\n");
-	}
-}
-
 int cmd__string_list(int argc, const char **argv)
 {
-	if (argc == 3 && !strcmp(argv[1], "remove_duplicates")) {
-		struct string_list list = STRING_LIST_INIT_DUP;
-
-		parse_string_list(&list, argv[2]);
-		string_list_remove_duplicates(&list, 0);
-		write_list_compact(&list);
-		string_list_clear(&list, 0);
-		return 0;
-	}
-
 	if (argc == 2 && !strcmp(argv[1], "sort")) {
 		struct string_list list = STRING_LIST_INIT_NODUP;
 		struct strbuf sb = STRBUF_INIT;
diff --git a/t/meson.build b/t/meson.build
index 424e7e445f..25af09a8d4 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -124,7 +124,6 @@ integration_tests = [
   't0060-path-utils.sh',
   't0061-run-command.sh',
   't0062-revision-walking.sh',
-  't0063-string-list.sh',
   't0066-dir-iterator.sh',
   't0067-parse_pathspec_file.sh',
   't0068-for-each-repo.sh',
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
deleted file mode 100755
index 31fd62bba8..0000000000
--- a/t/t0063-string-list.sh
+++ /dev/null
@@ -1,27 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2012 Michael Haggerty
-#
-
-test_description='Test string list functionality'
-
-. ./test-lib.sh
-
-test_expect_success "test remove_duplicates" '
-	test "x-" = "x$(test-tool string-list remove_duplicates -)" &&
-	test "x" = "x$(test-tool string-list remove_duplicates "")" &&
-	test a = "$(test-tool string-list remove_duplicates a)" &&
-	test a = "$(test-tool string-list remove_duplicates a:a)" &&
-	test a = "$(test-tool string-list remove_duplicates a:a:a:a:a)" &&
-	test a:b = "$(test-tool string-list remove_duplicates a:b)" &&
-	test a:b = "$(test-tool string-list remove_duplicates a:a:b)" &&
-	test a:b = "$(test-tool string-list remove_duplicates a:b:b)" &&
-	test a:b:c = "$(test-tool string-list remove_duplicates a:b:c)" &&
-	test a:b:c = "$(test-tool string-list remove_duplicates a:a:b:c)" &&
-	test a:b:c = "$(test-tool string-list remove_duplicates a:b:b:c)" &&
-	test a:b:c = "$(test-tool string-list remove_duplicates a:b:c:c)" &&
-	test a:b:c = "$(test-tool string-list remove_duplicates a:a:b:b:c:c)" &&
-	test a:b:c = "$(test-tool string-list remove_duplicates a:a:a:b:b:b:c:c:c)"
-'
-
-test_done
diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c
index e02a15ac04..a9fe5ade15 100644
--- a/t/unit-tests/u-string-list.c
+++ b/t/unit-tests/u-string-list.c
@@ -174,3 +174,65 @@ void test_string_list__filter(void)
 	t_string_list_clear(&list, 0);
 	t_string_list_clear(&expected_strings, 0);
 }
+
+static void t_string_list_remove_duplicates(struct string_list *list,
+					    struct string_list *expected_strings)
+{
+	string_list_remove_duplicates(list, 0);
+	t_check_string_list(list, expected_strings);
+}
+
+void test_string_list__remove_duplicates(void)
+{
+	struct string_list expected_strings = STRING_LIST_INIT_DUP;
+	struct string_list list = STRING_LIST_INIT_DUP;
+
+	t_string_list_remove_duplicates(&list, &expected_strings);
+
+	t_create_string_list_dup(&list, 0, "", NULL);
+	t_create_string_list_dup(&expected_strings, 0, "", NULL);
+	t_string_list_remove_duplicates(&list, &expected_strings);
+
+	t_create_string_list_dup(&expected_strings, 0, "a", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", NULL);
+	t_string_list_remove_duplicates(&list, &expected_strings);
+
+	t_create_string_list_dup(&list, 0, "a", "a", NULL);
+	t_string_list_remove_duplicates(&list, &expected_strings);
+
+	t_create_string_list_dup(&list, 0, "a", "a", "a", NULL);
+	t_string_list_remove_duplicates(&list, &expected_strings);
+
+	t_create_string_list_dup(&expected_strings, 0, "a", "b", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "a", "b", NULL);
+	t_string_list_remove_duplicates(&list, &expected_strings);
+
+	t_create_string_list_dup(&list, 0, "a", "b", "b", NULL);
+	t_string_list_remove_duplicates(&list, &expected_strings);
+
+	t_create_string_list_dup(&expected_strings, 0, "a", "b", "c", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "b", "c", NULL);
+	t_string_list_remove_duplicates(&list, &expected_strings);
+
+	t_create_string_list_dup(&list, 0, "a", "a", "b", "c", NULL);
+	t_string_list_remove_duplicates(&list, &expected_strings);
+
+	t_create_string_list_dup(&list, 0, "a", "b", "b", "c", NULL);
+	t_string_list_remove_duplicates(&list, &expected_strings);
+
+	t_create_string_list_dup(&list, 0, "a", "b", "c", "c", NULL);
+	t_string_list_remove_duplicates(&list, &expected_strings);
+
+	t_create_string_list_dup(&list, 0, "a", "a", "b", "b", "c", "c", NULL);
+	t_string_list_remove_duplicates(&list, &expected_strings);
+
+	t_create_string_list_dup(&list, 0, "a", "a", "a", "b", "b", "b",
+				 "c", "c", "c", NULL);
+	t_string_list_remove_duplicates(&list, &expected_strings);
+
+	t_string_list_clear(&list, 0);
+	t_string_list_clear(&expected_strings, 0);
+}
-- 
2.49.0

Re: [PATCH 5/5] u-string-list: move "remove duplicates" test to "u-string-list.c"

[Patrick Steinhardt]

On Tue, Apr 22, 2025 at 10:55:23PM +0800, shejialuo wrote:
> diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c
> index 262b28c599..6be0cdb8e2 100644
> --- a/t/helper/test-string-list.c
> +++ b/t/helper/test-string-list.c
> @@ -1,48 +1,9 @@
> -#define DISABLE_SIGN_COMPARE_WARNINGS
> -
>  #include "test-tool.h"
>  #include "strbuf.h"
>  #include "string-list.h"
>  
> -/*
> - * Parse an argument into a string list.  arg should either be a
> - * ':'-separated list of strings, or "-" to indicate an empty string
> - * list (as opposed to "", which indicates a string list containing a
> - * single empty string).  list->strdup_strings must be set.
> - */
> -static void parse_string_list(struct string_list *list, const char *arg)
> -{
> -	if (!strcmp(arg, "-"))
> -		return;
> -
> -	(void)string_list_split(list, arg, ':', -1);
> -}
> -
> -static void write_list_compact(const struct string_list *list)
> -{
> -	int i;
> -	if (!list->nr)
> -		printf("-\n");
> -	else {
> -		printf("%s", list->items[0].string);
> -		for (i = 1; i < list->nr; i++)
> -			printf(":%s", list->items[i].string);
> -		printf("\n");
> -	}
> -}
> -
>  int cmd__string_list(int argc, const char **argv)
>  {
> -	if (argc == 3 && !strcmp(argv[1], "remove_duplicates")) {
> -		struct string_list list = STRING_LIST_INIT_DUP;
> -
> -		parse_string_list(&list, argv[2]);
> -		string_list_remove_duplicates(&list, 0);
> -		write_list_compact(&list);
> -		string_list_clear(&list, 0);
> -		return 0;
> -	}
> -
>  	if (argc == 2 && !strcmp(argv[1], "sort")) {
>  		struct string_list list = STRING_LIST_INIT_NODUP;
>  		struct strbuf sb = STRBUF_INIT;

I'm a bit surprised that the patch series stops after this patch given
that the only remaining subcommand is "sort". Is there a specific reason
why you don't also convert that function? If you did we could declare
victory by deleting the whole "test-helper string-list" subcommand.

It seems that the only reason is p0071, where we benchmark performance
of sorting. I dunno... that one is of course not a good fit for our unit
testing framework. But it's a bit sad that we cannot remove the whole
infra only because of a performance test that nobody ever runs in the
first place.

Patrick

Re: [PATCH 5/5] u-string-list: move "remove duplicates" test to "u-string-list.c"

[shejialuo]

On Wed, Apr 23, 2025 at 01:53:19PM +0200, Patrick Steinhardt wrote:
> On Tue, Apr 22, 2025 at 10:55:23PM +0800, shejialuo wrote:
> > diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c
> > index 262b28c599..6be0cdb8e2 100644
> > --- a/t/helper/test-string-list.c
> > +++ b/t/helper/test-string-list.c
> > @@ -1,48 +1,9 @@
> > -#define DISABLE_SIGN_COMPARE_WARNINGS
> > -
> >  #include "test-tool.h"
> >  #include "strbuf.h"
> >  #include "string-list.h"
> >  
> > -/*
> > - * Parse an argument into a string list.  arg should either be a
> > - * ':'-separated list of strings, or "-" to indicate an empty string
> > - * list (as opposed to "", which indicates a string list containing a
> > - * single empty string).  list->strdup_strings must be set.
> > - */
> > -static void parse_string_list(struct string_list *list, const char *arg)
> > -{
> > -	if (!strcmp(arg, "-"))
> > -		return;
> > -
> > -	(void)string_list_split(list, arg, ':', -1);
> > -}
> > -
> > -static void write_list_compact(const struct string_list *list)
> > -{
> > -	int i;
> > -	if (!list->nr)
> > -		printf("-\n");
> > -	else {
> > -		printf("%s", list->items[0].string);
> > -		for (i = 1; i < list->nr; i++)
> > -			printf(":%s", list->items[i].string);
> > -		printf("\n");
> > -	}
> > -}
> > -
> >  int cmd__string_list(int argc, const char **argv)
> >  {
> > -	if (argc == 3 && !strcmp(argv[1], "remove_duplicates")) {
> > -		struct string_list list = STRING_LIST_INIT_DUP;
> > -
> > -		parse_string_list(&list, argv[2]);
> > -		string_list_remove_duplicates(&list, 0);
> > -		write_list_compact(&list);
> > -		string_list_clear(&list, 0);
> > -		return 0;
> > -	}
> > -
> >  	if (argc == 2 && !strcmp(argv[1], "sort")) {
> >  		struct string_list list = STRING_LIST_INIT_NODUP;
> >  		struct strbuf sb = STRBUF_INIT;
> 
> I'm a bit surprised that the patch series stops after this patch given
> that the only remaining subcommand is "sort". Is there a specific reason
> why you don't also convert that function? If you did we could declare
> victory by deleting the whole "test-helper string-list" subcommand.
> 

Actually I also want to delete the whole file. However, as you have said
as following shows:

> It seems that the only reason is p0071, where we benchmark performance
> of sorting. I dunno... that one is of course not a good fit for our unit
> testing framework. But it's a bit sad that we cannot remove the whole
> infra only because of a performance test that nobody ever runs in the
> first place.
> 

I think we should delete the whole file. I'll find out a way to do this.
It's wired that we keep the "test-helper string-list".

> Patrick

Thanks,
Jialuo

[PATCH v2 0/8] enhance "string_list" code and test

[shejialuo]

Hi All:

I finally finish the version 2. And I don't provide the range-diff due
to that I add more commits compared with version 1.

This patch could be organized into three parts:

1. [PATCH v2 1/8]

   Fix simple sign warnings of the loop iterator.

2. [PATCH v2 2/8] - [PATCH v2 4/8]

   Remove unncessary code, improve the logic and finally enable sign
   compare warnings check.

3. [PATCH v2 5/8] - [PATCH v2 8/8]

   Remove test to the unit test.

However, I want to tell Patrick a thing. I feel hard to remove the
performance test. So, I leave it here. The reason is that we want to
test performance of "string-list" sorting.

Thanks,
Jialuo

shejialuo (8):
  string-list: fix sign compare warnings for loop iterator
  string-list: remove unused "insert_at" parameter from add_entry
  string-list: return index directly when inserting an existing element
  string-list: enable sign compare warnings check
  u-string-list: move "test_split" into "u-string-list.c"
  u-string-list: move "test_split_in_place" to "u-string-list.c"
  u-string-list: move "filter string" test to "u-string-list.c"
  u-string-list: move "remove duplicates" test to "u-string-list.c"

 Makefile                     |   1 +
 string-list.c                |  48 +++-----
 t/helper/test-string-list.c  |  96 ---------------
 t/meson.build                |   2 +-
 t/t0063-string-list.sh       | 142 ---------------------
 t/unit-tests/u-string-list.c | 233 +++++++++++++++++++++++++++++++++++
 6 files changed, 255 insertions(+), 267 deletions(-)
 delete mode 100755 t/t0063-string-list.sh
 create mode 100644 t/unit-tests/u-string-list.c

-- 
2.49.0

[PATCH v2 1/8] string-list: fix sign compare warnings for loop iterator

[shejialuo]

In "string-list.c", there are six warnings which are emitted by
"Wsign-compare". And five warnings are caused by the loop iterator type
mismatch. Let's fix these five warnings by changing the `int` type to
`size_t` type of the loop iterator.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 string-list.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/string-list.c b/string-list.c
index bf061fec56..801ece0cba 100644
--- a/string-list.c
+++ b/string-list.c
@@ -116,9 +116,9 @@ struct string_list_item *string_list_lookup(struct string_list *list, const char
 void string_list_remove_duplicates(struct string_list *list, int free_util)
 {
 	if (list->nr > 1) {
-		int src, dst;
+		size_t dst = 1;
 		compare_strings_fn cmp = list->cmp ? list->cmp : strcmp;
-		for (src = dst = 1; src < list->nr; src++) {
+		for (size_t src = 1; src < list->nr; src++) {
 			if (!cmp(list->items[dst - 1].string, list->items[src].string)) {
 				if (list->strdup_strings)
 					free(list->items[src].string);
@@ -134,8 +134,8 @@ void string_list_remove_duplicates(struct string_list *list, int free_util)
 int for_each_string_list(struct string_list *list,
 			 string_list_each_func_t fn, void *cb_data)
 {
-	int i, ret = 0;
-	for (i = 0; i < list->nr; i++)
+	int ret = 0;
+	for (size_t i = 0; i < list->nr; i++)
 		if ((ret = fn(&list->items[i], cb_data)))
 			break;
 	return ret;
@@ -144,8 +144,8 @@ int for_each_string_list(struct string_list *list,
 void filter_string_list(struct string_list *list, int free_util,
 			string_list_each_func_t want, void *cb_data)
 {
-	int src, dst = 0;
-	for (src = 0; src < list->nr; src++) {
+	size_t dst = 0;
+	for (size_t src = 0; src < list->nr; src++) {
 		if (want(&list->items[src], cb_data)) {
 			list->items[dst++] = list->items[src];
 		} else {
@@ -171,13 +171,12 @@ void string_list_remove_empty_items(struct string_list *list, int free_util)
 void string_list_clear(struct string_list *list, int free_util)
 {
 	if (list->items) {
-		int i;
 		if (list->strdup_strings) {
-			for (i = 0; i < list->nr; i++)
+			for (size_t i = 0; i < list->nr; i++)
 				free(list->items[i].string);
 		}
 		if (free_util) {
-			for (i = 0; i < list->nr; i++)
+			for (size_t i = 0; i < list->nr; i++)
 				free(list->items[i].util);
 		}
 		free(list->items);
@@ -189,13 +188,12 @@ void string_list_clear(struct string_list *list, int free_util)
 void string_list_clear_func(struct string_list *list, string_list_clear_func_t clearfunc)
 {
 	if (list->items) {
-		int i;
 		if (clearfunc) {
-			for (i = 0; i < list->nr; i++)
+			for (size_t i = 0; i < list->nr; i++)
 				clearfunc(list->items[i].util, list->items[i].string);
 		}
 		if (list->strdup_strings) {
-			for (i = 0; i < list->nr; i++)
+			for (size_t i = 0; i < list->nr; i++)
 				free(list->items[i].string);
 		}
 		free(list->items);
-- 
2.49.0

Re: [PATCH v2 1/8] string-list: fix sign compare warnings for loop iterator

[Patrick Steinhardt]

On Sun, May 18, 2025 at 11:56:59PM +0800, shejialuo wrote:
> In "string-list.c", there are six warnings which are emitted by
> "Wsign-compare". And five warnings are caused by the loop iterator type
> mismatch. Let's fix these five warnings by changing the `int` type to
> `size_t` type of the loop iterator.

This naturally causes the question what the 6th warning is, and why it's
not fixed in this commit. The answer is that the last one is more
complex and handled by subsequent patches, which you should probably
point out here. E.g.:

    There are a couple of "-Wsign-compare" warnings in "string-list.c".
    Fix trivial ones that result from a mismatched loop iterator type.

    There is a single warning left after these fixes. This warning needs
    a bit more care and is thus handled in subsequent commits.

> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---
>  string-list.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)

All of these look obviously good to me, thanks!

Patrick

Re: [PATCH v2 1/8] string-list: fix sign compare warnings for loop iterator

[shejialuo]

On Mon, May 19, 2025 at 09:17:47AM +0200, Patrick Steinhardt wrote:
> On Sun, May 18, 2025 at 11:56:59PM +0800, shejialuo wrote:
> > In "string-list.c", there are six warnings which are emitted by
> > "Wsign-compare". And five warnings are caused by the loop iterator type
> > mismatch. Let's fix these five warnings by changing the `int` type to
> > `size_t` type of the loop iterator.
> 
> This naturally causes the question what the 6th warning is, and why it's
> not fixed in this commit. The answer is that the last one is more
> complex and handled by subsequent patches, which you should probably
> point out here. E.g.:
> 
>     There are a couple of "-Wsign-compare" warnings in "string-list.c".
>     Fix trivial ones that result from a mismatched loop iterator type.
> 
>     There is a single warning left after these fixes. This warning needs
>     a bit more care and is thus handled in subsequent commits.
> 

That's right. Thanks for the hint, will improve this in the next
version.

Jialuo

[PATCH v2 2/8] string-list: remove unused "insert_at" parameter from add_entry

[shejialuo]

In "add_entry", we accept "insert_at" parameter which must be either -1
(auto) or between 0 and `list->nr` inclusive. Any other value is
invalid. When caller specify any invalid "insert_at" value, we won't
check the range and move the element, which would definitely cause the
trouble.

However, we only use "add_entry" in "string_list_insert" function and we
always pass the "-1" for "insert_at" parameter. So, we never use this
parameter to insert element in a user specified position. Let's delete
this parameter. If there is any requirement later, we need to use a
better way to do this.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 string-list.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/string-list.c b/string-list.c
index 801ece0cba..8540c29bc9 100644
--- a/string-list.c
+++ b/string-list.c
@@ -41,10 +41,10 @@ static int get_entry_index(const struct string_list *list, const char *string,
 }
 
 /* returns -1-index if already exists */
-static int add_entry(int insert_at, struct string_list *list, const char *string)
+static int add_entry(struct string_list *list, const char *string)
 {
 	int exact_match = 0;
-	int index = insert_at != -1 ? insert_at : get_entry_index(list, string, &exact_match);
+	int index = get_entry_index(list, string, &exact_match);
 
 	if (exact_match)
 		return -1 - index;
@@ -63,7 +63,7 @@ static int add_entry(int insert_at, struct string_list *list, const char *string
 
 struct string_list_item *string_list_insert(struct string_list *list, const char *string)
 {
-	int index = add_entry(-1, list, string);
+	int index = add_entry(list, string);
 
 	if (index < 0)
 		index = -1 - index;
-- 
2.49.0

Re: [PATCH v2 2/8] string-list: remove unused "insert_at" parameter from add_entry

[Patrick Steinhardt]

On Sun, May 18, 2025 at 11:57:07PM +0800, shejialuo wrote:
> In "add_entry", we accept "insert_at" parameter which must be either -1
> (auto) or between 0 and `list->nr` inclusive. Any other value is
> invalid. When caller specify any invalid "insert_at" value, we won't
> check the range and move the element, which would definitely cause the
> trouble.

Maybe "which may easily cause an out-of-bounds write" instead of vague
"trouble"?

> However, we only use "add_entry" in "string_list_insert" function and we
> always pass the "-1" for "insert_at" parameter. So, we never use this
> parameter to insert element in a user specified position. Let's delete
> this parameter. If there is any requirement later, we need to use a
> better way to do this.

Makes sense.

Patrick

Re: [PATCH v2 2/8] string-list: remove unused "insert_at" parameter from add_entry

[shejialuo]

On Mon, May 19, 2025 at 09:17:53AM +0200, Patrick Steinhardt wrote:
> On Sun, May 18, 2025 at 11:57:07PM +0800, shejialuo wrote:
> > In "add_entry", we accept "insert_at" parameter which must be either -1
> > (auto) or between 0 and `list->nr` inclusive. Any other value is
> > invalid. When caller specify any invalid "insert_at" value, we won't
> > check the range and move the element, which would definitely cause the
> > trouble.
> 
> Maybe "which may easily cause an out-of-bounds write" instead of vague
> "trouble"?
> 

Make sense. I will improve this in the next version.

Thanks,
Jialuo

Re: [PATCH v2 2/8] string-list: remove unused "insert_at" parameter from add_entry

[Jeff King]

On Sun, May 18, 2025 at 11:57:07PM +0800, shejialuo wrote:

> In "add_entry", we accept "insert_at" parameter which must be either -1
> (auto) or between 0 and `list->nr` inclusive. Any other value is
> invalid. When caller specify any invalid "insert_at" value, we won't
> check the range and move the element, which would definitely cause the
> trouble.
> 
> However, we only use "add_entry" in "string_list_insert" function and we
> always pass the "-1" for "insert_at" parameter. So, we never use this
> parameter to insert element in a user specified position. Let's delete
> this parameter. If there is any requirement later, we need to use a
> better way to do this.

We can see from looking at the code that removing this will not change
the behavior. But that always makes me wonder why it was there in the
first place, and whether we might ever want it.

The answer in this case is that we used to have another function,
string_list_insert_at_index(), which used the extra insert_at parameter.
The idea being that you could call string_list_find_insert_index(),
decide whether there was something already there, and then insert
without repeating the binary search.

But you can see in callers like 63226218ba (mailmap: use higher level
string list functions, 2014-11-24) that this was not really that useful
(in that commit we just try to insert and check the util pointer to see
if we need to add the auxiliary structure).

So the function went away in f8c4ab611a (string_list: remove
string_list_insert_at_index() from its API, 2014-11-24), and I suspect
we won't need it again. (Also, I think these days we'd probably use a
strmap instead anyway).

-Peff

Re: [PATCH v2 2/8] string-list: remove unused "insert_at" parameter from add_entry

[shejialuo]

On Mon, May 19, 2025 at 03:51:19AM -0400, Jeff King wrote:
> On Sun, May 18, 2025 at 11:57:07PM +0800, shejialuo wrote:
> 
> > In "add_entry", we accept "insert_at" parameter which must be either -1
> > (auto) or between 0 and `list->nr` inclusive. Any other value is
> > invalid. When caller specify any invalid "insert_at" value, we won't
> > check the range and move the element, which would definitely cause the
> > trouble.
> > 
> > However, we only use "add_entry" in "string_list_insert" function and we
> > always pass the "-1" for "insert_at" parameter. So, we never use this
> > parameter to insert element in a user specified position. Let's delete
> > this parameter. If there is any requirement later, we need to use a
> > better way to do this.
> 
> We can see from looking at the code that removing this will not change
> the behavior. But that always makes me wonder why it was there in the
> first place, and whether we might ever want it.
> 

Yes, I agree. Actually, in my first implementation, I didn't realise
that this is redundant. However, when inspecting the code carefully, I
find out this is useless.

> The answer in this case is that we used to have another function,
> string_list_insert_at_index(), which used the extra insert_at parameter.
> The idea being that you could call string_list_find_insert_index(),
> decide whether there was something already there, and then insert
> without repeating the binary search.
> 
> But you can see in callers like 63226218ba (mailmap: use higher level
> string list functions, 2014-11-24) that this was not really that useful
> (in that commit we just try to insert and check the util pointer to see
> if we need to add the auxiliary structure).
> 
> So the function went away in f8c4ab611a (string_list: remove
> string_list_insert_at_index() from its API, 2014-11-24), and I suspect
> we won't need it again. (Also, I think these days we'd probably use a
> strmap instead anyway).
> 

Thanks for the hint. By seeing this commit, I totally understand the
history. Because we delete `string_list_insert_at_index`, we simply call
"add_entry" by specifying "auto" mode and somehow we don't delete the
legacy check in "add_entry".

But I have one question: should I include the information in the commit
message? I feel doing this would be chaty. But I somehow think we should
do this.

Thanks,
Jialuo

Re: [PATCH v2 2/8] string-list: remove unused "insert_at" parameter from add_entry

[Patrick Steinhardt]

On Mon, May 26, 2025 at 10:01:57PM +0800, shejialuo wrote:
> On Mon, May 19, 2025 at 03:51:19AM -0400, Jeff King wrote:
> > The answer in this case is that we used to have another function,
> > string_list_insert_at_index(), which used the extra insert_at parameter.
> > The idea being that you could call string_list_find_insert_index(),
> > decide whether there was something already there, and then insert
> > without repeating the binary search.
> > 
> > But you can see in callers like 63226218ba (mailmap: use higher level
> > string list functions, 2014-11-24) that this was not really that useful
> > (in that commit we just try to insert and check the util pointer to see
> > if we need to add the auxiliary structure).
> > 
> > So the function went away in f8c4ab611a (string_list: remove
> > string_list_insert_at_index() from its API, 2014-11-24), and I suspect
> > we won't need it again. (Also, I think these days we'd probably use a
> > strmap instead anyway).
> > 
> 
> Thanks for the hint. By seeing this commit, I totally understand the
> history. Because we delete `string_list_insert_at_index`, we simply call
> "add_entry" by specifying "auto" mode and somehow we don't delete the
> legacy check in "add_entry".
> 
> But I have one question: should I include the information in the commit
> message? I feel doing this would be chaty. But I somehow think we should
> do this.

I would recommend including such information in the commit message, yes.
It helps the reviewer to understand the context and makes it easier for
them to figure out why this seemingly nonsensical parameter exists in
the first place.

Patrick

[PATCH v2 3/8] string-list: return index directly when inserting an existing element

[shejialuo]

When inserting an existing element, "add_entry" would convert "index"
value to "-1-index" to indicate the caller that this element is in the
list already.

However, in "string_list_insert", we would simply convert this to the
original positive index without any further action. Let's directly
return the index as we don't care about whether the element is in the
list by using "add_entry".

In the future, if we want to let "add_entry" tell the caller, we may add
"int *exact_match" parameter to "add_entry" instead of converting the
index to negative to indicate.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 string-list.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/string-list.c b/string-list.c
index 8540c29bc9..171cef5dbb 100644
--- a/string-list.c
+++ b/string-list.c
@@ -40,14 +40,13 @@ static int get_entry_index(const struct string_list *list, const char *string,
 	return right;
 }
 
-/* returns -1-index if already exists */
 static int add_entry(struct string_list *list, const char *string)
 {
 	int exact_match = 0;
 	int index = get_entry_index(list, string, &exact_match);
 
 	if (exact_match)
-		return -1 - index;
+		return index;
 
 	ALLOC_GROW(list->items, list->nr+1, list->alloc);
 	if (index < list->nr)
@@ -65,9 +64,6 @@ struct string_list_item *string_list_insert(struct string_list *list, const char
 {
 	int index = add_entry(list, string);
 
-	if (index < 0)
-		index = -1 - index;
-
 	return list->items + index;
 }
 
-- 
2.49.0

Re: [PATCH v2 3/8] string-list: return index directly when inserting an existing element

[Patrick Steinhardt]

On Sun, May 18, 2025 at 11:57:15PM +0800, shejialuo wrote:
> When inserting an existing element, "add_entry" would convert "index"
> value to "-1-index" to indicate the caller that this element is in the
> list already.
> 
> However, in "string_list_insert", we would simply convert this to the
> original positive index without any further action. Let's directly
> return the index as we don't care about whether the element is in the
> list by using "add_entry".
> 
> In the future, if we want to let "add_entry" tell the caller, we may add
> "int *exact_match" parameter to "add_entry" instead of converting the
> index to negative to indicate.
> 
> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---
>  string-list.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/string-list.c b/string-list.c
> index 8540c29bc9..171cef5dbb 100644
> --- a/string-list.c
> +++ b/string-list.c
> @@ -40,14 +40,13 @@ static int get_entry_index(const struct string_list *list, const char *string,
>  	return right;
>  }
>  
> -/* returns -1-index if already exists */
>  static int add_entry(struct string_list *list, const char *string)
>  {
>  	int exact_match = 0;
>  	int index = get_entry_index(list, string, &exact_match);
>  
>  	if (exact_match)
> -		return -1 - index;
> +		return index;
>  
>  	ALLOC_GROW(list->items, list->nr+1, list->alloc);
>  	if (index < list->nr)

Okay, let's assume that "index == 2" here and we have an exact match.
We'd thus return `-1 - 2 == -3`.

> @@ -65,9 +64,6 @@ struct string_list_item *string_list_insert(struct string_list *list, const char
>  {
>  	int index = add_entry(list, string);
>  
> -	if (index < 0)
> -		index = -1 - index;
> -
>  	return list->items + index;
>  }

So we'd now realize that `index < 0` and thus calculate `-1 - -3 == 2`,
which is the original index indeed. So this is a nice simplification
that retains the original behaviour indeed.

I think we could simplify the code even further by inlining
`get_entry_index()` now that `string_list_insert()` is a trivial wrapper
around it. But I'll leave it up to you whether we want to do it or not.

Patrick

Re: [PATCH v2 3/8] string-list: return index directly when inserting an existing element

[shejialuo]

On Mon, May 19, 2025 at 09:18:00AM +0200, Patrick Steinhardt wrote:
> On Sun, May 18, 2025 at 11:57:15PM +0800, shejialuo wrote:
> > When inserting an existing element, "add_entry" would convert "index"
> > value to "-1-index" to indicate the caller that this element is in the
> > list already.
> > 
> > However, in "string_list_insert", we would simply convert this to the
> > original positive index without any further action. Let's directly
> > return the index as we don't care about whether the element is in the
> > list by using "add_entry".
> > 
> > In the future, if we want to let "add_entry" tell the caller, we may add
> > "int *exact_match" parameter to "add_entry" instead of converting the
> > index to negative to indicate.
> > 
> > Signed-off-by: shejialuo <shejialuo@gmail.com>
> > ---
> >  string-list.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/string-list.c b/string-list.c
> > index 8540c29bc9..171cef5dbb 100644
> > --- a/string-list.c
> > +++ b/string-list.c
> > @@ -40,14 +40,13 @@ static int get_entry_index(const struct string_list *list, const char *string,
> >  	return right;
> >  }
> >  
> > -/* returns -1-index if already exists */
> >  static int add_entry(struct string_list *list, const char *string)
> >  {
> >  	int exact_match = 0;
> >  	int index = get_entry_index(list, string, &exact_match);
> >  
> >  	if (exact_match)
> > -		return -1 - index;
> > +		return index;
> >  
> >  	ALLOC_GROW(list->items, list->nr+1, list->alloc);
> >  	if (index < list->nr)
> 
> Okay, let's assume that "index == 2" here and we have an exact match.
> We'd thus return `-1 - 2 == -3`.
> 
> > @@ -65,9 +64,6 @@ struct string_list_item *string_list_insert(struct string_list *list, const char
> >  {
> >  	int index = add_entry(list, string);
> >  
> > -	if (index < 0)
> > -		index = -1 - index;
> > -
> >  	return list->items + index;
> >  }
> 
> So we'd now realize that `index < 0` and thus calculate `-1 - -3 == 2`,
> which is the original index indeed. So this is a nice simplification
> that retains the original behaviour indeed.
> 

That's right. Actually, when I find out this by simply calculating `(-1
- (-1 - index)) == index`, I am a little surprised.

> I think we could simplify the code even further by inlining
> `get_entry_index()` now that `string_list_insert()` is a trivial wrapper
> around it. But I'll leave it up to you whether we want to do it or not.
> 

That's right. But as code it is, let's just keep this which won't hurt
too much.

Thanks,
Jialuo

Re: [PATCH v2 3/8] string-list: return index directly when inserting an existing element

[Jeff King]

On Sun, May 18, 2025 at 11:57:15PM +0800, shejialuo wrote:

> When inserting an existing element, "add_entry" would convert "index"
> value to "-1-index" to indicate the caller that this element is in the
> list already.
> 
> However, in "string_list_insert", we would simply convert this to the
> original positive index without any further action. Let's directly
> return the index as we don't care about whether the element is in the
> list by using "add_entry".
> 
> In the future, if we want to let "add_entry" tell the caller, we may add
> "int *exact_match" parameter to "add_entry" instead of converting the
> index to negative to indicate.

I assumed this was in the same boat as the previous change: something we
used to use and now don't. But I don't think we ever did. The "-1-index"
pattern goes all the way back to the beginning of the code.

It does match how other functions like string_list_find_insert_index()
behave. But I think that pattern doesn't make much sense for
add_entry(). After the function returns we know we've either found
something or added it, so the positive index will always point to a
matching entry.

So I think your patches are correct, but I was curious how we got to
this state.

-Peff

Re: [PATCH v2 3/8] string-list: return index directly when inserting an existing element

[shejialuo]

On Mon, May 19, 2025 at 03:58:13AM -0400, Jeff King wrote:
> On Sun, May 18, 2025 at 11:57:15PM +0800, shejialuo wrote:
> 
> > When inserting an existing element, "add_entry" would convert "index"
> > value to "-1-index" to indicate the caller that this element is in the
> > list already.
> > 
> > However, in "string_list_insert", we would simply convert this to the
> > original positive index without any further action. Let's directly
> > return the index as we don't care about whether the element is in the
> > list by using "add_entry".
> > 
> > In the future, if we want to let "add_entry" tell the caller, we may add
> > "int *exact_match" parameter to "add_entry" instead of converting the
> > index to negative to indicate.
> 
> I assumed this was in the same boat as the previous change: something we
> used to use and now don't. But I don't think we ever did. The "-1-index"
> pattern goes all the way back to the beginning of the code.
> 
> It does match how other functions like string_list_find_insert_index()
> behave. But I think that pattern doesn't make much sense for
> add_entry(). After the function returns we know we've either found
> something or added it, so the positive index will always point to a
> matching entry.
> 
> So I think your patches are correct, but I was curious how we got to
> this state.

It seems that we create this in a long time ago. In 8fd2cb4069 (Extract
helper bits from c-merge-recursive work, 2006-07-25), we introduce the
"path-list.c", at that time, we have the code already.

> 
> -Peff

Thanks,
Jialuo

[PATCH v2 4/8] string-list: enable sign compare warnings check

[shejialuo]

The only sign compare warning in "string-list" is that we compare the
`index` of the `int` type with the `list->nr` of unsigned type. We get
index by calling "get_entry_index", which would always return unsigned
index.

Let's change the return type of "get_entry_index" to be "size_t" by
slightly modifying the binary search algorithm. Instead of letting
"left" to be "-1" initially, assign 0 to it.

Then, we could delete "#define DISABLE_SIGN_COMPARE_WARNING" to enable
sign warnings check for "string-list"

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 string-list.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/string-list.c b/string-list.c
index 171cef5dbb..53faaa8420 100644
--- a/string-list.c
+++ b/string-list.c
@@ -1,5 +1,3 @@
-#define DISABLE_SIGN_COMPARE_WARNINGS
-
 #include "git-compat-util.h"
 #include "string-list.h"
 
@@ -17,19 +15,19 @@ void string_list_init_dup(struct string_list *list)
 
 /* if there is no exact match, point to the index where the entry could be
  * inserted */
-static int get_entry_index(const struct string_list *list, const char *string,
-		int *exact_match)
+static size_t get_entry_index(const struct string_list *list, const char *string,
+			      int *exact_match)
 {
-	int left = -1, right = list->nr;
+	size_t left = 0, right = list->nr;
 	compare_strings_fn cmp = list->cmp ? list->cmp : strcmp;
 
-	while (left + 1 < right) {
-		int middle = left + (right - left) / 2;
+	while (left < right) {
+		size_t middle = left + (right - left) / 2;
 		int compare = cmp(string, list->items[middle].string);
 		if (compare < 0)
 			right = middle;
 		else if (compare > 0)
-			left = middle;
+			left = middle + 1;
 		else {
 			*exact_match = 1;
 			return middle;
@@ -40,10 +38,10 @@ static int get_entry_index(const struct string_list *list, const char *string,
 	return right;
 }
 
-static int add_entry(struct string_list *list, const char *string)
+static size_t add_entry(struct string_list *list, const char *string)
 {
 	int exact_match = 0;
-	int index = get_entry_index(list, string, &exact_match);
+	size_t index = get_entry_index(list, string, &exact_match);
 
 	if (exact_match)
 		return index;
@@ -62,7 +60,7 @@ static int add_entry(struct string_list *list, const char *string)
 
 struct string_list_item *string_list_insert(struct string_list *list, const char *string)
 {
-	int index = add_entry(list, string);
+	size_t index = add_entry(list, string);
 
 	return list->items + index;
 }
-- 
2.49.0

Re: [PATCH v2 4/8] string-list: enable sign compare warnings check

[Patrick Steinhardt]

On Sun, May 18, 2025 at 11:57:23PM +0800, shejialuo wrote:
> The only sign compare warning in "string-list" is that we compare the
> `index` of the `int` type with the `list->nr` of unsigned type. We get
> index by calling "get_entry_index", which would always return unsigned
> index.
> 
> Let's change the return type of "get_entry_index" to be "size_t" by
> slightly modifying the binary search algorithm. Instead of letting
> "left" to be "-1" initially, assign 0 to it.

It would help the reader to explain why this change is equivalent to how
it worked before.

Patrick

Re: [PATCH v2 4/8] string-list: enable sign compare warnings check

[shejialuo]

On Mon, May 19, 2025 at 09:18:03AM +0200, Patrick Steinhardt wrote:
> On Sun, May 18, 2025 at 11:57:23PM +0800, shejialuo wrote:
> > The only sign compare warning in "string-list" is that we compare the
> > `index` of the `int` type with the `list->nr` of unsigned type. We get
> > index by calling "get_entry_index", which would always return unsigned
> > index.
> > 
> > Let's change the return type of "get_entry_index" to be "size_t" by
> > slightly modifying the binary search algorithm. Instead of letting
> > "left" to be "-1" initially, assign 0 to it.
> 
> It would help the reader to explain why this change is equivalent to how
> it worked before.
> 

Right, will improve this.

> Patrick

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

[shejialuo]

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" function to do
this.

Then port the shell script tests to C program and remove unused
"test-tool" code and tests.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 Makefile                     |  1 +
 t/helper/test-string-list.c  | 14 --------
 t/meson.build                |  1 +
 t/t0063-string-list.sh       | 53 -----------------------------
 t/unit-tests/u-string-list.c | 66 ++++++++++++++++++++++++++++++++++++
 5 files changed, 68 insertions(+), 67 deletions(-)
 create mode 100644 t/unit-tests/u-string-list.c

diff --git a/Makefile b/Makefile
index de73c6ddcd..cdffa13aba 100644
--- a/Makefile
+++ b/Makefile
@@ -1366,6 +1366,7 @@ CLAR_TEST_SUITES += u-prio-queue
 CLAR_TEST_SUITES += u-reftable-tree
 CLAR_TEST_SUITES += u-strbuf
 CLAR_TEST_SUITES += u-strcmp-offset
+CLAR_TEST_SUITES += u-string-list
 CLAR_TEST_SUITES += u-strvec
 CLAR_TEST_SUITES += u-trailer
 CLAR_TEST_SUITES += u-urlmatch-normalization
diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c
index 6f10c5a435..17c18c30f6 100644
--- a/t/helper/test-string-list.c
+++ b/t/helper/test-string-list.c
@@ -46,20 +46,6 @@ static int prefix_cb(struct string_list_item *item, void *cb_data)
 
 int cmd__string_list(int argc, const char **argv)
 {
-	if (argc == 5 && !strcmp(argv[1], "split")) {
-		struct string_list list = STRING_LIST_INIT_DUP;
-		int i;
-		const char *s = argv[2];
-		int delim = *argv[3];
-		int maxsplit = atoi(argv[4]);
-
-		i = string_list_split(&list, s, delim, maxsplit);
-		printf("%d\n", i);
-		write_list(&list);
-		string_list_clear(&list, 0);
-		return 0;
-	}
-
 	if (argc == 5 && !strcmp(argv[1], "split_in_place")) {
 		struct string_list list = STRING_LIST_INIT_NODUP;
 		int i;
diff --git a/t/meson.build b/t/meson.build
index fcfc1c2c2b..a3dbe572d8 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -11,6 +11,7 @@ clar_test_suites = [
   'unit-tests/u-reftable-tree.c',
   'unit-tests/u-strbuf.c',
   'unit-tests/u-strcmp-offset.c',
+  'unit-tests/u-string-list.c',
   'unit-tests/u-strvec.c',
   'unit-tests/u-trailer.c',
   'unit-tests/u-urlmatch-normalization.c',
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
index aac63ba506..6b20ffd206 100755
--- a/t/t0063-string-list.sh
+++ b/t/t0063-string-list.sh
@@ -7,16 +7,6 @@ test_description='Test string list functionality'
 
 . ./test-lib.sh
 
-test_split () {
-	cat >expected &&
-	test_expect_success "split $1 at $2, max $3" "
-		test-tool string-list split '$1' '$2' '$3' >actual &&
-		test_cmp expected actual &&
-		test-tool string-list split_in_place '$1' '$2' '$3' >actual &&
-		test_cmp expected actual
-	"
-}
-
 test_split_in_place() {
 	cat >expected &&
 	test_expect_success "split (in place) $1 at $2, max $3" "
@@ -25,49 +15,6 @@ test_split_in_place() {
 	"
 }
 
-test_split "foo:bar:baz" ":" "-1" <<EOF
-3
-[0]: "foo"
-[1]: "bar"
-[2]: "baz"
-EOF
-
-test_split "foo:bar:baz" ":" "0" <<EOF
-1
-[0]: "foo:bar:baz"
-EOF
-
-test_split "foo:bar:baz" ":" "1" <<EOF
-2
-[0]: "foo"
-[1]: "bar:baz"
-EOF
-
-test_split "foo:bar:baz" ":" "2" <<EOF
-3
-[0]: "foo"
-[1]: "bar"
-[2]: "baz"
-EOF
-
-test_split "foo:bar:" ":" "-1" <<EOF
-3
-[0]: "foo"
-[1]: "bar"
-[2]: ""
-EOF
-
-test_split "" ":" "-1" <<EOF
-1
-[0]: ""
-EOF
-
-test_split ":" ":" "-1" <<EOF
-2
-[0]: ""
-[1]: ""
-EOF
-
 test_split_in_place "foo:;:bar:;:baz:;:" ":;" "-1" <<EOF
 10
 [0]: "foo"
diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c
new file mode 100644
index 0000000000..c304934de2
--- /dev/null
+++ b/t/unit-tests/u-string-list.c
@@ -0,0 +1,66 @@
+#include "unit-test.h"
+#include "string-list.h"
+
+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);
+
+	string_list_clear(list, free_util);
+	while ((arg = va_arg(ap, const char *)))
+		string_list_append(list, arg);
+}
+
+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);
+}
+
+static void t_string_list_equal(struct string_list *list,
+				struct string_list *expected_strings)
+{
+	cl_assert_equal_i(list->nr, expected_strings->nr);
+	cl_assert(list->nr <= list->alloc);
+	for (size_t i = 0; i < expected_strings->nr; i++)
+		cl_assert_equal_s(list->items[i].string,
+				  expected_strings->items[i].string);
+}
+
+static void t_string_list_split(struct string_list *list, const char *data,
+				int delim, int maxsplit, ...)
+{
+	struct string_list expected_strings = STRING_LIST_INIT_DUP;
+	va_list ap;
+	int len;
+
+	va_start(ap, maxsplit);
+	t_vcreate_string_list_dup(&expected_strings, 0, ap);
+	va_end(ap);
+
+	string_list_clear(list, 0);
+	len = string_list_split(list, data, delim, maxsplit);
+	cl_assert_equal_i(len, expected_strings.nr);
+	t_string_list_equal(list, &expected_strings);
+
+	string_list_clear(&expected_strings, 0);
+}
+
+void test_string_list__split(void)
+{
+	struct string_list list = STRING_LIST_INIT_DUP;
+
+	t_string_list_split(&list, "foo:bar:baz", ':', -1, "foo", "bar", "baz", NULL);
+	t_string_list_split(&list, "foo:bar:baz", ':', 0, "foo:bar:baz", NULL);
+	t_string_list_split(&list, "foo:bar:baz", ':', 1, "foo", "bar:baz", NULL);
+	t_string_list_split(&list, "foo:bar:baz", ':', 2, "foo", "bar", "baz", NULL);
+	t_string_list_split(&list, "foo:bar:", ':', -1, "foo", "bar", "", NULL);
+	t_string_list_split(&list, "", ':', -1, "", NULL);
+	t_string_list_split(&list, ":", ':', -1, "", "", NULL);
+
+	t_string_list_clear(&list, 0);
+}
-- 
2.49.0

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

[Patrick Steinhardt]

On Sun, May 18, 2025 at 11:57:33PM +0800, shejialuo wrote:
> diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c
> new file mode 100644
> index 0000000000..c304934de2
> --- /dev/null
> +++ b/t/unit-tests/u-string-list.c
> @@ -0,0 +1,66 @@
> +#include "unit-test.h"
> +#include "string-list.h"
> +
> +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);
> +
> +	string_list_clear(list, free_util);
> +	while ((arg = va_arg(ap, const char *)))
> +		string_list_append(list, arg);
> +}
> +
> +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);
> +}
> +
> +static void t_string_list_equal(struct string_list *list,
> +				struct string_list *expected_strings)
> +{
> +	cl_assert_equal_i(list->nr, expected_strings->nr);
> +	cl_assert(list->nr <= list->alloc);
> +	for (size_t i = 0; i < expected_strings->nr; i++)
> +		cl_assert_equal_s(list->items[i].string,
> +				  expected_strings->items[i].string);
> +}
> +
> +static void t_string_list_split(struct string_list *list, const char *data,
> +				int delim, int maxsplit, ...)
> +{
> +	struct string_list expected_strings = STRING_LIST_INIT_DUP;
> +	va_list ap;
> +	int len;
> +
> +	va_start(ap, maxsplit);
> +	t_vcreate_string_list_dup(&expected_strings, 0, ap);
> +	va_end(ap);
> +
> +	string_list_clear(list, 0);
> +	len = string_list_split(list, data, delim, maxsplit);
> +	cl_assert_equal_i(len, expected_strings.nr);
> +	t_string_list_equal(list, &expected_strings);
> +
> +	string_list_clear(&expected_strings, 0);
> +}
> +
> +void test_string_list__split(void)
> +{
> +	struct string_list list = STRING_LIST_INIT_DUP;

Let's move this list into `t_string_list_split()`. Otherwise, tests may
negatively impact one another via this shared state. The same comment
also applies to subsequent commits.

> +	t_string_list_split(&list, "foo:bar:baz", ':', -1, "foo", "bar", "baz", NULL);
> +	t_string_list_split(&list, "foo:bar:baz", ':', 0, "foo:bar:baz", NULL);
> +	t_string_list_split(&list, "foo:bar:baz", ':', 1, "foo", "bar:baz", NULL);
> +	t_string_list_split(&list, "foo:bar:baz", ':', 2, "foo", "bar", "baz", NULL);
> +	t_string_list_split(&list, "foo:bar:", ':', -1, "foo", "bar", "", NULL);
> +	t_string_list_split(&list, "", ':', -1, "", NULL);
> +	t_string_list_split(&list, ":", ':', -1, "", "", NULL);
> +
> +	t_string_list_clear(&list, 0);

Patrick

[PATCH v2 6/8] u-string-list: move "test_split_in_place" to "u-string-list.c"

[shejialuo]

We use "test-tool string-list split_in_place" to test the
"string_list_split_in_place" function. As we have introduced the unit
test, we'd better remove the logic from shell script to C program to
improve test speed and readability.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 t/helper/test-string-list.c  | 22 ----------------
 t/t0063-string-list.sh       | 51 ------------------------------------
 t/unit-tests/u-string-list.c | 39 +++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 73 deletions(-)

diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c
index 17c18c30f6..8a344347ad 100644
--- a/t/helper/test-string-list.c
+++ b/t/helper/test-string-list.c
@@ -18,13 +18,6 @@ static void parse_string_list(struct string_list *list, const char *arg)
 	(void)string_list_split(list, arg, ':', -1);
 }
 
-static void write_list(const struct string_list *list)
-{
-	int i;
-	for (i = 0; i < list->nr; i++)
-		printf("[%d]: \"%s\"\n", i, list->items[i].string);
-}
-
 static void write_list_compact(const struct string_list *list)
 {
 	int i;
@@ -46,21 +39,6 @@ static int prefix_cb(struct string_list_item *item, void *cb_data)
 
 int cmd__string_list(int argc, const char **argv)
 {
-	if (argc == 5 && !strcmp(argv[1], "split_in_place")) {
-		struct string_list list = STRING_LIST_INIT_NODUP;
-		int i;
-		char *s = xstrdup(argv[2]);
-		const char *delim = argv[3];
-		int maxsplit = atoi(argv[4]);
-
-		i = string_list_split_in_place(&list, s, delim, maxsplit);
-		printf("%d\n", i);
-		write_list(&list);
-		string_list_clear(&list, 0);
-		free(s);
-		return 0;
-	}
-
 	if (argc == 4 && !strcmp(argv[1], "filter")) {
 		/*
 		 * Retain only the items that have the specified prefix.
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
index 6b20ffd206..1a9cf8bfcf 100755
--- a/t/t0063-string-list.sh
+++ b/t/t0063-string-list.sh
@@ -7,57 +7,6 @@ test_description='Test string list functionality'
 
 . ./test-lib.sh
 
-test_split_in_place() {
-	cat >expected &&
-	test_expect_success "split (in place) $1 at $2, max $3" "
-		test-tool string-list split_in_place '$1' '$2' '$3' >actual &&
-		test_cmp expected actual
-	"
-}
-
-test_split_in_place "foo:;:bar:;:baz:;:" ":;" "-1" <<EOF
-10
-[0]: "foo"
-[1]: ""
-[2]: ""
-[3]: "bar"
-[4]: ""
-[5]: ""
-[6]: "baz"
-[7]: ""
-[8]: ""
-[9]: ""
-EOF
-
-test_split_in_place "foo:;:bar:;:baz" ":;" "0" <<EOF
-1
-[0]: "foo:;:bar:;:baz"
-EOF
-
-test_split_in_place "foo:;:bar:;:baz" ":;" "1" <<EOF
-2
-[0]: "foo"
-[1]: ";:bar:;:baz"
-EOF
-
-test_split_in_place "foo:;:bar:;:baz" ":;" "2" <<EOF
-3
-[0]: "foo"
-[1]: ""
-[2]: ":bar:;:baz"
-EOF
-
-test_split_in_place "foo:;:bar:;:" ":;" "-1" <<EOF
-7
-[0]: "foo"
-[1]: ""
-[2]: ""
-[3]: "bar"
-[4]: ""
-[5]: ""
-[6]: ""
-EOF
-
 test_expect_success "test filter_string_list" '
 	test "x-" = "x$(test-tool string-list filter - y)" &&
 	test "x-" = "x$(test-tool string-list filter no y)" &&
diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c
index c304934de2..e4b8e38fb8 100644
--- a/t/unit-tests/u-string-list.c
+++ b/t/unit-tests/u-string-list.c
@@ -64,3 +64,42 @@ void test_string_list__split(void)
 
 	t_string_list_clear(&list, 0);
 }
+
+static void t_string_list_split_in_place(struct string_list *list, const char *data,
+					 const char *delim, int maxsplit, ...)
+{
+	struct string_list expected_strings = STRING_LIST_INIT_DUP;
+	char *string = xstrdup(data);
+	va_list ap;
+	int len;
+
+	va_start(ap, maxsplit);
+	t_vcreate_string_list_dup(&expected_strings, 0, ap);
+	va_end(ap);
+
+	string_list_clear(list, 0);
+	len = string_list_split_in_place(list, string, delim, maxsplit);
+	cl_assert_equal_i(len, expected_strings.nr);
+	t_string_list_equal(list, &expected_strings);
+
+	free(string);
+	string_list_clear(&expected_strings, 0);
+}
+
+void test_string_list__split_in_place(void)
+{
+	struct string_list list = STRING_LIST_INIT_NODUP;
+
+	t_string_list_split_in_place(&list, "foo:;:bar:;:baz:;:", ":;", -1,
+				     "foo", "", "", "bar", "", "", "baz", "", "", "", NULL);
+	t_string_list_split_in_place(&list, "foo:;:bar:;:baz", ":;", 0,
+				     "foo:;:bar:;:baz", NULL);
+	t_string_list_split_in_place(&list, "foo:;:bar:;:baz", ":;", 1,
+				     "foo", ";:bar:;:baz", NULL);
+	t_string_list_split_in_place(&list, "foo:;:bar:;:baz", ":;", 2,
+				     "foo", "", ":bar:;:baz", NULL);
+	t_string_list_split_in_place(&list, "foo:;:bar:;:", ":;", -1,
+				     "foo", "", "", "bar", "", "", "", NULL);
+
+	t_string_list_clear(&list, 0);
+}
-- 
2.49.0

[PATCH v2 7/8] u-string-list: move "filter string" test to "u-string-list.c"

[shejialuo]

We use "test-tool string-list filter" to test the "filter_string_list"
function. As we have introduced the unit test, we'd better remove the
logic from shell script to C program to improve test speed and
readability.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 t/helper/test-string-list.c  | 21 ------------
 t/t0063-string-list.sh       | 11 ------
 t/unit-tests/u-string-list.c | 66 ++++++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 32 deletions(-)

diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c
index 8a344347ad..262b28c599 100644
--- a/t/helper/test-string-list.c
+++ b/t/helper/test-string-list.c
@@ -31,29 +31,8 @@ static void write_list_compact(const struct string_list *list)
 	}
 }
 
-static int prefix_cb(struct string_list_item *item, void *cb_data)
-{
-	const char *prefix = (const char *)cb_data;
-	return starts_with(item->string, prefix);
-}
-
 int cmd__string_list(int argc, const char **argv)
 {
-	if (argc == 4 && !strcmp(argv[1], "filter")) {
-		/*
-		 * Retain only the items that have the specified prefix.
-		 * Arguments: list|- prefix
-		 */
-		struct string_list list = STRING_LIST_INIT_DUP;
-		const char *prefix = argv[3];
-
-		parse_string_list(&list, argv[2]);
-		filter_string_list(&list, 0, prefix_cb, (void *)prefix);
-		write_list_compact(&list);
-		string_list_clear(&list, 0);
-		return 0;
-	}
-
 	if (argc == 3 && !strcmp(argv[1], "remove_duplicates")) {
 		struct string_list list = STRING_LIST_INIT_DUP;
 
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
index 1a9cf8bfcf..31fd62bba8 100755
--- a/t/t0063-string-list.sh
+++ b/t/t0063-string-list.sh
@@ -7,17 +7,6 @@ test_description='Test string list functionality'
 
 . ./test-lib.sh
 
-test_expect_success "test filter_string_list" '
-	test "x-" = "x$(test-tool string-list filter - y)" &&
-	test "x-" = "x$(test-tool string-list filter no y)" &&
-	test yes = "$(test-tool string-list filter yes y)" &&
-	test yes = "$(test-tool string-list filter no:yes y)" &&
-	test yes = "$(test-tool string-list filter yes:no y)" &&
-	test y1:y2 = "$(test-tool string-list filter y1:y2 y)" &&
-	test y2:y1 = "$(test-tool string-list filter y2:y1 y)" &&
-	test "x-" = "x$(test-tool string-list filter x1:x2 y)"
-'
-
 test_expect_success "test remove_duplicates" '
 	test "x-" = "x$(test-tool string-list remove_duplicates -)" &&
 	test "x" = "x$(test-tool string-list remove_duplicates "")" &&
diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c
index e4b8e38fb8..be2bb5f103 100644
--- a/t/unit-tests/u-string-list.c
+++ b/t/unit-tests/u-string-list.c
@@ -13,6 +13,18 @@ static void t_vcreate_string_list_dup(struct string_list *list,
 		string_list_append(list, arg);
 }
 
+static void t_create_string_list_dup(struct string_list *list, int free_util, ...)
+{
+	va_list ap;
+
+	cl_assert(list->strdup_strings);
+
+	string_list_clear(list, free_util);
+	va_start(ap, free_util);
+	t_vcreate_string_list_dup(list, free_util, ap);
+	va_end(ap);
+}
+
 static void t_string_list_clear(struct string_list *list, int free_util)
 {
 	string_list_clear(list, free_util);
@@ -103,3 +115,57 @@ void test_string_list__split_in_place(void)
 
 	t_string_list_clear(&list, 0);
 }
+
+static int prefix_cb(struct string_list_item *item, void *cb_data)
+{
+	const char *prefix = (const char *)cb_data;
+	return starts_with(item->string, prefix);
+}
+
+static void t_string_list_filter(struct string_list *list,
+				 string_list_each_func_t want, void *cb_data, ...)
+{
+	struct string_list expected_strings = STRING_LIST_INIT_DUP;
+	va_list ap;
+
+	va_start(ap, cb_data);
+	t_vcreate_string_list_dup(&expected_strings, 0, ap);
+	va_end(ap);
+
+	filter_string_list(list, 0, want, cb_data);
+	t_string_list_equal(list, &expected_strings);
+
+	string_list_clear(&expected_strings, 0);
+}
+
+void test_string_list__filter(void)
+{
+	struct string_list list = STRING_LIST_INIT_DUP;
+	const char *prefix = "y";
+
+	t_create_string_list_dup(&list, 0, NULL);
+	t_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);
+
+	t_create_string_list_dup(&list, 0, "no", NULL);
+	t_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);
+
+	t_create_string_list_dup(&list, 0, "yes", NULL);
+	t_string_list_filter(&list, prefix_cb, (void*)prefix, "yes", NULL);
+
+	t_create_string_list_dup(&list, 0, "no", "yes", NULL);
+	t_string_list_filter(&list, prefix_cb, (void*)prefix, "yes", NULL);
+
+	t_create_string_list_dup(&list, 0, "yes", "no", NULL);
+	t_string_list_filter(&list, prefix_cb, (void*)prefix, "yes", NULL);
+
+	t_create_string_list_dup(&list, 0, "y1", "y2", NULL);
+	t_string_list_filter(&list, prefix_cb, (void*)prefix, "y1", "y2", NULL);
+
+	t_create_string_list_dup(&list, 0, "y2", "y1", NULL);
+	t_string_list_filter(&list, prefix_cb, (void*)prefix, "y2", "y1", NULL);
+
+	t_create_string_list_dup(&list, 0, "x1", "x2", NULL);
+	t_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);
+
+	t_string_list_clear(&list, 0);
+}
-- 
2.49.0

Re: [PATCH v2 7/8] u-string-list: move "filter string" test to "u-string-list.c"

[Patrick Steinhardt]

On Sun, May 18, 2025 at 11:58:09PM +0800, shejialuo wrote:
> diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c
> index e4b8e38fb8..be2bb5f103 100644
> --- a/t/unit-tests/u-string-list.c
> +++ b/t/unit-tests/u-string-list.c
> @@ -103,3 +115,57 @@ void test_string_list__split_in_place(void)
>  
>  	t_string_list_clear(&list, 0);
>  }
> +
> +static int prefix_cb(struct string_list_item *item, void *cb_data)
> +{
> +	const char *prefix = (const char *)cb_data;
> +	return starts_with(item->string, prefix);
> +}
> +
> +static void t_string_list_filter(struct string_list *list,
> +				 string_list_each_func_t want, void *cb_data, ...)
> +{
> +	struct string_list expected_strings = STRING_LIST_INIT_DUP;
> +	va_list ap;
> +
> +	va_start(ap, cb_data);
> +	t_vcreate_string_list_dup(&expected_strings, 0, ap);
> +	va_end(ap);
> +
> +	filter_string_list(list, 0, want, cb_data);
> +	t_string_list_equal(list, &expected_strings);
> +
> +	string_list_clear(&expected_strings, 0);
> +}
> +
> +void test_string_list__filter(void)
> +{
> +	struct string_list list = STRING_LIST_INIT_DUP;
> +	const char *prefix = "y";
> +
> +	t_create_string_list_dup(&list, 0, NULL);

Okay, here we have to manually create a list because you cannot pass two
vararg lists to `t_string_list_filter()`. It's not the prettiest and
feels a bit repetitive, but the alternatives I can think of aren't a lot
nicer, either.

> +	t_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);

Both the `prefix_cb` and `prefix` are the same across all function
calls, so I wondered whether we might want to move them into the
wrapper function directly.

The `void *` casts are also all unnecessary.

Patrick

Re: [PATCH v2 7/8] u-string-list: move "filter string" test to "u-string-list.c"

[shejialuo]

On Mon, May 19, 2025 at 09:18:06AM +0200, Patrick Steinhardt wrote:
> On Sun, May 18, 2025 at 11:58:09PM +0800, shejialuo wrote:
> > diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c
> > index e4b8e38fb8..be2bb5f103 100644
> > --- a/t/unit-tests/u-string-list.c
> > +++ b/t/unit-tests/u-string-list.c
> > @@ -103,3 +115,57 @@ void test_string_list__split_in_place(void)
> >  
> >  	t_string_list_clear(&list, 0);
> >  }
> > +
> > +static int prefix_cb(struct string_list_item *item, void *cb_data)
> > +{
> > +	const char *prefix = (const char *)cb_data;
> > +	return starts_with(item->string, prefix);
> > +}
> > +
> > +static void t_string_list_filter(struct string_list *list,
> > +				 string_list_each_func_t want, void *cb_data, ...)
> > +{
> > +	struct string_list expected_strings = STRING_LIST_INIT_DUP;
> > +	va_list ap;
> > +
> > +	va_start(ap, cb_data);
> > +	t_vcreate_string_list_dup(&expected_strings, 0, ap);
> > +	va_end(ap);
> > +
> > +	filter_string_list(list, 0, want, cb_data);
> > +	t_string_list_equal(list, &expected_strings);
> > +
> > +	string_list_clear(&expected_strings, 0);
> > +}
> > +
> > +void test_string_list__filter(void)
> > +{
> > +	struct string_list list = STRING_LIST_INIT_DUP;
> > +	const char *prefix = "y";
> > +
> > +	t_create_string_list_dup(&list, 0, NULL);
> 
> Okay, here we have to manually create a list because you cannot pass two
> vararg lists to `t_string_list_filter()`. It's not the prettiest and
> feels a bit repetitive, but the alternatives I can think of aren't a lot
> nicer, either.
> 

Yes, I am not very satisfied with this either. In the original shell
script test, it uses "string_list_split" to create a new "string-list".
However, this way is worse than the current, too hacky.

> > +	t_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);
> 
> Both the `prefix_cb` and `prefix` are the same across all function
> calls, so I wondered whether we might want to move them into the
> wrapper function directly.
> 

What if we want to use the different "prefix_cb" and "prefix"? I somehow
think we should make "t_string_list_filter" more flexible.

> The `void *` casts are also all unnecessary.
> 

Right, I will improve this in the next version.

> Patrick

[PATCH v2 8/8] u-string-list: move "remove duplicates" test to "u-string-list.c"

[shejialuo]

We use "test-tool string-list remove_duplicates" to test the
"string_list_remove_duplicates" function. As we have introduced the unit
test, we'd better remove the logic from shell script to C program to
improve test speed and readability.

As all the tests in shell script are removed, let's just delete the
"t0063-string-list.sh" and update the "meson.build" file to align with
this change.

Also we could simply remove "DISABLE_SIGN_COMPARE_WARNINGS" due to we
have already deleted related code.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 t/helper/test-string-list.c  | 39 -----------------------
 t/meson.build                |  1 -
 t/t0063-string-list.sh       | 27 ----------------
 t/unit-tests/u-string-list.c | 62 ++++++++++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+), 67 deletions(-)
 delete mode 100755 t/t0063-string-list.sh

diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c
index 262b28c599..6be0cdb8e2 100644
--- a/t/helper/test-string-list.c
+++ b/t/helper/test-string-list.c
@@ -1,48 +1,9 @@
-#define DISABLE_SIGN_COMPARE_WARNINGS
-
 #include "test-tool.h"
 #include "strbuf.h"
 #include "string-list.h"
 
-/*
- * Parse an argument into a string list.  arg should either be a
- * ':'-separated list of strings, or "-" to indicate an empty string
- * list (as opposed to "", which indicates a string list containing a
- * single empty string).  list->strdup_strings must be set.
- */
-static void parse_string_list(struct string_list *list, const char *arg)
-{
-	if (!strcmp(arg, "-"))
-		return;
-
-	(void)string_list_split(list, arg, ':', -1);
-}
-
-static void write_list_compact(const struct string_list *list)
-{
-	int i;
-	if (!list->nr)
-		printf("-\n");
-	else {
-		printf("%s", list->items[0].string);
-		for (i = 1; i < list->nr; i++)
-			printf(":%s", list->items[i].string);
-		printf("\n");
-	}
-}
-
 int cmd__string_list(int argc, const char **argv)
 {
-	if (argc == 3 && !strcmp(argv[1], "remove_duplicates")) {
-		struct string_list list = STRING_LIST_INIT_DUP;
-
-		parse_string_list(&list, argv[2]);
-		string_list_remove_duplicates(&list, 0);
-		write_list_compact(&list);
-		string_list_clear(&list, 0);
-		return 0;
-	}
-
 	if (argc == 2 && !strcmp(argv[1], "sort")) {
 		struct string_list list = STRING_LIST_INIT_NODUP;
 		struct strbuf sb = STRBUF_INIT;
diff --git a/t/meson.build b/t/meson.build
index a3dbe572d8..c7efcc6db9 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -124,7 +124,6 @@ integration_tests = [
   't0060-path-utils.sh',
   't0061-run-command.sh',
   't0062-revision-walking.sh',
-  't0063-string-list.sh',
   't0066-dir-iterator.sh',
   't0067-parse_pathspec_file.sh',
   't0068-for-each-repo.sh',
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
deleted file mode 100755
index 31fd62bba8..0000000000
--- a/t/t0063-string-list.sh
+++ /dev/null
@@ -1,27 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2012 Michael Haggerty
-#
-
-test_description='Test string list functionality'
-
-. ./test-lib.sh
-
-test_expect_success "test remove_duplicates" '
-	test "x-" = "x$(test-tool string-list remove_duplicates -)" &&
-	test "x" = "x$(test-tool string-list remove_duplicates "")" &&
-	test a = "$(test-tool string-list remove_duplicates a)" &&
-	test a = "$(test-tool string-list remove_duplicates a:a)" &&
-	test a = "$(test-tool string-list remove_duplicates a:a:a:a:a)" &&
-	test a:b = "$(test-tool string-list remove_duplicates a:b)" &&
-	test a:b = "$(test-tool string-list remove_duplicates a:a:b)" &&
-	test a:b = "$(test-tool string-list remove_duplicates a:b:b)" &&
-	test a:b:c = "$(test-tool string-list remove_duplicates a:b:c)" &&
-	test a:b:c = "$(test-tool string-list remove_duplicates a:a:b:c)" &&
-	test a:b:c = "$(test-tool string-list remove_duplicates a:b:b:c)" &&
-	test a:b:c = "$(test-tool string-list remove_duplicates a:b:c:c)" &&
-	test a:b:c = "$(test-tool string-list remove_duplicates a:a:b:b:c:c)" &&
-	test a:b:c = "$(test-tool string-list remove_duplicates a:a:a:b:b:b:c:c:c)"
-'
-
-test_done
diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c
index be2bb5f103..a575fdda97 100644
--- a/t/unit-tests/u-string-list.c
+++ b/t/unit-tests/u-string-list.c
@@ -169,3 +169,65 @@ void test_string_list__filter(void)
 
 	t_string_list_clear(&list, 0);
 }
+
+static void t_string_list_remove_duplicates(struct string_list *list, ...)
+{
+	struct string_list expected_strings = STRING_LIST_INIT_DUP;
+	va_list ap;
+
+	va_start(ap, list);
+	t_vcreate_string_list_dup(&expected_strings, 0, ap);
+	va_end(ap);
+
+	string_list_remove_duplicates(list, 0);
+	t_string_list_equal(list, &expected_strings);
+
+	string_list_clear(&expected_strings, 0);
+}
+
+void test_string_list__remove_duplicates(void)
+{
+	struct string_list list = STRING_LIST_INIT_DUP;
+
+	t_create_string_list_dup(&list, 0, NULL);
+	t_string_list_remove_duplicates(&list, NULL);
+
+	t_create_string_list_dup(&list, 0, "", NULL);
+	t_string_list_remove_duplicates(&list, "", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", NULL);
+	t_string_list_remove_duplicates(&list, "a", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "a", NULL);
+	t_string_list_remove_duplicates(&list, "a", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "a", "a", NULL);
+	t_string_list_remove_duplicates(&list, "a", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "a", "b", NULL);
+	t_string_list_remove_duplicates(&list, "a", "b", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "b", "b", NULL);
+	t_string_list_remove_duplicates(&list, "a", "b", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "b", "c", NULL);
+	t_string_list_remove_duplicates(&list, "a", "b", "c", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "a", "b", "c", NULL);
+	t_string_list_remove_duplicates(&list, "a", "b", "c", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "b", "b", "c", NULL);
+	t_string_list_remove_duplicates(&list, "a", "b", "c", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "b", "c", "c", NULL);
+	t_string_list_remove_duplicates(&list, "a", "b", "c", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "a", "b", "b", "c", "c", NULL);
+	t_string_list_remove_duplicates(&list, "a", "b", "c", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "a", "a", "b", "b", "b",
+				 "c", "c", "c", NULL);
+	t_string_list_remove_duplicates(&list, "a", "b", "c", NULL);
+
+	t_string_list_clear(&list, 0);
+}
-- 
2.49.0

Re: [PATCH v2 8/8] u-string-list: move "remove duplicates" test to "u-string-list.c"

[Patrick Steinhardt]

On Sun, May 18, 2025 at 11:58:25PM +0800, shejialuo wrote:
> We use "test-tool string-list remove_duplicates" to test the
> "string_list_remove_duplicates" function. As we have introduced the unit
> test, we'd better remove the logic from shell script to C program to
> improve test speed and readability.
> 
> As all the tests in shell script are removed, let's just delete the
> "t0063-string-list.sh" and update the "meson.build" file to align with
> this change.
> 
> Also we could simply remove "DISABLE_SIGN_COMPARE_WARNINGS" due to we
> have already deleted related code.

I think it would make sense to explain why the test helper itself isn't
being removed in this commit.

Patrick

[PATCH v3 0/8] enhance "string_list" code and test

[shejialuo]

Hi All:

I finally finish the version 2. And I don't provide the range-diff due
to that I add more commits compared with version 1.

This patch could be organized into three parts:

1. [PATCH v2 1/8]

   Fix simple sign warnings of the loop iterator.

2. [PATCH v2 2/8] - [PATCH v2 4/8]

   Remove unncessary code, improve the logic and finally enable sign
   compare warnings check.

3. [PATCH v2 5/8] - [PATCH v2 8/8]

   Remove test to the unit test.

---

Changes since v2:

1. [PATCH v3 1/8]: improve the commit message to explain that we would
   handle a warning in the later commits.
2. [PATCH v3 2/8] and [PATCH v2 3/8]: improve the commit message to add
   the history background.
3. [PATCH v3 4/8]: improve the commit message to show why the current
   bianry search algorithm introduces the sign warning and how to change
   it to fix the sign warning.
4. [PATCH v3 5/8] - [PATCH v3 8/8]: remove list into test helper instead
   of test itself for reducing the shared state.
5. [PATCH v3 8/8]: improve the commit message to say why we can't delete
   "test-tool string_list" totally.

Thanks,
Jialuo

shejialuo (8):
  string-list: fix sign compare warnings for loop iterator
  string-list: remove unused "insert_at" parameter from add_entry
  string-list: return index directly when inserting an existing element
  string-list: enable sign compare warnings check
  u-string-list: move "test_split" into "u-string-list.c"
  u-string-list: move "test_split_in_place" to "u-string-list.c"
  u-string-list: move "filter string" test to "u-string-list.c"
  u-string-list: move "remove duplicates" test to "u-string-list.c"

 Makefile                     |   1 +
 string-list.c                |  48 +++-----
 t/helper/test-string-list.c  |  96 ---------------
 t/meson.build                |   2 +-
 t/t0063-string-list.sh       | 142 ----------------------
 t/unit-tests/u-string-list.c | 227 +++++++++++++++++++++++++++++++++++
 6 files changed, 249 insertions(+), 267 deletions(-)
 delete mode 100755 t/t0063-string-list.sh
 create mode 100644 t/unit-tests/u-string-list.c

Range-diff against v2:
1:  4fb4546525 ! 1:  a69eb0f7b8 string-list: fix sign compare warnings for loop iterator
    @@ Metadata
      ## Commit message ##
         string-list: fix sign compare warnings for loop iterator
     
    -    In "string-list.c", there are six warnings which are emitted by
    -    "Wsign-compare". And five warnings are caused by the loop iterator type
    -    mismatch. Let's fix these five warnings by changing the `int` type to
    -    `size_t` type of the loop iterator.
    +    There are a couple of "-Wsign-compare" warnings in "string-list.c". Fix
    +    trivial ones that result from a mismatched loop iterator type.
    +
    +    There is a single warning left after these fixes. This warning needs
    +    a bit more care and is thus handled in subsequent commits.
     
         Signed-off-by: shejialuo <shejialuo@gmail.com>
     
2:  3721c92803 ! 2:  35550b3b7d string-list: remove unused "insert_at" parameter from add_entry
    @@ Commit message
     
         However, we only use "add_entry" in "string_list_insert" function and we
         always pass the "-1" for "insert_at" parameter. So, we never use this
    -    parameter to insert element in a user specified position. Let's delete
    -    this parameter. If there is any requirement later, we need to use a
    -    better way to do this.
    +    parameter to insert element in a user specified position.
    +
    +    And we should know why there is such code path in the first place. We
    +    used to have another function "string_list_insert_at_index()", which
    +    uses the extra "insert_at" parameter. And in f8c4ab611a (string_list:
    +    remove string_list_insert_at_index() from its API, 2014-11-24), we
    +    remove this function but we don't clean all the code path.
    +
    +    Let's simply delete this parameter as we'd better use "strmap" for such
    +    functionality.
     
         Signed-off-by: shejialuo <shejialuo@gmail.com>
     
3:  fa1d22c93d ! 3:  7877b528e4 string-list: return index directly when inserting an existing element
    @@ Commit message
     
         When inserting an existing element, "add_entry" would convert "index"
         value to "-1-index" to indicate the caller that this element is in the
    -    list already.
    +    list already. However, in "string_list_insert", we would simply convert
    +    this to the original positive index without any further action.
     
    -    However, in "string_list_insert", we would simply convert this to the
    -    original positive index without any further action. Let's directly
    -    return the index as we don't care about whether the element is in the
    -    list by using "add_entry".
    +    In 8fd2cb4069 (Extract helper bits from c-merge-recursive work,
    +    2006-07-25), we create "path-list.c" and then introduce above code path.
     
    -    In the future, if we want to let "add_entry" tell the caller, we may add
    -    "int *exact_match" parameter to "add_entry" instead of converting the
    -    index to negative to indicate.
    +    Let's directly return the index as we don't care about whether the
    +    element is in the list by using "add_entry". In the future, if we want
    +    to let "add_entry" tell the caller, we may add "int *exact_match"
    +    parameter to "add_entry" instead of converting the index to negative to
    +    indicate.
     
         Signed-off-by: shejialuo <shejialuo@gmail.com>
     
4:  02e5c4307b ! 4:  50c4dbff5c string-list: enable sign compare warnings check
    @@ Metadata
      ## Commit message ##
         string-list: enable sign compare warnings check
     
    -    The only sign compare warning in "string-list" is that we compare the
    -    `index` of the `int` type with the `list->nr` of unsigned type. We get
    -    index by calling "get_entry_index", which would always return unsigned
    -    index.
    +    In "add_entry", we call "get_entry_index" function to get the inserted
    +    position. However, as the return type of "get_entry_index" function is
    +    `int`, there is a sign compare warning when comparing the `index` with
    +    the `list-nr` of unsigned type.
     
    -    Let's change the return type of "get_entry_index" to be "size_t" by
    -    slightly modifying the binary search algorithm. Instead of letting
    -    "left" to be "-1" initially, assign 0 to it.
    +    "get_entry_index" would always return unsigned index. However, the
    +    current binary search algorithm initializes "left" to be "-1", which
    +    necessitates the use of signed `int` return type.
    +
    +    The reason why we need to assign "left" to be "-1" is that in the
    +    `while` loop, we increment "left" by 1 to determine whether the loop
    +    should end. This design choice, while functional, forces us to use
    +    signed arithmetic throughout the function.
    +
    +    To resolve this sign comparison issue, let's modify the binary search
    +    algorithm with the following approach:
    +
    +    1. Initialize "left" to 0 instead of -1
    +    2. Use `left < right` as the loop termination condition instead of
    +       `left + 1 < right`
    +    3. When searching the right part, set `left = middle + 1` instead of
    +       `middle`
     
         Then, we could delete "#define DISABLE_SIGN_COMPARE_WARNING" to enable
    -    sign warnings check for "string-list"
    +    sign warnings check for "string-list".
     
         Signed-off-by: shejialuo <shejialuo@gmail.com>
     
5:  990ce4db0f ! 5:  ab2aec5887 u-string-list: move "test_split" into "u-string-list.c"
    @@ t/unit-tests/u-string-list.c (new)
     +		string_list_append(list, arg);
     +}
     +
    -+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);
    -+}
    -+
     +static void t_string_list_equal(struct string_list *list,
     +				struct string_list *expected_strings)
     +{
    @@ t/unit-tests/u-string-list.c (new)
     +				  expected_strings->items[i].string);
     +}
     +
    -+static void t_string_list_split(struct string_list *list, const char *data,
    -+				int delim, int maxsplit, ...)
    ++static void t_string_list_split(const char *data, int delim, int maxsplit, ...)
     +{
     +	struct string_list expected_strings = STRING_LIST_INIT_DUP;
    ++	struct string_list list = STRING_LIST_INIT_DUP;
     +	va_list ap;
     +	int len;
     +
    @@ t/unit-tests/u-string-list.c (new)
     +	t_vcreate_string_list_dup(&expected_strings, 0, ap);
     +	va_end(ap);
     +
    -+	string_list_clear(list, 0);
    -+	len = string_list_split(list, data, delim, maxsplit);
    ++	string_list_clear(&list, 0);
    ++	len = string_list_split(&list, data, delim, maxsplit);
     +	cl_assert_equal_i(len, expected_strings.nr);
    -+	t_string_list_equal(list, &expected_strings);
    ++	t_string_list_equal(&list, &expected_strings);
     +
     +	string_list_clear(&expected_strings, 0);
    ++	string_list_clear(&list, 0);
     +}
     +
     +void test_string_list__split(void)
     +{
    -+	struct string_list list = STRING_LIST_INIT_DUP;
    -+
    -+	t_string_list_split(&list, "foo:bar:baz", ':', -1, "foo", "bar", "baz", NULL);
    -+	t_string_list_split(&list, "foo:bar:baz", ':', 0, "foo:bar:baz", NULL);
    -+	t_string_list_split(&list, "foo:bar:baz", ':', 1, "foo", "bar:baz", NULL);
    -+	t_string_list_split(&list, "foo:bar:baz", ':', 2, "foo", "bar", "baz", NULL);
    -+	t_string_list_split(&list, "foo:bar:", ':', -1, "foo", "bar", "", NULL);
    -+	t_string_list_split(&list, "", ':', -1, "", NULL);
    -+	t_string_list_split(&list, ":", ':', -1, "", "", NULL);
    -+
    -+	t_string_list_clear(&list, 0);
    ++	t_string_list_split("foo:bar:baz", ':', -1, "foo", "bar", "baz", NULL);
    ++	t_string_list_split("foo:bar:baz", ':', 0, "foo:bar:baz", NULL);
    ++	t_string_list_split("foo:bar:baz", ':', 1, "foo", "bar:baz", NULL);
    ++	t_string_list_split("foo:bar:baz", ':', 2, "foo", "bar", "baz", NULL);
    ++	t_string_list_split("foo:bar:", ':', -1, "foo", "bar", "", NULL);
    ++	t_string_list_split("", ':', -1, "", NULL);
    ++	t_string_list_split(":", ':', -1, "", "", NULL);
     +}
6:  5a02f590df ! 6:  06aca89b57 u-string-list: move "test_split_in_place" to "u-string-list.c"
    @@ t/t0063-string-list.sh: test_description='Test string list functionality'
     
      ## t/unit-tests/u-string-list.c ##
     @@ t/unit-tests/u-string-list.c: void test_string_list__split(void)
    - 
    - 	t_string_list_clear(&list, 0);
    + 	t_string_list_split("", ':', -1, "", NULL);
    + 	t_string_list_split(":", ':', -1, "", "", NULL);
      }
     +
    -+static void t_string_list_split_in_place(struct string_list *list, const char *data,
    -+					 const char *delim, int maxsplit, ...)
    ++static void t_string_list_split_in_place(const char *data, const char *delim,
    ++					 int maxsplit, ...)
     +{
     +	struct string_list expected_strings = STRING_LIST_INIT_DUP;
    ++	struct string_list list = STRING_LIST_INIT_NODUP;
     +	char *string = xstrdup(data);
     +	va_list ap;
     +	int len;
    @@ t/unit-tests/u-string-list.c: void test_string_list__split(void)
     +	t_vcreate_string_list_dup(&expected_strings, 0, ap);
     +	va_end(ap);
     +
    -+	string_list_clear(list, 0);
    -+	len = string_list_split_in_place(list, string, delim, maxsplit);
    ++	string_list_clear(&list, 0);
    ++	len = string_list_split_in_place(&list, string, delim, maxsplit);
     +	cl_assert_equal_i(len, expected_strings.nr);
    -+	t_string_list_equal(list, &expected_strings);
    ++	t_string_list_equal(&list, &expected_strings);
     +
     +	free(string);
     +	string_list_clear(&expected_strings, 0);
    ++	string_list_clear(&list, 0);
     +}
     +
     +void test_string_list__split_in_place(void)
     +{
    -+	struct string_list list = STRING_LIST_INIT_NODUP;
    -+
    -+	t_string_list_split_in_place(&list, "foo:;:bar:;:baz:;:", ":;", -1,
    ++	t_string_list_split_in_place("foo:;:bar:;:baz:;:", ":;", -1,
     +				     "foo", "", "", "bar", "", "", "baz", "", "", "", NULL);
    -+	t_string_list_split_in_place(&list, "foo:;:bar:;:baz", ":;", 0,
    ++	t_string_list_split_in_place("foo:;:bar:;:baz", ":;", 0,
     +				     "foo:;:bar:;:baz", NULL);
    -+	t_string_list_split_in_place(&list, "foo:;:bar:;:baz", ":;", 1,
    ++	t_string_list_split_in_place("foo:;:bar:;:baz", ":;", 1,
     +				     "foo", ";:bar:;:baz", NULL);
    -+	t_string_list_split_in_place(&list, "foo:;:bar:;:baz", ":;", 2,
    ++	t_string_list_split_in_place("foo:;:bar:;:baz", ":;", 2,
     +				     "foo", "", ":bar:;:baz", NULL);
    -+	t_string_list_split_in_place(&list, "foo:;:bar:;:", ":;", -1,
    ++	t_string_list_split_in_place("foo:;:bar:;:", ":;", -1,
     +				     "foo", "", "", "bar", "", "", "", NULL);
    -+
    -+	t_string_list_clear(&list, 0);
     +}
7:  87a6ec722d ! 7:  4cc76a6fb4 u-string-list: move "filter string" test to "u-string-list.c"
    @@ t/unit-tests/u-string-list.c: static void t_vcreate_string_list_dup(struct strin
     +	va_end(ap);
     +}
     +
    - static void t_string_list_clear(struct string_list *list, int free_util)
    ++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);
    ++}
    ++
    + static void t_string_list_equal(struct string_list *list,
    + 				struct string_list *expected_strings)
      {
    - 	string_list_clear(list, free_util);
     @@ t/unit-tests/u-string-list.c: void test_string_list__split_in_place(void)
    - 
    - 	t_string_list_clear(&list, 0);
    + 	t_string_list_split_in_place("foo:;:bar:;:", ":;", -1,
    + 				     "foo", "", "", "bar", "", "", "", NULL);
      }
     +
     +static int prefix_cb(struct string_list_item *item, void *cb_data)
    @@ t/unit-tests/u-string-list.c: void test_string_list__split_in_place(void)
     +	return starts_with(item->string, prefix);
     +}
     +
    -+static void t_string_list_filter(struct string_list *list,
    -+				 string_list_each_func_t want, void *cb_data, ...)
    ++static void t_string_list_filter(struct string_list *list, ...)
     +{
     +	struct string_list expected_strings = STRING_LIST_INIT_DUP;
    ++	const char *prefix = "y";
     +	va_list ap;
     +
    -+	va_start(ap, cb_data);
    ++	va_start(ap, list);
     +	t_vcreate_string_list_dup(&expected_strings, 0, ap);
     +	va_end(ap);
     +
    -+	filter_string_list(list, 0, want, cb_data);
    ++	filter_string_list(list, 0, prefix_cb, (void *)prefix);
     +	t_string_list_equal(list, &expected_strings);
     +
     +	string_list_clear(&expected_strings, 0);
    @@ t/unit-tests/u-string-list.c: void test_string_list__split_in_place(void)
     +void test_string_list__filter(void)
     +{
     +	struct string_list list = STRING_LIST_INIT_DUP;
    -+	const char *prefix = "y";
     +
     +	t_create_string_list_dup(&list, 0, NULL);
    -+	t_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);
    ++	t_string_list_filter(&list, NULL);
     +
     +	t_create_string_list_dup(&list, 0, "no", NULL);
    -+	t_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);
    ++	t_string_list_filter(&list, NULL);
     +
     +	t_create_string_list_dup(&list, 0, "yes", NULL);
    -+	t_string_list_filter(&list, prefix_cb, (void*)prefix, "yes", NULL);
    ++	t_string_list_filter(&list, "yes", NULL);
     +
     +	t_create_string_list_dup(&list, 0, "no", "yes", NULL);
    -+	t_string_list_filter(&list, prefix_cb, (void*)prefix, "yes", NULL);
    ++	t_string_list_filter(&list, "yes", NULL);
     +
     +	t_create_string_list_dup(&list, 0, "yes", "no", NULL);
    -+	t_string_list_filter(&list, prefix_cb, (void*)prefix, "yes", NULL);
    ++	t_string_list_filter(&list, "yes", NULL);
     +
     +	t_create_string_list_dup(&list, 0, "y1", "y2", NULL);
    -+	t_string_list_filter(&list, prefix_cb, (void*)prefix, "y1", "y2", NULL);
    ++	t_string_list_filter(&list, "y1", "y2", NULL);
     +
     +	t_create_string_list_dup(&list, 0, "y2", "y1", NULL);
    -+	t_string_list_filter(&list, prefix_cb, (void*)prefix, "y2", "y1", NULL);
    ++	t_string_list_filter(&list, "y2", "y1", NULL);
     +
     +	t_create_string_list_dup(&list, 0, "x1", "x2", NULL);
    -+	t_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);
    ++	t_string_list_filter(&list, NULL);
     +
     +	t_string_list_clear(&list, 0);
     +}
8:  55269e7fcb ! 8:  b608047d40 u-string-list: move "remove duplicates" test to "u-string-list.c"
    @@ Commit message
         Also we could simply remove "DISABLE_SIGN_COMPARE_WARNINGS" due to we
         have already deleted related code.
     
    +    Unfortunately, we cannot totally remove "test-string-list.c" due to that
    +    we would test the performance of sorting about string list by executing
    +    "test-tool string-list sort" in "p0071-sort.sh".
    +
         Signed-off-by: shejialuo <shejialuo@gmail.com>
     
      ## t/helper/test-string-list.c ##
-- 
2.50.0

[PATCH v3 1/8] string-list: fix sign compare warnings for loop iterator

[shejialuo]

There are a couple of "-Wsign-compare" warnings in "string-list.c". Fix
trivial ones that result from a mismatched loop iterator type.

There is a single warning left after these fixes. This warning needs
a bit more care and is thus handled in subsequent commits.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 string-list.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/string-list.c b/string-list.c
index bf061fec56..801ece0cba 100644
--- a/string-list.c
+++ b/string-list.c
@@ -116,9 +116,9 @@ struct string_list_item *string_list_lookup(struct string_list *list, const char
 void string_list_remove_duplicates(struct string_list *list, int free_util)
 {
 	if (list->nr > 1) {
-		int src, dst;
+		size_t dst = 1;
 		compare_strings_fn cmp = list->cmp ? list->cmp : strcmp;
-		for (src = dst = 1; src < list->nr; src++) {
+		for (size_t src = 1; src < list->nr; src++) {
 			if (!cmp(list->items[dst - 1].string, list->items[src].string)) {
 				if (list->strdup_strings)
 					free(list->items[src].string);
@@ -134,8 +134,8 @@ void string_list_remove_duplicates(struct string_list *list, int free_util)
 int for_each_string_list(struct string_list *list,
 			 string_list_each_func_t fn, void *cb_data)
 {
-	int i, ret = 0;
-	for (i = 0; i < list->nr; i++)
+	int ret = 0;
+	for (size_t i = 0; i < list->nr; i++)
 		if ((ret = fn(&list->items[i], cb_data)))
 			break;
 	return ret;
@@ -144,8 +144,8 @@ int for_each_string_list(struct string_list *list,
 void filter_string_list(struct string_list *list, int free_util,
 			string_list_each_func_t want, void *cb_data)
 {
-	int src, dst = 0;
-	for (src = 0; src < list->nr; src++) {
+	size_t dst = 0;
+	for (size_t src = 0; src < list->nr; src++) {
 		if (want(&list->items[src], cb_data)) {
 			list->items[dst++] = list->items[src];
 		} else {
@@ -171,13 +171,12 @@ void string_list_remove_empty_items(struct string_list *list, int free_util)
 void string_list_clear(struct string_list *list, int free_util)
 {
 	if (list->items) {
-		int i;
 		if (list->strdup_strings) {
-			for (i = 0; i < list->nr; i++)
+			for (size_t i = 0; i < list->nr; i++)
 				free(list->items[i].string);
 		}
 		if (free_util) {
-			for (i = 0; i < list->nr; i++)
+			for (size_t i = 0; i < list->nr; i++)
 				free(list->items[i].util);
 		}
 		free(list->items);
@@ -189,13 +188,12 @@ void string_list_clear(struct string_list *list, int free_util)
 void string_list_clear_func(struct string_list *list, string_list_clear_func_t clearfunc)
 {
 	if (list->items) {
-		int i;
 		if (clearfunc) {
-			for (i = 0; i < list->nr; i++)
+			for (size_t i = 0; i < list->nr; i++)
 				clearfunc(list->items[i].util, list->items[i].string);
 		}
 		if (list->strdup_strings) {
-			for (i = 0; i < list->nr; i++)
+			for (size_t i = 0; i < list->nr; i++)
 				free(list->items[i].string);
 		}
 		free(list->items);
-- 
2.50.0

[PATCH v3 2/8] string-list: remove unused "insert_at" parameter from add_entry

[shejialuo]

In "add_entry", we accept "insert_at" parameter which must be either -1
(auto) or between 0 and `list->nr` inclusive. Any other value is
invalid. When caller specify any invalid "insert_at" value, we won't
check the range and move the element, which would definitely cause the
trouble.

However, we only use "add_entry" in "string_list_insert" function and we
always pass the "-1" for "insert_at" parameter. So, we never use this
parameter to insert element in a user specified position.

And we should know why there is such code path in the first place. We
used to have another function "string_list_insert_at_index()", which
uses the extra "insert_at" parameter. And in f8c4ab611a (string_list:
remove string_list_insert_at_index() from its API, 2014-11-24), we
remove this function but we don't clean all the code path.

Let's simply delete this parameter as we'd better use "strmap" for such
functionality.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 string-list.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/string-list.c b/string-list.c
index 801ece0cba..8540c29bc9 100644
--- a/string-list.c
+++ b/string-list.c
@@ -41,10 +41,10 @@ static int get_entry_index(const struct string_list *list, const char *string,
 }
 
 /* returns -1-index if already exists */
-static int add_entry(int insert_at, struct string_list *list, const char *string)
+static int add_entry(struct string_list *list, const char *string)
 {
 	int exact_match = 0;
-	int index = insert_at != -1 ? insert_at : get_entry_index(list, string, &exact_match);
+	int index = get_entry_index(list, string, &exact_match);
 
 	if (exact_match)
 		return -1 - index;
@@ -63,7 +63,7 @@ static int add_entry(int insert_at, struct string_list *list, const char *string
 
 struct string_list_item *string_list_insert(struct string_list *list, const char *string)
 {
-	int index = add_entry(-1, list, string);
+	int index = add_entry(list, string);
 
 	if (index < 0)
 		index = -1 - index;
-- 
2.50.0

[PATCH v3 3/8] string-list: return index directly when inserting an existing element

[shejialuo]

When inserting an existing element, "add_entry" would convert "index"
value to "-1-index" to indicate the caller that this element is in the
list already. However, in "string_list_insert", we would simply convert
this to the original positive index without any further action.

In 8fd2cb4069 (Extract helper bits from c-merge-recursive work,
2006-07-25), we create "path-list.c" and then introduce above code path.

Let's directly return the index as we don't care about whether the
element is in the list by using "add_entry". In the future, if we want
to let "add_entry" tell the caller, we may add "int *exact_match"
parameter to "add_entry" instead of converting the index to negative to
indicate.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 string-list.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/string-list.c b/string-list.c
index 8540c29bc9..171cef5dbb 100644
--- a/string-list.c
+++ b/string-list.c
@@ -40,14 +40,13 @@ static int get_entry_index(const struct string_list *list, const char *string,
 	return right;
 }
 
-/* returns -1-index if already exists */
 static int add_entry(struct string_list *list, const char *string)
 {
 	int exact_match = 0;
 	int index = get_entry_index(list, string, &exact_match);
 
 	if (exact_match)
-		return -1 - index;
+		return index;
 
 	ALLOC_GROW(list->items, list->nr+1, list->alloc);
 	if (index < list->nr)
@@ -65,9 +64,6 @@ struct string_list_item *string_list_insert(struct string_list *list, const char
 {
 	int index = add_entry(list, string);
 
-	if (index < 0)
-		index = -1 - index;
-
 	return list->items + index;
 }
 
-- 
2.50.0

[PATCH v3 4/8] string-list: enable sign compare warnings check

[shejialuo]

In "add_entry", we call "get_entry_index" function to get the inserted
position. However, as the return type of "get_entry_index" function is
`int`, there is a sign compare warning when comparing the `index` with
the `list-nr` of unsigned type.

"get_entry_index" would always return unsigned index. However, the
current binary search algorithm initializes "left" to be "-1", which
necessitates the use of signed `int` return type.

The reason why we need to assign "left" to be "-1" is that in the
`while` loop, we increment "left" by 1 to determine whether the loop
should end. This design choice, while functional, forces us to use
signed arithmetic throughout the function.

To resolve this sign comparison issue, let's modify the binary search
algorithm with the following approach:

1. Initialize "left" to 0 instead of -1
2. Use `left < right` as the loop termination condition instead of
   `left + 1 < right`
3. When searching the right part, set `left = middle + 1` instead of
   `middle`

Then, we could delete "#define DISABLE_SIGN_COMPARE_WARNING" to enable
sign warnings check for "string-list".

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 string-list.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/string-list.c b/string-list.c
index 171cef5dbb..53faaa8420 100644
--- a/string-list.c
+++ b/string-list.c
@@ -1,5 +1,3 @@
-#define DISABLE_SIGN_COMPARE_WARNINGS
-
 #include "git-compat-util.h"
 #include "string-list.h"
 
@@ -17,19 +15,19 @@ void string_list_init_dup(struct string_list *list)
 
 /* if there is no exact match, point to the index where the entry could be
  * inserted */
-static int get_entry_index(const struct string_list *list, const char *string,
-		int *exact_match)
+static size_t get_entry_index(const struct string_list *list, const char *string,
+			      int *exact_match)
 {
-	int left = -1, right = list->nr;
+	size_t left = 0, right = list->nr;
 	compare_strings_fn cmp = list->cmp ? list->cmp : strcmp;
 
-	while (left + 1 < right) {
-		int middle = left + (right - left) / 2;
+	while (left < right) {
+		size_t middle = left + (right - left) / 2;
 		int compare = cmp(string, list->items[middle].string);
 		if (compare < 0)
 			right = middle;
 		else if (compare > 0)
-			left = middle;
+			left = middle + 1;
 		else {
 			*exact_match = 1;
 			return middle;
@@ -40,10 +38,10 @@ static int get_entry_index(const struct string_list *list, const char *string,
 	return right;
 }
 
-static int add_entry(struct string_list *list, const char *string)
+static size_t add_entry(struct string_list *list, const char *string)
 {
 	int exact_match = 0;
-	int index = get_entry_index(list, string, &exact_match);
+	size_t index = get_entry_index(list, string, &exact_match);
 
 	if (exact_match)
 		return index;
@@ -62,7 +60,7 @@ static int add_entry(struct string_list *list, const char *string)
 
 struct string_list_item *string_list_insert(struct string_list *list, const char *string)
 {
-	int index = add_entry(list, string);
+	size_t index = add_entry(list, string);
 
 	return list->items + index;
 }
-- 
2.50.0

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

[shejialuo]

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" function to do
this.

Then port the shell script tests to C program and remove unused
"test-tool" code and tests.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 Makefile                     |  1 +
 t/helper/test-string-list.c  | 14 ---------
 t/meson.build                |  1 +
 t/t0063-string-list.sh       | 53 ----------------------------------
 t/unit-tests/u-string-list.c | 55 ++++++++++++++++++++++++++++++++++++
 5 files changed, 57 insertions(+), 67 deletions(-)
 create mode 100644 t/unit-tests/u-string-list.c

diff --git a/Makefile b/Makefile
index 70d1543b6b..744f060e53 100644
--- a/Makefile
+++ b/Makefile
@@ -1367,6 +1367,7 @@ CLAR_TEST_SUITES += u-prio-queue
 CLAR_TEST_SUITES += u-reftable-tree
 CLAR_TEST_SUITES += u-strbuf
 CLAR_TEST_SUITES += u-strcmp-offset
+CLAR_TEST_SUITES += u-string-list
 CLAR_TEST_SUITES += u-strvec
 CLAR_TEST_SUITES += u-trailer
 CLAR_TEST_SUITES += u-urlmatch-normalization
diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c
index 6f10c5a435..17c18c30f6 100644
--- a/t/helper/test-string-list.c
+++ b/t/helper/test-string-list.c
@@ -46,20 +46,6 @@ static int prefix_cb(struct string_list_item *item, void *cb_data)
 
 int cmd__string_list(int argc, const char **argv)
 {
-	if (argc == 5 && !strcmp(argv[1], "split")) {
-		struct string_list list = STRING_LIST_INIT_DUP;
-		int i;
-		const char *s = argv[2];
-		int delim = *argv[3];
-		int maxsplit = atoi(argv[4]);
-
-		i = string_list_split(&list, s, delim, maxsplit);
-		printf("%d\n", i);
-		write_list(&list);
-		string_list_clear(&list, 0);
-		return 0;
-	}
-
 	if (argc == 5 && !strcmp(argv[1], "split_in_place")) {
 		struct string_list list = STRING_LIST_INIT_NODUP;
 		int i;
diff --git a/t/meson.build b/t/meson.build
index 50e89e764a..d3b3916580 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -11,6 +11,7 @@ clar_test_suites = [
   'unit-tests/u-reftable-tree.c',
   'unit-tests/u-strbuf.c',
   'unit-tests/u-strcmp-offset.c',
+  'unit-tests/u-string-list.c',
   'unit-tests/u-strvec.c',
   'unit-tests/u-trailer.c',
   'unit-tests/u-urlmatch-normalization.c',
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
index aac63ba506..6b20ffd206 100755
--- a/t/t0063-string-list.sh
+++ b/t/t0063-string-list.sh
@@ -7,16 +7,6 @@ test_description='Test string list functionality'
 
 . ./test-lib.sh
 
-test_split () {
-	cat >expected &&
-	test_expect_success "split $1 at $2, max $3" "
-		test-tool string-list split '$1' '$2' '$3' >actual &&
-		test_cmp expected actual &&
-		test-tool string-list split_in_place '$1' '$2' '$3' >actual &&
-		test_cmp expected actual
-	"
-}
-
 test_split_in_place() {
 	cat >expected &&
 	test_expect_success "split (in place) $1 at $2, max $3" "
@@ -25,49 +15,6 @@ test_split_in_place() {
 	"
 }
 
-test_split "foo:bar:baz" ":" "-1" <<EOF
-3
-[0]: "foo"
-[1]: "bar"
-[2]: "baz"
-EOF
-
-test_split "foo:bar:baz" ":" "0" <<EOF
-1
-[0]: "foo:bar:baz"
-EOF
-
-test_split "foo:bar:baz" ":" "1" <<EOF
-2
-[0]: "foo"
-[1]: "bar:baz"
-EOF
-
-test_split "foo:bar:baz" ":" "2" <<EOF
-3
-[0]: "foo"
-[1]: "bar"
-[2]: "baz"
-EOF
-
-test_split "foo:bar:" ":" "-1" <<EOF
-3
-[0]: "foo"
-[1]: "bar"
-[2]: ""
-EOF
-
-test_split "" ":" "-1" <<EOF
-1
-[0]: ""
-EOF
-
-test_split ":" ":" "-1" <<EOF
-2
-[0]: ""
-[1]: ""
-EOF
-
 test_split_in_place "foo:;:bar:;:baz:;:" ":;" "-1" <<EOF
 10
 [0]: "foo"
diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c
new file mode 100644
index 0000000000..881720ed6e
--- /dev/null
+++ b/t/unit-tests/u-string-list.c
@@ -0,0 +1,55 @@
+#include "unit-test.h"
+#include "string-list.h"
+
+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);
+
+	string_list_clear(list, free_util);
+	while ((arg = va_arg(ap, const char *)))
+		string_list_append(list, arg);
+}
+
+static void t_string_list_equal(struct string_list *list,
+				struct string_list *expected_strings)
+{
+	cl_assert_equal_i(list->nr, expected_strings->nr);
+	cl_assert(list->nr <= list->alloc);
+	for (size_t i = 0; i < expected_strings->nr; i++)
+		cl_assert_equal_s(list->items[i].string,
+				  expected_strings->items[i].string);
+}
+
+static void t_string_list_split(const char *data, int delim, int maxsplit, ...)
+{
+	struct string_list expected_strings = STRING_LIST_INIT_DUP;
+	struct string_list list = STRING_LIST_INIT_DUP;
+	va_list ap;
+	int len;
+
+	va_start(ap, maxsplit);
+	t_vcreate_string_list_dup(&expected_strings, 0, ap);
+	va_end(ap);
+
+	string_list_clear(&list, 0);
+	len = string_list_split(&list, data, delim, maxsplit);
+	cl_assert_equal_i(len, expected_strings.nr);
+	t_string_list_equal(&list, &expected_strings);
+
+	string_list_clear(&expected_strings, 0);
+	string_list_clear(&list, 0);
+}
+
+void test_string_list__split(void)
+{
+	t_string_list_split("foo:bar:baz", ':', -1, "foo", "bar", "baz", NULL);
+	t_string_list_split("foo:bar:baz", ':', 0, "foo:bar:baz", NULL);
+	t_string_list_split("foo:bar:baz", ':', 1, "foo", "bar:baz", NULL);
+	t_string_list_split("foo:bar:baz", ':', 2, "foo", "bar", "baz", NULL);
+	t_string_list_split("foo:bar:", ':', -1, "foo", "bar", "", NULL);
+	t_string_list_split("", ':', -1, "", NULL);
+	t_string_list_split(":", ':', -1, "", "", NULL);
+}
-- 
2.50.0

[PATCH v3 6/8] u-string-list: move "test_split_in_place" to "u-string-list.c"

[shejialuo]

We use "test-tool string-list split_in_place" to test the
"string_list_split_in_place" function. As we have introduced the unit
test, we'd better remove the logic from shell script to C program to
improve test speed and readability.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 t/helper/test-string-list.c  | 22 ----------------
 t/t0063-string-list.sh       | 51 ------------------------------------
 t/unit-tests/u-string-list.c | 37 ++++++++++++++++++++++++++
 3 files changed, 37 insertions(+), 73 deletions(-)

diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c
index 17c18c30f6..8a344347ad 100644
--- a/t/helper/test-string-list.c
+++ b/t/helper/test-string-list.c
@@ -18,13 +18,6 @@ static void parse_string_list(struct string_list *list, const char *arg)
 	(void)string_list_split(list, arg, ':', -1);
 }
 
-static void write_list(const struct string_list *list)
-{
-	int i;
-	for (i = 0; i < list->nr; i++)
-		printf("[%d]: \"%s\"\n", i, list->items[i].string);
-}
-
 static void write_list_compact(const struct string_list *list)
 {
 	int i;
@@ -46,21 +39,6 @@ static int prefix_cb(struct string_list_item *item, void *cb_data)
 
 int cmd__string_list(int argc, const char **argv)
 {
-	if (argc == 5 && !strcmp(argv[1], "split_in_place")) {
-		struct string_list list = STRING_LIST_INIT_NODUP;
-		int i;
-		char *s = xstrdup(argv[2]);
-		const char *delim = argv[3];
-		int maxsplit = atoi(argv[4]);
-
-		i = string_list_split_in_place(&list, s, delim, maxsplit);
-		printf("%d\n", i);
-		write_list(&list);
-		string_list_clear(&list, 0);
-		free(s);
-		return 0;
-	}
-
 	if (argc == 4 && !strcmp(argv[1], "filter")) {
 		/*
 		 * Retain only the items that have the specified prefix.
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
index 6b20ffd206..1a9cf8bfcf 100755
--- a/t/t0063-string-list.sh
+++ b/t/t0063-string-list.sh
@@ -7,57 +7,6 @@ test_description='Test string list functionality'
 
 . ./test-lib.sh
 
-test_split_in_place() {
-	cat >expected &&
-	test_expect_success "split (in place) $1 at $2, max $3" "
-		test-tool string-list split_in_place '$1' '$2' '$3' >actual &&
-		test_cmp expected actual
-	"
-}
-
-test_split_in_place "foo:;:bar:;:baz:;:" ":;" "-1" <<EOF
-10
-[0]: "foo"
-[1]: ""
-[2]: ""
-[3]: "bar"
-[4]: ""
-[5]: ""
-[6]: "baz"
-[7]: ""
-[8]: ""
-[9]: ""
-EOF
-
-test_split_in_place "foo:;:bar:;:baz" ":;" "0" <<EOF
-1
-[0]: "foo:;:bar:;:baz"
-EOF
-
-test_split_in_place "foo:;:bar:;:baz" ":;" "1" <<EOF
-2
-[0]: "foo"
-[1]: ";:bar:;:baz"
-EOF
-
-test_split_in_place "foo:;:bar:;:baz" ":;" "2" <<EOF
-3
-[0]: "foo"
-[1]: ""
-[2]: ":bar:;:baz"
-EOF
-
-test_split_in_place "foo:;:bar:;:" ":;" "-1" <<EOF
-7
-[0]: "foo"
-[1]: ""
-[2]: ""
-[3]: "bar"
-[4]: ""
-[5]: ""
-[6]: ""
-EOF
-
 test_expect_success "test filter_string_list" '
 	test "x-" = "x$(test-tool string-list filter - y)" &&
 	test "x-" = "x$(test-tool string-list filter no y)" &&
diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c
index 881720ed6e..d2761e1f2f 100644
--- a/t/unit-tests/u-string-list.c
+++ b/t/unit-tests/u-string-list.c
@@ -53,3 +53,40 @@ void test_string_list__split(void)
 	t_string_list_split("", ':', -1, "", NULL);
 	t_string_list_split(":", ':', -1, "", "", NULL);
 }
+
+static void t_string_list_split_in_place(const char *data, const char *delim,
+					 int maxsplit, ...)
+{
+	struct string_list expected_strings = STRING_LIST_INIT_DUP;
+	struct string_list list = STRING_LIST_INIT_NODUP;
+	char *string = xstrdup(data);
+	va_list ap;
+	int len;
+
+	va_start(ap, maxsplit);
+	t_vcreate_string_list_dup(&expected_strings, 0, ap);
+	va_end(ap);
+
+	string_list_clear(&list, 0);
+	len = string_list_split_in_place(&list, string, delim, maxsplit);
+	cl_assert_equal_i(len, expected_strings.nr);
+	t_string_list_equal(&list, &expected_strings);
+
+	free(string);
+	string_list_clear(&expected_strings, 0);
+	string_list_clear(&list, 0);
+}
+
+void test_string_list__split_in_place(void)
+{
+	t_string_list_split_in_place("foo:;:bar:;:baz:;:", ":;", -1,
+				     "foo", "", "", "bar", "", "", "baz", "", "", "", NULL);
+	t_string_list_split_in_place("foo:;:bar:;:baz", ":;", 0,
+				     "foo:;:bar:;:baz", NULL);
+	t_string_list_split_in_place("foo:;:bar:;:baz", ":;", 1,
+				     "foo", ";:bar:;:baz", NULL);
+	t_string_list_split_in_place("foo:;:bar:;:baz", ":;", 2,
+				     "foo", "", ":bar:;:baz", NULL);
+	t_string_list_split_in_place("foo:;:bar:;:", ":;", -1,
+				     "foo", "", "", "bar", "", "", "", NULL);
+}
-- 
2.50.0

[PATCH v3 7/8] u-string-list: move "filter string" test to "u-string-list.c"

[shejialuo]

We use "test-tool string-list filter" to test the "filter_string_list"
function. As we have introduced the unit test, we'd better remove the
logic from shell script to C program to improve test speed and
readability.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 t/helper/test-string-list.c  | 21 -----------
 t/t0063-string-list.sh       | 11 ------
 t/unit-tests/u-string-list.c | 73 ++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 32 deletions(-)

diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c
index 8a344347ad..262b28c599 100644
--- a/t/helper/test-string-list.c
+++ b/t/helper/test-string-list.c
@@ -31,29 +31,8 @@ static void write_list_compact(const struct string_list *list)
 	}
 }
 
-static int prefix_cb(struct string_list_item *item, void *cb_data)
-{
-	const char *prefix = (const char *)cb_data;
-	return starts_with(item->string, prefix);
-}
-
 int cmd__string_list(int argc, const char **argv)
 {
-	if (argc == 4 && !strcmp(argv[1], "filter")) {
-		/*
-		 * Retain only the items that have the specified prefix.
-		 * Arguments: list|- prefix
-		 */
-		struct string_list list = STRING_LIST_INIT_DUP;
-		const char *prefix = argv[3];
-
-		parse_string_list(&list, argv[2]);
-		filter_string_list(&list, 0, prefix_cb, (void *)prefix);
-		write_list_compact(&list);
-		string_list_clear(&list, 0);
-		return 0;
-	}
-
 	if (argc == 3 && !strcmp(argv[1], "remove_duplicates")) {
 		struct string_list list = STRING_LIST_INIT_DUP;
 
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
index 1a9cf8bfcf..31fd62bba8 100755
--- a/t/t0063-string-list.sh
+++ b/t/t0063-string-list.sh
@@ -7,17 +7,6 @@ test_description='Test string list functionality'
 
 . ./test-lib.sh
 
-test_expect_success "test filter_string_list" '
-	test "x-" = "x$(test-tool string-list filter - y)" &&
-	test "x-" = "x$(test-tool string-list filter no y)" &&
-	test yes = "$(test-tool string-list filter yes y)" &&
-	test yes = "$(test-tool string-list filter no:yes y)" &&
-	test yes = "$(test-tool string-list filter yes:no y)" &&
-	test y1:y2 = "$(test-tool string-list filter y1:y2 y)" &&
-	test y2:y1 = "$(test-tool string-list filter y2:y1 y)" &&
-	test "x-" = "x$(test-tool string-list filter x1:x2 y)"
-'
-
 test_expect_success "test remove_duplicates" '
 	test "x-" = "x$(test-tool string-list remove_duplicates -)" &&
 	test "x" = "x$(test-tool string-list remove_duplicates "")" &&
diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c
index d2761e1f2f..f061a3694b 100644
--- a/t/unit-tests/u-string-list.c
+++ b/t/unit-tests/u-string-list.c
@@ -13,6 +13,26 @@ static void t_vcreate_string_list_dup(struct string_list *list,
 		string_list_append(list, arg);
 }
 
+static void t_create_string_list_dup(struct string_list *list, int free_util, ...)
+{
+	va_list ap;
+
+	cl_assert(list->strdup_strings);
+
+	string_list_clear(list, free_util);
+	va_start(ap, free_util);
+	t_vcreate_string_list_dup(list, free_util, ap);
+	va_end(ap);
+}
+
+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);
+}
+
 static void t_string_list_equal(struct string_list *list,
 				struct string_list *expected_strings)
 {
@@ -90,3 +110,56 @@ void test_string_list__split_in_place(void)
 	t_string_list_split_in_place("foo:;:bar:;:", ":;", -1,
 				     "foo", "", "", "bar", "", "", "", NULL);
 }
+
+static int prefix_cb(struct string_list_item *item, void *cb_data)
+{
+	const char *prefix = (const char *)cb_data;
+	return starts_with(item->string, prefix);
+}
+
+static void t_string_list_filter(struct string_list *list, ...)
+{
+	struct string_list expected_strings = STRING_LIST_INIT_DUP;
+	const char *prefix = "y";
+	va_list ap;
+
+	va_start(ap, list);
+	t_vcreate_string_list_dup(&expected_strings, 0, ap);
+	va_end(ap);
+
+	filter_string_list(list, 0, prefix_cb, (void *)prefix);
+	t_string_list_equal(list, &expected_strings);
+
+	string_list_clear(&expected_strings, 0);
+}
+
+void test_string_list__filter(void)
+{
+	struct string_list list = STRING_LIST_INIT_DUP;
+
+	t_create_string_list_dup(&list, 0, NULL);
+	t_string_list_filter(&list, NULL);
+
+	t_create_string_list_dup(&list, 0, "no", NULL);
+	t_string_list_filter(&list, NULL);
+
+	t_create_string_list_dup(&list, 0, "yes", NULL);
+	t_string_list_filter(&list, "yes", NULL);
+
+	t_create_string_list_dup(&list, 0, "no", "yes", NULL);
+	t_string_list_filter(&list, "yes", NULL);
+
+	t_create_string_list_dup(&list, 0, "yes", "no", NULL);
+	t_string_list_filter(&list, "yes", NULL);
+
+	t_create_string_list_dup(&list, 0, "y1", "y2", NULL);
+	t_string_list_filter(&list, "y1", "y2", NULL);
+
+	t_create_string_list_dup(&list, 0, "y2", "y1", NULL);
+	t_string_list_filter(&list, "y2", "y1", NULL);
+
+	t_create_string_list_dup(&list, 0, "x1", "x2", NULL);
+	t_string_list_filter(&list, NULL);
+
+	t_string_list_clear(&list, 0);
+}
-- 
2.50.0

[PATCH v3 8/8] u-string-list: move "remove duplicates" test to "u-string-list.c"

[shejialuo]

We use "test-tool string-list remove_duplicates" to test the
"string_list_remove_duplicates" function. As we have introduced the unit
test, we'd better remove the logic from shell script to C program to
improve test speed and readability.

As all the tests in shell script are removed, let's just delete the
"t0063-string-list.sh" and update the "meson.build" file to align with
this change.

Also we could simply remove "DISABLE_SIGN_COMPARE_WARNINGS" due to we
have already deleted related code.

Unfortunately, we cannot totally remove "test-string-list.c" due to that
we would test the performance of sorting about string list by executing
"test-tool string-list sort" in "p0071-sort.sh".

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 t/helper/test-string-list.c  | 39 -----------------------
 t/meson.build                |  1 -
 t/t0063-string-list.sh       | 27 ----------------
 t/unit-tests/u-string-list.c | 62 ++++++++++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+), 67 deletions(-)
 delete mode 100755 t/t0063-string-list.sh

diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c
index 262b28c599..6be0cdb8e2 100644
--- a/t/helper/test-string-list.c
+++ b/t/helper/test-string-list.c
@@ -1,48 +1,9 @@
-#define DISABLE_SIGN_COMPARE_WARNINGS
-
 #include "test-tool.h"
 #include "strbuf.h"
 #include "string-list.h"
 
-/*
- * Parse an argument into a string list.  arg should either be a
- * ':'-separated list of strings, or "-" to indicate an empty string
- * list (as opposed to "", which indicates a string list containing a
- * single empty string).  list->strdup_strings must be set.
- */
-static void parse_string_list(struct string_list *list, const char *arg)
-{
-	if (!strcmp(arg, "-"))
-		return;
-
-	(void)string_list_split(list, arg, ':', -1);
-}
-
-static void write_list_compact(const struct string_list *list)
-{
-	int i;
-	if (!list->nr)
-		printf("-\n");
-	else {
-		printf("%s", list->items[0].string);
-		for (i = 1; i < list->nr; i++)
-			printf(":%s", list->items[i].string);
-		printf("\n");
-	}
-}
-
 int cmd__string_list(int argc, const char **argv)
 {
-	if (argc == 3 && !strcmp(argv[1], "remove_duplicates")) {
-		struct string_list list = STRING_LIST_INIT_DUP;
-
-		parse_string_list(&list, argv[2]);
-		string_list_remove_duplicates(&list, 0);
-		write_list_compact(&list);
-		string_list_clear(&list, 0);
-		return 0;
-	}
-
 	if (argc == 2 && !strcmp(argv[1], "sort")) {
 		struct string_list list = STRING_LIST_INIT_NODUP;
 		struct strbuf sb = STRBUF_INIT;
diff --git a/t/meson.build b/t/meson.build
index d3b3916580..276133a3d2 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -124,7 +124,6 @@ integration_tests = [
   't0060-path-utils.sh',
   't0061-run-command.sh',
   't0062-revision-walking.sh',
-  't0063-string-list.sh',
   't0066-dir-iterator.sh',
   't0067-parse_pathspec_file.sh',
   't0068-for-each-repo.sh',
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
deleted file mode 100755
index 31fd62bba8..0000000000
--- a/t/t0063-string-list.sh
+++ /dev/null
@@ -1,27 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2012 Michael Haggerty
-#
-
-test_description='Test string list functionality'
-
-. ./test-lib.sh
-
-test_expect_success "test remove_duplicates" '
-	test "x-" = "x$(test-tool string-list remove_duplicates -)" &&
-	test "x" = "x$(test-tool string-list remove_duplicates "")" &&
-	test a = "$(test-tool string-list remove_duplicates a)" &&
-	test a = "$(test-tool string-list remove_duplicates a:a)" &&
-	test a = "$(test-tool string-list remove_duplicates a:a:a:a:a)" &&
-	test a:b = "$(test-tool string-list remove_duplicates a:b)" &&
-	test a:b = "$(test-tool string-list remove_duplicates a:a:b)" &&
-	test a:b = "$(test-tool string-list remove_duplicates a:b:b)" &&
-	test a:b:c = "$(test-tool string-list remove_duplicates a:b:c)" &&
-	test a:b:c = "$(test-tool string-list remove_duplicates a:a:b:c)" &&
-	test a:b:c = "$(test-tool string-list remove_duplicates a:b:b:c)" &&
-	test a:b:c = "$(test-tool string-list remove_duplicates a:b:c:c)" &&
-	test a:b:c = "$(test-tool string-list remove_duplicates a:a:b:b:c:c)" &&
-	test a:b:c = "$(test-tool string-list remove_duplicates a:a:a:b:b:b:c:c:c)"
-'
-
-test_done
diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c
index f061a3694b..d4ba5f9fa5 100644
--- a/t/unit-tests/u-string-list.c
+++ b/t/unit-tests/u-string-list.c
@@ -163,3 +163,65 @@ void test_string_list__filter(void)
 
 	t_string_list_clear(&list, 0);
 }
+
+static void t_string_list_remove_duplicates(struct string_list *list, ...)
+{
+	struct string_list expected_strings = STRING_LIST_INIT_DUP;
+	va_list ap;
+
+	va_start(ap, list);
+	t_vcreate_string_list_dup(&expected_strings, 0, ap);
+	va_end(ap);
+
+	string_list_remove_duplicates(list, 0);
+	t_string_list_equal(list, &expected_strings);
+
+	string_list_clear(&expected_strings, 0);
+}
+
+void test_string_list__remove_duplicates(void)
+{
+	struct string_list list = STRING_LIST_INIT_DUP;
+
+	t_create_string_list_dup(&list, 0, NULL);
+	t_string_list_remove_duplicates(&list, NULL);
+
+	t_create_string_list_dup(&list, 0, "", NULL);
+	t_string_list_remove_duplicates(&list, "", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", NULL);
+	t_string_list_remove_duplicates(&list, "a", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "a", NULL);
+	t_string_list_remove_duplicates(&list, "a", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "a", "a", NULL);
+	t_string_list_remove_duplicates(&list, "a", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "a", "b", NULL);
+	t_string_list_remove_duplicates(&list, "a", "b", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "b", "b", NULL);
+	t_string_list_remove_duplicates(&list, "a", "b", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "b", "c", NULL);
+	t_string_list_remove_duplicates(&list, "a", "b", "c", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "a", "b", "c", NULL);
+	t_string_list_remove_duplicates(&list, "a", "b", "c", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "b", "b", "c", NULL);
+	t_string_list_remove_duplicates(&list, "a", "b", "c", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "b", "c", "c", NULL);
+	t_string_list_remove_duplicates(&list, "a", "b", "c", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "a", "b", "b", "c", "c", NULL);
+	t_string_list_remove_duplicates(&list, "a", "b", "c", NULL);
+
+	t_create_string_list_dup(&list, 0, "a", "a", "a", "b", "b", "b",
+				 "c", "c", "c", NULL);
+	t_string_list_remove_duplicates(&list, "a", "b", "c", NULL);
+
+	t_string_list_clear(&list, 0);
+}
-- 
2.50.0

Re: [PATCH v3 0/8] enhance "string_list" code and test

[Patrick Steinhardt]

On Sun, Jun 29, 2025 at 12:26:15PM +0800, shejialuo wrote:
> Changes since v2:
> 
> 1. [PATCH v3 1/8]: improve the commit message to explain that we would
>    handle a warning in the later commits.
> 2. [PATCH v3 2/8] and [PATCH v2 3/8]: improve the commit message to add
>    the history background.
> 3. [PATCH v3 4/8]: improve the commit message to show why the current
>    bianry search algorithm introduces the sign warning and how to change
>    it to fix the sign warning.
> 4. [PATCH v3 5/8] - [PATCH v3 8/8]: remove list into test helper instead
>    of test itself for reducing the shared state.
> 5. [PATCH v3 8/8]: improve the commit message to say why we can't delete
>    "test-tool string_list" totally.

Thanks, this version looks good to me!

Patrick

Re: [PATCH v3 0/8] enhance "string_list" code and test

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Sun, Jun 29, 2025 at 12:26:15PM +0800, shejialuo wrote:
>> Changes since v2:
>> 
>> 1. [PATCH v3 1/8]: improve the commit message to explain that we would
>>    handle a warning in the later commits.
>> 2. [PATCH v3 2/8] and [PATCH v2 3/8]: improve the commit message to add
>>    the history background.
>> 3. [PATCH v3 4/8]: improve the commit message to show why the current
>>    bianry search algorithm introduces the sign warning and how to change
>>    it to fix the sign warning.
>> 4. [PATCH v3 5/8] - [PATCH v3 8/8]: remove list into test helper instead
>>    of test itself for reducing the shared state.
>> 5. [PATCH v3 8/8]: improve the commit message to say why we can't delete
>>    "test-tool string_list" totally.
>
> Thanks, this version looks good to me!
>
> Patrick

Thanks. Will queue.