enhance "string_list" code and test

PATCH Superseded shejialuo 4 participants 52 messages Apr 22, 2025 – Jul 7, 2025

Subsystems (2)

Kbuild Arm/Amlogic Meson SoC support

lore

Share & tools

lore

shejialuo

author

2025-04-22 14:53 UTC #0 lore raw

cover enhance "string_list" code and test

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

Signature · 1 line

2.49.0

shejialuo

author

2025-04-22 14:54 UTC #1 lore raw

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

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 <[email protected]>

---
 string-list.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

string-list.c +13 ~~-17~~

index bf061fec56..a967421b60 100644
@@ -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);

Signature · 1 line

2.49.0

Junio C Hamano

2025-04-22 21:02 UTC #2 lore raw

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

Quoted 4 lines · shejialuo

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?

Quoted 3 lines

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

Quoted 4 lines

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.

Quoted 6 lines

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.

shejialuo

author

2025-04-24 12:27 UTC #3 lore raw

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

Quoted 7 lines · Junio C Hamano

Quoted 4 lines · shejialuo
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.

Quoted 6 lines

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.

Quoted 10 lines

Quoted 3 lines
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.

Quoted 6 lines

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.

Quoted 9 lines

Quoted 4 lines
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.

Quoted 12 lines

Quoted 6 lines
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.

Quoted 8 lines

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.

Quoted 10 lines

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.

Quoted 1 line

Thanks.

Thanks for the review.

shejialuo

author

2025-04-22 14:54 UTC #4 lore raw

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

In reply to 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 <[email protected]>

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

Makefile +1 -0

index 13f9062a05..58df1f1150 100644
@@ -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

t/helper/test-string-list.c +0 ~~-14~~

index 6f10c5a435..17c18c30f6 100644
@@ -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;

t/meson.build +1 -0

index bfb744e886..424e7e445f 100644
@@ -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',

t/t0063-string-list.sh +0 ~~-53~~

65 lines not on this page · raw message

t/unit-tests/u-string-list.c new file +86 -0

86 lines not on this page · raw message

Signature · 1 line

2.49.0

Junio C Hamano

2025-04-22 21:27 UTC #5 lore raw

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

Quoted 14 lines · 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.

Quoted 5 lines

Then port the shell script tests to C program and remove unused "test-tool" code and tests.
Signed-off-by shejialuo <[email protected]>
---

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.

Quoted 9 lines

t/unit-tests/u-string-list.c new file +3 -0

new file mode 100644
index 0000000000..0c148684ea
@@ -0,0 +1,86 @@
+#include "unit-test.h"
+#include "string-list.h"
+

t/unit-tests/u-string-list.c line 13

Quoted 10 lines

t/unit-tests/u-string-list.c new file +10 -0

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

t/unit-tests/u-string-list.c line 21

Quoted 7 lines

t/unit-tests/u-string-list.c new file +7 -0

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

t/unit-tests/u-string-list.c line 30

Quoted 8 lines

t/unit-tests/u-string-list.c new file +8 -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);

... this place.

t/unit-tests/u-string-list.c line 33

Quoted 3 lines

t/unit-tests/u-string-list.c new file +3 -0
+	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.

t/unit-tests/u-string-list.c line 86

Quoted 11 lines

t/unit-tests/u-string-list.c new file +11 -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_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.

shejialuo

author

2025-04-24 12:50 UTC #6 lore raw

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

Quoted 41 lines · Junio C Hamano

Quoted 14 lines · 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.
Quoted 5 lines
Then port the shell script tests to C program and remove unused "test-tool" code and tests.
Signed-off-by shejialuo <[email protected]>
---
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.
Quoted 15 lines
t/unit-tests/u-string-list.c new file +9 -0
new file mode 100644
index 0000000000..0c148684ea
@@ -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);

Quoted 9 lines

t/unit-tests/u-string-list.c line 13
Quoted 4 lines
t/unit-tests/u-string-list.c new file +4 -0
+	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.

Quoted 5 lines

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.

Quoted 5 lines

t/unit-tests/u-string-list.c new file +5 -0

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

Quoted 9 lines

t/unit-tests/u-string-list.c line 20
Quoted 2 lines
t/unit-tests/u-string-list.c new file +2 -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.

Quoted 18 lines

t/unit-tests/u-string-list.c line 30
Quoted 8 lines
t/unit-tests/u-string-list.c new file +8 -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);
... this place.
t/unit-tests/u-string-list.c line 33
Quoted 3 lines
t/unit-tests/u-string-list.c new file +3 -0
+	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

Quoted 8 lines

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

