{"thread":{"id":"63329","subject":"[PATCH 0/5] enhance \"string_list\" code and test","startedAt":"2025-04-22T14:53:10Z","lastAt":"2025-07-07T15:10:10Z","messageCount":52,"participants":["shejialuo","Junio C Hamano","Patrick Steinhardt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"516498","messageId":"aAetW0dan8S3Fljq@ArchLinux","threadId":"63329","inReplyTo":null,"subject":"[PATCH 0/5] enhance \"string_list\" code and test","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-04-22T14:53:15Z","receivedAt":"2025-04-22T14:53:10Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"Hi all:\n\nDuring I study and learn the Git source code, I have found something\nwhich could be improved for \"string_list\".\n\nAnd this patch mainly enhances the \"string_list\" code and test.\n\n 1. For code, I mainly fix sign compare warnings.\n 2. For test, I move the shell script to clar based unit test.\n\nThanks,\nJialuo\n\nshejialuo (5):\n string-list: fix sign compare warnings\n u-string-list: move \"test_split\" into \"u-string-list.c\"\n u-string-list: move \"test_split_in_place\" to \"u-string-list.c\"\n u-string-list: move \"filter string\" test to \"u-string-list.c\"\n u-string-list: move \"remove duplicates\" test to \"u-string-list.c\"\n\n Makefile | 1 +\n string-list.c | 30 ++---\n t/helper/test-string-list.c | 96 --------------\n t/meson.build | 2 +-\n t/t0063-string-list.sh | 142 ---------------------\n t/unit-tests/u-string-list.c | 238 +++++++++++++++++++++++++++++++++++\n 6 files changed, 253 insertions(+), 256 deletions(-)\n delete mode 100755 t/t0063-string-list.sh\n create mode 100644 t/unit-tests/u-string-list.c\n\n-- \n2.49.0\n\n"},{"id":"516499","messageId":"aAett8cJuDJ_FSdw@ArchLinux","threadId":"63329","inReplyTo":"aAetW0dan8S3Fljq@ArchLinux","subject":"[PATCH 1/5] string-list: fix sign compare warnings","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-04-22T14:54:47Z","receivedAt":"2025-04-22T14:54:41Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"In \"string-list.c\", there are six warnings which are emitted by\n\"Wsign-compare\". And five warnings are caused by the loop iterator type\nmismatch, which could be simply fixed by changing the `int` type to\n`size_t` type.\n\nHowever, for \"string-list.c::add_entry\" function, we compare the `index`\nof the `int` type with the `list->nr` of unsigned type. It seems that\nwe could just simply convert the type of `index` from `int` to\n`size_t`. But actually this is a correct behavior.\n\nWe would set the `index` value by checking whether `insert_at` is -1.\nIf not, we would set `index` to be `insert_at`, otherwise we would use\n\"get_entry_index` to find the inserted position.\n\nWhat if the caller passes a negative value except \"-1\", the compiler\nwould convert the `index` to be a positive value which would make the\n`if` statement be false to avoid moving array. However, we would\ndefinitely encounter trouble when setting the inserted item.\n\nAnd we only call \"add_entry\" in \"string_list_insert\" function, and we\nsimply pass \"-1\" for \"insert_at\" parameter. So, we never use this\nparameter to insert element in a user specified position. Let's delete\nthis parameter. If there is any requirement later, we may use a better\nway to do this. And then we could safely convert the index to be\n`size_t` when comparing.\n\nSigned-off-by: shejialuo \n---\n string-list.c | 30 +++++++++++++-----------------\n 1 file changed, 13 insertions(+), 17 deletions(-)\n\ndiff --git a/string-list.c b/string-list.c\nindex bf061fec56..a967421b60 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -1,5 +1,3 @@\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n-\n #include \"git-compat-util.h\"\n #include \"string-list.h\"\n \n@@ -41,16 +39,16 @@ static int get_entry_index(const struct string_list *list, const char *string,\n }\n \n /* returns -1-index if already exists */\n-static int add_entry(int insert_at, struct string_list *list, const char *string)\n+static int add_entry(struct string_list *list, const char *string)\n {\n \tint exact_match = 0;\n-\tint index = insert_at != -1 ? insert_at : get_entry_index(list, string, &exact_match);\n+\tint index = get_entry_index(list, string, &exact_match);\n \n \tif (exact_match)\n \t\treturn -1 - index;\n \n \tALLOC_GROW(list->items, list->nr+1, list->alloc);\n-\tif (index < list->nr)\n+\tif ((size_t)index < list->nr)\n \t\tMOVE_ARRAY(list->items + index + 1, list->items + index,\n \t\t\t list->nr - index);\n \tlist->items[index].string = list->strdup_strings ?\n@@ -63,7 +61,7 @@ static int add_entry(int insert_at, struct string_list *list, const char *string\n \n struct string_list_item *string_list_insert(struct string_list *list, const char *string)\n {\n-\tint index = add_entry(-1, list, string);\n+\tint index = add_entry(list, string);\n \n \tif (index < 0)\n \t\tindex = -1 - index;\n@@ -116,7 +114,7 @@ struct string_list_item *string_list_lookup(struct string_list *list, const char\n void string_list_remove_duplicates(struct string_list *list, int free_util)\n {\n \tif (list->nr > 1) {\n-\t\tint src, dst;\n+\t\tsize_t src, dst;\n \t\tcompare_strings_fn cmp = list->cmp ? list->cmp : strcmp;\n \t\tfor (src = dst = 1; src < list->nr; src++) {\n \t\t\tif (!cmp(list->items[dst - 1].string, list->items[src].string)) {\n@@ -134,8 +132,8 @@ void string_list_remove_duplicates(struct string_list *list, int free_util)\n int for_each_string_list(struct string_list *list,\n \t\t\t string_list_each_func_t fn, void *cb_data)\n {\n-\tint i, ret = 0;\n-\tfor (i = 0; i < list->nr; i++)\n+\tint ret = 0;\n+\tfor (size_t i = 0; i < list->nr; i++)\n \t\tif ((ret = fn(&list->items[i], cb_data)))\n \t\t\tbreak;\n \treturn ret;\n@@ -144,8 +142,8 @@ int for_each_string_list(struct string_list *list,\n void filter_string_list(struct string_list *list, int free_util,\n \t\t\tstring_list_each_func_t want, void *cb_data)\n {\n-\tint src, dst = 0;\n-\tfor (src = 0; src < list->nr; src++) {\n+\tsize_t dst = 0;\n+\tfor (size_t src = 0; src < list->nr; src++) {\n \t\tif (want(&list->items[src], cb_data)) {\n \t\t\tlist->items[dst++] = list->items[src];\n \t\t} else {\n@@ -171,13 +169,12 @@ void string_list_remove_empty_items(struct string_list *list, int free_util)\n void string_list_clear(struct string_list *list, int free_util)\n {\n \tif (list->items) {\n-\t\tint i;\n \t\tif (list->strdup_strings) {\n-\t\t\tfor (i = 0; i < list->nr; i++)\n+\t\t\tfor (size_t i = 0; i < list->nr; i++)\n \t\t\t\tfree(list->items[i].string);\n \t\t}\n \t\tif (free_util) {\n-\t\t\tfor (i = 0; i < list->nr; i++)\n+\t\t\tfor (size_t i = 0; i < list->nr; i++)\n \t\t\t\tfree(list->items[i].util);\n \t\t}\n \t\tfree(list->items);\n@@ -189,13 +186,12 @@ void string_list_clear(struct string_list *list, int free_util)\n void string_list_clear_func(struct string_list *list, string_list_clear_func_t clearfunc)\n {\n \tif (list->items) {\n-\t\tint i;\n \t\tif (clearfunc) {\n-\t\t\tfor (i = 0; i < list->nr; i++)\n+\t\t\tfor (size_t i = 0; i < list->nr; i++)\n \t\t\t\tclearfunc(list->items[i].util, list->items[i].string);\n \t\t}\n \t\tif (list->strdup_strings) {\n-\t\t\tfor (i = 0; i < list->nr; i++)\n+\t\t\tfor (size_t i = 0; i < list->nr; i++)\n \t\t\t\tfree(list->items[i].string);\n \t\t}\n \t\tfree(list->items);\n-- \n2.49.0\n\n"},{"id":"516500","messageId":"aAetv8l8jrxvEywB@ArchLinux","threadId":"63329","inReplyTo":"aAetW0dan8S3Fljq@ArchLinux","subject":"[PATCH 2/5] u-string-list: move \"test_split\" into \"u-string-list.c\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-04-22T14:54:55Z","receivedAt":"2025-04-22T14:54:49Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"We rely on \"test-tool string-list\" command to test the functionality of\nthe \"string-list\". However, as we have introduced clar test framework,\nwe'd better move the shell script into C program to improve speed and\nreadability.\n\nCreate a new file \"u-string-list.c\" under \"t/unit-tests\", then update\nthe Makefile and \"meson.build\" to build the file. And let's first move\n\"test_split\" into unit test and gradually convert the shell script into\nC program.\n\nIn order to create `string_list` easily by simply specifying strings in\nthe function call, create \"t_vcreate_string_list_dup\" and\n\"t_create_string_list_dup\" functions to do above.\n\nThen port the shell script tests to C program and remove unused\n\"test-tool\" code and tests.\n\nSigned-off-by: shejialuo \n---\n Makefile | 1 +\n t/helper/test-string-list.c | 14 ------\n t/meson.build | 1 +\n t/t0063-string-list.sh | 53 ----------------------\n t/unit-tests/u-string-list.c | 86 ++++++++++++++++++++++++++++++++++++\n 5 files changed, 88 insertions(+), 67 deletions(-)\n create mode 100644 t/unit-tests/u-string-list.c\n\ndiff --git a/Makefile b/Makefile\nindex 13f9062a05..58df1f1150 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1365,6 +1365,7 @@ CLAR_TEST_SUITES += u-prio-queue\n CLAR_TEST_SUITES += u-reftable-tree\n CLAR_TEST_SUITES += u-strbuf\n CLAR_TEST_SUITES += u-strcmp-offset\n+CLAR_TEST_SUITES += u-string-list\n CLAR_TEST_SUITES += u-strvec\n CLAR_TEST_SUITES += u-trailer\n CLAR_TEST_SUITES += u-urlmatch-normalization\ndiff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c\nindex 6f10c5a435..17c18c30f6 100644\n--- a/t/helper/test-string-list.c\n+++ b/t/helper/test-string-list.c\n@@ -46,20 +46,6 @@ static int prefix_cb(struct string_list_item *item, void *cb_data)\n \n int cmd__string_list(int argc, const char **argv)\n {\n-\tif (argc == 5 && !strcmp(argv[1], \"split\")) {\n-\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n-\t\tint i;\n-\t\tconst char *s = argv[2];\n-\t\tint delim = *argv[3];\n-\t\tint maxsplit = atoi(argv[4]);\n-\n-\t\ti = string_list_split(&list, s, delim, maxsplit);\n-\t\tprintf(\"%d\\n\", i);\n-\t\twrite_list(&list);\n-\t\tstring_list_clear(&list, 0);\n-\t\treturn 0;\n-\t}\n-\n \tif (argc == 5 && !strcmp(argv[1], \"split_in_place\")) {\n \t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n \t\tint i;\ndiff --git a/t/meson.build b/t/meson.build\nindex bfb744e886..424e7e445f 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -11,6 +11,7 @@ clar_test_suites = [\n 'unit-tests/u-reftable-tree.c',\n 'unit-tests/u-strbuf.c',\n 'unit-tests/u-strcmp-offset.c',\n+ 'unit-tests/u-string-list.c',\n 'unit-tests/u-strvec.c',\n 'unit-tests/u-trailer.c',\n 'unit-tests/u-urlmatch-normalization.c',\ndiff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\nindex aac63ba506..6b20ffd206 100755\n--- a/t/t0063-string-list.sh\n+++ b/t/t0063-string-list.sh\n@@ -7,16 +7,6 @@ test_description='Test string list functionality'\n \n . ./test-lib.sh\n \n-test_split () {\n-\tcat >expected &&\n-\ttest_expect_success \"split $1 at $2, max $3\" \"\n-\t\ttest-tool string-list split '$1' '$2' '$3' >actual &&\n-\t\ttest_cmp expected actual &&\n-\t\ttest-tool string-list split_in_place '$1' '$2' '$3' >actual &&\n-\t\ttest_cmp expected actual\n-\t\"\n-}\n-\n test_split_in_place() {\n \tcat >expected &&\n \ttest_expect_success \"split (in place) $1 at $2, max $3\" \"\n@@ -25,49 +15,6 @@ test_split_in_place() {\n \t\"\n }\n \n-test_split \"foo:bar:baz\" \":\" \"-1\" <nr;\n+\tcl_assert_equal_i(list->nr, expect_len);\n+\tcl_assert(list->nr <= list->alloc);\n+\tfor (size_t i = 0; i < expect_len; i++)\n+\t\tcl_assert_equal_s(list->items[i].string,\n+\t\t\t\t expected_strings->items[i].string);\n+}\n+\n+static void t_string_list_clear(struct string_list *list, int free_util)\n+{\n+\tstring_list_clear(list, free_util);\n+\tcl_assert_equal_p(list->items, NULL);\n+\tcl_assert_equal_i(list->nr, 0);\n+\tcl_assert_equal_i(list->alloc, 0);\n+}\n+\n+static void t_vcreate_string_list_dup(struct string_list *list,\n+\t\t\t\t int free_util, va_list ap)\n+{\n+\tconst char *arg;\n+\n+\tcl_assert(list->strdup_strings);\n+\n+\tt_string_list_clear(list, free_util);\n+\twhile ((arg = va_arg(ap, const char *)))\n+\t\tstring_list_append(list, arg);\n+}\n+\n+static void t_create_string_list_dup(struct string_list *list, int free_util, ...)\n+{\n+\tva_list ap;\n+\n+\tcl_assert(list->strdup_strings);\n+\n+\tt_string_list_clear(list, free_util);\n+\tva_start(ap, free_util);\n+\tt_vcreate_string_list_dup(list, free_util, ap);\n+\tva_end(ap);\n+}\n+\n+static void t_string_list_split(const char *data, int delim, int maxsplit,\n+\t\t\t\tstruct string_list *expected_strings)\n+{\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\tint len;\n+\n+\tlen = string_list_split(&list, data, delim, maxsplit);\n+\tcl_assert_equal_i(len, expected_strings->nr);\n+\tt_check_string_list(&list, expected_strings);\n+\n+\tt_string_list_clear(&list, 0);\n+}\n+\n+void test_string_list__split(void)\n+{\n+\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n+\n+\tt_create_string_list_dup(&expected_strings, 0, \"foo\", \"bar\", \"baz\", NULL);\n+\tt_string_list_split(\"foo:bar:baz\", ':', -1, &expected_strings);\n+\n+\tt_create_string_list_dup(&expected_strings, 0, \"foo:bar:baz\", NULL);\n+\tt_string_list_split(\"foo:bar:baz\", ':', 0, &expected_strings);\n+\n+\tt_create_string_list_dup(&expected_strings, 0, \"foo\", \"bar:baz\", NULL);\n+\tt_string_list_split(\"foo:bar:baz\", ':', 1, &expected_strings);\n+\n+\tt_create_string_list_dup(&expected_strings, 0, \"foo\", \"bar\", \"baz\", NULL);\n+\tt_string_list_split(\"foo:bar:baz\", ':', 2, &expected_strings);\n+\n+\tt_create_string_list_dup(&expected_strings, 0, \"foo\", \"bar\", \"\", NULL);\n+\tt_string_list_split(\"foo:bar:\", ':', -1, &expected_strings);\n+\n+\tt_create_string_list_dup(&expected_strings, 0, \"\", NULL);\n+\tt_string_list_split(\"\", ':', -1, &expected_strings);\n+\n+\tt_create_string_list_dup(&expected_strings, 0, \"\", \"\", NULL);\n+\tt_string_list_split(\":\", ':', -1, &expected_strings);\n+\n+\tt_string_list_clear(&expected_strings, 0);\n+}\n-- \n2.49.0\n\n"},{"id":"516501","messageId":"aAetyvcw7ZgXa3f7@ArchLinux","threadId":"63329","inReplyTo":"aAetW0dan8S3Fljq@ArchLinux","subject":"[PATCH 3/5] u-string-list: move \"test_split_in_place\" to \"u-string-list.c\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-04-22T14:55:06Z","receivedAt":"2025-04-22T14:55:01Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"We use \"test-tool string-list split_in_place\" to test the\n\"string_list_split_in_place\" function. As we have introduced the unit\ntest, we'd better remove the logic from shell script to C program to\nimprove test speed and readability.\n\nSigned-off-by: shejialuo \n---\n t/helper/test-string-list.c | 22 ----------------\n t/t0063-string-list.sh | 51 ------------------------------------\n t/unit-tests/u-string-list.c | 39 +++++++++++++++++++++++++++\n 3 files changed, 39 insertions(+), 73 deletions(-)\n\ndiff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c\nindex 17c18c30f6..8a344347ad 100644\n--- a/t/helper/test-string-list.c\n+++ b/t/helper/test-string-list.c\n@@ -18,13 +18,6 @@ static void parse_string_list(struct string_list *list, const char *arg)\n \t(void)string_list_split(list, arg, ':', -1);\n }\n \n-static void write_list(const struct string_list *list)\n-{\n-\tint i;\n-\tfor (i = 0; i < list->nr; i++)\n-\t\tprintf(\"[%d]: \\\"%s\\\"\\n\", i, list->items[i].string);\n-}\n-\n static void write_list_compact(const struct string_list *list)\n {\n \tint i;\n@@ -46,21 +39,6 @@ static int prefix_cb(struct string_list_item *item, void *cb_data)\n \n int cmd__string_list(int argc, const char **argv)\n {\n-\tif (argc == 5 && !strcmp(argv[1], \"split_in_place\")) {\n-\t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n-\t\tint i;\n-\t\tchar *s = xstrdup(argv[2]);\n-\t\tconst char *delim = argv[3];\n-\t\tint maxsplit = atoi(argv[4]);\n-\n-\t\ti = string_list_split_in_place(&list, s, delim, maxsplit);\n-\t\tprintf(\"%d\\n\", i);\n-\t\twrite_list(&list);\n-\t\tstring_list_clear(&list, 0);\n-\t\tfree(s);\n-\t\treturn 0;\n-\t}\n-\n \tif (argc == 4 && !strcmp(argv[1], \"filter\")) {\n \t\t/*\n \t\t * Retain only the items that have the specified prefix.\ndiff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\nindex 6b20ffd206..1a9cf8bfcf 100755\n--- a/t/t0063-string-list.sh\n+++ b/t/t0063-string-list.sh\n@@ -7,57 +7,6 @@ test_description='Test string list functionality'\n \n . ./test-lib.sh\n \n-test_split_in_place() {\n-\tcat >expected &&\n-\ttest_expect_success \"split (in place) $1 at $2, max $3\" \"\n-\t\ttest-tool string-list split_in_place '$1' '$2' '$3' >actual &&\n-\t\ttest_cmp expected actual\n-\t\"\n-}\n-\n-test_split_in_place \"foo:;:bar:;:baz:;:\" \":;\" \"-1\" <nr);\n+\tt_check_string_list(&list, expected_strings);\n+\n+\tfree(string);\n+\tt_string_list_clear(&list, 0);\n+}\n+\n+void test_string_list__split_in_place(void)\n+{\n+\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n+\n+\tt_create_string_list_dup(&expected_strings, 0, \"foo\", \"\", \"\", \"bar\",\n+\t\t\t\t \"\", \"\", \"baz\", \"\", \"\", \"\", NULL);\n+\tt_string_list_split_in_place(\"foo:;:bar:;:baz:;:\", \":;\", -1, &expected_strings);\n+\n+\tt_create_string_list_dup(&expected_strings, 0, \"foo:;:bar:;:baz\", NULL);\n+\tt_string_list_split_in_place(\"foo:;:bar:;:baz\", \":;\", 0, &expected_strings);\n+\n+\tt_create_string_list_dup(&expected_strings, 0, \"foo\", \";:bar:;:baz\", NULL);\n+\tt_string_list_split_in_place(\"foo:;:bar:;:baz\", \":;\", 1, &expected_strings);\n+\n+\tt_create_string_list_dup(&expected_strings, 0, \"foo\", \"\", \":bar:;:baz\", NULL);\n+\tt_string_list_split_in_place(\"foo:;:bar:;:baz\", \":;\", 2, &expected_strings);\n+\n+\tt_create_string_list_dup(&expected_strings, 0, \"foo\", \"\", \"\", \"bar\",\n+\t\t\t\t \"\", \"\", \"\", NULL);\n+\tt_string_list_split_in_place(\"foo:;:bar:;:\", \":;\", -1, &expected_strings);\n+\n+\tt_string_list_clear(&expected_strings, 0);\n+}\n-- \n2.49.0\n\n"},{"id":"516502","messageId":"aAet07CAKsJUklrS@ArchLinux","threadId":"63329","inReplyTo":"aAetW0dan8S3Fljq@ArchLinux","subject":"[PATCH 4/5] u-string-list: move \"filter string\" test to \"u-string-list.c\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-04-22T14:55:15Z","receivedAt":"2025-04-22T14:55:09Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"We use \"test-tool string-list filter\" to test the \"filter_string_list\"\nfunction. As we have introduced the unit test, we'd better remove the\nlogic from shell script to C program to improve test speed and\nreadability.\n\nSigned-off-by: shejialuo \n---\n t/helper/test-string-list.c | 21 ---------------\n t/t0063-string-list.sh | 11 --------\n t/unit-tests/u-string-list.c | 51 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 51 insertions(+), 32 deletions(-)\n\ndiff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c\nindex 8a344347ad..262b28c599 100644\n--- a/t/helper/test-string-list.c\n+++ b/t/helper/test-string-list.c\n@@ -31,29 +31,8 @@ static void write_list_compact(const struct string_list *list)\n \t}\n }\n \n-static int prefix_cb(struct string_list_item *item, void *cb_data)\n-{\n-\tconst char *prefix = (const char *)cb_data;\n-\treturn starts_with(item->string, prefix);\n-}\n-\n int cmd__string_list(int argc, const char **argv)\n {\n-\tif (argc == 4 && !strcmp(argv[1], \"filter\")) {\n-\t\t/*\n-\t\t * Retain only the items that have the specified prefix.\n-\t\t * Arguments: list|- prefix\n-\t\t */\n-\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n-\t\tconst char *prefix = argv[3];\n-\n-\t\tparse_string_list(&list, argv[2]);\n-\t\tfilter_string_list(&list, 0, prefix_cb, (void *)prefix);\n-\t\twrite_list_compact(&list);\n-\t\tstring_list_clear(&list, 0);\n-\t\treturn 0;\n-\t}\n-\n \tif (argc == 3 && !strcmp(argv[1], \"remove_duplicates\")) {\n \t\tstruct string_list list = STRING_LIST_INIT_DUP;\n \ndiff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\nindex 1a9cf8bfcf..31fd62bba8 100755\n--- a/t/t0063-string-list.sh\n+++ b/t/t0063-string-list.sh\n@@ -7,17 +7,6 @@ test_description='Test string list functionality'\n \n . ./test-lib.sh\n \n-test_expect_success \"test filter_string_list\" '\n-\ttest \"x-\" = \"x$(test-tool string-list filter - y)\" &&\n-\ttest \"x-\" = \"x$(test-tool string-list filter no y)\" &&\n-\ttest yes = \"$(test-tool string-list filter yes y)\" &&\n-\ttest yes = \"$(test-tool string-list filter no:yes y)\" &&\n-\ttest yes = \"$(test-tool string-list filter yes:no y)\" &&\n-\ttest y1:y2 = \"$(test-tool string-list filter y1:y2 y)\" &&\n-\ttest y2:y1 = \"$(test-tool string-list filter y2:y1 y)\" &&\n-\ttest \"x-\" = \"x$(test-tool string-list filter x1:x2 y)\"\n-'\n-\n test_expect_success \"test remove_duplicates\" '\n \ttest \"x-\" = \"x$(test-tool string-list remove_duplicates -)\" &&\n \ttest \"x\" = \"x$(test-tool string-list remove_duplicates \"\")\" &&\ndiff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c\nindex 44ec8de3d0..e02a15ac04 100644\n--- a/t/unit-tests/u-string-list.c\n+++ b/t/unit-tests/u-string-list.c\n@@ -123,3 +123,54 @@ void test_string_list__split_in_place(void)\n \n \tt_string_list_clear(&expected_strings, 0);\n }\n+\n+static int prefix_cb(struct string_list_item *item, void *cb_data)\n+{\n+\tconst char *prefix = (const char *)cb_data;\n+\treturn starts_with(item->string, prefix);\n+}\n+\n+static void t_string_list_filter(struct string_list *list,\n+\t\t\t\t string_list_each_func_t want, void *cb_data,\n+\t\t\t\t struct string_list *expected_strings)\n+{\n+\tfilter_string_list(list, 0, want, cb_data);\n+\tt_check_string_list(list, expected_strings);\n+}\n+\n+void test_string_list__filter(void)\n+{\n+\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\tconst char *prefix = \"y\";\n+\n+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, &expected_strings);\n+\n+\tt_create_string_list_dup(&list, 0, \"no\", NULL);\n+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, &expected_strings);\n+\n+\tt_create_string_list_dup(&list, 0, \"yes\", NULL);\n+\tt_create_string_list_dup(&expected_strings, 0, \"yes\", NULL);\n+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, &expected_strings);\n+\n+\tt_create_string_list_dup(&list, 0, \"no\", \"yes\", NULL);\n+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, &expected_strings);\n+\n+\tt_create_string_list_dup(&list, 0, \"yes\", \"no\", NULL);\n+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, &expected_strings);\n+\n+\tt_create_string_list_dup(&list, 0, \"y1\", \"y2\", NULL);\n+\tt_create_string_list_dup(&expected_strings, 0, \"y1\", \"y2\", NULL);\n+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, &expected_strings);\n+\n+\tt_create_string_list_dup(&list, 0, \"y2\", \"y1\", NULL);\n+\tt_create_string_list_dup(&expected_strings, 0, \"y2\", \"y1\", NULL);\n+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, &expected_strings);\n+\n+\tt_create_string_list_dup(&list, 0, \"x1\", \"x2\", NULL);\n+\tt_create_string_list_dup(&expected_strings, 0, NULL);\n+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, &expected_strings);\n+\n+\tt_string_list_clear(&list, 0);\n+\tt_string_list_clear(&expected_strings, 0);\n+}\n-- \n2.49.0\n\n"},{"id":"516503","messageId":"aAet23peGs2OZUcn@ArchLinux","threadId":"63329","inReplyTo":"aAetW0dan8S3Fljq@ArchLinux","subject":"[PATCH 5/5] u-string-list: move \"remove duplicates\" test to \"u-string-list.c\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-04-22T14:55:23Z","receivedAt":"2025-04-22T14:55:17Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"We use \"test-tool string-list remove_duplicates\" to test the\n\"string_list_remove_duplicates\" function. As we have introduced the unit\ntest, we'd better remove the logic from shell script to C program to\nimprove test speed and readability.\n\nAs all the tests in shell script are removed, let's just delete the\n\"t0063-string-list.sh\" and update the \"meson.build\" file to align with\nthis change.\n\nAlso we could simply remove \"DISABLE_SIGN_COMPARE_WARNINGS\" due to we\nhave already deleted related code.\n\nSigned-off-by: shejialuo \n---\n t/helper/test-string-list.c | 39 -----------------------\n t/meson.build | 1 -\n t/t0063-string-list.sh | 27 ----------------\n t/unit-tests/u-string-list.c | 62 ++++++++++++++++++++++++++++++++++++\n 4 files changed, 62 insertions(+), 67 deletions(-)\n delete mode 100755 t/t0063-string-list.sh\n\ndiff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c\nindex 262b28c599..6be0cdb8e2 100644\n--- a/t/helper/test-string-list.c\n+++ b/t/helper/test-string-list.c\n@@ -1,48 +1,9 @@\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n-\n #include \"test-tool.h\"\n #include \"strbuf.h\"\n #include \"string-list.h\"\n \n-/*\n- * Parse an argument into a string list. arg should either be a\n- * ':'-separated list of strings, or \"-\" to indicate an empty string\n- * list (as opposed to \"\", which indicates a string list containing a\n- * single empty string). list->strdup_strings must be set.\n- */\n-static void parse_string_list(struct string_list *list, const char *arg)\n-{\n-\tif (!strcmp(arg, \"-\"))\n-\t\treturn;\n-\n-\t(void)string_list_split(list, arg, ':', -1);\n-}\n-\n-static void write_list_compact(const struct string_list *list)\n-{\n-\tint i;\n-\tif (!list->nr)\n-\t\tprintf(\"-\\n\");\n-\telse {\n-\t\tprintf(\"%s\", list->items[0].string);\n-\t\tfor (i = 1; i < list->nr; i++)\n-\t\t\tprintf(\":%s\", list->items[i].string);\n-\t\tprintf(\"\\n\");\n-\t}\n-}\n-\n int cmd__string_list(int argc, const char **argv)\n {\n-\tif (argc == 3 && !strcmp(argv[1], \"remove_duplicates\")) {\n-\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n-\n-\t\tparse_string_list(&list, argv[2]);\n-\t\tstring_list_remove_duplicates(&list, 0);\n-\t\twrite_list_compact(&list);\n-\t\tstring_list_clear(&list, 0);\n-\t\treturn 0;\n-\t}\n-\n \tif (argc == 2 && !strcmp(argv[1], \"sort\")) {\n \t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n \t\tstruct strbuf sb = STRBUF_INIT;\ndiff --git a/t/meson.build b/t/meson.build\nindex 424e7e445f..25af09a8d4 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -124,7 +124,6 @@ integration_tests = [\n 't0060-path-utils.sh',\n 't0061-run-command.sh',\n 't0062-revision-walking.sh',\n- 't0063-string-list.sh',\n 't0066-dir-iterator.sh',\n 't0067-parse_pathspec_file.sh',\n 't0068-for-each-repo.sh',\ndiff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\ndeleted file mode 100755\nindex 31fd62bba8..0000000000\n--- a/t/t0063-string-list.sh\n+++ /dev/null\n@@ -1,27 +0,0 @@\n-#!/bin/sh\n-#\n-# Copyright (c) 2012 Michael Haggerty\n-#\n-\n-test_description='Test string list functionality'\n-\n-. ./test-lib.sh\n-\n-test_expect_success \"test remove_duplicates\" '\n-\ttest \"x-\" = \"x$(test-tool string-list remove_duplicates -)\" &&\n-\ttest \"x\" = \"x$(test-tool string-list remove_duplicates \"\")\" &&\n-\ttest a = \"$(test-tool string-list remove_duplicates a)\" &&\n-\ttest a = \"$(test-tool string-list remove_duplicates a:a)\" &&\n-\ttest a = \"$(test-tool string-list remove_duplicates a:a:a:a:a)\" &&\n-\ttest a:b = \"$(test-tool string-list remove_duplicates a:b)\" &&\n-\ttest a:b = \"$(test-tool string-list remove_duplicates a:a:b)\" &&\n-\ttest a:b = \"$(test-tool string-list remove_duplicates a:b:b)\" &&\n-\ttest a:b:c = \"$(test-tool string-list remove_duplicates a:b:c)\" &&\n-\ttest a:b:c = \"$(test-tool string-list remove_duplicates a:a:b:c)\" &&\n-\ttest a:b:c = \"$(test-tool string-list remove_duplicates a:b:b:c)\" &&\n-\ttest a:b:c = \"$(test-tool string-list remove_duplicates a:b:c:c)\" &&\n-\ttest a:b:c = \"$(test-tool string-list remove_duplicates a:a:b:b:c:c)\" &&\n-\ttest a:b:c = \"$(test-tool string-list remove_duplicates a:a:a:b:b:b:c:c:c)\"\n-'\n-\n-test_done\ndiff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c\nindex e02a15ac04..a9fe5ade15 100644\n--- a/t/unit-tests/u-string-list.c\n+++ b/t/unit-tests/u-string-list.c\n@@ -174,3 +174,65 @@ void test_string_list__filter(void)\n \tt_string_list_clear(&list, 0);\n \tt_string_list_clear(&expected_strings, 0);\n }\n+\n+static void t_string_list_remove_duplicates(struct string_list *list,\n+\t\t\t\t\t struct string_list *expected_strings)\n+{\n+\tstring_list_remove_duplicates(list, 0);\n+\tt_check_string_list(list, expected_strings);\n+}\n+\n+void test_string_list__remove_duplicates(void)\n+{\n+\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\n+\tt_string_list_remove_duplicates(&list, &expected_strings);\n+\n+\tt_create_string_list_dup(&list, 0, \"\", NULL);\n+\tt_create_string_list_dup(&expected_strings, 0, \"\", NULL);\n+\tt_string_list_remove_duplicates(&list, &expected_strings);\n+\n+\tt_create_string_list_dup(&expected_strings, 0, \"a\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", NULL);\n+\tt_string_list_remove_duplicates(&list, &expected_strings);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"a\", NULL);\n+\tt_string_list_remove_duplicates(&list, &expected_strings);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"a\", \"a\", NULL);\n+\tt_string_list_remove_duplicates(&list, &expected_strings);\n+\n+\tt_create_string_list_dup(&expected_strings, 0, \"a\", \"b\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"a\", \"b\", NULL);\n+\tt_string_list_remove_duplicates(&list, &expected_strings);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"b\", \"b\", NULL);\n+\tt_string_list_remove_duplicates(&list, &expected_strings);\n+\n+\tt_create_string_list_dup(&expected_strings, 0, \"a\", \"b\", \"c\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"b\", \"c\", NULL);\n+\tt_string_list_remove_duplicates(&list, &expected_strings);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"a\", \"b\", \"c\", NULL);\n+\tt_string_list_remove_duplicates(&list, &expected_strings);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"b\", \"b\", \"c\", NULL);\n+\tt_string_list_remove_duplicates(&list, &expected_strings);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"b\", \"c\", \"c\", NULL);\n+\tt_string_list_remove_duplicates(&list, &expected_strings);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"a\", \"b\", \"b\", \"c\", \"c\", NULL);\n+\tt_string_list_remove_duplicates(&list, &expected_strings);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"a\", \"a\", \"b\", \"b\", \"b\",\n+\t\t\t\t \"c\", \"c\", \"c\", NULL);\n+\tt_string_list_remove_duplicates(&list, &expected_strings);\n+\n+\tt_string_list_clear(&list, 0);\n+\tt_string_list_clear(&expected_strings, 0);\n+}\n-- \n2.49.0\n\n"},{"id":"516532","messageId":"xmqqy0vr3o6d.fsf@gitster.g","threadId":"63329","inReplyTo":"aAett8cJuDJ_FSdw@ArchLinux","subject":"Re: [PATCH 1/5] string-list: fix sign compare warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-22T21:02:18Z","receivedAt":"2025-04-22T21:02:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo writes:\n\n> However, for \"string-list.c::add_entry\" function, we compare the `index`\n> of the `int` type with the `list->nr` of unsigned type. It seems that\n> we could just simply convert the type of `index` from `int` to\n> `size_t`. But actually this is a correct behavior.\n\nSorry, but I am lost by the last sentence.\n\n\"this\" that is a correct behavior refers to...? That the incoming\nparameter insert_at and the local variable index are both of signed\ninteger?\n\n> We would set the `index` value by checking whether `insert_at` is -1.\n> If not, we would set `index` to be `insert_at`, otherwise we would use\n> \"get_entry_index` to find the inserted position.\n\nTo rephrase the above (simply because the above is literal English\ntranslation from what C says), the caller either can pass -1 to mean\n\"find an appropriate location in the list to keep it sorted\", or an\nindex into the list->items[] array to specify exactly where the item\nshould be inserted.\n\nNaturally, insert_at must be either -1 (auto), or between 0\n(i.e. the candidate is smaller than anything in the list) and\nlist->nr (i.e. the candidate is larger than everything in the list)\ninclusive. Any other value is invalid. I think that is a more\nappropriate thing to say than ...\n\n> What if the caller passes a negative value except \"-1\", the compiler\n> would convert the `index` to be a positive value which would make the\n> `if` statement be false to avoid moving array. However, we would\n> definitely encounter trouble when setting the inserted item.\n\n... this paragraph. Not moving is _not_ avoiding problem, so it is\nimmaterial. The lack of valid range check before using the index\nis.\n\n> And we only call \"add_entry\" in \"string_list_insert\" function, and we\n> simply pass \"-1\" for \"insert_at\" parameter. So, we never use this\n> parameter to insert element in a user specified position. Let's delete\n> this parameter. If there is any requirement later, we may use a better\n> way to do this. And then we could safely convert the index to be\n> `size_t` when comparing.\n\nGood. As we only use the \"auto\" setting with this code now, as long\nas get_entry_index() returns a value between 0 and list->nr, the\nlack of such range checking in the original code no longer is an\nissue.\n\nHaving said that, in the longer run, get_entry_index() would want to\nreturn size_t simply because it is returning a value between 0 and\nlist->nr, whose type is size_t. left/mid/right variables also need\nto become size_t and the loop initialization may have to be tweaked\n(since the current code strangely starts left with -1 which would\nnever be the index into the array), but fixing that should probably\nmake the loop easier to read, which is a bonus.\n\nAnd add_entry(), since it needs to do the usual -1-pos dance to\nindicate where things would have been returned, would return\nssize_t---or better yet, it can just turned into returning size_t\nwith an extra out parameter (just like the exact_match out parameter\nget_entry_index() has) to indicate if we already had the same item\nin the list already. It is perfectly fine to leave it outside the\nscope of this series, but if you are tweaking all the callers of\nadd_entry() anyway in this step, you may want to bite the bullet and\njust go all the way.\n\nThanks.\n"},{"id":"516534","messageId":"xmqqr01j3n15.fsf@gitster.g","threadId":"63329","inReplyTo":"aAetv8l8jrxvEywB@ArchLinux","subject":"Re: [PATCH 2/5] u-string-list: move \"test_split\" into \"u-string-list.c\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-22T21:27:02Z","receivedAt":"2025-04-22T21:27:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo writes:\n\n> We rely on \"test-tool string-list\" command to test the functionality of\n> the \"string-list\". However, as we have introduced clar test framework,\n> we'd better move the shell script into C program to improve speed and\n> readability.\n>\n> Create a new file \"u-string-list.c\" under \"t/unit-tests\", then update\n> the Makefile and \"meson.build\" to build the file. And let's first move\n> \"test_split\" into unit test and gradually convert the shell script into\n> C program.\n>\n> In order to create `string_list` easily by simply specifying strings in\n> the function call, create \"t_vcreate_string_list_dup\" and\n> \"t_create_string_list_dup\" functions to do above.\n>\n> Then port the shell script tests to C program and remove unused\n> \"test-tool\" code and tests.\n>\n> Signed-off-by: shejialuo \n> ---\n\nThis is the most interesting in the u-string-list patches, as it\nadds not just a moved test but adds supporting functions that are\nshared with test functions added in later steps.\n\n> diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c\n> new file mode 100644\n> index 0000000000..0c148684ea\n> --- /dev/null\n> +++ b/t/unit-tests/u-string-list.c\n> @@ -0,0 +1,86 @@\n> +#include \"unit-test.h\"\n> +#include \"string-list.h\"\n> +\n> +static void t_check_string_list(struct string_list *list,\n> +\t\t\t\tstruct string_list *expected_strings)\n> +{\n> +\tsize_t expect_len = expected_strings->nr;\n> +\tcl_assert_equal_i(list->nr, expect_len);\n> +\tcl_assert(list->nr <= list->alloc);\n> +\tfor (size_t i = 0; i < expect_len; i++)\n> +\t\tcl_assert_equal_s(list->items[i].string,\n> +\t\t\t\t expected_strings->items[i].string);\n> +}\n\nPerhaps call it \"string_list_equal()\" or something? \"check\" is a\nconvenient name that can mean different kind of validation that is\nnot limited to \"is the actual answer identical to the expected one?\"\n\nWouldn't it be cleaner to read if you wrote it without an extra\nvariable expect_len? The compiler would notice repeated reference\nof expected_strings->nr and optimize them away anyway, I would\nimagine.\n\n> +static void t_string_list_clear(struct string_list *list, int free_util)\n> +{\n> +\tstring_list_clear(list, free_util);\n> +\tcl_assert_equal_p(list->items, NULL);\n> +\tcl_assert_equal_i(list->nr, 0);\n> +\tcl_assert_equal_i(list->alloc, 0);\n> +}\n\nValidating the result of clearing a list may be a good thing to do\nat least once in the test suite, but this is called from many places\nin other tests. Conceptually it feels kludgy to call this from\nother places where they should all just call string_list_clear(),\nlike ...\n\n> +static void t_vcreate_string_list_dup(struct string_list *list,\n> +\t\t\t\t int free_util, va_list ap)\n> +{\n> +\tconst char *arg;\n> +\n> +\tcl_assert(list->strdup_strings);\n> +\n> +\tt_string_list_clear(list, free_util);\n\n... this place.\n\n> +\twhile ((arg = va_arg(ap, const char *)))\n> +\t\tstring_list_append(list, arg);\n> +}\n\nTo put it differently, you could be calling t_string_list_append()\nin this loop, which would \n\n - remember list->nr\n - call string_list_append()\n - cl_assert_equal() to ensure that list->nr is one larger than\n the value we remembered upon entry to the function.\n\nwhich is not wrong per-se, but hopefully you'd agree that it is\noverkill. t_stirng_list_clear() is overkill in the same way.\n\n> +void test_string_list__split(void)\n> +{\n> +\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n> +\n> +\tt_create_string_list_dup(&expected_strings, 0, \"foo\", \"bar\", \"baz\", NULL);\n> +...\n> +\tt_create_string_list_dup(&expected_strings, 0, \"\", \"\", NULL);\n> +\tt_string_list_split(\":\", ':', -1, &expected_strings);\n> +\n> +\tt_string_list_clear(&expected_strings, 0);\n> +}\n\nThis is a good place to call t_string_list_clear(), just once in\nthis script. All other callers are conceptually simpler to call\nstring_list_clear(), as the point at their callsites is to clear\nafter themselves, not about testing string_list_clear() works\ncorrectly.\n\nThanks.\n\n"},{"id":"516596","messageId":"aAjUmi4ccemvO7XT@pks.im","threadId":"63329","inReplyTo":"aAetv8l8jrxvEywB@ArchLinux","subject":"Re: [PATCH 2/5] u-string-list: move \"test_split\" into \"u-string-list.c\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-04-23T11:52:58Z","receivedAt":"2025-04-23T11:53:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 22, 2025 at 10:54:55PM +0800, shejialuo wrote:\n> diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c\n> new file mode 100644\n> index 0000000000..0c148684ea\n> --- /dev/null\n> +++ b/t/unit-tests/u-string-list.c\n> @@ -0,0 +1,86 @@\n> +static void t_string_list_split(const char *data, int delim, int maxsplit,\n> +\t\t\t\tstruct string_list *expected_strings)\n> +{\n> +\tstruct string_list list = STRING_LIST_INIT_DUP;\n> +\tint len;\n> +\n> +\tlen = string_list_split(&list, data, delim, maxsplit);\n> +\tcl_assert_equal_i(len, expected_strings->nr);\n> +\tt_check_string_list(&list, expected_strings);\n> +\n> +\tt_string_list_clear(&list, 0);\n> +}\n> +\n> +void test_string_list__split(void)\n> +{\n> +\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n> +\n> +\tt_create_string_list_dup(&expected_strings, 0, \"foo\", \"bar\", \"baz\", NULL);\n> +\tt_string_list_split(\"foo:bar:baz\", ':', -1, &expected_strings);\n\nCould we adapt `t_string_list_split()` so that it accepts the expected\nstrings as varargs? If so we could simplify the logic in this function\nhere.\n\nPatrick\n"},{"id":"516597","messageId":"aAjUrNTsL966mGeN@pks.im","threadId":"63329","inReplyTo":"aAetyvcw7ZgXa3f7@ArchLinux","subject":"Re: [PATCH 3/5] u-string-list: move \"test_split_in_place\" to \"u-string-list.c\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-04-23T11:53:16Z","receivedAt":"2025-04-23T11:53:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 22, 2025 at 10:55:06PM +0800, shejialuo wrote:\n> diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c\n> index 0c148684ea..44ec8de3d0 100644\n> --- a/t/unit-tests/u-string-list.c\n> +++ b/t/unit-tests/u-string-list.c\n> @@ -84,3 +84,42 @@ void test_string_list__split(void)\n> \n> \tt_string_list_clear(&expected_strings, 0);\n> }\n> +\n> +static void t_string_list_split_in_place(const char *data, const char *delim, int maxsplit,\n> +\t\t\t\t\t struct string_list *expected_strings)\n> +{\n> +\tstruct string_list list = STRING_LIST_INIT_NODUP;\n> +\n\nNit: this empty newline should be removed.\n\n> +\tchar *string = xstrdup(data);\n> +\n> +\tint len = string_list_split_in_place(&list, string, delim, maxsplit);\n> +\tcl_assert_equal_i(len, expected_strings->nr);\n> +\tt_check_string_list(&list, expected_strings);\n> +\n> +\tfree(string);\n> +\tt_string_list_clear(&list, 0);\n> +}\n> +\n> +void test_string_list__split_in_place(void)\n> +{\n> +\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n> +\n> +\tt_create_string_list_dup(&expected_strings, 0, \"foo\", \"\", \"\", \"bar\",\n> +\t\t\t\t \"\", \"\", \"baz\", \"\", \"\", \"\", NULL);\n> +\tt_string_list_split_in_place(\"foo:;:bar:;:baz:;:\", \":;\", -1, &expected_strings);\n\nSame question here, can we handle expected strings via varargs to avoid\ncode duplication? Also for subsequent patches.\n\nPatrick\n"},{"id":"516598","messageId":"aAjUr5AY9gTCpVlS@pks.im","threadId":"63329","inReplyTo":"aAet23peGs2OZUcn@ArchLinux","subject":"Re: [PATCH 5/5] u-string-list: move \"remove duplicates\" test to \"u-string-list.c\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-04-23T11:53:19Z","receivedAt":"2025-04-23T11:53:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 22, 2025 at 10:55:23PM +0800, shejialuo wrote:\n> diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c\n> index 262b28c599..6be0cdb8e2 100644\n> --- a/t/helper/test-string-list.c\n> +++ b/t/helper/test-string-list.c\n> @@ -1,48 +1,9 @@\n> -#define DISABLE_SIGN_COMPARE_WARNINGS\n> -\n> #include \"test-tool.h\"\n> #include \"strbuf.h\"\n> #include \"string-list.h\"\n> \n> -/*\n> - * Parse an argument into a string list. arg should either be a\n> - * ':'-separated list of strings, or \"-\" to indicate an empty string\n> - * list (as opposed to \"\", which indicates a string list containing a\n> - * single empty string). list->strdup_strings must be set.\n> - */\n> -static void parse_string_list(struct string_list *list, const char *arg)\n> -{\n> -\tif (!strcmp(arg, \"-\"))\n> -\t\treturn;\n> -\n> -\t(void)string_list_split(list, arg, ':', -1);\n> -}\n> -\n> -static void write_list_compact(const struct string_list *list)\n> -{\n> -\tint i;\n> -\tif (!list->nr)\n> -\t\tprintf(\"-\\n\");\n> -\telse {\n> -\t\tprintf(\"%s\", list->items[0].string);\n> -\t\tfor (i = 1; i < list->nr; i++)\n> -\t\t\tprintf(\":%s\", list->items[i].string);\n> -\t\tprintf(\"\\n\");\n> -\t}\n> -}\n> -\n> int cmd__string_list(int argc, const char **argv)\n> {\n> -\tif (argc == 3 && !strcmp(argv[1], \"remove_duplicates\")) {\n> -\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n> -\n> -\t\tparse_string_list(&list, argv[2]);\n> -\t\tstring_list_remove_duplicates(&list, 0);\n> -\t\twrite_list_compact(&list);\n> -\t\tstring_list_clear(&list, 0);\n> -\t\treturn 0;\n> -\t}\n> -\n> \tif (argc == 2 && !strcmp(argv[1], \"sort\")) {\n> \t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n> \t\tstruct strbuf sb = STRBUF_INIT;\n\nI'm a bit surprised that the patch series stops after this patch given\nthat the only remaining subcommand is \"sort\". Is there a specific reason\nwhy you don't also convert that function? If you did we could declare\nvictory by deleting the whole \"test-helper string-list\" subcommand.\n\nIt seems that the only reason is p0071, where we benchmark performance\nof sorting. I dunno... that one is of course not a good fit for our unit\ntesting framework. But it's a bit sad that we cannot remove the whole\ninfra only because of a performance test that nobody ever runs in the\nfirst place.\n\nPatrick\n"},{"id":"516658","messageId":"aAouJnCw3nTQHQTG@ArchLinux","threadId":"63329","inReplyTo":"xmqqy0vr3o6d.fsf@gitster.g","subject":"Re: [PATCH 1/5] string-list: fix sign compare warnings","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-04-24T12:27:18Z","receivedAt":"2025-04-24T12:27:09Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Tue, Apr 22, 2025 at 02:02:18PM -0700, Junio C Hamano wrote:\n> shejialuo writes:\n> \n> > However, for \"string-list.c::add_entry\" function, we compare the `index`\n> > of the `int` type with the `list->nr` of unsigned type. It seems that\n> > we could just simply convert the type of `index` from `int` to\n> > `size_t`. But actually this is a correct behavior.\n> \n> Sorry, but I am lost by the last sentence.\n> \n> \"this\" that is a correct behavior refers to...? That the incoming\n> parameter insert_at and the local variable index are both of signed\n> integer?\n> \n\nWell, please ignore. I think I made a mistake. I should not say this is\ncorrect but say that this is not harmful.\n\n> > We would set the `index` value by checking whether `insert_at` is -1.\n> > If not, we would set `index` to be `insert_at`, otherwise we would use\n> > \"get_entry_index` to find the inserted position.\n> \n> To rephrase the above (simply because the above is literal English\n> translation from what C says), the caller either can pass -1 to mean\n> \"find an appropriate location in the list to keep it sorted\", or an\n> index into the list->items[] array to specify exactly where the item\n> should be inserted.\n> \n> Naturally, insert_at must be either -1 (auto), or between 0\n> (i.e. the candidate is smaller than anything in the list) and\n> list->nr (i.e. the candidate is larger than everything in the list)\n> inclusive. Any other value is invalid. I think that is a more\n> appropriate thing to say than ...\n> \n\nYes, thanks for the hint.\n\n> > What if the caller passes a negative value except \"-1\", the compiler\n> > would convert the `index` to be a positive value which would make the\n> > `if` statement be false to avoid moving array. However, we would\n> > definitely encounter trouble when setting the inserted item.\n> \n> ... this paragraph. Not moving is _not_ avoiding problem, so it is\n> immaterial. The lack of valid range check before using the index\n> is.\n> \n\nYes, that's right. Ideally, we should check whether the index is OK. And\nI somehow think that I should not make the commit message complicate. I\nshould just say we should remove \"insert_at\" function in a separate\ncommit and then fix sign comparing warnings.\n\n> > And we only call \"add_entry\" in \"string_list_insert\" function, and we\n> > simply pass \"-1\" for \"insert_at\" parameter. So, we never use this\n> > parameter to insert element in a user specified position. Let's delete\n> > this parameter. If there is any requirement later, we may use a better\n> > way to do this. And then we could safely convert the index to be\n> > `size_t` when comparing.\n> \n> Good. As we only use the \"auto\" setting with this code now, as long\n> as get_entry_index() returns a value between 0 and list->nr, the\n> lack of such range checking in the original code no longer is an\n> issue.\n> \n> Having said that, in the longer run, get_entry_index() would want to\n> return size_t simply because it is returning a value between 0 and\n> list->nr, whose type is size_t. left/mid/right variables also need\n> to become size_t and the loop initialization may have to be tweaked\n> (since the current code strangely starts left with -1 which would\n> never be the index into the array), but fixing that should probably\n> make the loop easier to read, which is a bonus.\n> \n\nThat's right. And I would clean the code a bit more.\n\n> And add_entry(), since it needs to do the usual -1-pos dance to\n> indicate where things would have been returned, would return\n> ssize_t---or better yet, it can just turned into returning size_t\n> with an extra out parameter (just like the exact_match out parameter\n> get_entry_index() has) to indicate if we already had the same item\n> in the list already. It is perfectly fine to leave it outside the\n> scope of this series, but if you are tweaking all the callers of\n> add_entry() anyway in this step, you may want to bite the bullet and\n> just go all the way.\n> \n\nI agree. I'll add more patches to clean.\n\n> Thanks.\n\nThanks for the review.\n"},{"id":"516661","messageId":"aAozhkSJXkJ_nRPs@ArchLinux","threadId":"63329","inReplyTo":"xmqqr01j3n15.fsf@gitster.g","subject":"Re: [PATCH 2/5] u-string-list: move \"test_split\" into \"u-string-list.c\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-04-24T12:50:14Z","receivedAt":"2025-04-24T12:50:05Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Tue, Apr 22, 2025 at 02:27:02PM -0700, Junio C Hamano wrote:\n> shejialuo writes:\n> \n> > We rely on \"test-tool string-list\" command to test the functionality of\n> > the \"string-list\". However, as we have introduced clar test framework,\n> > we'd better move the shell script into C program to improve speed and\n> > readability.\n> >\n> > Create a new file \"u-string-list.c\" under \"t/unit-tests\", then update\n> > the Makefile and \"meson.build\" to build the file. And let's first move\n> > \"test_split\" into unit test and gradually convert the shell script into\n> > C program.\n> >\n> > In order to create `string_list` easily by simply specifying strings in\n> > the function call, create \"t_vcreate_string_list_dup\" and\n> > \"t_create_string_list_dup\" functions to do above.\n> >\n> > Then port the shell script tests to C program and remove unused\n> > \"test-tool\" code and tests.\n> >\n> > Signed-off-by: shejialuo \n> > ---\n> \n> This is the most interesting in the u-string-list patches, as it\n> adds not just a moved test but adds supporting functions that are\n> shared with test functions added in later steps.\n> \n> > diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c\n> > new file mode 100644\n> > index 0000000000..0c148684ea\n> > --- /dev/null\n> > +++ b/t/unit-tests/u-string-list.c\n> > @@ -0,0 +1,86 @@\n> > +#include \"unit-test.h\"\n> > +#include \"string-list.h\"\n> > +\n> > +static void t_check_string_list(struct string_list *list,\n> > +\t\t\t\tstruct string_list *expected_strings)\n> > +{\n> > +\tsize_t expect_len = expected_strings->nr;\n> > +\tcl_assert_equal_i(list->nr, expect_len);\n> > +\tcl_assert(list->nr <= list->alloc);\n> > +\tfor (size_t i = 0; i < expect_len; i++)\n> > +\t\tcl_assert_equal_s(list->items[i].string,\n> > +\t\t\t\t expected_strings->items[i].string);\n> > +}\n> \n> Perhaps call it \"string_list_equal()\" or something? \"check\" is a\n> convenient name that can mean different kind of validation that is\n> not limited to \"is the actual answer identical to the expected one?\"\n> \n\nGood idea.\n\n> Wouldn't it be cleaner to read if you wrote it without an extra\n> variable expect_len? The compiler would notice repeated reference\n> of expected_strings->nr and optimize them away anyway, I would\n> imagine.\n> \n\nYeah, the compiler would definitely optimize this. Will update in the\nnext version.\n\n> > +static void t_string_list_clear(struct string_list *list, int free_util)\n> > +{\n> > +\tstring_list_clear(list, free_util);\n> > +\tcl_assert_equal_p(list->items, NULL);\n> > +\tcl_assert_equal_i(list->nr, 0);\n> > +\tcl_assert_equal_i(list->alloc, 0);\n> > +}\n> \n> Validating the result of clearing a list may be a good thing to do\n> at least once in the test suite, but this is called from many places\n> in other tests. Conceptually it feels kludgy to call this from\n> other places where they should all just call string_list_clear(),\n> like ...\n> \n\nI agree that we should not call `t_string_list_clear` in many places.\nIt's overkill.\n\n> > +static void t_vcreate_string_list_dup(struct string_list *list,\n> > +\t\t\t\t int free_util, va_list ap)\n> > +{\n> > +\tconst char *arg;\n> > +\n> > +\tcl_assert(list->strdup_strings);\n> > +\n> > +\tt_string_list_clear(list, free_util);\n> \n> ... this place.\n> \n> > +\twhile ((arg = va_arg(ap, const char *)))\n> > +\t\tstring_list_append(list, arg);\n> > +}\n> \n> To put it differently, you could be calling t_string_list_append()\n> in this loop, which would \n> \n> - remember list->nr\n> - call string_list_append()\n> - cl_assert_equal() to ensure that list->nr is one larger than\n> the value we remembered upon entry to the function.\n> \n> which is not wrong per-se, but hopefully you'd agree that it is\n> overkill. t_stirng_list_clear() is overkill in the same way.\n> \n\nYou're right. I will improve this in the next version.\n\n> > +void test_string_list__split(void)\n> > +{\n> > +\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n> > +\n> > +\tt_create_string_list_dup(&expected_strings, 0, \"foo\", \"bar\", \"baz\", NULL);\n> > +...\n> > +\tt_create_string_list_dup(&expected_strings, 0, \"\", \"\", NULL);\n> > +\tt_string_list_split(\":\", ':', -1, &expected_strings);\n> > +\n> > +\tt_string_list_clear(&expected_strings, 0);\n> > +}\n> \n> This is a good place to call t_string_list_clear(), just once in\n> this script. All other callers are conceptually simpler to call\n> string_list_clear(), as the point at their callsites is to clear\n> after themselves, not about testing string_list_clear() works\n> correctly.\n> \n> Thanks.\n\nThanks,\nJialuo\n> \n"},{"id":"516663","messageId":"aAo0PKaIrmqkuzeG@ArchLinux","threadId":"63329","inReplyTo":"aAjUmi4ccemvO7XT@pks.im","subject":"Re: [PATCH 2/5] u-string-list: move \"test_split\" into \"u-string-list.c\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-04-24T12:53:16Z","receivedAt":"2025-04-24T12:53:07Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Wed, Apr 23, 2025 at 01:52:58PM +0200, Patrick Steinhardt wrote:\n> On Tue, Apr 22, 2025 at 10:54:55PM +0800, shejialuo wrote:\n> > diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c\n> > new file mode 100644\n> > index 0000000000..0c148684ea\n> > --- /dev/null\n> > +++ b/t/unit-tests/u-string-list.c\n> > @@ -0,0 +1,86 @@\n> > +static void t_string_list_split(const char *data, int delim, int maxsplit,\n> > +\t\t\t\tstruct string_list *expected_strings)\n> > +{\n> > +\tstruct string_list list = STRING_LIST_INIT_DUP;\n> > +\tint len;\n> > +\n> > +\tlen = string_list_split(&list, data, delim, maxsplit);\n> > +\tcl_assert_equal_i(len, expected_strings->nr);\n> > +\tt_check_string_list(&list, expected_strings);\n> > +\n> > +\tt_string_list_clear(&list, 0);\n> > +}\n> > +\n> > +void test_string_list__split(void)\n> > +{\n> > +\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n> > +\n> > +\tt_create_string_list_dup(&expected_strings, 0, \"foo\", \"bar\", \"baz\", NULL);\n> > +\tt_string_list_split(\"foo:bar:baz\", ':', -1, &expected_strings);\n> \n> Could we adapt `t_string_list_split()` so that it accepts the expected\n> strings as varargs? If so we could simplify the logic in this function\n> here.\n> \n\nThat's a good suggestion. I will update in the next version.\n\n> Patrick\n"},{"id":"516665","messageId":"aAo1CvpcXYfNonTF@ArchLinux","threadId":"63329","inReplyTo":"aAjUr5AY9gTCpVlS@pks.im","subject":"Re: [PATCH 5/5] u-string-list: move \"remove duplicates\" test to \"u-string-list.c\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-04-24T12:56:42Z","receivedAt":"2025-04-24T12:56:33Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Wed, Apr 23, 2025 at 01:53:19PM +0200, Patrick Steinhardt wrote:\n> On Tue, Apr 22, 2025 at 10:55:23PM +0800, shejialuo wrote:\n> > diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c\n> > index 262b28c599..6be0cdb8e2 100644\n> > --- a/t/helper/test-string-list.c\n> > +++ b/t/helper/test-string-list.c\n> > @@ -1,48 +1,9 @@\n> > -#define DISABLE_SIGN_COMPARE_WARNINGS\n> > -\n> > #include \"test-tool.h\"\n> > #include \"strbuf.h\"\n> > #include \"string-list.h\"\n> > \n> > -/*\n> > - * Parse an argument into a string list. arg should either be a\n> > - * ':'-separated list of strings, or \"-\" to indicate an empty string\n> > - * list (as opposed to \"\", which indicates a string list containing a\n> > - * single empty string). list->strdup_strings must be set.\n> > - */\n> > -static void parse_string_list(struct string_list *list, const char *arg)\n> > -{\n> > -\tif (!strcmp(arg, \"-\"))\n> > -\t\treturn;\n> > -\n> > -\t(void)string_list_split(list, arg, ':', -1);\n> > -}\n> > -\n> > -static void write_list_compact(const struct string_list *list)\n> > -{\n> > -\tint i;\n> > -\tif (!list->nr)\n> > -\t\tprintf(\"-\\n\");\n> > -\telse {\n> > -\t\tprintf(\"%s\", list->items[0].string);\n> > -\t\tfor (i = 1; i < list->nr; i++)\n> > -\t\t\tprintf(\":%s\", list->items[i].string);\n> > -\t\tprintf(\"\\n\");\n> > -\t}\n> > -}\n> > -\n> > int cmd__string_list(int argc, const char **argv)\n> > {\n> > -\tif (argc == 3 && !strcmp(argv[1], \"remove_duplicates\")) {\n> > -\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n> > -\n> > -\t\tparse_string_list(&list, argv[2]);\n> > -\t\tstring_list_remove_duplicates(&list, 0);\n> > -\t\twrite_list_compact(&list);\n> > -\t\tstring_list_clear(&list, 0);\n> > -\t\treturn 0;\n> > -\t}\n> > -\n> > \tif (argc == 2 && !strcmp(argv[1], \"sort\")) {\n> > \t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n> > \t\tstruct strbuf sb = STRBUF_INIT;\n> \n> I'm a bit surprised that the patch series stops after this patch given\n> that the only remaining subcommand is \"sort\". Is there a specific reason\n> why you don't also convert that function? If you did we could declare\n> victory by deleting the whole \"test-helper string-list\" subcommand.\n> \n\nActually I also want to delete the whole file. However, as you have said\nas following shows:\n\n> It seems that the only reason is p0071, where we benchmark performance\n> of sorting. I dunno... that one is of course not a good fit for our unit\n> testing framework. But it's a bit sad that we cannot remove the whole\n> infra only because of a performance test that nobody ever runs in the\n> first place.\n> \n\nI think we should delete the whole file. I'll find out a way to do this.\nIt's wired that we keep the \"test-helper string-list\".\n\n> Patrick\n\nThanks,\nJialuo\n"},{"id":"518364","messageId":"aCoDB9P5XV1lHMil@ArchLinux","threadId":"63329","inReplyTo":"aAetW0dan8S3Fljq@ArchLinux","subject":"[PATCH v2 0/8] enhance \"string_list\" code and test","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-18T15:55:51Z","receivedAt":"2025-05-18T15:55:55Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"Hi All:\n\nI finally finish the version 2. And I don't provide the range-diff due\nto that I add more commits compared with version 1.\n\nThis patch could be organized into three parts:\n\n1. [PATCH v2 1/8]\n\n Fix simple sign warnings of the loop iterator.\n\n2. [PATCH v2 2/8] - [PATCH v2 4/8]\n\n Remove unncessary code, improve the logic and finally enable sign\n compare warnings check.\n\n3. [PATCH v2 5/8] - [PATCH v2 8/8]\n\n Remove test to the unit test.\n\nHowever, I want to tell Patrick a thing. I feel hard to remove the\nperformance test. So, I leave it here. The reason is that we want to\ntest performance of \"string-list\" sorting.\n\nThanks,\nJialuo\n\nshejialuo (8):\n string-list: fix sign compare warnings for loop iterator\n string-list: remove unused \"insert_at\" parameter from add_entry\n string-list: return index directly when inserting an existing element\n string-list: enable sign compare warnings check\n u-string-list: move \"test_split\" into \"u-string-list.c\"\n u-string-list: move \"test_split_in_place\" to \"u-string-list.c\"\n u-string-list: move \"filter string\" test to \"u-string-list.c\"\n u-string-list: move \"remove duplicates\" test to \"u-string-list.c\"\n\n Makefile | 1 +\n string-list.c | 48 +++-----\n t/helper/test-string-list.c | 96 ---------------\n t/meson.build | 2 +-\n t/t0063-string-list.sh | 142 ---------------------\n t/unit-tests/u-string-list.c | 233 +++++++++++++++++++++++++++++++++++\n 6 files changed, 255 insertions(+), 267 deletions(-)\n delete mode 100755 t/t0063-string-list.sh\n create mode 100644 t/unit-tests/u-string-list.c\n\n-- \n2.49.0\n\n"},{"id":"518365","messageId":"aCoDSyycHNvFCT93@ArchLinux","threadId":"63329","inReplyTo":"aCoDB9P5XV1lHMil@ArchLinux","subject":"[PATCH v2 1/8] string-list: fix sign compare warnings for loop iterator","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-18T15:56:59Z","receivedAt":"2025-05-18T15:57:04Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"In \"string-list.c\", there are six warnings which are emitted by\n\"Wsign-compare\". And five warnings are caused by the loop iterator type\nmismatch. Let's fix these five warnings by changing the `int` type to\n`size_t` type of the loop iterator.\n\nSigned-off-by: shejialuo \n---\n string-list.c | 22 ++++++++++------------\n 1 file changed, 10 insertions(+), 12 deletions(-)\n\ndiff --git a/string-list.c b/string-list.c\nindex bf061fec56..801ece0cba 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -116,9 +116,9 @@ struct string_list_item *string_list_lookup(struct string_list *list, const char\n void string_list_remove_duplicates(struct string_list *list, int free_util)\n {\n \tif (list->nr > 1) {\n-\t\tint src, dst;\n+\t\tsize_t dst = 1;\n \t\tcompare_strings_fn cmp = list->cmp ? list->cmp : strcmp;\n-\t\tfor (src = dst = 1; src < list->nr; src++) {\n+\t\tfor (size_t src = 1; src < list->nr; src++) {\n \t\t\tif (!cmp(list->items[dst - 1].string, list->items[src].string)) {\n \t\t\t\tif (list->strdup_strings)\n \t\t\t\t\tfree(list->items[src].string);\n@@ -134,8 +134,8 @@ void string_list_remove_duplicates(struct string_list *list, int free_util)\n int for_each_string_list(struct string_list *list,\n \t\t\t string_list_each_func_t fn, void *cb_data)\n {\n-\tint i, ret = 0;\n-\tfor (i = 0; i < list->nr; i++)\n+\tint ret = 0;\n+\tfor (size_t i = 0; i < list->nr; i++)\n \t\tif ((ret = fn(&list->items[i], cb_data)))\n \t\t\tbreak;\n \treturn ret;\n@@ -144,8 +144,8 @@ int for_each_string_list(struct string_list *list,\n void filter_string_list(struct string_list *list, int free_util,\n \t\t\tstring_list_each_func_t want, void *cb_data)\n {\n-\tint src, dst = 0;\n-\tfor (src = 0; src < list->nr; src++) {\n+\tsize_t dst = 0;\n+\tfor (size_t src = 0; src < list->nr; src++) {\n \t\tif (want(&list->items[src], cb_data)) {\n \t\t\tlist->items[dst++] = list->items[src];\n \t\t} else {\n@@ -171,13 +171,12 @@ void string_list_remove_empty_items(struct string_list *list, int free_util)\n void string_list_clear(struct string_list *list, int free_util)\n {\n \tif (list->items) {\n-\t\tint i;\n \t\tif (list->strdup_strings) {\n-\t\t\tfor (i = 0; i < list->nr; i++)\n+\t\t\tfor (size_t i = 0; i < list->nr; i++)\n \t\t\t\tfree(list->items[i].string);\n \t\t}\n \t\tif (free_util) {\n-\t\t\tfor (i = 0; i < list->nr; i++)\n+\t\t\tfor (size_t i = 0; i < list->nr; i++)\n \t\t\t\tfree(list->items[i].util);\n \t\t}\n \t\tfree(list->items);\n@@ -189,13 +188,12 @@ void string_list_clear(struct string_list *list, int free_util)\n void string_list_clear_func(struct string_list *list, string_list_clear_func_t clearfunc)\n {\n \tif (list->items) {\n-\t\tint i;\n \t\tif (clearfunc) {\n-\t\t\tfor (i = 0; i < list->nr; i++)\n+\t\t\tfor (size_t i = 0; i < list->nr; i++)\n \t\t\t\tclearfunc(list->items[i].util, list->items[i].string);\n \t\t}\n \t\tif (list->strdup_strings) {\n-\t\t\tfor (i = 0; i < list->nr; i++)\n+\t\t\tfor (size_t i = 0; i < list->nr; i++)\n \t\t\t\tfree(list->items[i].string);\n \t\t}\n \t\tfree(list->items);\n-- \n2.49.0\n\n"},{"id":"518366","messageId":"aCoDU46MmoGPB60b@ArchLinux","threadId":"63329","inReplyTo":"aCoDB9P5XV1lHMil@ArchLinux","subject":"[PATCH v2 2/8] string-list: remove unused \"insert_at\" parameter from add_entry","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-18T15:57:07Z","receivedAt":"2025-05-18T15:57:11Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"In \"add_entry\", we accept \"insert_at\" parameter which must be either -1\n(auto) or between 0 and `list->nr` inclusive. Any other value is\ninvalid. When caller specify any invalid \"insert_at\" value, we won't\ncheck the range and move the element, which would definitely cause the\ntrouble.\n\nHowever, we only use \"add_entry\" in \"string_list_insert\" function and we\nalways pass the \"-1\" for \"insert_at\" parameter. So, we never use this\nparameter to insert element in a user specified position. Let's delete\nthis parameter. If there is any requirement later, we need to use a\nbetter way to do this.\n\nSigned-off-by: shejialuo \n---\n string-list.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/string-list.c b/string-list.c\nindex 801ece0cba..8540c29bc9 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -41,10 +41,10 @@ static int get_entry_index(const struct string_list *list, const char *string,\n }\n \n /* returns -1-index if already exists */\n-static int add_entry(int insert_at, struct string_list *list, const char *string)\n+static int add_entry(struct string_list *list, const char *string)\n {\n \tint exact_match = 0;\n-\tint index = insert_at != -1 ? insert_at : get_entry_index(list, string, &exact_match);\n+\tint index = get_entry_index(list, string, &exact_match);\n \n \tif (exact_match)\n \t\treturn -1 - index;\n@@ -63,7 +63,7 @@ static int add_entry(int insert_at, struct string_list *list, const char *string\n \n struct string_list_item *string_list_insert(struct string_list *list, const char *string)\n {\n-\tint index = add_entry(-1, list, string);\n+\tint index = add_entry(list, string);\n \n \tif (index < 0)\n \t\tindex = -1 - index;\n-- \n2.49.0\n\n"},{"id":"518367","messageId":"aCoDW8CcWeq8T9hp@ArchLinux","threadId":"63329","inReplyTo":"aCoDB9P5XV1lHMil@ArchLinux","subject":"[PATCH v2 3/8] string-list: return index directly when inserting an existing element","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-18T15:57:15Z","receivedAt":"2025-05-18T15:57:19Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"When inserting an existing element, \"add_entry\" would convert \"index\"\nvalue to \"-1-index\" to indicate the caller that this element is in the\nlist already.\n\nHowever, in \"string_list_insert\", we would simply convert this to the\noriginal positive index without any further action. Let's directly\nreturn the index as we don't care about whether the element is in the\nlist by using \"add_entry\".\n\nIn the future, if we want to let \"add_entry\" tell the caller, we may add\n\"int *exact_match\" parameter to \"add_entry\" instead of converting the\nindex to negative to indicate.\n\nSigned-off-by: shejialuo \n---\n string-list.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/string-list.c b/string-list.c\nindex 8540c29bc9..171cef5dbb 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -40,14 +40,13 @@ static int get_entry_index(const struct string_list *list, const char *string,\n \treturn right;\n }\n \n-/* returns -1-index if already exists */\n static int add_entry(struct string_list *list, const char *string)\n {\n \tint exact_match = 0;\n \tint index = get_entry_index(list, string, &exact_match);\n \n \tif (exact_match)\n-\t\treturn -1 - index;\n+\t\treturn index;\n \n \tALLOC_GROW(list->items, list->nr+1, list->alloc);\n \tif (index < list->nr)\n@@ -65,9 +64,6 @@ struct string_list_item *string_list_insert(struct string_list *list, const char\n {\n \tint index = add_entry(list, string);\n \n-\tif (index < 0)\n-\t\tindex = -1 - index;\n-\n \treturn list->items + index;\n }\n \n-- \n2.49.0\n\n"},{"id":"518368","messageId":"aCoDY4A62uWb-_MV@ArchLinux","threadId":"63329","inReplyTo":"aCoDB9P5XV1lHMil@ArchLinux","subject":"[PATCH v2 4/8] string-list: enable sign compare warnings check","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-18T15:57:23Z","receivedAt":"2025-05-18T15:57:27Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"The only sign compare warning in \"string-list\" is that we compare the\n`index` of the `int` type with the `list->nr` of unsigned type. We get\nindex by calling \"get_entry_index\", which would always return unsigned\nindex.\n\nLet's change the return type of \"get_entry_index\" to be \"size_t\" by\nslightly modifying the binary search algorithm. Instead of letting\n\"left\" to be \"-1\" initially, assign 0 to it.\n\nThen, we could delete \"#define DISABLE_SIGN_COMPARE_WARNING\" to enable\nsign warnings check for \"string-list\"\n\nSigned-off-by: shejialuo \n---\n string-list.c | 20 +++++++++-----------\n 1 file changed, 9 insertions(+), 11 deletions(-)\n\ndiff --git a/string-list.c b/string-list.c\nindex 171cef5dbb..53faaa8420 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -1,5 +1,3 @@\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n-\n #include \"git-compat-util.h\"\n #include \"string-list.h\"\n \n@@ -17,19 +15,19 @@ void string_list_init_dup(struct string_list *list)\n \n /* if there is no exact match, point to the index where the entry could be\n * inserted */\n-static int get_entry_index(const struct string_list *list, const char *string,\n-\t\tint *exact_match)\n+static size_t get_entry_index(const struct string_list *list, const char *string,\n+\t\t\t int *exact_match)\n {\n-\tint left = -1, right = list->nr;\n+\tsize_t left = 0, right = list->nr;\n \tcompare_strings_fn cmp = list->cmp ? list->cmp : strcmp;\n \n-\twhile (left + 1 < right) {\n-\t\tint middle = left + (right - left) / 2;\n+\twhile (left < right) {\n+\t\tsize_t middle = left + (right - left) / 2;\n \t\tint compare = cmp(string, list->items[middle].string);\n \t\tif (compare < 0)\n \t\t\tright = middle;\n \t\telse if (compare > 0)\n-\t\t\tleft = middle;\n+\t\t\tleft = middle + 1;\n \t\telse {\n \t\t\t*exact_match = 1;\n \t\t\treturn middle;\n@@ -40,10 +38,10 @@ static int get_entry_index(const struct string_list *list, const char *string,\n \treturn right;\n }\n \n-static int add_entry(struct string_list *list, const char *string)\n+static size_t add_entry(struct string_list *list, const char *string)\n {\n \tint exact_match = 0;\n-\tint index = get_entry_index(list, string, &exact_match);\n+\tsize_t index = get_entry_index(list, string, &exact_match);\n \n \tif (exact_match)\n \t\treturn index;\n@@ -62,7 +60,7 @@ static int add_entry(struct string_list *list, const char *string)\n \n struct string_list_item *string_list_insert(struct string_list *list, const char *string)\n {\n-\tint index = add_entry(list, string);\n+\tsize_t index = add_entry(list, string);\n \n \treturn list->items + index;\n }\n-- \n2.49.0\n\n"},{"id":"518369","messageId":"aCoDbRCQSjAXV7H1@ArchLinux","threadId":"63329","inReplyTo":"aCoDB9P5XV1lHMil@ArchLinux","subject":"[PATCH v2 5/8] u-string-list: move \"test_split\" into \"u-string-list.c\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-18T15:57:33Z","receivedAt":"2025-05-18T15:57:37Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"We rely on \"test-tool string-list\" command to test the functionality of\nthe \"string-list\". However, as we have introduced clar test framework,\nwe'd better move the shell script into C program to improve speed and\nreadability.\n\nCreate a new file \"u-string-list.c\" under \"t/unit-tests\", then update\nthe Makefile and \"meson.build\" to build the file. And let's first move\n\"test_split\" into unit test and gradually convert the shell script into\nC program.\n\nIn order to create `string_list` easily by simply specifying strings in\nthe function call, create \"t_vcreate_string_list_dup\" function to do\nthis.\n\nThen port the shell script tests to C program and remove unused\n\"test-tool\" code and tests.\n\nSigned-off-by: shejialuo \n---\n Makefile | 1 +\n t/helper/test-string-list.c | 14 --------\n t/meson.build | 1 +\n t/t0063-string-list.sh | 53 -----------------------------\n t/unit-tests/u-string-list.c | 66 ++++++++++++++++++++++++++++++++++++\n 5 files changed, 68 insertions(+), 67 deletions(-)\n create mode 100644 t/unit-tests/u-string-list.c\n\ndiff --git a/Makefile b/Makefile\nindex de73c6ddcd..cdffa13aba 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1366,6 +1366,7 @@ CLAR_TEST_SUITES += u-prio-queue\n CLAR_TEST_SUITES += u-reftable-tree\n CLAR_TEST_SUITES += u-strbuf\n CLAR_TEST_SUITES += u-strcmp-offset\n+CLAR_TEST_SUITES += u-string-list\n CLAR_TEST_SUITES += u-strvec\n CLAR_TEST_SUITES += u-trailer\n CLAR_TEST_SUITES += u-urlmatch-normalization\ndiff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c\nindex 6f10c5a435..17c18c30f6 100644\n--- a/t/helper/test-string-list.c\n+++ b/t/helper/test-string-list.c\n@@ -46,20 +46,6 @@ static int prefix_cb(struct string_list_item *item, void *cb_data)\n \n int cmd__string_list(int argc, const char **argv)\n {\n-\tif (argc == 5 && !strcmp(argv[1], \"split\")) {\n-\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n-\t\tint i;\n-\t\tconst char *s = argv[2];\n-\t\tint delim = *argv[3];\n-\t\tint maxsplit = atoi(argv[4]);\n-\n-\t\ti = string_list_split(&list, s, delim, maxsplit);\n-\t\tprintf(\"%d\\n\", i);\n-\t\twrite_list(&list);\n-\t\tstring_list_clear(&list, 0);\n-\t\treturn 0;\n-\t}\n-\n \tif (argc == 5 && !strcmp(argv[1], \"split_in_place\")) {\n \t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n \t\tint i;\ndiff --git a/t/meson.build b/t/meson.build\nindex fcfc1c2c2b..a3dbe572d8 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -11,6 +11,7 @@ clar_test_suites = [\n 'unit-tests/u-reftable-tree.c',\n 'unit-tests/u-strbuf.c',\n 'unit-tests/u-strcmp-offset.c',\n+ 'unit-tests/u-string-list.c',\n 'unit-tests/u-strvec.c',\n 'unit-tests/u-trailer.c',\n 'unit-tests/u-urlmatch-normalization.c',\ndiff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\nindex aac63ba506..6b20ffd206 100755\n--- a/t/t0063-string-list.sh\n+++ b/t/t0063-string-list.sh\n@@ -7,16 +7,6 @@ test_description='Test string list functionality'\n \n . ./test-lib.sh\n \n-test_split () {\n-\tcat >expected &&\n-\ttest_expect_success \"split $1 at $2, max $3\" \"\n-\t\ttest-tool string-list split '$1' '$2' '$3' >actual &&\n-\t\ttest_cmp expected actual &&\n-\t\ttest-tool string-list split_in_place '$1' '$2' '$3' >actual &&\n-\t\ttest_cmp expected actual\n-\t\"\n-}\n-\n test_split_in_place() {\n \tcat >expected &&\n \ttest_expect_success \"split (in place) $1 at $2, max $3\" \"\n@@ -25,49 +15,6 @@ test_split_in_place() {\n \t\"\n }\n \n-test_split \"foo:bar:baz\" \":\" \"-1\" <strdup_strings);\n+\n+\tstring_list_clear(list, free_util);\n+\twhile ((arg = va_arg(ap, const char *)))\n+\t\tstring_list_append(list, arg);\n+}\n+\n+static void t_string_list_clear(struct string_list *list, int free_util)\n+{\n+\tstring_list_clear(list, free_util);\n+\tcl_assert_equal_p(list->items, NULL);\n+\tcl_assert_equal_i(list->nr, 0);\n+\tcl_assert_equal_i(list->alloc, 0);\n+}\n+\n+static void t_string_list_equal(struct string_list *list,\n+\t\t\t\tstruct string_list *expected_strings)\n+{\n+\tcl_assert_equal_i(list->nr, expected_strings->nr);\n+\tcl_assert(list->nr <= list->alloc);\n+\tfor (size_t i = 0; i < expected_strings->nr; i++)\n+\t\tcl_assert_equal_s(list->items[i].string,\n+\t\t\t\t expected_strings->items[i].string);\n+}\n+\n+static void t_string_list_split(struct string_list *list, const char *data,\n+\t\t\t\tint delim, int maxsplit, ...)\n+{\n+\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n+\tva_list ap;\n+\tint len;\n+\n+\tva_start(ap, maxsplit);\n+\tt_vcreate_string_list_dup(&expected_strings, 0, ap);\n+\tva_end(ap);\n+\n+\tstring_list_clear(list, 0);\n+\tlen = string_list_split(list, data, delim, maxsplit);\n+\tcl_assert_equal_i(len, expected_strings.nr);\n+\tt_string_list_equal(list, &expected_strings);\n+\n+\tstring_list_clear(&expected_strings, 0);\n+}\n+\n+void test_string_list__split(void)\n+{\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\n+\tt_string_list_split(&list, \"foo:bar:baz\", ':', -1, \"foo\", \"bar\", \"baz\", NULL);\n+\tt_string_list_split(&list, \"foo:bar:baz\", ':', 0, \"foo:bar:baz\", NULL);\n+\tt_string_list_split(&list, \"foo:bar:baz\", ':', 1, \"foo\", \"bar:baz\", NULL);\n+\tt_string_list_split(&list, \"foo:bar:baz\", ':', 2, \"foo\", \"bar\", \"baz\", NULL);\n+\tt_string_list_split(&list, \"foo:bar:\", ':', -1, \"foo\", \"bar\", \"\", NULL);\n+\tt_string_list_split(&list, \"\", ':', -1, \"\", NULL);\n+\tt_string_list_split(&list, \":\", ':', -1, \"\", \"\", NULL);\n+\n+\tt_string_list_clear(&list, 0);\n+}\n-- \n2.49.0\n\n"},{"id":"518370","messageId":"aCoDdf5IS3jkwpjl@ArchLinux","threadId":"63329","inReplyTo":"aCoDB9P5XV1lHMil@ArchLinux","subject":"[PATCH v2 6/8] u-string-list: move \"test_split_in_place\" to \"u-string-list.c\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-18T15:57:41Z","receivedAt":"2025-05-18T15:57:45Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"We use \"test-tool string-list split_in_place\" to test the\n\"string_list_split_in_place\" function. As we have introduced the unit\ntest, we'd better remove the logic from shell script to C program to\nimprove test speed and readability.\n\nSigned-off-by: shejialuo \n---\n t/helper/test-string-list.c | 22 ----------------\n t/t0063-string-list.sh | 51 ------------------------------------\n t/unit-tests/u-string-list.c | 39 +++++++++++++++++++++++++++\n 3 files changed, 39 insertions(+), 73 deletions(-)\n\ndiff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c\nindex 17c18c30f6..8a344347ad 100644\n--- a/t/helper/test-string-list.c\n+++ b/t/helper/test-string-list.c\n@@ -18,13 +18,6 @@ static void parse_string_list(struct string_list *list, const char *arg)\n \t(void)string_list_split(list, arg, ':', -1);\n }\n \n-static void write_list(const struct string_list *list)\n-{\n-\tint i;\n-\tfor (i = 0; i < list->nr; i++)\n-\t\tprintf(\"[%d]: \\\"%s\\\"\\n\", i, list->items[i].string);\n-}\n-\n static void write_list_compact(const struct string_list *list)\n {\n \tint i;\n@@ -46,21 +39,6 @@ static int prefix_cb(struct string_list_item *item, void *cb_data)\n \n int cmd__string_list(int argc, const char **argv)\n {\n-\tif (argc == 5 && !strcmp(argv[1], \"split_in_place\")) {\n-\t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n-\t\tint i;\n-\t\tchar *s = xstrdup(argv[2]);\n-\t\tconst char *delim = argv[3];\n-\t\tint maxsplit = atoi(argv[4]);\n-\n-\t\ti = string_list_split_in_place(&list, s, delim, maxsplit);\n-\t\tprintf(\"%d\\n\", i);\n-\t\twrite_list(&list);\n-\t\tstring_list_clear(&list, 0);\n-\t\tfree(s);\n-\t\treturn 0;\n-\t}\n-\n \tif (argc == 4 && !strcmp(argv[1], \"filter\")) {\n \t\t/*\n \t\t * Retain only the items that have the specified prefix.\ndiff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\nindex 6b20ffd206..1a9cf8bfcf 100755\n--- a/t/t0063-string-list.sh\n+++ b/t/t0063-string-list.sh\n@@ -7,57 +7,6 @@ test_description='Test string list functionality'\n \n . ./test-lib.sh\n \n-test_split_in_place() {\n-\tcat >expected &&\n-\ttest_expect_success \"split (in place) $1 at $2, max $3\" \"\n-\t\ttest-tool string-list split_in_place '$1' '$2' '$3' >actual &&\n-\t\ttest_cmp expected actual\n-\t\"\n-}\n-\n-test_split_in_place \"foo:;:bar:;:baz:;:\" \":;\" \"-1\" <\n---\n t/helper/test-string-list.c | 21 ------------\n t/t0063-string-list.sh | 11 ------\n t/unit-tests/u-string-list.c | 66 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 66 insertions(+), 32 deletions(-)\n\ndiff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c\nindex 8a344347ad..262b28c599 100644\n--- a/t/helper/test-string-list.c\n+++ b/t/helper/test-string-list.c\n@@ -31,29 +31,8 @@ static void write_list_compact(const struct string_list *list)\n \t}\n }\n \n-static int prefix_cb(struct string_list_item *item, void *cb_data)\n-{\n-\tconst char *prefix = (const char *)cb_data;\n-\treturn starts_with(item->string, prefix);\n-}\n-\n int cmd__string_list(int argc, const char **argv)\n {\n-\tif (argc == 4 && !strcmp(argv[1], \"filter\")) {\n-\t\t/*\n-\t\t * Retain only the items that have the specified prefix.\n-\t\t * Arguments: list|- prefix\n-\t\t */\n-\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n-\t\tconst char *prefix = argv[3];\n-\n-\t\tparse_string_list(&list, argv[2]);\n-\t\tfilter_string_list(&list, 0, prefix_cb, (void *)prefix);\n-\t\twrite_list_compact(&list);\n-\t\tstring_list_clear(&list, 0);\n-\t\treturn 0;\n-\t}\n-\n \tif (argc == 3 && !strcmp(argv[1], \"remove_duplicates\")) {\n \t\tstruct string_list list = STRING_LIST_INIT_DUP;\n \ndiff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\nindex 1a9cf8bfcf..31fd62bba8 100755\n--- a/t/t0063-string-list.sh\n+++ b/t/t0063-string-list.sh\n@@ -7,17 +7,6 @@ test_description='Test string list functionality'\n \n . ./test-lib.sh\n \n-test_expect_success \"test filter_string_list\" '\n-\ttest \"x-\" = \"x$(test-tool string-list filter - y)\" &&\n-\ttest \"x-\" = \"x$(test-tool string-list filter no y)\" &&\n-\ttest yes = \"$(test-tool string-list filter yes y)\" &&\n-\ttest yes = \"$(test-tool string-list filter no:yes y)\" &&\n-\ttest yes = \"$(test-tool string-list filter yes:no y)\" &&\n-\ttest y1:y2 = \"$(test-tool string-list filter y1:y2 y)\" &&\n-\ttest y2:y1 = \"$(test-tool string-list filter y2:y1 y)\" &&\n-\ttest \"x-\" = \"x$(test-tool string-list filter x1:x2 y)\"\n-'\n-\n test_expect_success \"test remove_duplicates\" '\n \ttest \"x-\" = \"x$(test-tool string-list remove_duplicates -)\" &&\n \ttest \"x\" = \"x$(test-tool string-list remove_duplicates \"\")\" &&\ndiff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c\nindex e4b8e38fb8..be2bb5f103 100644\n--- a/t/unit-tests/u-string-list.c\n+++ b/t/unit-tests/u-string-list.c\n@@ -13,6 +13,18 @@ static void t_vcreate_string_list_dup(struct string_list *list,\n \t\tstring_list_append(list, arg);\n }\n \n+static void t_create_string_list_dup(struct string_list *list, int free_util, ...)\n+{\n+\tva_list ap;\n+\n+\tcl_assert(list->strdup_strings);\n+\n+\tstring_list_clear(list, free_util);\n+\tva_start(ap, free_util);\n+\tt_vcreate_string_list_dup(list, free_util, ap);\n+\tva_end(ap);\n+}\n+\n static void t_string_list_clear(struct string_list *list, int free_util)\n {\n \tstring_list_clear(list, free_util);\n@@ -103,3 +115,57 @@ void test_string_list__split_in_place(void)\n \n \tt_string_list_clear(&list, 0);\n }\n+\n+static int prefix_cb(struct string_list_item *item, void *cb_data)\n+{\n+\tconst char *prefix = (const char *)cb_data;\n+\treturn starts_with(item->string, prefix);\n+}\n+\n+static void t_string_list_filter(struct string_list *list,\n+\t\t\t\t string_list_each_func_t want, void *cb_data, ...)\n+{\n+\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n+\tva_list ap;\n+\n+\tva_start(ap, cb_data);\n+\tt_vcreate_string_list_dup(&expected_strings, 0, ap);\n+\tva_end(ap);\n+\n+\tfilter_string_list(list, 0, want, cb_data);\n+\tt_string_list_equal(list, &expected_strings);\n+\n+\tstring_list_clear(&expected_strings, 0);\n+}\n+\n+void test_string_list__filter(void)\n+{\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\tconst char *prefix = \"y\";\n+\n+\tt_create_string_list_dup(&list, 0, NULL);\n+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"no\", NULL);\n+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"yes\", NULL);\n+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, \"yes\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"no\", \"yes\", NULL);\n+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, \"yes\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"yes\", \"no\", NULL);\n+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, \"yes\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"y1\", \"y2\", NULL);\n+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, \"y1\", \"y2\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"y2\", \"y1\", NULL);\n+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, \"y2\", \"y1\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"x1\", \"x2\", NULL);\n+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);\n+\n+\tt_string_list_clear(&list, 0);\n+}\n-- \n2.49.0\n\n"},{"id":"518372","messageId":"aCoDobl95P_VSaes@ArchLinux","threadId":"63329","inReplyTo":"aCoDB9P5XV1lHMil@ArchLinux","subject":"[PATCH v2 8/8] u-string-list: move \"remove duplicates\" test to \"u-string-list.c\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-18T15:58:25Z","receivedAt":"2025-05-18T15:58:29Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"We use \"test-tool string-list remove_duplicates\" to test the\n\"string_list_remove_duplicates\" function. As we have introduced the unit\ntest, we'd better remove the logic from shell script to C program to\nimprove test speed and readability.\n\nAs all the tests in shell script are removed, let's just delete the\n\"t0063-string-list.sh\" and update the \"meson.build\" file to align with\nthis change.\n\nAlso we could simply remove \"DISABLE_SIGN_COMPARE_WARNINGS\" due to we\nhave already deleted related code.\n\nSigned-off-by: shejialuo \n---\n t/helper/test-string-list.c | 39 -----------------------\n t/meson.build | 1 -\n t/t0063-string-list.sh | 27 ----------------\n t/unit-tests/u-string-list.c | 62 ++++++++++++++++++++++++++++++++++++\n 4 files changed, 62 insertions(+), 67 deletions(-)\n delete mode 100755 t/t0063-string-list.sh\n\ndiff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c\nindex 262b28c599..6be0cdb8e2 100644\n--- a/t/helper/test-string-list.c\n+++ b/t/helper/test-string-list.c\n@@ -1,48 +1,9 @@\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n-\n #include \"test-tool.h\"\n #include \"strbuf.h\"\n #include \"string-list.h\"\n \n-/*\n- * Parse an argument into a string list. arg should either be a\n- * ':'-separated list of strings, or \"-\" to indicate an empty string\n- * list (as opposed to \"\", which indicates a string list containing a\n- * single empty string). list->strdup_strings must be set.\n- */\n-static void parse_string_list(struct string_list *list, const char *arg)\n-{\n-\tif (!strcmp(arg, \"-\"))\n-\t\treturn;\n-\n-\t(void)string_list_split(list, arg, ':', -1);\n-}\n-\n-static void write_list_compact(const struct string_list *list)\n-{\n-\tint i;\n-\tif (!list->nr)\n-\t\tprintf(\"-\\n\");\n-\telse {\n-\t\tprintf(\"%s\", list->items[0].string);\n-\t\tfor (i = 1; i < list->nr; i++)\n-\t\t\tprintf(\":%s\", list->items[i].string);\n-\t\tprintf(\"\\n\");\n-\t}\n-}\n-\n int cmd__string_list(int argc, const char **argv)\n {\n-\tif (argc == 3 && !strcmp(argv[1], \"remove_duplicates\")) {\n-\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n-\n-\t\tparse_string_list(&list, argv[2]);\n-\t\tstring_list_remove_duplicates(&list, 0);\n-\t\twrite_list_compact(&list);\n-\t\tstring_list_clear(&list, 0);\n-\t\treturn 0;\n-\t}\n-\n \tif (argc == 2 && !strcmp(argv[1], \"sort\")) {\n \t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n \t\tstruct strbuf sb = STRBUF_INIT;\ndiff --git a/t/meson.build b/t/meson.build\nindex a3dbe572d8..c7efcc6db9 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -124,7 +124,6 @@ integration_tests = [\n 't0060-path-utils.sh',\n 't0061-run-command.sh',\n 't0062-revision-walking.sh',\n- 't0063-string-list.sh',\n 't0066-dir-iterator.sh',\n 't0067-parse_pathspec_file.sh',\n 't0068-for-each-repo.sh',\ndiff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\ndeleted file mode 100755\nindex 31fd62bba8..0000000000\n--- a/t/t0063-string-list.sh\n+++ /dev/null\n@@ -1,27 +0,0 @@\n-#!/bin/sh\n-#\n-# Copyright (c) 2012 Michael Haggerty\n-#\n-\n-test_description='Test string list functionality'\n-\n-. ./test-lib.sh\n-\n-test_expect_success \"test remove_duplicates\" '\n-\ttest \"x-\" = \"x$(test-tool string-list remove_duplicates -)\" &&\n-\ttest \"x\" = \"x$(test-tool string-list remove_duplicates \"\")\" &&\n-\ttest a = \"$(test-tool string-list remove_duplicates a)\" &&\n-\ttest a = \"$(test-tool string-list remove_duplicates a:a)\" &&\n-\ttest a = \"$(test-tool string-list remove_duplicates a:a:a:a:a)\" &&\n-\ttest a:b = \"$(test-tool string-list remove_duplicates a:b)\" &&\n-\ttest a:b = \"$(test-tool string-list remove_duplicates a:a:b)\" &&\n-\ttest a:b = \"$(test-tool string-list remove_duplicates a:b:b)\" &&\n-\ttest a:b:c = \"$(test-tool string-list remove_duplicates a:b:c)\" &&\n-\ttest a:b:c = \"$(test-tool string-list remove_duplicates a:a:b:c)\" &&\n-\ttest a:b:c = \"$(test-tool string-list remove_duplicates a:b:b:c)\" &&\n-\ttest a:b:c = \"$(test-tool string-list remove_duplicates a:b:c:c)\" &&\n-\ttest a:b:c = \"$(test-tool string-list remove_duplicates a:a:b:b:c:c)\" &&\n-\ttest a:b:c = \"$(test-tool string-list remove_duplicates a:a:a:b:b:b:c:c:c)\"\n-'\n-\n-test_done\ndiff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c\nindex be2bb5f103..a575fdda97 100644\n--- a/t/unit-tests/u-string-list.c\n+++ b/t/unit-tests/u-string-list.c\n@@ -169,3 +169,65 @@ void test_string_list__filter(void)\n \n \tt_string_list_clear(&list, 0);\n }\n+\n+static void t_string_list_remove_duplicates(struct string_list *list, ...)\n+{\n+\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n+\tva_list ap;\n+\n+\tva_start(ap, list);\n+\tt_vcreate_string_list_dup(&expected_strings, 0, ap);\n+\tva_end(ap);\n+\n+\tstring_list_remove_duplicates(list, 0);\n+\tt_string_list_equal(list, &expected_strings);\n+\n+\tstring_list_clear(&expected_strings, 0);\n+}\n+\n+void test_string_list__remove_duplicates(void)\n+{\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\n+\tt_create_string_list_dup(&list, 0, NULL);\n+\tt_string_list_remove_duplicates(&list, NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"a\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"a\", \"a\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"a\", \"b\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", \"b\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"b\", \"b\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", \"b\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"b\", \"c\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", \"b\", \"c\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"a\", \"b\", \"c\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", \"b\", \"c\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"b\", \"b\", \"c\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", \"b\", \"c\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"b\", \"c\", \"c\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", \"b\", \"c\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"a\", \"b\", \"b\", \"c\", \"c\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", \"b\", \"c\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"a\", \"a\", \"b\", \"b\", \"b\",\n+\t\t\t\t \"c\", \"c\", \"c\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", \"b\", \"c\", NULL);\n+\n+\tt_string_list_clear(&list, 0);\n+}\n-- \n2.49.0\n\n"},{"id":"518382","messageId":"aCrbG0lavNa9Plc5@pks.im","threadId":"63329","inReplyTo":"aCoDSyycHNvFCT93@ArchLinux","subject":"Re: [PATCH v2 1/8] string-list: fix sign compare warnings for loop iterator","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-19T07:17:47Z","receivedAt":"2025-05-19T07:17:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, May 18, 2025 at 11:56:59PM +0800, shejialuo wrote:\n> In \"string-list.c\", there are six warnings which are emitted by\n> \"Wsign-compare\". And five warnings are caused by the loop iterator type\n> mismatch. Let's fix these five warnings by changing the `int` type to\n> `size_t` type of the loop iterator.\n\nThis naturally causes the question what the 6th warning is, and why it's\nnot fixed in this commit. The answer is that the last one is more\ncomplex and handled by subsequent patches, which you should probably\npoint out here. E.g.:\n\n There are a couple of \"-Wsign-compare\" warnings in \"string-list.c\".\n Fix trivial ones that result from a mismatched loop iterator type.\n\n There is a single warning left after these fixes. This warning needs\n a bit more care and is thus handled in subsequent commits.\n\n> Signed-off-by: shejialuo \n> ---\n> string-list.c | 22 ++++++++++------------\n> 1 file changed, 10 insertions(+), 12 deletions(-)\n\nAll of these look obviously good to me, thanks!\n\nPatrick\n"},{"id":"518383","messageId":"aCrbIbB8DDw0eeae@pks.im","threadId":"63329","inReplyTo":"aCoDU46MmoGPB60b@ArchLinux","subject":"Re: [PATCH v2 2/8] string-list: remove unused \"insert_at\" parameter from add_entry","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-19T07:17:53Z","receivedAt":"2025-05-19T07:17:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, May 18, 2025 at 11:57:07PM +0800, shejialuo wrote:\n> In \"add_entry\", we accept \"insert_at\" parameter which must be either -1\n> (auto) or between 0 and `list->nr` inclusive. Any other value is\n> invalid. When caller specify any invalid \"insert_at\" value, we won't\n> check the range and move the element, which would definitely cause the\n> trouble.\n\nMaybe \"which may easily cause an out-of-bounds write\" instead of vague\n\"trouble\"?\n\n> However, we only use \"add_entry\" in \"string_list_insert\" function and we\n> always pass the \"-1\" for \"insert_at\" parameter. So, we never use this\n> parameter to insert element in a user specified position. Let's delete\n> this parameter. If there is any requirement later, we need to use a\n> better way to do this.\n\nMakes sense.\n\nPatrick\n"},{"id":"518384","messageId":"aCrbJcUMl554xMUg@pks.im","threadId":"63329","inReplyTo":"aCoDbRCQSjAXV7H1@ArchLinux","subject":"Re: [PATCH v2 5/8] u-string-list: move \"test_split\" into \"u-string-list.c\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-19T07:17:57Z","receivedAt":"2025-05-19T07:18:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, May 18, 2025 at 11:57:33PM +0800, shejialuo wrote:\n> diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c\n> new file mode 100644\n> index 0000000000..c304934de2\n> --- /dev/null\n> +++ b/t/unit-tests/u-string-list.c\n> @@ -0,0 +1,66 @@\n> +#include \"unit-test.h\"\n> +#include \"string-list.h\"\n> +\n> +static void t_vcreate_string_list_dup(struct string_list *list,\n> +\t\t\t\t int free_util, va_list ap)\n> +{\n> +\tconst char *arg;\n> +\n> +\tcl_assert(list->strdup_strings);\n> +\n> +\tstring_list_clear(list, free_util);\n> +\twhile ((arg = va_arg(ap, const char *)))\n> +\t\tstring_list_append(list, arg);\n> +}\n> +\n> +static void t_string_list_clear(struct string_list *list, int free_util)\n> +{\n> +\tstring_list_clear(list, free_util);\n> +\tcl_assert_equal_p(list->items, NULL);\n> +\tcl_assert_equal_i(list->nr, 0);\n> +\tcl_assert_equal_i(list->alloc, 0);\n> +}\n> +\n> +static void t_string_list_equal(struct string_list *list,\n> +\t\t\t\tstruct string_list *expected_strings)\n> +{\n> +\tcl_assert_equal_i(list->nr, expected_strings->nr);\n> +\tcl_assert(list->nr <= list->alloc);\n> +\tfor (size_t i = 0; i < expected_strings->nr; i++)\n> +\t\tcl_assert_equal_s(list->items[i].string,\n> +\t\t\t\t expected_strings->items[i].string);\n> +}\n> +\n> +static void t_string_list_split(struct string_list *list, const char *data,\n> +\t\t\t\tint delim, int maxsplit, ...)\n> +{\n> +\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n> +\tva_list ap;\n> +\tint len;\n> +\n> +\tva_start(ap, maxsplit);\n> +\tt_vcreate_string_list_dup(&expected_strings, 0, ap);\n> +\tva_end(ap);\n> +\n> +\tstring_list_clear(list, 0);\n> +\tlen = string_list_split(list, data, delim, maxsplit);\n> +\tcl_assert_equal_i(len, expected_strings.nr);\n> +\tt_string_list_equal(list, &expected_strings);\n> +\n> +\tstring_list_clear(&expected_strings, 0);\n> +}\n> +\n> +void test_string_list__split(void)\n> +{\n> +\tstruct string_list list = STRING_LIST_INIT_DUP;\n\nLet's move this list into `t_string_list_split()`. Otherwise, tests may\nnegatively impact one another via this shared state. The same comment\nalso applies to subsequent commits.\n\n> +\tt_string_list_split(&list, \"foo:bar:baz\", ':', -1, \"foo\", \"bar\", \"baz\", NULL);\n> +\tt_string_list_split(&list, \"foo:bar:baz\", ':', 0, \"foo:bar:baz\", NULL);\n> +\tt_string_list_split(&list, \"foo:bar:baz\", ':', 1, \"foo\", \"bar:baz\", NULL);\n> +\tt_string_list_split(&list, \"foo:bar:baz\", ':', 2, \"foo\", \"bar\", \"baz\", NULL);\n> +\tt_string_list_split(&list, \"foo:bar:\", ':', -1, \"foo\", \"bar\", \"\", NULL);\n> +\tt_string_list_split(&list, \"\", ':', -1, \"\", NULL);\n> +\tt_string_list_split(&list, \":\", ':', -1, \"\", \"\", NULL);\n> +\n> +\tt_string_list_clear(&list, 0);\n\nPatrick\n"},{"id":"518385","messageId":"aCrbKI_c5Bma8ZDQ@pks.im","threadId":"63329","inReplyTo":"aCoDW8CcWeq8T9hp@ArchLinux","subject":"Re: [PATCH v2 3/8] string-list: return index directly when inserting an existing element","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-19T07:18:00Z","receivedAt":"2025-05-19T07:18:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, May 18, 2025 at 11:57:15PM +0800, shejialuo wrote:\n> When inserting an existing element, \"add_entry\" would convert \"index\"\n> value to \"-1-index\" to indicate the caller that this element is in the\n> list already.\n> \n> However, in \"string_list_insert\", we would simply convert this to the\n> original positive index without any further action. Let's directly\n> return the index as we don't care about whether the element is in the\n> list by using \"add_entry\".\n> \n> In the future, if we want to let \"add_entry\" tell the caller, we may add\n> \"int *exact_match\" parameter to \"add_entry\" instead of converting the\n> index to negative to indicate.\n> \n> Signed-off-by: shejialuo \n> ---\n> string-list.c | 6 +-----\n> 1 file changed, 1 insertion(+), 5 deletions(-)\n> \n> diff --git a/string-list.c b/string-list.c\n> index 8540c29bc9..171cef5dbb 100644\n> --- a/string-list.c\n> +++ b/string-list.c\n> @@ -40,14 +40,13 @@ static int get_entry_index(const struct string_list *list, const char *string,\n> \treturn right;\n> }\n> \n> -/* returns -1-index if already exists */\n> static int add_entry(struct string_list *list, const char *string)\n> {\n> \tint exact_match = 0;\n> \tint index = get_entry_index(list, string, &exact_match);\n> \n> \tif (exact_match)\n> -\t\treturn -1 - index;\n> +\t\treturn index;\n> \n> \tALLOC_GROW(list->items, list->nr+1, list->alloc);\n> \tif (index < list->nr)\n\nOkay, let's assume that \"index == 2\" here and we have an exact match.\nWe'd thus return `-1 - 2 == -3`.\n\n> @@ -65,9 +64,6 @@ struct string_list_item *string_list_insert(struct string_list *list, const char\n> {\n> \tint index = add_entry(list, string);\n> \n> -\tif (index < 0)\n> -\t\tindex = -1 - index;\n> -\n> \treturn list->items + index;\n> }\n\nSo we'd now realize that `index < 0` and thus calculate `-1 - -3 == 2`,\nwhich is the original index indeed. So this is a nice simplification\nthat retains the original behaviour indeed.\n\nI think we could simplify the code even further by inlining\n`get_entry_index()` now that `string_list_insert()` is a trivial wrapper\naround it. But I'll leave it up to you whether we want to do it or not.\n\nPatrick\n"},{"id":"518386","messageId":"aCrbKz6tr0vj7ytY@pks.im","threadId":"63329","inReplyTo":"aCoDY4A62uWb-_MV@ArchLinux","subject":"Re: [PATCH v2 4/8] string-list: enable sign compare warnings check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-19T07:18:03Z","receivedAt":"2025-05-19T07:18:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, May 18, 2025 at 11:57:23PM +0800, shejialuo wrote:\n> The only sign compare warning in \"string-list\" is that we compare the\n> `index` of the `int` type with the `list->nr` of unsigned type. We get\n> index by calling \"get_entry_index\", which would always return unsigned\n> index.\n> \n> Let's change the return type of \"get_entry_index\" to be \"size_t\" by\n> slightly modifying the binary search algorithm. Instead of letting\n> \"left\" to be \"-1\" initially, assign 0 to it.\n\nIt would help the reader to explain why this change is equivalent to how\nit worked before.\n\nPatrick\n"},{"id":"518387","messageId":"aCrbLoElTJJq6jwZ@pks.im","threadId":"63329","inReplyTo":"aCoDkaAyxRGgYMJZ@ArchLinux","subject":"Re: [PATCH v2 7/8] u-string-list: move \"filter string\" test to \"u-string-list.c\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-19T07:18:06Z","receivedAt":"2025-05-19T07:18:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, May 18, 2025 at 11:58:09PM +0800, shejialuo wrote:\n> diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c\n> index e4b8e38fb8..be2bb5f103 100644\n> --- a/t/unit-tests/u-string-list.c\n> +++ b/t/unit-tests/u-string-list.c\n> @@ -103,3 +115,57 @@ void test_string_list__split_in_place(void)\n> \n> \tt_string_list_clear(&list, 0);\n> }\n> +\n> +static int prefix_cb(struct string_list_item *item, void *cb_data)\n> +{\n> +\tconst char *prefix = (const char *)cb_data;\n> +\treturn starts_with(item->string, prefix);\n> +}\n> +\n> +static void t_string_list_filter(struct string_list *list,\n> +\t\t\t\t string_list_each_func_t want, void *cb_data, ...)\n> +{\n> +\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n> +\tva_list ap;\n> +\n> +\tva_start(ap, cb_data);\n> +\tt_vcreate_string_list_dup(&expected_strings, 0, ap);\n> +\tva_end(ap);\n> +\n> +\tfilter_string_list(list, 0, want, cb_data);\n> +\tt_string_list_equal(list, &expected_strings);\n> +\n> +\tstring_list_clear(&expected_strings, 0);\n> +}\n> +\n> +void test_string_list__filter(void)\n> +{\n> +\tstruct string_list list = STRING_LIST_INIT_DUP;\n> +\tconst char *prefix = \"y\";\n> +\n> +\tt_create_string_list_dup(&list, 0, NULL);\n\nOkay, here we have to manually create a list because you cannot pass two\nvararg lists to `t_string_list_filter()`. It's not the prettiest and\nfeels a bit repetitive, but the alternatives I can think of aren't a lot\nnicer, either.\n\n> +\tt_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);\n\nBoth the `prefix_cb` and `prefix` are the same across all function\ncalls, so I wondered whether we might want to move them into the\nwrapper function directly.\n\nThe `void *` casts are also all unnecessary.\n\nPatrick\n"},{"id":"518388","messageId":"aCrbMeRgC1sdRm8O@pks.im","threadId":"63329","inReplyTo":"aCoDobl95P_VSaes@ArchLinux","subject":"Re: [PATCH v2 8/8] u-string-list: move \"remove duplicates\" test to \"u-string-list.c\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-19T07:18:09Z","receivedAt":"2025-05-19T07:18:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, May 18, 2025 at 11:58:25PM +0800, shejialuo wrote:\n> We use \"test-tool string-list remove_duplicates\" to test the\n> \"string_list_remove_duplicates\" function. As we have introduced the unit\n> test, we'd better remove the logic from shell script to C program to\n> improve test speed and readability.\n> \n> As all the tests in shell script are removed, let's just delete the\n> \"t0063-string-list.sh\" and update the \"meson.build\" file to align with\n> this change.\n> \n> Also we could simply remove \"DISABLE_SIGN_COMPARE_WARNINGS\" due to we\n> have already deleted related code.\n\nI think it would make sense to explain why the test helper itself isn't\nbeing removed in this commit.\n\nPatrick\n"},{"id":"518394","messageId":"20250519075119.GE102701@coredump.intra.peff.net","threadId":"63329","inReplyTo":"aCoDU46MmoGPB60b@ArchLinux","subject":"Re: [PATCH v2 2/8] string-list: remove unused \"insert_at\" parameter from add_entry","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-19T07:51:19Z","receivedAt":"2025-05-19T07:51:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 18, 2025 at 11:57:07PM +0800, shejialuo wrote:\n\n> In \"add_entry\", we accept \"insert_at\" parameter which must be either -1\n> (auto) or between 0 and `list->nr` inclusive. Any other value is\n> invalid. When caller specify any invalid \"insert_at\" value, we won't\n> check the range and move the element, which would definitely cause the\n> trouble.\n> \n> However, we only use \"add_entry\" in \"string_list_insert\" function and we\n> always pass the \"-1\" for \"insert_at\" parameter. So, we never use this\n> parameter to insert element in a user specified position. Let's delete\n> this parameter. If there is any requirement later, we need to use a\n> better way to do this.\n\nWe can see from looking at the code that removing this will not change\nthe behavior. But that always makes me wonder why it was there in the\nfirst place, and whether we might ever want it.\n\nThe answer in this case is that we used to have another function,\nstring_list_insert_at_index(), which used the extra insert_at parameter.\nThe idea being that you could call string_list_find_insert_index(),\ndecide whether there was something already there, and then insert\nwithout repeating the binary search.\n\nBut you can see in callers like 63226218ba (mailmap: use higher level\nstring list functions, 2014-11-24) that this was not really that useful\n(in that commit we just try to insert and check the util pointer to see\nif we need to add the auxiliary structure).\n\nSo the function went away in f8c4ab611a (string_list: remove\nstring_list_insert_at_index() from its API, 2014-11-24), and I suspect\nwe won't need it again. (Also, I think these days we'd probably use a\nstrmap instead anyway).\n\n-Peff\n"},{"id":"518395","messageId":"20250519075813.GF102701@coredump.intra.peff.net","threadId":"63329","inReplyTo":"aCoDW8CcWeq8T9hp@ArchLinux","subject":"Re: [PATCH v2 3/8] string-list: return index directly when inserting an existing element","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-19T07:58:13Z","receivedAt":"2025-05-19T07:58:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 18, 2025 at 11:57:15PM +0800, shejialuo wrote:\n\n> When inserting an existing element, \"add_entry\" would convert \"index\"\n> value to \"-1-index\" to indicate the caller that this element is in the\n> list already.\n> \n> However, in \"string_list_insert\", we would simply convert this to the\n> original positive index without any further action. Let's directly\n> return the index as we don't care about whether the element is in the\n> list by using \"add_entry\".\n> \n> In the future, if we want to let \"add_entry\" tell the caller, we may add\n> \"int *exact_match\" parameter to \"add_entry\" instead of converting the\n> index to negative to indicate.\n\nI assumed this was in the same boat as the previous change: something we\nused to use and now don't. But I don't think we ever did. The \"-1-index\"\npattern goes all the way back to the beginning of the code.\n\nIt does match how other functions like string_list_find_insert_index()\nbehave. But I think that pattern doesn't make much sense for\nadd_entry(). After the function returns we know we've either found\nsomething or added it, so the positive index will always point to a\nmatching entry.\n\nSo I think your patches are correct, but I was curious how we got to\nthis state.\n\n-Peff\n"},{"id":"518930","messageId":"aDRxlsr8_ElgeWa8@ArchLinux","threadId":"63329","inReplyTo":"aCrbG0lavNa9Plc5@pks.im","subject":"Re: [PATCH v2 1/8] string-list: fix sign compare warnings for loop iterator","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-26T13:50:14Z","receivedAt":"2025-05-26T13:50:10Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, May 19, 2025 at 09:17:47AM +0200, Patrick Steinhardt wrote:\n> On Sun, May 18, 2025 at 11:56:59PM +0800, shejialuo wrote:\n> > In \"string-list.c\", there are six warnings which are emitted by\n> > \"Wsign-compare\". And five warnings are caused by the loop iterator type\n> > mismatch. Let's fix these five warnings by changing the `int` type to\n> > `size_t` type of the loop iterator.\n> \n> This naturally causes the question what the 6th warning is, and why it's\n> not fixed in this commit. The answer is that the last one is more\n> complex and handled by subsequent patches, which you should probably\n> point out here. E.g.:\n> \n> There are a couple of \"-Wsign-compare\" warnings in \"string-list.c\".\n> Fix trivial ones that result from a mismatched loop iterator type.\n> \n> There is a single warning left after these fixes. This warning needs\n> a bit more care and is thus handled in subsequent commits.\n> \n\nThat's right. Thanks for the hint, will improve this in the next\nversion.\n\nJialuo\n"},{"id":"518931","messageId":"aDRyL57_jkyNjaGQ@ArchLinux","threadId":"63329","inReplyTo":"aCrbIbB8DDw0eeae@pks.im","subject":"Re: [PATCH v2 2/8] string-list: remove unused \"insert_at\" parameter from add_entry","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-26T13:52:47Z","receivedAt":"2025-05-26T13:52:43Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, May 19, 2025 at 09:17:53AM +0200, Patrick Steinhardt wrote:\n> On Sun, May 18, 2025 at 11:57:07PM +0800, shejialuo wrote:\n> > In \"add_entry\", we accept \"insert_at\" parameter which must be either -1\n> > (auto) or between 0 and `list->nr` inclusive. Any other value is\n> > invalid. When caller specify any invalid \"insert_at\" value, we won't\n> > check the range and move the element, which would definitely cause the\n> > trouble.\n> \n> Maybe \"which may easily cause an out-of-bounds write\" instead of vague\n> \"trouble\"?\n> \n\nMake sense. I will improve this in the next version.\n\nThanks,\nJialuo\n"},{"id":"518936","messageId":"aDR0VS_4n8Io0QYp@ArchLinux","threadId":"63329","inReplyTo":"20250519075119.GE102701@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/8] string-list: remove unused \"insert_at\" parameter from add_entry","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-26T14:01:57Z","receivedAt":"2025-05-26T14:01:54Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, May 19, 2025 at 03:51:19AM -0400, Jeff King wrote:\n> On Sun, May 18, 2025 at 11:57:07PM +0800, shejialuo wrote:\n> \n> > In \"add_entry\", we accept \"insert_at\" parameter which must be either -1\n> > (auto) or between 0 and `list->nr` inclusive. Any other value is\n> > invalid. When caller specify any invalid \"insert_at\" value, we won't\n> > check the range and move the element, which would definitely cause the\n> > trouble.\n> > \n> > However, we only use \"add_entry\" in \"string_list_insert\" function and we\n> > always pass the \"-1\" for \"insert_at\" parameter. So, we never use this\n> > parameter to insert element in a user specified position. Let's delete\n> > this parameter. If there is any requirement later, we need to use a\n> > better way to do this.\n> \n> We can see from looking at the code that removing this will not change\n> the behavior. But that always makes me wonder why it was there in the\n> first place, and whether we might ever want it.\n> \n\nYes, I agree. Actually, in my first implementation, I didn't realise\nthat this is redundant. However, when inspecting the code carefully, I\nfind out this is useless.\n\n> The answer in this case is that we used to have another function,\n> string_list_insert_at_index(), which used the extra insert_at parameter.\n> The idea being that you could call string_list_find_insert_index(),\n> decide whether there was something already there, and then insert\n> without repeating the binary search.\n> \n> But you can see in callers like 63226218ba (mailmap: use higher level\n> string list functions, 2014-11-24) that this was not really that useful\n> (in that commit we just try to insert and check the util pointer to see\n> if we need to add the auxiliary structure).\n> \n> So the function went away in f8c4ab611a (string_list: remove\n> string_list_insert_at_index() from its API, 2014-11-24), and I suspect\n> we won't need it again. (Also, I think these days we'd probably use a\n> strmap instead anyway).\n> \n\nThanks for the hint. By seeing this commit, I totally understand the\nhistory. Because we delete `string_list_insert_at_index`, we simply call\n\"add_entry\" by specifying \"auto\" mode and somehow we don't delete the\nlegacy check in \"add_entry\".\n\nBut I have one question: should I include the information in the commit\nmessage? I feel doing this would be chaty. But I somehow think we should\ndo this.\n\nThanks,\nJialuo\n"},{"id":"518937","messageId":"aDR27iI_cAFAnTWI@ArchLinux","threadId":"63329","inReplyTo":"aCrbKI_c5Bma8ZDQ@pks.im","subject":"Re: [PATCH v2 3/8] string-list: return index directly when inserting an existing element","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-26T14:13:02Z","receivedAt":"2025-05-26T14:12:58Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, May 19, 2025 at 09:18:00AM +0200, Patrick Steinhardt wrote:\n> On Sun, May 18, 2025 at 11:57:15PM +0800, shejialuo wrote:\n> > When inserting an existing element, \"add_entry\" would convert \"index\"\n> > value to \"-1-index\" to indicate the caller that this element is in the\n> > list already.\n> > \n> > However, in \"string_list_insert\", we would simply convert this to the\n> > original positive index without any further action. Let's directly\n> > return the index as we don't care about whether the element is in the\n> > list by using \"add_entry\".\n> > \n> > In the future, if we want to let \"add_entry\" tell the caller, we may add\n> > \"int *exact_match\" parameter to \"add_entry\" instead of converting the\n> > index to negative to indicate.\n> > \n> > Signed-off-by: shejialuo \n> > ---\n> > string-list.c | 6 +-----\n> > 1 file changed, 1 insertion(+), 5 deletions(-)\n> > \n> > diff --git a/string-list.c b/string-list.c\n> > index 8540c29bc9..171cef5dbb 100644\n> > --- a/string-list.c\n> > +++ b/string-list.c\n> > @@ -40,14 +40,13 @@ static int get_entry_index(const struct string_list *list, const char *string,\n> > \treturn right;\n> > }\n> > \n> > -/* returns -1-index if already exists */\n> > static int add_entry(struct string_list *list, const char *string)\n> > {\n> > \tint exact_match = 0;\n> > \tint index = get_entry_index(list, string, &exact_match);\n> > \n> > \tif (exact_match)\n> > -\t\treturn -1 - index;\n> > +\t\treturn index;\n> > \n> > \tALLOC_GROW(list->items, list->nr+1, list->alloc);\n> > \tif (index < list->nr)\n> \n> Okay, let's assume that \"index == 2\" here and we have an exact match.\n> We'd thus return `-1 - 2 == -3`.\n> \n> > @@ -65,9 +64,6 @@ struct string_list_item *string_list_insert(struct string_list *list, const char\n> > {\n> > \tint index = add_entry(list, string);\n> > \n> > -\tif (index < 0)\n> > -\t\tindex = -1 - index;\n> > -\n> > \treturn list->items + index;\n> > }\n> \n> So we'd now realize that `index < 0` and thus calculate `-1 - -3 == 2`,\n> which is the original index indeed. So this is a nice simplification\n> that retains the original behaviour indeed.\n> \n\nThat's right. Actually, when I find out this by simply calculating `(-1\n- (-1 - index)) == index`, I am a little surprised.\n\n> I think we could simplify the code even further by inlining\n> `get_entry_index()` now that `string_list_insert()` is a trivial wrapper\n> around it. But I'll leave it up to you whether we want to do it or not.\n> \n\nThat's right. But as code it is, let's just keep this which won't hurt\ntoo much.\n\nThanks,\nJialuo\n"},{"id":"518938","messageId":"aDR4oBlmuiQQolTJ@ArchLinux","threadId":"63329","inReplyTo":"20250519075813.GF102701@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/8] string-list: return index directly when inserting an existing element","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-26T14:20:16Z","receivedAt":"2025-05-26T14:20:13Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, May 19, 2025 at 03:58:13AM -0400, Jeff King wrote:\n> On Sun, May 18, 2025 at 11:57:15PM +0800, shejialuo wrote:\n> \n> > When inserting an existing element, \"add_entry\" would convert \"index\"\n> > value to \"-1-index\" to indicate the caller that this element is in the\n> > list already.\n> > \n> > However, in \"string_list_insert\", we would simply convert this to the\n> > original positive index without any further action. Let's directly\n> > return the index as we don't care about whether the element is in the\n> > list by using \"add_entry\".\n> > \n> > In the future, if we want to let \"add_entry\" tell the caller, we may add\n> > \"int *exact_match\" parameter to \"add_entry\" instead of converting the\n> > index to negative to indicate.\n> \n> I assumed this was in the same boat as the previous change: something we\n> used to use and now don't. But I don't think we ever did. The \"-1-index\"\n> pattern goes all the way back to the beginning of the code.\n> \n> It does match how other functions like string_list_find_insert_index()\n> behave. But I think that pattern doesn't make much sense for\n> add_entry(). After the function returns we know we've either found\n> something or added it, so the positive index will always point to a\n> matching entry.\n> \n> So I think your patches are correct, but I was curious how we got to\n> this state.\n\nIt seems that we create this in a long time ago. In 8fd2cb4069 (Extract\nhelper bits from c-merge-recursive work, 2006-07-25), we introduce the\n\"path-list.c\", at that time, we have the code already.\n\n> \n> -Peff\n\nThanks,\nJialuo\n"},{"id":"518939","messageId":"aDR42KocPhLEZCbn@pks.im","threadId":"63329","inReplyTo":"aDR0VS_4n8Io0QYp@ArchLinux","subject":"Re: [PATCH v2 2/8] string-list: remove unused \"insert_at\" parameter from add_entry","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-26T14:21:12Z","receivedAt":"2025-05-26T14:21:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, May 26, 2025 at 10:01:57PM +0800, shejialuo wrote:\n> On Mon, May 19, 2025 at 03:51:19AM -0400, Jeff King wrote:\n> > The answer in this case is that we used to have another function,\n> > string_list_insert_at_index(), which used the extra insert_at parameter.\n> > The idea being that you could call string_list_find_insert_index(),\n> > decide whether there was something already there, and then insert\n> > without repeating the binary search.\n> > \n> > But you can see in callers like 63226218ba (mailmap: use higher level\n> > string list functions, 2014-11-24) that this was not really that useful\n> > (in that commit we just try to insert and check the util pointer to see\n> > if we need to add the auxiliary structure).\n> > \n> > So the function went away in f8c4ab611a (string_list: remove\n> > string_list_insert_at_index() from its API, 2014-11-24), and I suspect\n> > we won't need it again. (Also, I think these days we'd probably use a\n> > strmap instead anyway).\n> > \n> \n> Thanks for the hint. By seeing this commit, I totally understand the\n> history. Because we delete `string_list_insert_at_index`, we simply call\n> \"add_entry\" by specifying \"auto\" mode and somehow we don't delete the\n> legacy check in \"add_entry\".\n> \n> But I have one question: should I include the information in the commit\n> message? I feel doing this would be chaty. But I somehow think we should\n> do this.\n\nI would recommend including such information in the commit message, yes.\nIt helps the reviewer to understand the context and makes it easier for\nthem to figure out why this seemingly nonsensical parameter exists in\nthe first place.\n\nPatrick\n"},{"id":"518941","messageId":"aDR6Y-osR4s-clRt@ArchLinux","threadId":"63329","inReplyTo":"aCrbKz6tr0vj7ytY@pks.im","subject":"Re: [PATCH v2 4/8] string-list: enable sign compare warnings check","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-26T14:27:47Z","receivedAt":"2025-05-26T14:27:43Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, May 19, 2025 at 09:18:03AM +0200, Patrick Steinhardt wrote:\n> On Sun, May 18, 2025 at 11:57:23PM +0800, shejialuo wrote:\n> > The only sign compare warning in \"string-list\" is that we compare the\n> > `index` of the `int` type with the `list->nr` of unsigned type. We get\n> > index by calling \"get_entry_index\", which would always return unsigned\n> > index.\n> > \n> > Let's change the return type of \"get_entry_index\" to be \"size_t\" by\n> > slightly modifying the binary search algorithm. Instead of letting\n> > \"left\" to be \"-1\" initially, assign 0 to it.\n> \n> It would help the reader to explain why this change is equivalent to how\n> it worked before.\n> \n\nRight, will improve this.\n\n> Patrick\n"},{"id":"518942","messageId":"aDR71ftk0KJVTA62@ArchLinux","threadId":"63329","inReplyTo":"aCrbLoElTJJq6jwZ@pks.im","subject":"Re: [PATCH v2 7/8] u-string-list: move \"filter string\" test to \"u-string-list.c\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-26T14:33:57Z","receivedAt":"2025-05-26T14:33:53Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, May 19, 2025 at 09:18:06AM +0200, Patrick Steinhardt wrote:\n> On Sun, May 18, 2025 at 11:58:09PM +0800, shejialuo wrote:\n> > diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c\n> > index e4b8e38fb8..be2bb5f103 100644\n> > --- a/t/unit-tests/u-string-list.c\n> > +++ b/t/unit-tests/u-string-list.c\n> > @@ -103,3 +115,57 @@ void test_string_list__split_in_place(void)\n> > \n> > \tt_string_list_clear(&list, 0);\n> > }\n> > +\n> > +static int prefix_cb(struct string_list_item *item, void *cb_data)\n> > +{\n> > +\tconst char *prefix = (const char *)cb_data;\n> > +\treturn starts_with(item->string, prefix);\n> > +}\n> > +\n> > +static void t_string_list_filter(struct string_list *list,\n> > +\t\t\t\t string_list_each_func_t want, void *cb_data, ...)\n> > +{\n> > +\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n> > +\tva_list ap;\n> > +\n> > +\tva_start(ap, cb_data);\n> > +\tt_vcreate_string_list_dup(&expected_strings, 0, ap);\n> > +\tva_end(ap);\n> > +\n> > +\tfilter_string_list(list, 0, want, cb_data);\n> > +\tt_string_list_equal(list, &expected_strings);\n> > +\n> > +\tstring_list_clear(&expected_strings, 0);\n> > +}\n> > +\n> > +void test_string_list__filter(void)\n> > +{\n> > +\tstruct string_list list = STRING_LIST_INIT_DUP;\n> > +\tconst char *prefix = \"y\";\n> > +\n> > +\tt_create_string_list_dup(&list, 0, NULL);\n> \n> Okay, here we have to manually create a list because you cannot pass two\n> vararg lists to `t_string_list_filter()`. It's not the prettiest and\n> feels a bit repetitive, but the alternatives I can think of aren't a lot\n> nicer, either.\n> \n\nYes, I am not very satisfied with this either. In the original shell\nscript test, it uses \"string_list_split\" to create a new \"string-list\".\nHowever, this way is worse than the current, too hacky.\n\n> > +\tt_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);\n> \n> Both the `prefix_cb` and `prefix` are the same across all function\n> calls, so I wondered whether we might want to move them into the\n> wrapper function directly.\n> \n\nWhat if we want to use the different \"prefix_cb\" and \"prefix\"? I somehow\nthink we should make \"t_string_list_filter\" more flexible.\n\n> The `void *` casts are also all unnecessary.\n> \n\nRight, I will improve this in the next version.\n\n> Patrick\n"},{"id":"520861","messageId":"aGDAZ6a0-PyXXGmK@ArchLinux","threadId":"63329","inReplyTo":"aCoDB9P5XV1lHMil@ArchLinux","subject":"[PATCH v3 0/8] enhance \"string_list\" code and test","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-06-29T04:26:15Z","receivedAt":"2025-06-29T04:26:05Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"Hi All:\n\nI finally finish the version 2. And I don't provide the range-diff due\nto that I add more commits compared with version 1.\n\nThis patch could be organized into three parts:\n\n1. [PATCH v2 1/8]\n\n Fix simple sign warnings of the loop iterator.\n\n2. [PATCH v2 2/8] - [PATCH v2 4/8]\n\n Remove unncessary code, improve the logic and finally enable sign\n compare warnings check.\n\n3. [PATCH v2 5/8] - [PATCH v2 8/8]\n\n Remove test to the unit test.\n\n---\n\nChanges since v2:\n\n1. [PATCH v3 1/8]: improve the commit message to explain that we would\n handle a warning in the later commits.\n2. [PATCH v3 2/8] and [PATCH v2 3/8]: improve the commit message to add\n the history background.\n3. [PATCH v3 4/8]: improve the commit message to show why the current\n bianry search algorithm introduces the sign warning and how to change\n it to fix the sign warning.\n4. [PATCH v3 5/8] - [PATCH v3 8/8]: remove list into test helper instead\n of test itself for reducing the shared state.\n5. [PATCH v3 8/8]: improve the commit message to say why we can't delete\n \"test-tool string_list\" totally.\n\nThanks,\nJialuo\n\nshejialuo (8):\n string-list: fix sign compare warnings for loop iterator\n string-list: remove unused \"insert_at\" parameter from add_entry\n string-list: return index directly when inserting an existing element\n string-list: enable sign compare warnings check\n u-string-list: move \"test_split\" into \"u-string-list.c\"\n u-string-list: move \"test_split_in_place\" to \"u-string-list.c\"\n u-string-list: move \"filter string\" test to \"u-string-list.c\"\n u-string-list: move \"remove duplicates\" test to \"u-string-list.c\"\n\n Makefile | 1 +\n string-list.c | 48 +++-----\n t/helper/test-string-list.c | 96 ---------------\n t/meson.build | 2 +-\n t/t0063-string-list.sh | 142 ----------------------\n t/unit-tests/u-string-list.c | 227 +++++++++++++++++++++++++++++++++++\n 6 files changed, 249 insertions(+), 267 deletions(-)\n delete mode 100755 t/t0063-string-list.sh\n create mode 100644 t/unit-tests/u-string-list.c\n\nRange-diff against v2:\n1: 4fb4546525 ! 1: a69eb0f7b8 string-list: fix sign compare warnings for loop iterator\n @@ Metadata\n ## Commit message ##\n string-list: fix sign compare warnings for loop iterator\n \n - In \"string-list.c\", there are six warnings which are emitted by\n - \"Wsign-compare\". And five warnings are caused by the loop iterator type\n - mismatch. Let's fix these five warnings by changing the `int` type to\n - `size_t` type of the loop iterator.\n + There are a couple of \"-Wsign-compare\" warnings in \"string-list.c\". Fix\n + trivial ones that result from a mismatched loop iterator type.\n +\n + There is a single warning left after these fixes. This warning needs\n + a bit more care and is thus handled in subsequent commits.\n \n Signed-off-by: shejialuo \n \n2: 3721c92803 ! 2: 35550b3b7d string-list: remove unused \"insert_at\" parameter from add_entry\n @@ Commit message\n \n However, we only use \"add_entry\" in \"string_list_insert\" function and we\n always pass the \"-1\" for \"insert_at\" parameter. So, we never use this\n - parameter to insert element in a user specified position. Let's delete\n - this parameter. If there is any requirement later, we need to use a\n - better way to do this.\n + parameter to insert element in a user specified position.\n +\n + And we should know why there is such code path in the first place. We\n + used to have another function \"string_list_insert_at_index()\", which\n + uses the extra \"insert_at\" parameter. And in f8c4ab611a (string_list:\n + remove string_list_insert_at_index() from its API, 2014-11-24), we\n + remove this function but we don't clean all the code path.\n +\n + Let's simply delete this parameter as we'd better use \"strmap\" for such\n + functionality.\n \n Signed-off-by: shejialuo \n \n3: fa1d22c93d ! 3: 7877b528e4 string-list: return index directly when inserting an existing element\n @@ Commit message\n \n When inserting an existing element, \"add_entry\" would convert \"index\"\n value to \"-1-index\" to indicate the caller that this element is in the\n - list already.\n + list already. However, in \"string_list_insert\", we would simply convert\n + this to the original positive index without any further action.\n \n - However, in \"string_list_insert\", we would simply convert this to the\n - original positive index without any further action. Let's directly\n - return the index as we don't care about whether the element is in the\n - list by using \"add_entry\".\n + In 8fd2cb4069 (Extract helper bits from c-merge-recursive work,\n + 2006-07-25), we create \"path-list.c\" and then introduce above code path.\n \n - In the future, if we want to let \"add_entry\" tell the caller, we may add\n - \"int *exact_match\" parameter to \"add_entry\" instead of converting the\n - index to negative to indicate.\n + Let's directly return the index as we don't care about whether the\n + element is in the list by using \"add_entry\". In the future, if we want\n + to let \"add_entry\" tell the caller, we may add \"int *exact_match\"\n + parameter to \"add_entry\" instead of converting the index to negative to\n + indicate.\n \n Signed-off-by: shejialuo \n \n4: 02e5c4307b ! 4: 50c4dbff5c string-list: enable sign compare warnings check\n @@ Metadata\n ## Commit message ##\n string-list: enable sign compare warnings check\n \n - The only sign compare warning in \"string-list\" is that we compare the\n - `index` of the `int` type with the `list->nr` of unsigned type. We get\n - index by calling \"get_entry_index\", which would always return unsigned\n - index.\n + In \"add_entry\", we call \"get_entry_index\" function to get the inserted\n + position. However, as the return type of \"get_entry_index\" function is\n + `int`, there is a sign compare warning when comparing the `index` with\n + the `list-nr` of unsigned type.\n \n - Let's change the return type of \"get_entry_index\" to be \"size_t\" by\n - slightly modifying the binary search algorithm. Instead of letting\n - \"left\" to be \"-1\" initially, assign 0 to it.\n + \"get_entry_index\" would always return unsigned index. However, the\n + current binary search algorithm initializes \"left\" to be \"-1\", which\n + necessitates the use of signed `int` return type.\n +\n + The reason why we need to assign \"left\" to be \"-1\" is that in the\n + `while` loop, we increment \"left\" by 1 to determine whether the loop\n + should end. This design choice, while functional, forces us to use\n + signed arithmetic throughout the function.\n +\n + To resolve this sign comparison issue, let's modify the binary search\n + algorithm with the following approach:\n +\n + 1. Initialize \"left\" to 0 instead of -1\n + 2. Use `left < right` as the loop termination condition instead of\n + `left + 1 < right`\n + 3. When searching the right part, set `left = middle + 1` instead of\n + `middle`\n \n Then, we could delete \"#define DISABLE_SIGN_COMPARE_WARNING\" to enable\n - sign warnings check for \"string-list\"\n + sign warnings check for \"string-list\".\n \n Signed-off-by: shejialuo \n \n5: 990ce4db0f ! 5: ab2aec5887 u-string-list: move \"test_split\" into \"u-string-list.c\"\n @@ t/unit-tests/u-string-list.c (new)\n +\t\tstring_list_append(list, arg);\n +}\n +\n -+static void t_string_list_clear(struct string_list *list, int free_util)\n -+{\n -+\tstring_list_clear(list, free_util);\n -+\tcl_assert_equal_p(list->items, NULL);\n -+\tcl_assert_equal_i(list->nr, 0);\n -+\tcl_assert_equal_i(list->alloc, 0);\n -+}\n -+\n +static void t_string_list_equal(struct string_list *list,\n +\t\t\t\tstruct string_list *expected_strings)\n +{\n @@ t/unit-tests/u-string-list.c (new)\n +\t\t\t\t expected_strings->items[i].string);\n +}\n +\n -+static void t_string_list_split(struct string_list *list, const char *data,\n -+\t\t\t\tint delim, int maxsplit, ...)\n ++static void t_string_list_split(const char *data, int delim, int maxsplit, ...)\n +{\n +\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n ++\tstruct string_list list = STRING_LIST_INIT_DUP;\n +\tva_list ap;\n +\tint len;\n +\n @@ t/unit-tests/u-string-list.c (new)\n +\tt_vcreate_string_list_dup(&expected_strings, 0, ap);\n +\tva_end(ap);\n +\n -+\tstring_list_clear(list, 0);\n -+\tlen = string_list_split(list, data, delim, maxsplit);\n ++\tstring_list_clear(&list, 0);\n ++\tlen = string_list_split(&list, data, delim, maxsplit);\n +\tcl_assert_equal_i(len, expected_strings.nr);\n -+\tt_string_list_equal(list, &expected_strings);\n ++\tt_string_list_equal(&list, &expected_strings);\n +\n +\tstring_list_clear(&expected_strings, 0);\n ++\tstring_list_clear(&list, 0);\n +}\n +\n +void test_string_list__split(void)\n +{\n -+\tstruct string_list list = STRING_LIST_INIT_DUP;\n -+\n -+\tt_string_list_split(&list, \"foo:bar:baz\", ':', -1, \"foo\", \"bar\", \"baz\", NULL);\n -+\tt_string_list_split(&list, \"foo:bar:baz\", ':', 0, \"foo:bar:baz\", NULL);\n -+\tt_string_list_split(&list, \"foo:bar:baz\", ':', 1, \"foo\", \"bar:baz\", NULL);\n -+\tt_string_list_split(&list, \"foo:bar:baz\", ':', 2, \"foo\", \"bar\", \"baz\", NULL);\n -+\tt_string_list_split(&list, \"foo:bar:\", ':', -1, \"foo\", \"bar\", \"\", NULL);\n -+\tt_string_list_split(&list, \"\", ':', -1, \"\", NULL);\n -+\tt_string_list_split(&list, \":\", ':', -1, \"\", \"\", NULL);\n -+\n -+\tt_string_list_clear(&list, 0);\n ++\tt_string_list_split(\"foo:bar:baz\", ':', -1, \"foo\", \"bar\", \"baz\", NULL);\n ++\tt_string_list_split(\"foo:bar:baz\", ':', 0, \"foo:bar:baz\", NULL);\n ++\tt_string_list_split(\"foo:bar:baz\", ':', 1, \"foo\", \"bar:baz\", NULL);\n ++\tt_string_list_split(\"foo:bar:baz\", ':', 2, \"foo\", \"bar\", \"baz\", NULL);\n ++\tt_string_list_split(\"foo:bar:\", ':', -1, \"foo\", \"bar\", \"\", NULL);\n ++\tt_string_list_split(\"\", ':', -1, \"\", NULL);\n ++\tt_string_list_split(\":\", ':', -1, \"\", \"\", NULL);\n +}\n6: 5a02f590df ! 6: 06aca89b57 u-string-list: move \"test_split_in_place\" to \"u-string-list.c\"\n @@ t/t0063-string-list.sh: test_description='Test string list functionality'\n \n ## t/unit-tests/u-string-list.c ##\n @@ t/unit-tests/u-string-list.c: void test_string_list__split(void)\n - \n - \tt_string_list_clear(&list, 0);\n + \tt_string_list_split(\"\", ':', -1, \"\", NULL);\n + \tt_string_list_split(\":\", ':', -1, \"\", \"\", NULL);\n }\n +\n -+static void t_string_list_split_in_place(struct string_list *list, const char *data,\n -+\t\t\t\t\t const char *delim, int maxsplit, ...)\n ++static void t_string_list_split_in_place(const char *data, const char *delim,\n ++\t\t\t\t\t int maxsplit, ...)\n +{\n +\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n ++\tstruct string_list list = STRING_LIST_INIT_NODUP;\n +\tchar *string = xstrdup(data);\n +\tva_list ap;\n +\tint len;\n @@ t/unit-tests/u-string-list.c: void test_string_list__split(void)\n +\tt_vcreate_string_list_dup(&expected_strings, 0, ap);\n +\tva_end(ap);\n +\n -+\tstring_list_clear(list, 0);\n -+\tlen = string_list_split_in_place(list, string, delim, maxsplit);\n ++\tstring_list_clear(&list, 0);\n ++\tlen = string_list_split_in_place(&list, string, delim, maxsplit);\n +\tcl_assert_equal_i(len, expected_strings.nr);\n -+\tt_string_list_equal(list, &expected_strings);\n ++\tt_string_list_equal(&list, &expected_strings);\n +\n +\tfree(string);\n +\tstring_list_clear(&expected_strings, 0);\n ++\tstring_list_clear(&list, 0);\n +}\n +\n +void test_string_list__split_in_place(void)\n +{\n -+\tstruct string_list list = STRING_LIST_INIT_NODUP;\n -+\n -+\tt_string_list_split_in_place(&list, \"foo:;:bar:;:baz:;:\", \":;\", -1,\n ++\tt_string_list_split_in_place(\"foo:;:bar:;:baz:;:\", \":;\", -1,\n +\t\t\t\t \"foo\", \"\", \"\", \"bar\", \"\", \"\", \"baz\", \"\", \"\", \"\", NULL);\n -+\tt_string_list_split_in_place(&list, \"foo:;:bar:;:baz\", \":;\", 0,\n ++\tt_string_list_split_in_place(\"foo:;:bar:;:baz\", \":;\", 0,\n +\t\t\t\t \"foo:;:bar:;:baz\", NULL);\n -+\tt_string_list_split_in_place(&list, \"foo:;:bar:;:baz\", \":;\", 1,\n ++\tt_string_list_split_in_place(\"foo:;:bar:;:baz\", \":;\", 1,\n +\t\t\t\t \"foo\", \";:bar:;:baz\", NULL);\n -+\tt_string_list_split_in_place(&list, \"foo:;:bar:;:baz\", \":;\", 2,\n ++\tt_string_list_split_in_place(\"foo:;:bar:;:baz\", \":;\", 2,\n +\t\t\t\t \"foo\", \"\", \":bar:;:baz\", NULL);\n -+\tt_string_list_split_in_place(&list, \"foo:;:bar:;:\", \":;\", -1,\n ++\tt_string_list_split_in_place(\"foo:;:bar:;:\", \":;\", -1,\n +\t\t\t\t \"foo\", \"\", \"\", \"bar\", \"\", \"\", \"\", NULL);\n -+\n -+\tt_string_list_clear(&list, 0);\n +}\n7: 87a6ec722d ! 7: 4cc76a6fb4 u-string-list: move \"filter string\" test to \"u-string-list.c\"\n @@ t/unit-tests/u-string-list.c: static void t_vcreate_string_list_dup(struct strin\n +\tva_end(ap);\n +}\n +\n - static void t_string_list_clear(struct string_list *list, int free_util)\n ++static void t_string_list_clear(struct string_list *list, int free_util)\n ++{\n ++\tstring_list_clear(list, free_util);\n ++\tcl_assert_equal_p(list->items, NULL);\n ++\tcl_assert_equal_i(list->nr, 0);\n ++\tcl_assert_equal_i(list->alloc, 0);\n ++}\n ++\n + static void t_string_list_equal(struct string_list *list,\n + \t\t\t\tstruct string_list *expected_strings)\n {\n - \tstring_list_clear(list, free_util);\n @@ t/unit-tests/u-string-list.c: void test_string_list__split_in_place(void)\n - \n - \tt_string_list_clear(&list, 0);\n + \tt_string_list_split_in_place(\"foo:;:bar:;:\", \":;\", -1,\n + \t\t\t\t \"foo\", \"\", \"\", \"bar\", \"\", \"\", \"\", NULL);\n }\n +\n +static int prefix_cb(struct string_list_item *item, void *cb_data)\n @@ t/unit-tests/u-string-list.c: void test_string_list__split_in_place(void)\n +\treturn starts_with(item->string, prefix);\n +}\n +\n -+static void t_string_list_filter(struct string_list *list,\n -+\t\t\t\t string_list_each_func_t want, void *cb_data, ...)\n ++static void t_string_list_filter(struct string_list *list, ...)\n +{\n +\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n ++\tconst char *prefix = \"y\";\n +\tva_list ap;\n +\n -+\tva_start(ap, cb_data);\n ++\tva_start(ap, list);\n +\tt_vcreate_string_list_dup(&expected_strings, 0, ap);\n +\tva_end(ap);\n +\n -+\tfilter_string_list(list, 0, want, cb_data);\n ++\tfilter_string_list(list, 0, prefix_cb, (void *)prefix);\n +\tt_string_list_equal(list, &expected_strings);\n +\n +\tstring_list_clear(&expected_strings, 0);\n @@ t/unit-tests/u-string-list.c: void test_string_list__split_in_place(void)\n +void test_string_list__filter(void)\n +{\n +\tstruct string_list list = STRING_LIST_INIT_DUP;\n -+\tconst char *prefix = \"y\";\n +\n +\tt_create_string_list_dup(&list, 0, NULL);\n -+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);\n ++\tt_string_list_filter(&list, NULL);\n +\n +\tt_create_string_list_dup(&list, 0, \"no\", NULL);\n -+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);\n ++\tt_string_list_filter(&list, NULL);\n +\n +\tt_create_string_list_dup(&list, 0, \"yes\", NULL);\n -+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, \"yes\", NULL);\n ++\tt_string_list_filter(&list, \"yes\", NULL);\n +\n +\tt_create_string_list_dup(&list, 0, \"no\", \"yes\", NULL);\n -+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, \"yes\", NULL);\n ++\tt_string_list_filter(&list, \"yes\", NULL);\n +\n +\tt_create_string_list_dup(&list, 0, \"yes\", \"no\", NULL);\n -+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, \"yes\", NULL);\n ++\tt_string_list_filter(&list, \"yes\", NULL);\n +\n +\tt_create_string_list_dup(&list, 0, \"y1\", \"y2\", NULL);\n -+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, \"y1\", \"y2\", NULL);\n ++\tt_string_list_filter(&list, \"y1\", \"y2\", NULL);\n +\n +\tt_create_string_list_dup(&list, 0, \"y2\", \"y1\", NULL);\n -+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, \"y2\", \"y1\", NULL);\n ++\tt_string_list_filter(&list, \"y2\", \"y1\", NULL);\n +\n +\tt_create_string_list_dup(&list, 0, \"x1\", \"x2\", NULL);\n -+\tt_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);\n ++\tt_string_list_filter(&list, NULL);\n +\n +\tt_string_list_clear(&list, 0);\n +}\n8: 55269e7fcb ! 8: b608047d40 u-string-list: move \"remove duplicates\" test to \"u-string-list.c\"\n @@ Commit message\n Also we could simply remove \"DISABLE_SIGN_COMPARE_WARNINGS\" due to we\n have already deleted related code.\n \n + Unfortunately, we cannot totally remove \"test-string-list.c\" due to that\n + we would test the performance of sorting about string list by executing\n + \"test-tool string-list sort\" in \"p0071-sort.sh\".\n +\n Signed-off-by: shejialuo \n \n ## t/helper/test-string-list.c ##\n-- \n2.50.0\n\n"},{"id":"520862","messageId":"aGDAvebseECTvGEP@ArchLinux","threadId":"63329","inReplyTo":"aGDAZ6a0-PyXXGmK@ArchLinux","subject":"[PATCH v3 1/8] string-list: fix sign compare warnings for loop iterator","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-06-29T04:27:41Z","receivedAt":"2025-06-29T04:27:30Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"There are a couple of \"-Wsign-compare\" warnings in \"string-list.c\". Fix\ntrivial ones that result from a mismatched loop iterator type.\n\nThere is a single warning left after these fixes. This warning needs\na bit more care and is thus handled in subsequent commits.\n\nSigned-off-by: shejialuo \n---\n string-list.c | 22 ++++++++++------------\n 1 file changed, 10 insertions(+), 12 deletions(-)\n\ndiff --git a/string-list.c b/string-list.c\nindex bf061fec56..801ece0cba 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -116,9 +116,9 @@ struct string_list_item *string_list_lookup(struct string_list *list, const char\n void string_list_remove_duplicates(struct string_list *list, int free_util)\n {\n \tif (list->nr > 1) {\n-\t\tint src, dst;\n+\t\tsize_t dst = 1;\n \t\tcompare_strings_fn cmp = list->cmp ? list->cmp : strcmp;\n-\t\tfor (src = dst = 1; src < list->nr; src++) {\n+\t\tfor (size_t src = 1; src < list->nr; src++) {\n \t\t\tif (!cmp(list->items[dst - 1].string, list->items[src].string)) {\n \t\t\t\tif (list->strdup_strings)\n \t\t\t\t\tfree(list->items[src].string);\n@@ -134,8 +134,8 @@ void string_list_remove_duplicates(struct string_list *list, int free_util)\n int for_each_string_list(struct string_list *list,\n \t\t\t string_list_each_func_t fn, void *cb_data)\n {\n-\tint i, ret = 0;\n-\tfor (i = 0; i < list->nr; i++)\n+\tint ret = 0;\n+\tfor (size_t i = 0; i < list->nr; i++)\n \t\tif ((ret = fn(&list->items[i], cb_data)))\n \t\t\tbreak;\n \treturn ret;\n@@ -144,8 +144,8 @@ int for_each_string_list(struct string_list *list,\n void filter_string_list(struct string_list *list, int free_util,\n \t\t\tstring_list_each_func_t want, void *cb_data)\n {\n-\tint src, dst = 0;\n-\tfor (src = 0; src < list->nr; src++) {\n+\tsize_t dst = 0;\n+\tfor (size_t src = 0; src < list->nr; src++) {\n \t\tif (want(&list->items[src], cb_data)) {\n \t\t\tlist->items[dst++] = list->items[src];\n \t\t} else {\n@@ -171,13 +171,12 @@ void string_list_remove_empty_items(struct string_list *list, int free_util)\n void string_list_clear(struct string_list *list, int free_util)\n {\n \tif (list->items) {\n-\t\tint i;\n \t\tif (list->strdup_strings) {\n-\t\t\tfor (i = 0; i < list->nr; i++)\n+\t\t\tfor (size_t i = 0; i < list->nr; i++)\n \t\t\t\tfree(list->items[i].string);\n \t\t}\n \t\tif (free_util) {\n-\t\t\tfor (i = 0; i < list->nr; i++)\n+\t\t\tfor (size_t i = 0; i < list->nr; i++)\n \t\t\t\tfree(list->items[i].util);\n \t\t}\n \t\tfree(list->items);\n@@ -189,13 +188,12 @@ void string_list_clear(struct string_list *list, int free_util)\n void string_list_clear_func(struct string_list *list, string_list_clear_func_t clearfunc)\n {\n \tif (list->items) {\n-\t\tint i;\n \t\tif (clearfunc) {\n-\t\t\tfor (i = 0; i < list->nr; i++)\n+\t\t\tfor (size_t i = 0; i < list->nr; i++)\n \t\t\t\tclearfunc(list->items[i].util, list->items[i].string);\n \t\t}\n \t\tif (list->strdup_strings) {\n-\t\t\tfor (i = 0; i < list->nr; i++)\n+\t\t\tfor (size_t i = 0; i < list->nr; i++)\n \t\t\t\tfree(list->items[i].string);\n \t\t}\n \t\tfree(list->items);\n-- \n2.50.0\n\n"},{"id":"520863","messageId":"aGDAxQKvqecRqG7U@ArchLinux","threadId":"63329","inReplyTo":"aGDAZ6a0-PyXXGmK@ArchLinux","subject":"[PATCH v3 2/8] string-list: remove unused \"insert_at\" parameter from add_entry","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-06-29T04:27:49Z","receivedAt":"2025-06-29T04:27:39Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"In \"add_entry\", we accept \"insert_at\" parameter which must be either -1\n(auto) or between 0 and `list->nr` inclusive. Any other value is\ninvalid. When caller specify any invalid \"insert_at\" value, we won't\ncheck the range and move the element, which would definitely cause the\ntrouble.\n\nHowever, we only use \"add_entry\" in \"string_list_insert\" function and we\nalways pass the \"-1\" for \"insert_at\" parameter. So, we never use this\nparameter to insert element in a user specified position.\n\nAnd we should know why there is such code path in the first place. We\nused to have another function \"string_list_insert_at_index()\", which\nuses the extra \"insert_at\" parameter. And in f8c4ab611a (string_list:\nremove string_list_insert_at_index() from its API, 2014-11-24), we\nremove this function but we don't clean all the code path.\n\nLet's simply delete this parameter as we'd better use \"strmap\" for such\nfunctionality.\n\nSigned-off-by: shejialuo \n---\n string-list.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/string-list.c b/string-list.c\nindex 801ece0cba..8540c29bc9 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -41,10 +41,10 @@ static int get_entry_index(const struct string_list *list, const char *string,\n }\n \n /* returns -1-index if already exists */\n-static int add_entry(int insert_at, struct string_list *list, const char *string)\n+static int add_entry(struct string_list *list, const char *string)\n {\n \tint exact_match = 0;\n-\tint index = insert_at != -1 ? insert_at : get_entry_index(list, string, &exact_match);\n+\tint index = get_entry_index(list, string, &exact_match);\n \n \tif (exact_match)\n \t\treturn -1 - index;\n@@ -63,7 +63,7 @@ static int add_entry(int insert_at, struct string_list *list, const char *string\n \n struct string_list_item *string_list_insert(struct string_list *list, const char *string)\n {\n-\tint index = add_entry(-1, list, string);\n+\tint index = add_entry(list, string);\n \n \tif (index < 0)\n \t\tindex = -1 - index;\n-- \n2.50.0\n\n"},{"id":"520864","messageId":"aGDAzeRqrBEUG7lX@ArchLinux","threadId":"63329","inReplyTo":"aGDAZ6a0-PyXXGmK@ArchLinux","subject":"[PATCH v3 3/8] string-list: return index directly when inserting an existing element","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-06-29T04:27:57Z","receivedAt":"2025-06-29T04:27:47Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"When inserting an existing element, \"add_entry\" would convert \"index\"\nvalue to \"-1-index\" to indicate the caller that this element is in the\nlist already. However, in \"string_list_insert\", we would simply convert\nthis to the original positive index without any further action.\n\nIn 8fd2cb4069 (Extract helper bits from c-merge-recursive work,\n2006-07-25), we create \"path-list.c\" and then introduce above code path.\n\nLet's directly return the index as we don't care about whether the\nelement is in the list by using \"add_entry\". In the future, if we want\nto let \"add_entry\" tell the caller, we may add \"int *exact_match\"\nparameter to \"add_entry\" instead of converting the index to negative to\nindicate.\n\nSigned-off-by: shejialuo \n---\n string-list.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/string-list.c b/string-list.c\nindex 8540c29bc9..171cef5dbb 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -40,14 +40,13 @@ static int get_entry_index(const struct string_list *list, const char *string,\n \treturn right;\n }\n \n-/* returns -1-index if already exists */\n static int add_entry(struct string_list *list, const char *string)\n {\n \tint exact_match = 0;\n \tint index = get_entry_index(list, string, &exact_match);\n \n \tif (exact_match)\n-\t\treturn -1 - index;\n+\t\treturn index;\n \n \tALLOC_GROW(list->items, list->nr+1, list->alloc);\n \tif (index < list->nr)\n@@ -65,9 +64,6 @@ struct string_list_item *string_list_insert(struct string_list *list, const char\n {\n \tint index = add_entry(list, string);\n \n-\tif (index < 0)\n-\t\tindex = -1 - index;\n-\n \treturn list->items + index;\n }\n \n-- \n2.50.0\n\n"},{"id":"520865","messageId":"aGDA1lr_-CXbq75G@ArchLinux","threadId":"63329","inReplyTo":"aGDAZ6a0-PyXXGmK@ArchLinux","subject":"[PATCH v3 4/8] string-list: enable sign compare warnings check","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-06-29T04:28:06Z","receivedAt":"2025-06-29T04:27:55Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"In \"add_entry\", we call \"get_entry_index\" function to get the inserted\nposition. However, as the return type of \"get_entry_index\" function is\n`int`, there is a sign compare warning when comparing the `index` with\nthe `list-nr` of unsigned type.\n\n\"get_entry_index\" would always return unsigned index. However, the\ncurrent binary search algorithm initializes \"left\" to be \"-1\", which\nnecessitates the use of signed `int` return type.\n\nThe reason why we need to assign \"left\" to be \"-1\" is that in the\n`while` loop, we increment \"left\" by 1 to determine whether the loop\nshould end. This design choice, while functional, forces us to use\nsigned arithmetic throughout the function.\n\nTo resolve this sign comparison issue, let's modify the binary search\nalgorithm with the following approach:\n\n1. Initialize \"left\" to 0 instead of -1\n2. Use `left < right` as the loop termination condition instead of\n `left + 1 < right`\n3. When searching the right part, set `left = middle + 1` instead of\n `middle`\n\nThen, we could delete \"#define DISABLE_SIGN_COMPARE_WARNING\" to enable\nsign warnings check for \"string-list\".\n\nSigned-off-by: shejialuo \n---\n string-list.c | 20 +++++++++-----------\n 1 file changed, 9 insertions(+), 11 deletions(-)\n\ndiff --git a/string-list.c b/string-list.c\nindex 171cef5dbb..53faaa8420 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -1,5 +1,3 @@\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n-\n #include \"git-compat-util.h\"\n #include \"string-list.h\"\n \n@@ -17,19 +15,19 @@ void string_list_init_dup(struct string_list *list)\n \n /* if there is no exact match, point to the index where the entry could be\n * inserted */\n-static int get_entry_index(const struct string_list *list, const char *string,\n-\t\tint *exact_match)\n+static size_t get_entry_index(const struct string_list *list, const char *string,\n+\t\t\t int *exact_match)\n {\n-\tint left = -1, right = list->nr;\n+\tsize_t left = 0, right = list->nr;\n \tcompare_strings_fn cmp = list->cmp ? list->cmp : strcmp;\n \n-\twhile (left + 1 < right) {\n-\t\tint middle = left + (right - left) / 2;\n+\twhile (left < right) {\n+\t\tsize_t middle = left + (right - left) / 2;\n \t\tint compare = cmp(string, list->items[middle].string);\n \t\tif (compare < 0)\n \t\t\tright = middle;\n \t\telse if (compare > 0)\n-\t\t\tleft = middle;\n+\t\t\tleft = middle + 1;\n \t\telse {\n \t\t\t*exact_match = 1;\n \t\t\treturn middle;\n@@ -40,10 +38,10 @@ static int get_entry_index(const struct string_list *list, const char *string,\n \treturn right;\n }\n \n-static int add_entry(struct string_list *list, const char *string)\n+static size_t add_entry(struct string_list *list, const char *string)\n {\n \tint exact_match = 0;\n-\tint index = get_entry_index(list, string, &exact_match);\n+\tsize_t index = get_entry_index(list, string, &exact_match);\n \n \tif (exact_match)\n \t\treturn index;\n@@ -62,7 +60,7 @@ static int add_entry(struct string_list *list, const char *string)\n \n struct string_list_item *string_list_insert(struct string_list *list, const char *string)\n {\n-\tint index = add_entry(list, string);\n+\tsize_t index = add_entry(list, string);\n \n \treturn list->items + index;\n }\n-- \n2.50.0\n\n"},{"id":"520866","messageId":"aGDA3vLgdoj3d3h4@ArchLinux","threadId":"63329","inReplyTo":"aGDAZ6a0-PyXXGmK@ArchLinux","subject":"[PATCH v3 5/8] u-string-list: move \"test_split\" into \"u-string-list.c\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-06-29T04:28:14Z","receivedAt":"2025-06-29T04:28:04Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"We rely on \"test-tool string-list\" command to test the functionality of\nthe \"string-list\". However, as we have introduced clar test framework,\nwe'd better move the shell script into C program to improve speed and\nreadability.\n\nCreate a new file \"u-string-list.c\" under \"t/unit-tests\", then update\nthe Makefile and \"meson.build\" to build the file. And let's first move\n\"test_split\" into unit test and gradually convert the shell script into\nC program.\n\nIn order to create `string_list` easily by simply specifying strings in\nthe function call, create \"t_vcreate_string_list_dup\" function to do\nthis.\n\nThen port the shell script tests to C program and remove unused\n\"test-tool\" code and tests.\n\nSigned-off-by: shejialuo \n---\n Makefile | 1 +\n t/helper/test-string-list.c | 14 ---------\n t/meson.build | 1 +\n t/t0063-string-list.sh | 53 ----------------------------------\n t/unit-tests/u-string-list.c | 55 ++++++++++++++++++++++++++++++++++++\n 5 files changed, 57 insertions(+), 67 deletions(-)\n create mode 100644 t/unit-tests/u-string-list.c\n\ndiff --git a/Makefile b/Makefile\nindex 70d1543b6b..744f060e53 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1367,6 +1367,7 @@ CLAR_TEST_SUITES += u-prio-queue\n CLAR_TEST_SUITES += u-reftable-tree\n CLAR_TEST_SUITES += u-strbuf\n CLAR_TEST_SUITES += u-strcmp-offset\n+CLAR_TEST_SUITES += u-string-list\n CLAR_TEST_SUITES += u-strvec\n CLAR_TEST_SUITES += u-trailer\n CLAR_TEST_SUITES += u-urlmatch-normalization\ndiff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c\nindex 6f10c5a435..17c18c30f6 100644\n--- a/t/helper/test-string-list.c\n+++ b/t/helper/test-string-list.c\n@@ -46,20 +46,6 @@ static int prefix_cb(struct string_list_item *item, void *cb_data)\n \n int cmd__string_list(int argc, const char **argv)\n {\n-\tif (argc == 5 && !strcmp(argv[1], \"split\")) {\n-\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n-\t\tint i;\n-\t\tconst char *s = argv[2];\n-\t\tint delim = *argv[3];\n-\t\tint maxsplit = atoi(argv[4]);\n-\n-\t\ti = string_list_split(&list, s, delim, maxsplit);\n-\t\tprintf(\"%d\\n\", i);\n-\t\twrite_list(&list);\n-\t\tstring_list_clear(&list, 0);\n-\t\treturn 0;\n-\t}\n-\n \tif (argc == 5 && !strcmp(argv[1], \"split_in_place\")) {\n \t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n \t\tint i;\ndiff --git a/t/meson.build b/t/meson.build\nindex 50e89e764a..d3b3916580 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -11,6 +11,7 @@ clar_test_suites = [\n 'unit-tests/u-reftable-tree.c',\n 'unit-tests/u-strbuf.c',\n 'unit-tests/u-strcmp-offset.c',\n+ 'unit-tests/u-string-list.c',\n 'unit-tests/u-strvec.c',\n 'unit-tests/u-trailer.c',\n 'unit-tests/u-urlmatch-normalization.c',\ndiff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\nindex aac63ba506..6b20ffd206 100755\n--- a/t/t0063-string-list.sh\n+++ b/t/t0063-string-list.sh\n@@ -7,16 +7,6 @@ test_description='Test string list functionality'\n \n . ./test-lib.sh\n \n-test_split () {\n-\tcat >expected &&\n-\ttest_expect_success \"split $1 at $2, max $3\" \"\n-\t\ttest-tool string-list split '$1' '$2' '$3' >actual &&\n-\t\ttest_cmp expected actual &&\n-\t\ttest-tool string-list split_in_place '$1' '$2' '$3' >actual &&\n-\t\ttest_cmp expected actual\n-\t\"\n-}\n-\n test_split_in_place() {\n \tcat >expected &&\n \ttest_expect_success \"split (in place) $1 at $2, max $3\" \"\n@@ -25,49 +15,6 @@ test_split_in_place() {\n \t\"\n }\n \n-test_split \"foo:bar:baz\" \":\" \"-1\" <strdup_strings);\n+\n+\tstring_list_clear(list, free_util);\n+\twhile ((arg = va_arg(ap, const char *)))\n+\t\tstring_list_append(list, arg);\n+}\n+\n+static void t_string_list_equal(struct string_list *list,\n+\t\t\t\tstruct string_list *expected_strings)\n+{\n+\tcl_assert_equal_i(list->nr, expected_strings->nr);\n+\tcl_assert(list->nr <= list->alloc);\n+\tfor (size_t i = 0; i < expected_strings->nr; i++)\n+\t\tcl_assert_equal_s(list->items[i].string,\n+\t\t\t\t expected_strings->items[i].string);\n+}\n+\n+static void t_string_list_split(const char *data, int delim, int maxsplit, ...)\n+{\n+\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\tva_list ap;\n+\tint len;\n+\n+\tva_start(ap, maxsplit);\n+\tt_vcreate_string_list_dup(&expected_strings, 0, ap);\n+\tva_end(ap);\n+\n+\tstring_list_clear(&list, 0);\n+\tlen = string_list_split(&list, data, delim, maxsplit);\n+\tcl_assert_equal_i(len, expected_strings.nr);\n+\tt_string_list_equal(&list, &expected_strings);\n+\n+\tstring_list_clear(&expected_strings, 0);\n+\tstring_list_clear(&list, 0);\n+}\n+\n+void test_string_list__split(void)\n+{\n+\tt_string_list_split(\"foo:bar:baz\", ':', -1, \"foo\", \"bar\", \"baz\", NULL);\n+\tt_string_list_split(\"foo:bar:baz\", ':', 0, \"foo:bar:baz\", NULL);\n+\tt_string_list_split(\"foo:bar:baz\", ':', 1, \"foo\", \"bar:baz\", NULL);\n+\tt_string_list_split(\"foo:bar:baz\", ':', 2, \"foo\", \"bar\", \"baz\", NULL);\n+\tt_string_list_split(\"foo:bar:\", ':', -1, \"foo\", \"bar\", \"\", NULL);\n+\tt_string_list_split(\"\", ':', -1, \"\", NULL);\n+\tt_string_list_split(\":\", ':', -1, \"\", \"\", NULL);\n+}\n-- \n2.50.0\n\n"},{"id":"520867","messageId":"aGDA59ywwEMXsKlg@ArchLinux","threadId":"63329","inReplyTo":"aGDAZ6a0-PyXXGmK@ArchLinux","subject":"[PATCH v3 6/8] u-string-list: move \"test_split_in_place\" to \"u-string-list.c\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-06-29T04:28:23Z","receivedAt":"2025-06-29T04:28:13Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"We use \"test-tool string-list split_in_place\" to test the\n\"string_list_split_in_place\" function. As we have introduced the unit\ntest, we'd better remove the logic from shell script to C program to\nimprove test speed and readability.\n\nSigned-off-by: shejialuo \n---\n t/helper/test-string-list.c | 22 ----------------\n t/t0063-string-list.sh | 51 ------------------------------------\n t/unit-tests/u-string-list.c | 37 ++++++++++++++++++++++++++\n 3 files changed, 37 insertions(+), 73 deletions(-)\n\ndiff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c\nindex 17c18c30f6..8a344347ad 100644\n--- a/t/helper/test-string-list.c\n+++ b/t/helper/test-string-list.c\n@@ -18,13 +18,6 @@ static void parse_string_list(struct string_list *list, const char *arg)\n \t(void)string_list_split(list, arg, ':', -1);\n }\n \n-static void write_list(const struct string_list *list)\n-{\n-\tint i;\n-\tfor (i = 0; i < list->nr; i++)\n-\t\tprintf(\"[%d]: \\\"%s\\\"\\n\", i, list->items[i].string);\n-}\n-\n static void write_list_compact(const struct string_list *list)\n {\n \tint i;\n@@ -46,21 +39,6 @@ static int prefix_cb(struct string_list_item *item, void *cb_data)\n \n int cmd__string_list(int argc, const char **argv)\n {\n-\tif (argc == 5 && !strcmp(argv[1], \"split_in_place\")) {\n-\t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n-\t\tint i;\n-\t\tchar *s = xstrdup(argv[2]);\n-\t\tconst char *delim = argv[3];\n-\t\tint maxsplit = atoi(argv[4]);\n-\n-\t\ti = string_list_split_in_place(&list, s, delim, maxsplit);\n-\t\tprintf(\"%d\\n\", i);\n-\t\twrite_list(&list);\n-\t\tstring_list_clear(&list, 0);\n-\t\tfree(s);\n-\t\treturn 0;\n-\t}\n-\n \tif (argc == 4 && !strcmp(argv[1], \"filter\")) {\n \t\t/*\n \t\t * Retain only the items that have the specified prefix.\ndiff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\nindex 6b20ffd206..1a9cf8bfcf 100755\n--- a/t/t0063-string-list.sh\n+++ b/t/t0063-string-list.sh\n@@ -7,57 +7,6 @@ test_description='Test string list functionality'\n \n . ./test-lib.sh\n \n-test_split_in_place() {\n-\tcat >expected &&\n-\ttest_expect_success \"split (in place) $1 at $2, max $3\" \"\n-\t\ttest-tool string-list split_in_place '$1' '$2' '$3' >actual &&\n-\t\ttest_cmp expected actual\n-\t\"\n-}\n-\n-test_split_in_place \"foo:;:bar:;:baz:;:\" \":;\" \"-1\" <\n---\n t/helper/test-string-list.c | 21 -----------\n t/t0063-string-list.sh | 11 ------\n t/unit-tests/u-string-list.c | 73 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 73 insertions(+), 32 deletions(-)\n\ndiff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c\nindex 8a344347ad..262b28c599 100644\n--- a/t/helper/test-string-list.c\n+++ b/t/helper/test-string-list.c\n@@ -31,29 +31,8 @@ static void write_list_compact(const struct string_list *list)\n \t}\n }\n \n-static int prefix_cb(struct string_list_item *item, void *cb_data)\n-{\n-\tconst char *prefix = (const char *)cb_data;\n-\treturn starts_with(item->string, prefix);\n-}\n-\n int cmd__string_list(int argc, const char **argv)\n {\n-\tif (argc == 4 && !strcmp(argv[1], \"filter\")) {\n-\t\t/*\n-\t\t * Retain only the items that have the specified prefix.\n-\t\t * Arguments: list|- prefix\n-\t\t */\n-\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n-\t\tconst char *prefix = argv[3];\n-\n-\t\tparse_string_list(&list, argv[2]);\n-\t\tfilter_string_list(&list, 0, prefix_cb, (void *)prefix);\n-\t\twrite_list_compact(&list);\n-\t\tstring_list_clear(&list, 0);\n-\t\treturn 0;\n-\t}\n-\n \tif (argc == 3 && !strcmp(argv[1], \"remove_duplicates\")) {\n \t\tstruct string_list list = STRING_LIST_INIT_DUP;\n \ndiff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\nindex 1a9cf8bfcf..31fd62bba8 100755\n--- a/t/t0063-string-list.sh\n+++ b/t/t0063-string-list.sh\n@@ -7,17 +7,6 @@ test_description='Test string list functionality'\n \n . ./test-lib.sh\n \n-test_expect_success \"test filter_string_list\" '\n-\ttest \"x-\" = \"x$(test-tool string-list filter - y)\" &&\n-\ttest \"x-\" = \"x$(test-tool string-list filter no y)\" &&\n-\ttest yes = \"$(test-tool string-list filter yes y)\" &&\n-\ttest yes = \"$(test-tool string-list filter no:yes y)\" &&\n-\ttest yes = \"$(test-tool string-list filter yes:no y)\" &&\n-\ttest y1:y2 = \"$(test-tool string-list filter y1:y2 y)\" &&\n-\ttest y2:y1 = \"$(test-tool string-list filter y2:y1 y)\" &&\n-\ttest \"x-\" = \"x$(test-tool string-list filter x1:x2 y)\"\n-'\n-\n test_expect_success \"test remove_duplicates\" '\n \ttest \"x-\" = \"x$(test-tool string-list remove_duplicates -)\" &&\n \ttest \"x\" = \"x$(test-tool string-list remove_duplicates \"\")\" &&\ndiff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c\nindex d2761e1f2f..f061a3694b 100644\n--- a/t/unit-tests/u-string-list.c\n+++ b/t/unit-tests/u-string-list.c\n@@ -13,6 +13,26 @@ static void t_vcreate_string_list_dup(struct string_list *list,\n \t\tstring_list_append(list, arg);\n }\n \n+static void t_create_string_list_dup(struct string_list *list, int free_util, ...)\n+{\n+\tva_list ap;\n+\n+\tcl_assert(list->strdup_strings);\n+\n+\tstring_list_clear(list, free_util);\n+\tva_start(ap, free_util);\n+\tt_vcreate_string_list_dup(list, free_util, ap);\n+\tva_end(ap);\n+}\n+\n+static void t_string_list_clear(struct string_list *list, int free_util)\n+{\n+\tstring_list_clear(list, free_util);\n+\tcl_assert_equal_p(list->items, NULL);\n+\tcl_assert_equal_i(list->nr, 0);\n+\tcl_assert_equal_i(list->alloc, 0);\n+}\n+\n static void t_string_list_equal(struct string_list *list,\n \t\t\t\tstruct string_list *expected_strings)\n {\n@@ -90,3 +110,56 @@ void test_string_list__split_in_place(void)\n \tt_string_list_split_in_place(\"foo:;:bar:;:\", \":;\", -1,\n \t\t\t\t \"foo\", \"\", \"\", \"bar\", \"\", \"\", \"\", NULL);\n }\n+\n+static int prefix_cb(struct string_list_item *item, void *cb_data)\n+{\n+\tconst char *prefix = (const char *)cb_data;\n+\treturn starts_with(item->string, prefix);\n+}\n+\n+static void t_string_list_filter(struct string_list *list, ...)\n+{\n+\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n+\tconst char *prefix = \"y\";\n+\tva_list ap;\n+\n+\tva_start(ap, list);\n+\tt_vcreate_string_list_dup(&expected_strings, 0, ap);\n+\tva_end(ap);\n+\n+\tfilter_string_list(list, 0, prefix_cb, (void *)prefix);\n+\tt_string_list_equal(list, &expected_strings);\n+\n+\tstring_list_clear(&expected_strings, 0);\n+}\n+\n+void test_string_list__filter(void)\n+{\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\n+\tt_create_string_list_dup(&list, 0, NULL);\n+\tt_string_list_filter(&list, NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"no\", NULL);\n+\tt_string_list_filter(&list, NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"yes\", NULL);\n+\tt_string_list_filter(&list, \"yes\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"no\", \"yes\", NULL);\n+\tt_string_list_filter(&list, \"yes\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"yes\", \"no\", NULL);\n+\tt_string_list_filter(&list, \"yes\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"y1\", \"y2\", NULL);\n+\tt_string_list_filter(&list, \"y1\", \"y2\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"y2\", \"y1\", NULL);\n+\tt_string_list_filter(&list, \"y2\", \"y1\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"x1\", \"x2\", NULL);\n+\tt_string_list_filter(&list, NULL);\n+\n+\tt_string_list_clear(&list, 0);\n+}\n-- \n2.50.0\n\n"},{"id":"520869","messageId":"aGDA-V45RVBUUbrg@ArchLinux","threadId":"63329","inReplyTo":"aGDAZ6a0-PyXXGmK@ArchLinux","subject":"[PATCH v3 8/8] u-string-list: move \"remove duplicates\" test to \"u-string-list.c\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-06-29T04:28:41Z","receivedAt":"2025-06-29T04:28:31Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"We use \"test-tool string-list remove_duplicates\" to test the\n\"string_list_remove_duplicates\" function. As we have introduced the unit\ntest, we'd better remove the logic from shell script to C program to\nimprove test speed and readability.\n\nAs all the tests in shell script are removed, let's just delete the\n\"t0063-string-list.sh\" and update the \"meson.build\" file to align with\nthis change.\n\nAlso we could simply remove \"DISABLE_SIGN_COMPARE_WARNINGS\" due to we\nhave already deleted related code.\n\nUnfortunately, we cannot totally remove \"test-string-list.c\" due to that\nwe would test the performance of sorting about string list by executing\n\"test-tool string-list sort\" in \"p0071-sort.sh\".\n\nSigned-off-by: shejialuo \n---\n t/helper/test-string-list.c | 39 -----------------------\n t/meson.build | 1 -\n t/t0063-string-list.sh | 27 ----------------\n t/unit-tests/u-string-list.c | 62 ++++++++++++++++++++++++++++++++++++\n 4 files changed, 62 insertions(+), 67 deletions(-)\n delete mode 100755 t/t0063-string-list.sh\n\ndiff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c\nindex 262b28c599..6be0cdb8e2 100644\n--- a/t/helper/test-string-list.c\n+++ b/t/helper/test-string-list.c\n@@ -1,48 +1,9 @@\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n-\n #include \"test-tool.h\"\n #include \"strbuf.h\"\n #include \"string-list.h\"\n \n-/*\n- * Parse an argument into a string list. arg should either be a\n- * ':'-separated list of strings, or \"-\" to indicate an empty string\n- * list (as opposed to \"\", which indicates a string list containing a\n- * single empty string). list->strdup_strings must be set.\n- */\n-static void parse_string_list(struct string_list *list, const char *arg)\n-{\n-\tif (!strcmp(arg, \"-\"))\n-\t\treturn;\n-\n-\t(void)string_list_split(list, arg, ':', -1);\n-}\n-\n-static void write_list_compact(const struct string_list *list)\n-{\n-\tint i;\n-\tif (!list->nr)\n-\t\tprintf(\"-\\n\");\n-\telse {\n-\t\tprintf(\"%s\", list->items[0].string);\n-\t\tfor (i = 1; i < list->nr; i++)\n-\t\t\tprintf(\":%s\", list->items[i].string);\n-\t\tprintf(\"\\n\");\n-\t}\n-}\n-\n int cmd__string_list(int argc, const char **argv)\n {\n-\tif (argc == 3 && !strcmp(argv[1], \"remove_duplicates\")) {\n-\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n-\n-\t\tparse_string_list(&list, argv[2]);\n-\t\tstring_list_remove_duplicates(&list, 0);\n-\t\twrite_list_compact(&list);\n-\t\tstring_list_clear(&list, 0);\n-\t\treturn 0;\n-\t}\n-\n \tif (argc == 2 && !strcmp(argv[1], \"sort\")) {\n \t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n \t\tstruct strbuf sb = STRBUF_INIT;\ndiff --git a/t/meson.build b/t/meson.build\nindex d3b3916580..276133a3d2 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -124,7 +124,6 @@ integration_tests = [\n 't0060-path-utils.sh',\n 't0061-run-command.sh',\n 't0062-revision-walking.sh',\n- 't0063-string-list.sh',\n 't0066-dir-iterator.sh',\n 't0067-parse_pathspec_file.sh',\n 't0068-for-each-repo.sh',\ndiff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\ndeleted file mode 100755\nindex 31fd62bba8..0000000000\n--- a/t/t0063-string-list.sh\n+++ /dev/null\n@@ -1,27 +0,0 @@\n-#!/bin/sh\n-#\n-# Copyright (c) 2012 Michael Haggerty\n-#\n-\n-test_description='Test string list functionality'\n-\n-. ./test-lib.sh\n-\n-test_expect_success \"test remove_duplicates\" '\n-\ttest \"x-\" = \"x$(test-tool string-list remove_duplicates -)\" &&\n-\ttest \"x\" = \"x$(test-tool string-list remove_duplicates \"\")\" &&\n-\ttest a = \"$(test-tool string-list remove_duplicates a)\" &&\n-\ttest a = \"$(test-tool string-list remove_duplicates a:a)\" &&\n-\ttest a = \"$(test-tool string-list remove_duplicates a:a:a:a:a)\" &&\n-\ttest a:b = \"$(test-tool string-list remove_duplicates a:b)\" &&\n-\ttest a:b = \"$(test-tool string-list remove_duplicates a:a:b)\" &&\n-\ttest a:b = \"$(test-tool string-list remove_duplicates a:b:b)\" &&\n-\ttest a:b:c = \"$(test-tool string-list remove_duplicates a:b:c)\" &&\n-\ttest a:b:c = \"$(test-tool string-list remove_duplicates a:a:b:c)\" &&\n-\ttest a:b:c = \"$(test-tool string-list remove_duplicates a:b:b:c)\" &&\n-\ttest a:b:c = \"$(test-tool string-list remove_duplicates a:b:c:c)\" &&\n-\ttest a:b:c = \"$(test-tool string-list remove_duplicates a:a:b:b:c:c)\" &&\n-\ttest a:b:c = \"$(test-tool string-list remove_duplicates a:a:a:b:b:b:c:c:c)\"\n-'\n-\n-test_done\ndiff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c\nindex f061a3694b..d4ba5f9fa5 100644\n--- a/t/unit-tests/u-string-list.c\n+++ b/t/unit-tests/u-string-list.c\n@@ -163,3 +163,65 @@ void test_string_list__filter(void)\n \n \tt_string_list_clear(&list, 0);\n }\n+\n+static void t_string_list_remove_duplicates(struct string_list *list, ...)\n+{\n+\tstruct string_list expected_strings = STRING_LIST_INIT_DUP;\n+\tva_list ap;\n+\n+\tva_start(ap, list);\n+\tt_vcreate_string_list_dup(&expected_strings, 0, ap);\n+\tva_end(ap);\n+\n+\tstring_list_remove_duplicates(list, 0);\n+\tt_string_list_equal(list, &expected_strings);\n+\n+\tstring_list_clear(&expected_strings, 0);\n+}\n+\n+void test_string_list__remove_duplicates(void)\n+{\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\n+\tt_create_string_list_dup(&list, 0, NULL);\n+\tt_string_list_remove_duplicates(&list, NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"a\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"a\", \"a\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"a\", \"b\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", \"b\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"b\", \"b\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", \"b\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"b\", \"c\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", \"b\", \"c\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"a\", \"b\", \"c\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", \"b\", \"c\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"b\", \"b\", \"c\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", \"b\", \"c\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"b\", \"c\", \"c\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", \"b\", \"c\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"a\", \"b\", \"b\", \"c\", \"c\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", \"b\", \"c\", NULL);\n+\n+\tt_create_string_list_dup(&list, 0, \"a\", \"a\", \"a\", \"b\", \"b\", \"b\",\n+\t\t\t\t \"c\", \"c\", \"c\", NULL);\n+\tt_string_list_remove_duplicates(&list, \"a\", \"b\", \"c\", NULL);\n+\n+\tt_string_list_clear(&list, 0);\n+}\n-- \n2.50.0\n\n"},{"id":"521294","messageId":"aGdllONkAbTJt-Ud@pks.im","threadId":"63329","inReplyTo":"aGDAZ6a0-PyXXGmK@ArchLinux","subject":"Re: [PATCH v3 0/8] enhance \"string_list\" code and test","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-04T05:24:36Z","receivedAt":"2025-07-04T05:24:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Jun 29, 2025 at 12:26:15PM +0800, shejialuo wrote:\n> Changes since v2:\n> \n> 1. [PATCH v3 1/8]: improve the commit message to explain that we would\n> handle a warning in the later commits.\n> 2. [PATCH v3 2/8] and [PATCH v2 3/8]: improve the commit message to add\n> the history background.\n> 3. [PATCH v3 4/8]: improve the commit message to show why the current\n> bianry search algorithm introduces the sign warning and how to change\n> it to fix the sign warning.\n> 4. [PATCH v3 5/8] - [PATCH v3 8/8]: remove list into test helper instead\n> of test itself for reducing the shared state.\n> 5. [PATCH v3 8/8]: improve the commit message to say why we can't delete\n> \"test-tool string_list\" totally.\n\nThanks, this version looks good to me!\n\nPatrick\n"},{"id":"521442","messageId":"xmqqms9g5967.fsf@gitster.g","threadId":"63329","inReplyTo":"aGdllONkAbTJt-Ud@pks.im","subject":"Re: [PATCH v3 0/8] enhance \"string_list\" code and test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-07T15:10:08Z","receivedAt":"2025-07-07T15:10:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Sun, Jun 29, 2025 at 12:26:15PM +0800, shejialuo wrote:\n>> Changes since v2:\n>> \n>> 1. [PATCH v3 1/8]: improve the commit message to explain that we would\n>> handle a warning in the later commits.\n>> 2. [PATCH v3 2/8] and [PATCH v2 3/8]: improve the commit message to add\n>> the history background.\n>> 3. [PATCH v3 4/8]: improve the commit message to show why the current\n>> bianry search algorithm introduces the sign warning and how to change\n>> it to fix the sign warning.\n>> 4. [PATCH v3 5/8] - [PATCH v3 8/8]: remove list into test helper instead\n>> of test itself for reducing the shared state.\n>> 5. [PATCH v3 8/8]: improve the commit message to say why we can't delete\n>> \"test-tool string_list\" totally.\n>\n> Thanks, this version looks good to me!\n>\n> Patrick\n\nThanks. Will queue.\n"}]}