Quoted 18 lines

t/unit-tests/u-string-list.c line 86
Quoted 11 lines
t/unit-tests/u-string-list.c new file +11 -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_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.

Quoted 1 line

Thanks.

Thanks,
Jialuo

Quoted 1 line

Patrick Steinhardt

2025-04-23 11:53 UTC #7 lore raw

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

In reply to shejialuo

Quoted 15 lines · shejialuo

t/unit-tests/u-string-list.c new file +9 -0

new file mode 100644
index 0000000000..0c148684ea
@@ -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/unit-tests/u-string-list.c line 19

Quoted 10 lines

t/unit-tests/u-string-list.c new file +10 -0

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

shejialuo

author

2025-04-24 12:53 UTC #8 lore raw

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

Quoted 22 lines · Patrick Steinhardt

Quoted 21 lines · shejialuo

t/unit-tests/u-string-list.c new file +15 -0

new file mode 100644
index 0000000000..0c148684ea
@@ -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)
+{

Quoted 9 lines

t/unit-tests/u-string-list.c line 65
Quoted 4 lines
t/unit-tests/u-string-list.c new file +4 -0
+	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.

Quoted 1 line

Patrick

shejialuo

author

2025-04-22 14:55 UTC #9 lore raw

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

In reply to 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 <[email protected]>

---
 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(-)

t/helper/test-string-list.c +0 ~~-22~~

index 17c18c30f6..8a344347ad 100644
@@ -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.

t/t0063-string-list.sh +0 ~~-51~~

57 lines not on this page · raw message

t/unit-tests/u-string-list.c +39 -0

index 0c148684ea..44ec8de3d0 100644
@@ -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);
+}

Signature · 1 line

2.49.0

Patrick Steinhardt

2025-04-23 11:53 UTC #10 lore raw

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

Quoted 5 lines · shejialuo

t/unit-tests/u-string-list.c +0 -0
index 0c148684ea..44ec8de3d0 100644

t/unit-tests/u-string-list.c void test_string_list__split(void) · line 92

Quoted 9 lines

t/unit-tests/u-string-list.c +6 -0

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

Quoted 7 lines

t/unit-tests/u-string-list.c +7 -0

+	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/unit-tests/u-string-list.c void test_string_list__split(void) · line 109

Quoted 10 lines

t/unit-tests/u-string-list.c +10 -0

+	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

shejialuo

author

2025-04-22 14:55 UTC #11 lore raw

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

In reply to 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 <[email protected]>

---
 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(-)

t/helper/test-string-list.c +0 ~~-21~~

index 8a344347ad..262b28c599 100644
@@ -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;

t/t0063-string-list.sh +0 ~~-11~~

index 1a9cf8bfcf..31fd62bba8 100755
@@ -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 "")" &&

t/unit-tests/u-string-list.c +51 -0

54 lines not on this page · raw message

Signature · 1 line

2.49.0

shejialuo

author

2025-04-22 14:55 UTC #12 lore raw

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

In reply to 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 <[email protected]>

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

t/helper/test-string-list.c +0 ~~-39~~

index 262b28c599..6be0cdb8e2 100644
@@ -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;

t/meson.build +0 -1

index 424e7e445f..25af09a8d4 100644
@@ -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',

t/t0063-string-list.sh deleted +0 ~~-27~~

deleted file mode 100755
index 31fd62bba8..0000000000
@@ -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

t/unit-tests/u-string-list.c +62 -0

65 lines not on this page · raw message

Signature · 1 line

2.49.0

Patrick Steinhardt

2025-04-23 11:53 UTC #13 lore raw

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

Quoted 43 lines · shejialuo

t/helper/test-string-list.c +0 ~~-32~~

index 262b28c599..6be0cdb8e2 100644
@@ -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;
-

t/helper/test-string-list.c line 9

Quoted 10 lines

t/helper/test-string-list.c +0 -7

-		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

shejialuo

author

2025-04-24 12:56 UTC #14 lore raw

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

Quoted 51 lines · Patrick Steinhardt

Quoted 50 lines · shejialuo

t/helper/test-string-list.c +0 ~~-39~~

index 262b28c599..6be0cdb8e2 100644
@@ -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;
-	}
-

Quoted 9 lines

t/helper/test-string-list.c line 9
Quoted 3 lines
t/helper/test-string-list.c +0 -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:

Quoted 6 lines

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

Quoted 1 line

Patrick

Thanks,
Jialuo

shejialuo

author

2025-05-18 15:55 UTC #15 lore raw

v2 cover enhance "string_list" code and test

In reply to 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

Signature · 1 line

2.49.0

shejialuo

author

2025-05-18 15:57 UTC #16 lore raw

PATCH v2 1/8 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.

Signed-off-by shejialuo <[email protected]>

---
 string-list.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

string-list.c +10 ~~-12~~

index bf061fec56..801ece0cba 100644
@@ -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);

Signature · 1 line

2.49.0

Patrick Steinhardt

2025-05-19 07:17 UTC #17 lore raw

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

Quoted 4 lines · 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.

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.

Quoted 4 lines

Signed-off-by shejialuo <[email protected]>
---
 string-list.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

All of these look obviously good to me, thanks!

Patrick

shejialuo

author

2025-05-26 13:50 UTC #18 lore raw

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

Quoted 11 lines · Patrick Steinhardt

Quoted 4 lines · 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.
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.:

Quoted 6 lines

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

shejialuo

author

2025-05-18 15:57 UTC #19 lore raw

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

In reply to 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 <[email protected]>

---
 string-list.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

string-list.c +3 -3

index 801ece0cba..8540c29bc9 100644
@@ -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;

Signature · 1 line

2.49.0

Patrick Steinhardt

2025-05-19 07:17 UTC #20 lore raw

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

Quoted 5 lines · 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.

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

Quoted 5 lines

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

shejialuo

author

2025-05-26 13:52 UTC #21 lore raw

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

Quoted 10 lines · Patrick Steinhardt

Quoted 5 lines · 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.
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

Jeff King

2025-05-19 07:51 UTC #22 lore raw

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

In reply to shejialuo

Quoted 11 lines · 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.

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

shejialuo

author

2025-05-26 14:01 UTC #23 lore raw

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

Quoted 14 lines · Jeff King

Quoted 11 lines · 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.

Quoted 4 lines

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.

Quoted 11 lines

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

Quoted 5 lines

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

Patrick Steinhardt

2025-05-26 14:21 UTC #24 lore raw

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

Quoted 18 lines · shejialuo

Quoted 11 lines · Jeff King
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).
Quoted 5 lines
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).

Quoted 8 lines

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

shejialuo

author

2025-05-18 15:57 UTC #25 lore raw

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

In reply to 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 <[email protected]>

---
 string-list.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

string-list.c +1 -5

index 8540c29bc9..171cef5dbb 100644
@@ -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;
 }

Signature · 1 line

2.49.0

Patrick Steinhardt

2025-05-19 07:18 UTC #26 lore raw

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

Quoted 28 lines · 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 <[email protected]>
---
 string-list.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
string-list.c +0 -1
index 8540c29bc9..171cef5dbb 100644
@@ -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)

string-list.c static int get_entry_index(const struct string_list *list, const char *string, · line 52

Quoted 10 lines

string-list.c +1 -1

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

string-list.c struct string_list_item *string_list_insert(struct string_list *list, const char · line 68

Quoted 9 lines

string-list.c +0 -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

shejialuo

author

2025-05-26 14:12 UTC #27 lore raw

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

Quoted 47 lines · Patrick Steinhardt

Quoted 28 lines · 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 <[email protected]>
---
 string-list.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
string-list.c +0 -1
index 8540c29bc9..171cef5dbb 100644
@@ -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)
string-list.c static int get_entry_index(const struct string_list *list, const char *string, · line 52
Quoted 10 lines
string-list.c +1 -1
 {
 	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.
Quoted 4 lines
string-list.c +0 -0
@@ -65,9 +64,6 @@ struct string_list_item *string_list_insert(struct string_list *list, const char
 {
 	int index = add_entry(list, string);
 

Quoted 10 lines

string-list.c struct string_list_item *string_list_insert(struct string_list *list, const char · line 68
Quoted 5 lines
string-list.c +0 -3
-	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.

Quoted 4 lines

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

Jeff King

2025-05-19 07:58 UTC #28 lore raw

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

In reply to shejialuo

Quoted 4 lines · 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.

Quoted 8 lines

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

shejialuo

author

2025-05-26 14:20 UTC #29 lore raw

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

Quoted 19 lines · Jeff King

Quoted 4 lines · 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.
Quoted 8 lines
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.

Quoted 8 lines

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.

Quoted 2 lines

-Peff

Thanks,
Jialuo

shejialuo

author

2025-05-18 15:57 UTC #30 lore raw

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

In reply to 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 <[email protected]>

---
 string-list.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

string-list.c +9 ~~-11~~

index 171cef5dbb..53faaa8420 100644
@@ -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;
 }

Signature · 1 line

2.49.0

Patrick Steinhardt

2025-05-19 07:18 UTC #31 lore raw

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

Quoted 8 lines · 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.

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

Patrick

shejialuo

author

2025-05-26 14:27 UTC #32 lore raw

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

Quoted 6 lines · Patrick Steinhardt

Quoted 5 lines · 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.

Quoted 7 lines

Quoted 3 lines
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.

Quoted 1 line

Patrick

shejialuo

author

2025-05-18 15:57 UTC #33 lore raw

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

In reply to shejialuo

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 <[email protected]>

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

Makefile +1 -0

index de73c6ddcd..cdffa13aba 100644
@@ -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

t/helper/test-string-list.c +0 ~~-14~~

index 6f10c5a435..17c18c30f6 100644
@@ -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;

t/meson.build +1 -0

index fcfc1c2c2b..a3dbe572d8 100644
@@ -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',

t/t0063-string-list.sh +0 ~~-53~~

65 lines not on this page · raw message

t/unit-tests/u-string-list.c new file +66 -0

66 lines not on this page · raw message

Signature · 1 line

2.49.0

Patrick Steinhardt

2025-05-19 07:18 UTC #34 lore raw

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

Quoted 51 lines · shejialuo

t/unit-tests/u-string-list.c new file +45 -0

new file mode 100644
index 0000000000..c304934de2
@@ -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);

t/unit-tests/u-string-list.c line 55

Quoted 10 lines

t/unit-tests/u-string-list.c new file +10 -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/unit-tests/u-string-list.c line 65

Quoted 9 lines

t/unit-tests/u-string-list.c new file +9 -0

+	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

shejialuo

author

2025-05-18 15:57 UTC #35 lore raw

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

In reply to shejialuo

Signed-off-by shejialuo <[email protected]>

---
 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(-)

t/helper/test-string-list.c +0 ~~-22~~

index 17c18c30f6..8a344347ad 100644
@@ -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.

t/t0063-string-list.sh +0 ~~-51~~

57 lines not on this page · raw message

t/unit-tests/u-string-list.c +39 -0

index c304934de2..e4b8e38fb8 100644
@@ -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);
+}

Signature · 1 line

2.49.0

shejialuo

author

2025-05-18 15:58 UTC #36 lore raw

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

In reply to shejialuo

Signed-off-by shejialuo <[email protected]>

---
 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(-)

t/helper/test-string-list.c +0 ~~-21~~

index 8a344347ad..262b28c599 100644
@@ -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;

t/t0063-string-list.sh +0 ~~-11~~

index 1a9cf8bfcf..31fd62bba8 100755
@@ -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 "")" &&

t/unit-tests/u-string-list.c +66 -0

75 lines not on this page · raw message

Signature · 1 line

2.49.0

Patrick Steinhardt

2025-05-19 07:18 UTC #37 lore raw

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

Quoted 27 lines · shejialuo

t/unit-tests/u-string-list.c +19 -0

index e4b8e38fb8..be2bb5f103 100644
@@ -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);

t/unit-tests/u-string-list.c void test_string_list__split_in_place(void) · line 146

Quoted 10 lines

t/unit-tests/u-string-list.c +10 -0

+
+	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/unit-tests/u-string-list.c void test_string_list__split_in_place(void) · line 147

Quoted 1 line

t/unit-tests/u-string-list.c +1 -0
+	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

shejialuo

author

2025-05-26 14:33 UTC #38 lore raw

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

Quoted 35 lines · Patrick Steinhardt

Quoted 34 lines · shejialuo

t/unit-tests/u-string-list.c +26 -0

index e4b8e38fb8..be2bb5f103 100644
@@ -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;

Quoted 9 lines

t/unit-tests/u-string-list.c void test_string_list__split_in_place(void) · line 146
Quoted 3 lines
t/unit-tests/u-string-list.c +3 -0
+	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.

Quoted 6 lines

t/unit-tests/u-string-list.c void test_string_list__split_in_place(void) · line 147
Quoted 1 line
t/unit-tests/u-string-list.c +1 -0
+	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.

Quoted 2 lines

The void * casts are also all unnecessary.

Right, I will improve this in the next version.

Quoted 1 line

Patrick

shejialuo

author

2025-05-18 15:58 UTC #39 lore raw

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

In reply to shejialuo

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 <[email protected]>

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

t/helper/test-string-list.c +0 ~~-39~~

index 262b28c599..6be0cdb8e2 100644
@@ -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;

t/meson.build +0 -1

index a3dbe572d8..c7efcc6db9 100644
@@ -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',

t/t0063-string-list.sh deleted +0 ~~-27~~

deleted file mode 100755
index 31fd62bba8..0000000000
@@ -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

t/unit-tests/u-string-list.c +62 -0

65 lines not on this page · raw message

Signature · 1 line

2.49.0

Patrick Steinhardt

2025-05-19 07:18 UTC #40 lore raw

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

Quoted 11 lines · 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.

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

Patrick

shejialuo

author

2025-06-29 04:26 UTC #41 lore raw

v3 cover enhance "string_list" code and test

In reply to 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

 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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

## t/helper/test-string-list.c ##

Signature · 1 line

2.50.0

shejialuo

author

2025-06-29 04:27 UTC #42 lore raw

PATCH v3 1/8 string-list: fix sign compare warnings for 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 <[email protected]>

---
 string-list.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

string-list.c +10 ~~-12~~

index bf061fec56..801ece0cba 100644
@@ -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);

Signature · 1 line

2.50.0

shejialuo

author

2025-06-29 04:27 UTC #43 lore raw

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

In reply to shejialuo

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 <[email protected]>

---
 string-list.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

string-list.c +3 -3

index 801ece0cba..8540c29bc9 100644
@@ -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;

Signature · 1 line

2.50.0

shejialuo

author

2025-06-29 04:27 UTC #44 lore raw

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

In reply to 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 <[email protected]>

---
 string-list.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

string-list.c +1 -5

index 8540c29bc9..171cef5dbb 100644
@@ -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;
 }

Signature · 1 line

2.50.0

shejialuo

author

2025-06-29 04:27 UTC #45 lore raw

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

In reply to 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 <[email protected]>

---
 string-list.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

string-list.c +9 ~~-11~~

index 171cef5dbb..53faaa8420 100644
@@ -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;
 }

Signature · 1 line

2.50.0

shejialuo

author

2025-06-29 04:28 UTC #46 lore raw

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

In reply to shejialuo

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 <[email protected]>

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

Makefile +1 -0

index 70d1543b6b..744f060e53 100644
@@ -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

t/helper/test-string-list.c +0 ~~-14~~

index 6f10c5a435..17c18c30f6 100644
@@ -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;

t/meson.build +1 -0

index 50e89e764a..d3b3916580 100644
@@ -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',

t/t0063-string-list.sh +0 ~~-53~~

65 lines not on this page · raw message

t/unit-tests/u-string-list.c new file +55 -0

55 lines not on this page · raw message

Signature · 1 line

2.50.0

shejialuo

author

2025-06-29 04:28 UTC #47 lore raw

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

In reply to shejialuo

Signed-off-by shejialuo <[email protected]>

---
 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(-)

t/helper/test-string-list.c +0 ~~-22~~

index 17c18c30f6..8a344347ad 100644
@@ -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.

t/t0063-string-list.sh +0 ~~-51~~

57 lines not on this page · raw message

t/unit-tests/u-string-list.c +37 -0

index 881720ed6e..d2761e1f2f 100644
@@ -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);
+}

Signature · 1 line

2.50.0

shejialuo

author

2025-06-29 04:28 UTC #48 lore raw

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

In reply to shejialuo

Signed-off-by shejialuo <[email protected]>

---
 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(-)

t/helper/test-string-list.c +0 ~~-21~~

index 8a344347ad..262b28c599 100644
@@ -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;

t/t0063-string-list.sh +0 ~~-11~~

index 1a9cf8bfcf..31fd62bba8 100755
@@ -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 "")" &&

t/unit-tests/u-string-list.c +73 -0

82 lines not on this page · raw message

Signature · 1 line

2.50.0

shejialuo

author

2025-06-29 04:28 UTC #49 lore raw

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

In reply to shejialuo

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 <[email protected]>

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

t/helper/test-string-list.c +0 ~~-39~~

index 262b28c599..6be0cdb8e2 100644
@@ -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;

t/meson.build +0 -1

index d3b3916580..276133a3d2 100644
@@ -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',

t/t0063-string-list.sh deleted +0 ~~-27~~

deleted file mode 100755
index 31fd62bba8..0000000000
@@ -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

t/unit-tests/u-string-list.c +62 -0

65 lines not on this page · raw message

Signature · 1 line

2.50.0

Patrick Steinhardt

2025-07-04 05:24 UTC #50 lore raw

In reply to shejialuo

Quoted 13 lines · shejialuo

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

Junio C Hamano

2025-07-07 15:10 UTC #51 lore raw

Quoted 15 lines · Patrick Steinhardt

Quoted 13 lines · shejialuo
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.

Quoted 3 lines

Thanks, this version looks good to me!
Patrick

Thanks. Will queue.