{"thread":{"id":"59592","subject":"[PATCH 0/5] banned: mark `strok()`, `strtok_r()` as banned","startedAt":"2023-04-13T23:31:45Z","lastAt":"2023-04-25T21:03:18Z","messageCount":47,"participants":["Taylor Blau","Junio C Hamano","Chris Torek","Jeff King","René Scharfe","Jeff Hostetler"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"475322","messageId":"cover.1681428696.git.me@ttaylorr.com","threadId":"59592","inReplyTo":null,"subject":"[PATCH 0/5] banned: mark `strok()`, `strtok_r()` as banned","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-13T23:31:39Z","receivedAt":"2023-04-13T23:31:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This series adds `strtok()`, and its reentrant variant to the list of\nbanned functions.\n\nThe rationale is described in the final patch, but the gist is that even\nthough `strtok_r()` is thread-safe, it imposes a burden on the caller\nand has confusing dataflow. For more details, the final patch has a full\nexplanation.\n\nThe series is structured as follows:\n\n - The first patch introduces `string_list_split_in_place_multi()`,\n which allows us to split token delimited strings in place where\n the set of accepted tokens is more than a single character.\n\n - The next three patches replace the only in-tree uses of strtok() we\n have, which are all in test helpers.\n\n - The final patch marks the two functions as banned.\n\nThanks in advance for your review!\n\nTaylor Blau (5):\n string-list: introduce `string_list_split_in_place_multi()`\n t/helper/test-hashmap.c: avoid using `strtok()`\n t/helper/test-oidmap.c: avoid using `strtok()`\n t/helper/test-json-writer.c: avoid using `strtok()`\n banned.h: mark `strtok()`, `strtok_r()` as banned\n\n banned.h | 6 +++\n string-list.c | 15 ++++++--\n string-list.h | 6 +++\n t/helper/test-hashmap.c | 30 +++++++++++----\n t/helper/test-json-writer.c | 76 +++++++++++++++++++++++--------------\n t/helper/test-oidmap.c | 20 +++++++---\n 6 files changed, 109 insertions(+), 44 deletions(-)\n\n-- \n2.38.0.16.g393fd4c6db\n"},{"id":"475323","messageId":"dda218c8c1fdc0ca2e4352b820f3432565a74a23.1681428696.git.me@ttaylorr.com","threadId":"59592","inReplyTo":"cover.1681428696.git.me@ttaylorr.com","subject":"[PATCH 1/5] string-list: introduce `string_list_split_in_place_multi()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-13T23:31:43Z","receivedAt":"2023-04-13T23:31:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Introduce a variant of the `string_list_split_in_place()` function that\ntakes a string of accepted delimiters.\n\nBy contrast to its cousin `string_list_split_in_place()` which splits\nthe given string at every instance of the single character `delim`, the\n`_multi` variant splits the given string any any character appearing in\nthe string `delim`.\n\nInstead of using `strchr(2)` to locate the first occurrence of the given\ndelimiter character, `string_list_split_in_place_multi()` uses\n`strpbrk(2)` to find the first occurrence of *any* character in the given\ndelimiter string.\n\nSince the `_multi` variant is a generalization of the original\nimplementation, reimplement `string_list_split_in_place()` in terms of\nthe more general function by providing a single-character string for the\nlist of accepted delimiters.\n\nSigned-off-by: Taylor Blau \n---\n string-list.c | 15 ++++++++++++---\n string-list.h | 6 ++++++\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/string-list.c b/string-list.c\nindex db473f273e1..67f9ff18904 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -300,8 +300,8 @@ int string_list_split(struct string_list *list, const char *string,\n \t}\n }\n \n-int string_list_split_in_place(struct string_list *list, char *string,\n-\t\t\t int delim, int maxsplit)\n+int string_list_split_in_place_multi(struct string_list *list, char *string,\n+\t\t\t\t const char *delim, int maxsplit)\n {\n \tint count = 0;\n \tchar *p = string, *end;\n@@ -315,7 +315,7 @@ int string_list_split_in_place(struct string_list *list, char *string,\n \t\t\tstring_list_append(list, p);\n \t\t\treturn count;\n \t\t}\n-\t\tend = strchr(p, delim);\n+\t\tend = strpbrk(p, delim);\n \t\tif (end) {\n \t\t\t*end = '\\0';\n \t\t\tstring_list_append(list, p);\n@@ -326,3 +326,12 @@ int string_list_split_in_place(struct string_list *list, char *string,\n \t\t}\n \t}\n }\n+\n+int string_list_split_in_place(struct string_list *list, char *string,\n+\t\t\t int delim, int maxsplit)\n+{\n+\tchar delim_s[2] = { delim, 0 };\n+\n+\treturn string_list_split_in_place_multi(list, string, delim_s,\n+\t\t\t\t\t\tmaxsplit);\n+}\ndiff --git a/string-list.h b/string-list.h\nindex c7b0d5d0008..670d4fc8fb7 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -268,7 +268,13 @@ int string_list_split(struct string_list *list, const char *string,\n * new string_list_items point into string (which therefore must not\n * be modified or freed while the string_list is in use).\n * list->strdup_strings must *not* be set.\n+ *\n+ * The \"_multi\" variant splits the given string on any character\n+ * appearing in \"delim\", and the non-\"_multi\" variant splits only on the\n+ * given character.\n */\n+int string_list_split_in_place_multi(struct string_list *list, char *string,\n+\t\t\t\t const char *delim, int maxsplit);\n int string_list_split_in_place(struct string_list *list, char *string,\n \t\t\t int delim, int maxsplit);\n #endif /* STRING_LIST_H */\n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"475324","messageId":"0f199468a3b8375dbec0f56fdc831c3ac298eb4e.1681428696.git.me@ttaylorr.com","threadId":"59592","inReplyTo":"cover.1681428696.git.me@ttaylorr.com","subject":"[PATCH 2/5] t/helper/test-hashmap.c: avoid using `strtok()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-13T23:31:46Z","receivedAt":"2023-04-13T23:31:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Avoid using the non-reentrant `strtok()` to separate the parts of each\nincoming command. Instead of replacing it with `strtok_r()`, let's\ninstead use the more friendly `string_list_split_in_place_multi()`.\n\nSigned-off-by: Taylor Blau \n---\n t/helper/test-hashmap.c | 30 +++++++++++++++++++++++-------\n 1 file changed, 23 insertions(+), 7 deletions(-)\n\ndiff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\nindex 36ff07bd4be..902ceb55113 100644\n--- a/t/helper/test-hashmap.c\n+++ b/t/helper/test-hashmap.c\n@@ -2,6 +2,7 @@\n #include \"git-compat-util.h\"\n #include \"hashmap.h\"\n #include \"strbuf.h\"\n+#include \"string-list.h\"\n \n struct test_entry\n {\n@@ -150,6 +151,7 @@ static void perf_hashmap(unsigned int method, unsigned int rounds)\n */\n int cmd__hashmap(int argc, const char **argv)\n {\n+\tstruct string_list parts = STRING_LIST_INIT_NODUP;\n \tstruct strbuf line = STRBUF_INIT;\n \tint icase;\n \tstruct hashmap map = HASHMAP_INIT(test_entry_cmp, &icase);\n@@ -159,21 +161,34 @@ int cmd__hashmap(int argc, const char **argv)\n \n \t/* process commands from stdin */\n \twhile (strbuf_getline(&line, stdin) != EOF) {\n-\t\tchar *cmd, *p1 = NULL, *p2 = NULL;\n+\t\tchar *cmd, *p1, *p2;\n \t\tunsigned int hash = 0;\n \t\tstruct test_entry *entry;\n \n+\t\t/*\n+\t\t * Because we memdup() the arguments out of the\n+\t\t * string_list before inserting them into the hashmap,\n+\t\t * it's OK to set its length back to zero to avoid\n+\t\t * re-allocating the items array once per line.\n+\t\t *\n+\t\t * By doing so, we'll instead overwrite the existing\n+\t\t * entries and avoid re-allocating.\n+\t\t */\n+\t\tparts.nr = 0;\n \t\t/* break line into command and up to two parameters */\n-\t\tcmd = strtok(line.buf, DELIM);\n+\t\tstring_list_split_in_place_multi(&parts, line.buf, DELIM, 2);\n+\n \t\t/* ignore empty lines */\n-\t\tif (!cmd || *cmd == '#')\n+\t\tif (!parts.nr)\n+\t\t\tcontinue;\n+\t\tif (!*parts.items[0].string || *parts.items[0].string == '#')\n \t\t\tcontinue;\n \n-\t\tp1 = strtok(NULL, DELIM);\n-\t\tif (p1) {\n+\t\tcmd = parts.items[0].string;\n+\t\tp1 = parts.nr >= 1 ? parts.items[1].string : NULL;\n+\t\tp2 = parts.nr >= 2 ? parts.items[2].string : NULL;\n+\t\tif (p1)\n \t\t\thash = icase ? strihash(p1) : strhash(p1);\n-\t\t\tp2 = strtok(NULL, DELIM);\n-\t\t}\n \n \t\tif (!strcmp(\"add\", cmd) && p1 && p2) {\n \n@@ -260,6 +275,7 @@ int cmd__hashmap(int argc, const char **argv)\n \t\t}\n \t}\n \n+\tstring_list_clear(&parts, 0);\n \tstrbuf_release(&line);\n \thashmap_clear_and_free(&map, struct test_entry, ent);\n \treturn 0;\n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"475325","messageId":"135222d04e4d9cb85c149e0e12ea531d5f43d9cd.1681428696.git.me@ttaylorr.com","threadId":"59592","inReplyTo":"cover.1681428696.git.me@ttaylorr.com","subject":"[PATCH 3/5] t/helper/test-oidmap.c: avoid using `strtok()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-13T23:31:48Z","receivedAt":"2023-04-13T23:31:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Apply similar treatment as in the previous commit to remove usage of\n`strtok()` from the \"oidmap\" test helper.\n\nSigned-off-by: Taylor Blau \n---\n t/helper/test-oidmap.c | 20 ++++++++++++++------\n 1 file changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c\nindex a7b7b38df1f..cf4d2f7c085 100644\n--- a/t/helper/test-oidmap.c\n+++ b/t/helper/test-oidmap.c\n@@ -4,6 +4,7 @@\n #include \"oidmap.h\"\n #include \"setup.h\"\n #include \"strbuf.h\"\n+#include \"string-list.h\"\n \n /* key is an oid and value is a name (could be a refname for example) */\n struct test_entry {\n@@ -25,6 +26,7 @@ struct test_entry {\n */\n int cmd__oidmap(int argc UNUSED, const char **argv UNUSED)\n {\n+\tstruct string_list parts = STRING_LIST_INIT_NODUP;\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct oidmap map = OIDMAP_INIT;\n \n@@ -35,19 +37,24 @@ int cmd__oidmap(int argc UNUSED, const char **argv UNUSED)\n \n \t/* process commands from stdin */\n \twhile (strbuf_getline(&line, stdin) != EOF) {\n-\t\tchar *cmd, *p1 = NULL, *p2 = NULL;\n+\t\tchar *cmd, *p1, *p2;\n \t\tstruct test_entry *entry;\n \t\tstruct object_id oid;\n \n+\t\t/* see the comment in cmd__hashmap() */\n+\t\tparts.nr = 0;\n \t\t/* break line into command and up to two parameters */\n-\t\tcmd = strtok(line.buf, DELIM);\n+\t\tstring_list_split_in_place_multi(&parts, line.buf, DELIM, 2);\n+\n \t\t/* ignore empty lines */\n-\t\tif (!cmd || *cmd == '#')\n+\t\tif (!parts.nr)\n+\t\t\tcontinue;\n+\t\tif (!*parts.items[0].string || *parts.items[0].string == '#')\n \t\t\tcontinue;\n \n-\t\tp1 = strtok(NULL, DELIM);\n-\t\tif (p1)\n-\t\t\tp2 = strtok(NULL, DELIM);\n+\t\tcmd = parts.items[0].string;\n+\t\tp1 = parts.nr >= 1 ? parts.items[1].string : NULL;\n+\t\tp2 = parts.nr >= 2 ? parts.items[2].string : NULL;\n \n \t\tif (!strcmp(\"put\", cmd) && p1 && p2) {\n \n@@ -108,6 +115,7 @@ int cmd__oidmap(int argc UNUSED, const char **argv UNUSED)\n \t\t}\n \t}\n \n+\tstring_list_clear(&parts, 0);\n \tstrbuf_release(&line);\n \toidmap_free(&map, 1);\n \treturn 0;\n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"475326","messageId":"ae29d4d892a502367cd64f309cdc8e456da4c576.1681428696.git.me@ttaylorr.com","threadId":"59592","inReplyTo":"cover.1681428696.git.me@ttaylorr.com","subject":"[PATCH 4/5] t/helper/test-json-writer.c: avoid using `strtok()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-13T23:31:51Z","receivedAt":"2023-04-13T23:32:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Apply similar treatment as in the previous commit to remove usage of\n`strtok()` from the \"oidmap\" test helper.\n\nEach of the different commands that the \"json-writer\" helper accepts\npops the next space-delimited token from the current line and interprets\nit as a string, integer, or double (with the exception of the very first\ntoken, which is the command itself).\n\nTo accommodate this, split the line in place by the space character, and\npass the corresponding string_list to each of the specialized `get_s()`,\n`get_i()`, and `get_d()` functions.\n\n`get_i()` and `get_d()` are thin wrappers around `get_s()` that convert\ntheir result into the appropriate type by either calling `strtol()` or\n`strtod()`, respectively. In `get_s()`, we mark the token as \"consumed\"\nby incrementing the `consumed_nr` counter, indicating how many tokens we\nhave read up to that point.\n\nBecause each of these functions needs the string-list parts, the number\nof tokens consumed, and the line number, these three are wrapped up in\nto a struct representing the line state.\n\nSigned-off-by: Taylor Blau \n---\n t/helper/test-json-writer.c | 76 +++++++++++++++++++++++--------------\n 1 file changed, 48 insertions(+), 28 deletions(-)\n\ndiff --git a/t/helper/test-json-writer.c b/t/helper/test-json-writer.c\nindex 86887f53203..64bfa1c5f3c 100644\n--- a/t/helper/test-json-writer.c\n+++ b/t/helper/test-json-writer.c\n@@ -1,5 +1,6 @@\n #include \"test-tool.h\"\n #include \"json-writer.h\"\n+#include \"string-list.h\"\n \n static const char *expect_obj1 = \"{\\\"a\\\":\\\"abc\\\",\\\"b\\\":42,\\\"c\\\":true}\";\n static const char *expect_obj2 = \"{\\\"a\\\":-1,\\\"b\\\":2147483647,\\\"c\\\":0}\";\n@@ -394,35 +395,41 @@ static int unit_tests(void)\n \treturn 0;\n }\n \n-static void get_s(int line_nr, char **s_in)\n+struct line {\n+\tstruct string_list *parts;\n+\tsize_t consumed_nr;\n+\tint nr;\n+};\n+\n+static void get_s(struct line *line, char **s_in)\n {\n-\t*s_in = strtok(NULL, \" \");\n-\tif (!*s_in)\n-\t\tdie(\"line[%d]: expected: \", line_nr);\n+\tif (line->consumed_nr > line->parts->nr)\n+\t\tdie(\"line[%d]: expected: \", line->nr);\n+\t*s_in = line->parts->items[line->consumed_nr++].string;\n }\n \n-static void get_i(int line_nr, intmax_t *s_in)\n+static void get_i(struct line *line, intmax_t *s_in)\n {\n \tchar *s;\n \tchar *endptr;\n \n-\tget_s(line_nr, &s);\n+\tget_s(line, &s);\n \n \t*s_in = strtol(s, &endptr, 10);\n \tif (*endptr || errno == ERANGE)\n-\t\tdie(\"line[%d]: invalid integer value\", line_nr);\n+\t\tdie(\"line[%d]: invalid integer value\", line->nr);\n }\n \n-static void get_d(int line_nr, double *s_in)\n+static void get_d(struct line *line, double *s_in)\n {\n \tchar *s;\n \tchar *endptr;\n \n-\tget_s(line_nr, &s);\n+\tget_s(line, &s);\n \n \t*s_in = strtod(s, &endptr);\n \tif (*endptr || errno == ERANGE)\n-\t\tdie(\"line[%d]: invalid float value\", line_nr);\n+\t\tdie(\"line[%d]: invalid float value\", line->nr);\n }\n \n static int pretty;\n@@ -453,6 +460,7 @@ static char *get_trimmed_line(char *buf, int buf_size)\n \n static int scripted(void)\n {\n+\tstruct string_list parts = STRING_LIST_INIT_NODUP;\n \tstruct json_writer jw = JSON_WRITER_INIT;\n \tchar buf[MAX_LINE_LENGTH];\n \tchar *line;\n@@ -470,66 +478,77 @@ static int scripted(void)\n \t\tdie(\"expected first line to be 'object' or 'array'\");\n \n \twhile ((line = get_trimmed_line(buf, MAX_LINE_LENGTH)) != NULL) {\n+\t\tstruct line state = { 0 };\n \t\tchar *verb;\n \t\tchar *key;\n \t\tchar *s_value;\n \t\tintmax_t i_value;\n \t\tdouble d_value;\n \n-\t\tline_nr++;\n+\t\tstate.parts = &parts;\n+\t\tstate.nr = ++line_nr;\n \n-\t\tverb = strtok(line, \" \");\n+\t\t/* see the comment in cmd__hashmap() */\n+\t\tparts.nr = 0;\n+\t\t/* break line into command and zero or more tokens */\n+\t\tstring_list_split_in_place(&parts, line, ' ', -1);\n+\n+\t\t/* ignore empty lines */\n+\t\tif (!parts.nr || !*parts.items[0].string)\n+\t\t\tcontinue;\n+\n+\t\tverb = parts.items[state.consumed_nr++].string;\n \n \t\tif (!strcmp(verb, \"end\")) {\n \t\t\tjw_end(&jw);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-string\")) {\n-\t\t\tget_s(line_nr, &key);\n-\t\t\tget_s(line_nr, &s_value);\n+\t\t\tget_s(&state, &key);\n+\t\t\tget_s(&state, &s_value);\n \t\t\tjw_object_string(&jw, key, s_value);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-int\")) {\n-\t\t\tget_s(line_nr, &key);\n-\t\t\tget_i(line_nr, &i_value);\n+\t\t\tget_s(&state, &key);\n+\t\t\tget_i(&state, &i_value);\n \t\t\tjw_object_intmax(&jw, key, i_value);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-double\")) {\n-\t\t\tget_s(line_nr, &key);\n-\t\t\tget_i(line_nr, &i_value);\n-\t\t\tget_d(line_nr, &d_value);\n+\t\t\tget_s(&state, &key);\n+\t\t\tget_i(&state, &i_value);\n+\t\t\tget_d(&state, &d_value);\n \t\t\tjw_object_double(&jw, key, i_value, d_value);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-true\")) {\n-\t\t\tget_s(line_nr, &key);\n+\t\t\tget_s(&state, &key);\n \t\t\tjw_object_true(&jw, key);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-false\")) {\n-\t\t\tget_s(line_nr, &key);\n+\t\t\tget_s(&state, &key);\n \t\t\tjw_object_false(&jw, key);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-null\")) {\n-\t\t\tget_s(line_nr, &key);\n+\t\t\tget_s(&state, &key);\n \t\t\tjw_object_null(&jw, key);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-object\")) {\n-\t\t\tget_s(line_nr, &key);\n+\t\t\tget_s(&state, &key);\n \t\t\tjw_object_inline_begin_object(&jw, key);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-array\")) {\n-\t\t\tget_s(line_nr, &key);\n+\t\t\tget_s(&state, &key);\n \t\t\tjw_object_inline_begin_array(&jw, key);\n \t\t}\n \t\telse if (!strcmp(verb, \"array-string\")) {\n-\t\t\tget_s(line_nr, &s_value);\n+\t\t\tget_s(&state, &s_value);\n \t\t\tjw_array_string(&jw, s_value);\n \t\t}\n \t\telse if (!strcmp(verb, \"array-int\")) {\n-\t\t\tget_i(line_nr, &i_value);\n+\t\t\tget_i(&state, &i_value);\n \t\t\tjw_array_intmax(&jw, i_value);\n \t\t}\n \t\telse if (!strcmp(verb, \"array-double\")) {\n-\t\t\tget_i(line_nr, &i_value);\n-\t\t\tget_d(line_nr, &d_value);\n+\t\t\tget_i(&state, &i_value);\n+\t\t\tget_d(&state, &d_value);\n \t\t\tjw_array_double(&jw, i_value, d_value);\n \t\t}\n \t\telse if (!strcmp(verb, \"array-true\"))\n@@ -552,6 +571,7 @@ static int scripted(void)\n \tprintf(\"%s\\n\", jw.json.buf);\n \n \tjw_release(&jw);\n+\tstring_list_clear(&parts, 0);\n \treturn 0;\n }\n \n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"475327","messageId":"1d955f8bc6d2797def516897d019a186e461b648.1681428696.git.me@ttaylorr.com","threadId":"59592","inReplyTo":"cover.1681428696.git.me@ttaylorr.com","subject":"[PATCH 5/5] banned.h: mark `strtok()`, `strtok_r()` as banned","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-13T23:31:54Z","receivedAt":"2023-04-13T23:32:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"`strtok_r()` is reentrant, but `strtok()` is not, meaning that using it\nis not thread-safe.\n\nWe could ban `strtok()` and force callers to use its reentrant\ncounterpart, but there are a few drawbacks to doing so:\n\n - `strtok_r()` forces the caller to maintain an extra string pointer\n to pass as its `saveptr` value\n\n - `strtok_r()` also requires that its `saveptr` value be unmodified\n between calls.\n\n - `strtok()` (and by extension, `strtok_r()`) is confusing when used\n across multiple functions, since the caller is supposed to pass NULL\n as its first argument after the first call. This makes it difficult\n to determine what string is actually being tokenized without clear\n dataflow.\n\nSo while we could ban only `strtok()`, there really is no good reason to\nuse either when callers could instead use the much friendlier\n`string_list_split_in_place()` API, which avoids the above issues.\n\nSigned-off-by: Taylor Blau \n---\n banned.h | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/banned.h b/banned.h\nindex 6ccf46bc197..9bd23ce5732 100644\n--- a/banned.h\n+++ b/banned.h\n@@ -18,6 +18,12 @@\n #define strncpy(x,y,n) BANNED(strncpy)\n #undef strncat\n #define strncat(x,y,n) BANNED(strncat)\n+#if 0\n+#undef strtok\n+#define strtok(x,y) BANNED(strtok)\n+#undef strtok_r\n+#define strtok_r(x,y,z) BANNED(strtok_r)\n+#endif\n \n #undef sprintf\n #undef vsprintf\n-- \n2.38.0.16.g393fd4c6db\n"},{"id":"475329","messageId":"xmqq8revw8c9.fsf@gitster.g","threadId":"59592","inReplyTo":"1d955f8bc6d2797def516897d019a186e461b648.1681428696.git.me@ttaylorr.com","subject":"Re: [PATCH 5/5] banned.h: mark `strtok()`, `strtok_r()` as banned","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-14T01:39:18Z","receivedAt":"2023-04-14T01:39:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> - `strtok_r()` forces the caller to maintain an extra string pointer\n> to pass as its `saveptr` value\n>\n> - `strtok_r()` also requires that its `saveptr` value be unmodified\n> between calls.\n>\n> - `strtok()` (and by extension, `strtok_r()`) is confusing when used\n> across multiple functions, since the caller is supposed to pass NULL\n> as its first argument after the first call. This makes it difficult\n> to determine what string is actually being tokenized without clear\n> dataflow.\n\nIt seems that the only existing users of strtok() are all in\nt/helper/ directory, so I personally do not think it is a huge loss\nif these two are forbidden. While I do not see why we should use\nstrtok(), none of the above sound like sensible reasons to ban\nstrtok_r(). At best, they may point out awkwardness of the function\nto make you try finding an alternative that is easier-to-use before\nchoosing strtok_r() for your application on a case-by-case basis.\n\nIf your application wants to chomp a string into tokens from left to\nright, inspecting the resulting token one-by-one as it goes until it\nhits a token that satisfies some condition and then terminate\nwithout wasting cycles on the rest, string_list_split_in_place() is\na poor choice. In such a use case, you do not know upfront where in\nthe string the sought-after token would be, so you have to split the\nstring in full without taking an early exit via maxsplit. Also, you\nare restricted to a single byte value for the delimiter, and unlike\nstrtok[_r](), string_list_split_in_place() does not squash a run of\ndelimiter bytes into one inter-token delimiter.\n\nOne gripe I have against use of strtok() is actually not with\nthreading but because people often misuse it when strcspn() is what\nthey want (i.e. measure the length of the \"first token\", so that\nthey can then xmemdupz() a copy out), forgetting that strtok[_r]()\nis destructive.\n\nSo, I dunno.\n"},{"id":"475331","messageId":"CAPx1Gvf8JgsPg3V-VO+RA3XL_DxMaHEv8MLOh+DjEo67nT=v_g@mail.gmail.com","threadId":"59592","inReplyTo":"xmqq8revw8c9.fsf@gitster.g","subject":"Re: [PATCH 5/5] banned.h: mark `strtok()`, `strtok_r()` as banned","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2023-04-14T02:08:23Z","receivedAt":"2023-04-14T02:08:39Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Thu, Apr 13, 2023 at 7:00 PM Junio C Hamano wrote:\n> ... At best, they may point out awkwardness of the function [strtok_r]\n\nThe awkward interface for strtok as extended to strtok_r is why\nwe (Keith Bostic and I) came up with what's now strsep. Of course\nstrsep is not standard and has its own issues but it can be used\nto parse $PATH for instance, or old-style /etc/passwd entries.\n\nIn this case (tests) it's probably not that important to be efficient\nanyway,\n\nChris\n"},{"id":"475379","messageId":"ZDlYCgrPNG+htzJ3@nand.local","threadId":"59592","inReplyTo":"xmqq8revw8c9.fsf@gitster.g","subject":"Re: [PATCH 5/5] banned.h: mark `strtok()`, `strtok_r()` as banned","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-14T13:41:30Z","receivedAt":"2023-04-14T13:41:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 13, 2023 at 06:39:18PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > - `strtok_r()` forces the caller to maintain an extra string pointer\n> > to pass as its `saveptr` value\n> >\n> > - `strtok_r()` also requires that its `saveptr` value be unmodified\n> > between calls.\n> >\n> > - `strtok()` (and by extension, `strtok_r()`) is confusing when used\n> > across multiple functions, since the caller is supposed to pass NULL\n> > as its first argument after the first call. This makes it difficult\n> > to determine what string is actually being tokenized without clear\n> > dataflow.\n>\n> It seems that the only existing users of strtok() are all in\n> t/helper/ directory, so I personally do not think it is a huge loss\n> if these two are forbidden. While I do not see why we should use\n> strtok(), none of the above sound like sensible reasons to ban\n> strtok_r(). At best, they may point out awkwardness of the function\n> to make you try finding an alternative that is easier-to-use before\n> choosing strtok_r() for your application on a case-by-case basis.\n\nFor what it's worth, I could certainly live if we accomplished getting\nstrtok(2) on the banned list, but left strtok_r(2) un-banned.\n\nTBH, I think that leaving the reenterant version of a banned function as\nun-banned is a little awkward, I don't mind it if you don't feel like\nthe above are sufficient reasons to ban it.\n\n> If your application wants to chomp a string into tokens from left to\n> right, inspecting the resulting token one-by-one as it goes until it\n> hits a token that satisfies some condition and then terminate\n> without wasting cycles on the rest, string_list_split_in_place() is\n> a poor choice. In such a use case, you do not know upfront where in\n> the string the sought-after token would be, so you have to split the\n> string in full without taking an early exit via maxsplit. Also, you\n> are restricted to a single byte value for the delimiter, and unlike\n> strtok[_r](), string_list_split_in_place() does not squash a run of\n> delimiter bytes into one inter-token delimiter.\n\nI don't quite agree with this. In practice, you could repeatedly call\n`string_list_split_in_place()` with maxsplit of \"1\", using the tail of\nthe string list you're splitting into as the string to split. That would\nallow you to split tokens one at a time into the string list without\nhaving to split the whole line up front.\n\nThat all said, I don't think that we have such a use case in the tree,\nat least from my searching for strtok() and\nstring_list_split_in_place().\n\nIt may be that using strtok_r() for such a thing would be less awkward:\nnot having tried both (and having no examples to reference) I honestly\ndo not know for certain.\n\n> One gripe I have against use of strtok() is actually not with\n> threading but because people often misuse it when strcspn() is what\n> they want (i.e. measure the length of the \"first token\", so that\n> they can then xmemdupz() a copy out), forgetting that strtok[_r]()\n> is destructive.\n\nHeh, I'm happy to add that to this, if you want ;-).\n\nThanks,\nTaylor\n"},{"id":"475603","messageId":"20230418101058.GA508219@coredump.intra.peff.net","threadId":"59592","inReplyTo":"dda218c8c1fdc0ca2e4352b820f3432565a74a23.1681428696.git.me@ttaylorr.com","subject":"Re: [PATCH 1/5] string-list: introduce `string_list_split_in_place_multi()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-18T10:10:58Z","receivedAt":"2023-04-18T10:11:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 13, 2023 at 07:31:43PM -0400, Taylor Blau wrote:\n\n> Instead of using `strchr(2)` to locate the first occurrence of the given\n> delimiter character, `string_list_split_in_place_multi()` uses\n> `strpbrk(2)` to find the first occurrence of *any* character in the given\n> delimiter string.\n> \n> Since the `_multi` variant is a generalization of the original\n> implementation, reimplement `string_list_split_in_place()` in terms of\n> the more general function by providing a single-character string for the\n> list of accepted delimiters.\n\nI'd imagine that strpbrk() is potentially a lot slower than strchr().\nBut I kind of doubt that splitting is such a hot path that it will\nmatter. If we want to care, I think we could do something like:\n\n end = delim[1] ? strpbrk(p, delim) : strchr(p, delim[0]);\n\nIt's entirely possible that a half-decent strpbrk() implementation does\nthis already.\n\nSo I mention it mostly in case we need to revisit this later. I think\nit's OK to ignore for now.\n\n-Peff\n"},{"id":"475604","messageId":"20230418102320.GB508219@coredump.intra.peff.net","threadId":"59592","inReplyTo":"0f199468a3b8375dbec0f56fdc831c3ac298eb4e.1681428696.git.me@ttaylorr.com","subject":"Re: [PATCH 2/5] t/helper/test-hashmap.c: avoid using `strtok()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-18T10:23:20Z","receivedAt":"2023-04-18T10:24:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 13, 2023 at 07:31:46PM -0400, Taylor Blau wrote:\n\n> Avoid using the non-reentrant `strtok()` to separate the parts of each\n> incoming command. Instead of replacing it with `strtok_r()`, let's\n> instead use the more friendly `string_list_split_in_place_multi()`.\n\nJunio mentioned this offhand in his response, but I wanted to highlight\none difference in before/after behavior here.\n\n [before]\n $ printf 'add foo bar\\niterate\\n' | t/helper/test-tool hashmap\n foo bar\n\n [after]\n $ printf 'add foo bar\\niterate\\n' | t/helper/test-tool hashmap\n foo bar\n\nI think that's fine for this test script, but it may be an indication\nthat we want string-list's split to support different semantics.\n\n> @@ -159,21 +161,34 @@ int cmd__hashmap(int argc, const char **argv)\n> \n> \t/* process commands from stdin */\n> \twhile (strbuf_getline(&line, stdin) != EOF) {\n> -\t\tchar *cmd, *p1 = NULL, *p2 = NULL;\n> +\t\tchar *cmd, *p1, *p2;\n> \t\tunsigned int hash = 0;\n> \t\tstruct test_entry *entry;\n> \n> +\t\t/*\n> +\t\t * Because we memdup() the arguments out of the\n> +\t\t * string_list before inserting them into the hashmap,\n> +\t\t * it's OK to set its length back to zero to avoid\n> +\t\t * re-allocating the items array once per line.\n> +\t\t *\n> +\t\t * By doing so, we'll instead overwrite the existing\n> +\t\t * entries and avoid re-allocating.\n> +\t\t */\n> +\t\tparts.nr = 0;\n\nI think this is OK, but I wonder if we should wrap it in a function that\nmakes sure the strings aren't owned by the strdup (and thus aren't being\nleaked). Something like:\n\n void string_list_setlen(struct string_list *sl, size_t nr)\n {\n\t/* alternatively, I guess we could actually free nr..sl->nr */\n\tif (sl->strdup_strings)\n\t\tBUG(\"you can't setlen on a string-list which owns its strings\");\n\tif (nr > sl->nr)\n\t\tBUG(\"you can't grow a string-list with setlen\");\n\tsl->nr = nr;\n }\n\nThat is probably overkill for these two test helpers, but I wonder if\nthe pattern might escape into \"real\" code (especially if we suggest\nusing string-list instead of strtok).\n\n-Peff\n"},{"id":"475634","messageId":"ZD7OlWjApwvE8Aih@nand.local","threadId":"59592","inReplyTo":"20230418101058.GA508219@coredump.intra.peff.net","subject":"Re: [PATCH 1/5] string-list: introduce `string_list_split_in_place_multi()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T17:08:37Z","receivedAt":"2023-04-18T17:09:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 18, 2023 at 06:10:58AM -0400, Jeff King wrote:\n> On Thu, Apr 13, 2023 at 07:31:43PM -0400, Taylor Blau wrote:\n>\n> > Instead of using `strchr(2)` to locate the first occurrence of the given\n> > delimiter character, `string_list_split_in_place_multi()` uses\n> > `strpbrk(2)` to find the first occurrence of *any* character in the given\n> > delimiter string.\n> >\n> > Since the `_multi` variant is a generalization of the original\n> > implementation, reimplement `string_list_split_in_place()` in terms of\n> > the more general function by providing a single-character string for the\n> > list of accepted delimiters.\n>\n> I'd imagine that strpbrk() is potentially a lot slower than strchr().\n> But I kind of doubt that splitting is such a hot path that it will\n> matter. If we want to care, I think we could do something like:\n>\n> end = delim[1] ? strpbrk(p, delim) : strchr(p, delim[0]);\n>\n> It's entirely possible that a half-decent strpbrk() implementation does\n> this already.\n\nI did a little research here, and it looks like both glibc[1] and\nmusl[2] already do this. Those links are for their implementations of\nstrcspn(), but which both implement[^1] strpbrk() as \"p += strcspn(p,\nd)\".\n\n> So I mention it mostly in case we need to revisit this later. I think\n> it's OK to ignore for now.\n\nIn addition to being OK from a performance standpoint I think solving\nthe semantics concern you noted lower down in the thread[3] is fairly\nstraightforward.\n\nWhen in \"_multi\" mode, I think this boils down to moving past any number\nof characters in `delim` before each iteration of the loop. And that's\ndoable with \"p += strspn(p, delim)\".\n\nThat avoids any behavior change, and more closely matches what strtok()\ndoes, so I think it's a better path.\n\nThanks,\nTaylor\n\n[1]: https://sourceware.org/git/?p=glibc.git;a=blob;f=string/strcspn.c;hb=glibc-2.37#l35\n[2]: https://git.musl-libc.org/cgit/musl/tree/src/string/strcspn.c?h=v1.2.3#n11\n[3]: https://lore.kernel.org/git/20230418102320.GB508219@coredump.intra.peff.net/\n\n[^1]: Not entirely. If strcspn() takes you to the end of the string,\n strpbrk() will return NULL, instead of a pointer to the end of the\n string.\n"},{"id":"475640","messageId":"ZD7cGx2zKQ2KYnLS@nand.local","threadId":"59592","inReplyTo":"20230418102320.GB508219@coredump.intra.peff.net","subject":"Re: [PATCH 2/5] t/helper/test-hashmap.c: avoid using `strtok()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T18:06:19Z","receivedAt":"2023-04-18T18:06:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 18, 2023 at 06:23:20AM -0400, Jeff King wrote:\n> On Thu, Apr 13, 2023 at 07:31:46PM -0400, Taylor Blau wrote:\n>\n> > Avoid using the non-reentrant `strtok()` to separate the parts of each\n> > incoming command. Instead of replacing it with `strtok_r()`, let's\n> > instead use the more friendly `string_list_split_in_place_multi()`.\n>\n> Junio mentioned this offhand in his response, but I wanted to highlight\n> one difference in before/after behavior here.\n>\n> [before]\n> $ printf 'add foo bar\\niterate\\n' | t/helper/test-tool hashmap\n> foo bar\n>\n> [after]\n> $ printf 'add foo bar\\niterate\\n' | t/helper/test-tool hashmap\n> foo bar\n>\n> I think that's fine for this test script, but it may be an indication\n> that we want string-list's split to support different semantics.\n\nI agree that it's OK for the test scripts, but probably isn't the right\nthing for all cases. In the reroll I'll send shortly, I reimplemented\nthis in a way that (a) doesn't change the behavior of\n`string_list_split_in_place()`, and (b) allows us to easily change the\nsemantics for `string_list_split_in_place_multi()`.\n\n> > @@ -159,21 +161,34 @@ int cmd__hashmap(int argc, const char **argv)\n> >\n> > \t/* process commands from stdin */\n> > \twhile (strbuf_getline(&line, stdin) != EOF) {\n> > -\t\tchar *cmd, *p1 = NULL, *p2 = NULL;\n> > +\t\tchar *cmd, *p1, *p2;\n> > \t\tunsigned int hash = 0;\n> > \t\tstruct test_entry *entry;\n> >\n> > +\t\t/*\n> > +\t\t * Because we memdup() the arguments out of the\n> > +\t\t * string_list before inserting them into the hashmap,\n> > +\t\t * it's OK to set its length back to zero to avoid\n> > +\t\t * re-allocating the items array once per line.\n> > +\t\t *\n> > +\t\t * By doing so, we'll instead overwrite the existing\n> > +\t\t * entries and avoid re-allocating.\n> > +\t\t */\n> > +\t\tparts.nr = 0;\n>\n> I think this is OK, but I wonder if we should wrap it in a function that\n> makes sure the strings aren't owned by the strdup (and thus aren't being\n> leaked). Something like:\n>\n> void string_list_setlen(struct string_list *sl, size_t nr)\n> {\n> \t/* alternatively, I guess we could actually free nr..sl->nr */\n> \tif (sl->strdup_strings)\n> \t\tBUG(\"you can't setlen on a string-list which owns its strings\");\n> \tif (nr > sl->nr)\n> \t\tBUG(\"you can't grow a string-list with setlen\");\n> \tsl->nr = nr;\n> }\n>\n> That is probably overkill for these two test helpers, but I wonder if\n> the pattern might escape into \"real\" code (especially if we suggest\n> using string-list instead of strtok).\n\nI like this suggestion. I added a new patch (for which you are listed\nunder Co-authored-by) which adds this function as-is.\n\nThanks,\nTaylor\n"},{"id":"475644","messageId":"cover.1681845518.git.me@ttaylorr.com","threadId":"59592","inReplyTo":"cover.1681428696.git.me@ttaylorr.com","subject":"[PATCH v2 0/6] banned: mark `strok()` as banned","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T19:18:40Z","receivedAt":"2023-04-18T19:20:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a medium-sized reroll of my series to add `strtok()` to the list\nof banned functions.\n\nSome notable things that have changed since the first round[1] include:\n\n - The behavior of `string_list_split_in_place()` is unchanged, and\n `string_list_split_in_place_multi()` now has the more sane behavior\n of removing all delimiter characters from its output, similar to\n strtok().\n\n - We now guard against dangerous cases of assigning `list->nr` with a\n new `string_list_setlen()` function.\n\n - More test cases are added in t0063.\n\n - `strtok_r()` is no longer on the banned list, and `strtok()` now\n *is* on the list, after I forgot to remove a stray `#if 0` left over\n from debugging.\n\nAs always, a range-diff is included below for convenience. Thanks in\nadvance for your review!\n\n[1]: https://lore.kernel.org/git/cover.1681428696.git.me@ttaylorr.com/\n\nTaylor Blau (6):\n string-list: introduce `string_list_split_in_place_multi()`\n string-list: introduce `string_list_setlen()`\n t/helper/test-hashmap.c: avoid using `strtok()`\n t/helper/test-oidmap.c: avoid using `strtok()`\n t/helper/test-json-writer.c: avoid using `strtok()`\n banned.h: mark `strtok()` as banned\n\n banned.h | 2 +\n string-list.c | 35 ++++++++++--\n string-list.h | 17 ++++++\n t/helper/test-hashmap.c | 30 ++++++++---\n t/helper/test-json-writer.c | 76 ++++++++++++++++----------\n t/helper/test-oidmap.c | 20 ++++---\n t/helper/test-string-list.c | 15 ++++++\n t/t0063-string-list.sh | 105 +++++++++++++++++++++++++-----------\n 8 files changed, 224 insertions(+), 76 deletions(-)\n\nRange-diff against v1:\n1: dda218c8c1 ! 1: 6658b231a9 string-list: introduce `string_list_split_in_place_multi()`\n @@ Commit message\n `_multi` variant splits the given string any any character appearing in\n the string `delim`.\n \n + Like `strtok()`, the `_multi` variant skips past sequential delimiting\n + characters. For example:\n +\n + string_list_split_in_place(&xs, xstrdup(\"foo::bar::baz\"), \":\", -1);\n +\n + would place in `xs` the elements \"foo\", \"bar\", and \"baz\".\n +\n Instead of using `strchr(2)` to locate the first occurrence of the given\n delimiter character, `string_list_split_in_place_multi()` uses\n - `strpbrk(2)` to find the first occurrence of *any* character in the given\n - delimiter string.\n + `strcspn(2)` to move past the initial segment of characters comprised of\n + any characters in the delimiting set.\n +\n + When only a single delimiting character is provided, `strcspn(2)` has\n + equivalent performance to `strchr(2)`. Modern `strcspn(2)`\n + implementations treat an empty delimiter or the singleton delimiter as a\n + special case and fall back to calling strchrnul(). Both glibc[1] and\n + musl[2] implement `strcspn(2)` this way.\n \n Since the `_multi` variant is a generalization of the original\n implementation, reimplement `string_list_split_in_place()` in terms of\n the more general function by providing a single-character string for the\n list of accepted delimiters.\n \n + To avoid regressions, update t0063 in this patch as well. Any \"common\"\n + test cases (i.e., those that produce the same result whether you call\n + `string_list_split()` or `string_list_split_in_place_multi()`) are\n + grouped into a loop which is parameterized over the function to test.\n +\n + Any cases which aren't common (of which there is one existing case, and\n + a handful of new ones added which are specific to the `_multi` variant)\n + are tested independently.\n +\n + [1]: https://sourceware.org/git/?p=glibc.git;a=blob;f=string/strcspn.c;hb=glibc-2.37#l35\n + [2]: https://git.musl-libc.org/cgit/musl/tree/src/string/strcspn.c?h=v1.2.3#n11\n +\n Signed-off-by: Taylor Blau \n \n ## string-list.c ##\n @@ string-list.c: int string_list_split(struct string_list *list, const char *strin\n \n -int string_list_split_in_place(struct string_list *list, char *string,\n -\t\t\t int delim, int maxsplit)\n -+int string_list_split_in_place_multi(struct string_list *list, char *string,\n -+\t\t\t\t const char *delim, int maxsplit)\n ++static int string_list_split_in_place_1(struct string_list *list, char *string,\n ++\t\t\t\t\tconst char *delim, int maxsplit,\n ++\t\t\t\t\tunsigned runs)\n {\n \tint count = 0;\n \tchar *p = string, *end;\n @@ string-list.c: int string_list_split_in_place(struct string_list *list, char *string,\n + \t\tdie(\"internal error in string_list_split_in_place(): \"\n + \t\t \"list->strdup_strings must not be set\");\n + \tfor (;;) {\n ++\t\tif (runs)\n ++\t\t\tp += strspn(p, delim);\n ++\n + \t\tcount++;\n + \t\tif (maxsplit >= 0 && count > maxsplit) {\n \t\t\tstring_list_append(list, p);\n \t\t\treturn count;\n \t\t}\n -\t\tend = strchr(p, delim);\n -+\t\tend = strpbrk(p, delim);\n - \t\tif (end) {\n +-\t\tif (end) {\n ++\t\tend = p + strcspn(p, delim);\n ++\t\tif (end && *end) {\n \t\t\t*end = '\\0';\n \t\t\tstring_list_append(list, p);\n + \t\t\tp = end + 1;\n @@ string-list.c: int string_list_split_in_place(struct string_list *list, char *string,\n \t\t}\n \t}\n }\n +\n ++int string_list_split_in_place_multi(struct string_list *list, char *string,\n ++\t\t\t\t const char *delim, int maxsplit)\n ++{\n ++\treturn string_list_split_in_place_1(list, string, delim, maxsplit, 1);\n ++}\n ++\n +int string_list_split_in_place(struct string_list *list, char *string,\n +\t\t\t int delim, int maxsplit)\n +{\n +\tchar delim_s[2] = { delim, 0 };\n +\n -+\treturn string_list_split_in_place_multi(list, string, delim_s,\n -+\t\t\t\t\t\tmaxsplit);\n ++\treturn string_list_split_in_place_1(list, string, delim_s, maxsplit, 0);\n +}\n \n ## string-list.h ##\n @@ string-list.h: int string_list_split(struct string_list *list, const char *strin\n + *\n + * The \"_multi\" variant splits the given string on any character\n + * appearing in \"delim\", and the non-\"_multi\" variant splits only on the\n -+ * given character.\n ++ * given character. The \"_multi\" variant behaves like `strtok()` where\n ++ * no element contains the delimiting byte(s).\n */\n +int string_list_split_in_place_multi(struct string_list *list, char *string,\n +\t\t\t\t const char *delim, int maxsplit);\n int string_list_split_in_place(struct string_list *list, char *string,\n \t\t\t int delim, int maxsplit);\n #endif /* STRING_LIST_H */\n +\n + ## t/helper/test-string-list.c ##\n +@@ t/helper/test-string-list.c: int cmd__string_list(int argc, const char **argv)\n + \t\treturn 0;\n + \t}\n + \n ++\tif (argc == 5 && !strcmp(argv[1], \"split_in_place_multi\")) {\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_multi(&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.\n +\n + ## t/t0063-string-list.sh ##\n +@@ t/t0063-string-list.sh: test_split () {\n + \t\"\n + }\n + \n +-test_split \"foo:bar:baz\" \":\" \"-1\" <expected &&\n ++\ttest_expect_success \"split_in_place_multi $1 at $2, max $3\" \"\n ++\t\ttest-tool string-list split_in_place_multi '$1' '$2' '$3' >actual &&\n ++\t\ttest_cmp expected actual\n ++\t\"\n ++}\n + \n +-test_split \"foo:bar:baz\" \":\" \"0\" < 2: 2a20ad8bc5 string-list: introduce `string_list_setlen()`\n2: 0f199468a3 ! 3: 0ae07dec36 t/helper/test-hashmap.c: avoid using `strtok()`\n @@ t/helper/test-hashmap.c: int cmd__hashmap(int argc, const char **argv)\n +\t\t * By doing so, we'll instead overwrite the existing\n +\t\t * entries and avoid re-allocating.\n +\t\t */\n -+\t\tparts.nr = 0;\n ++\t\tstring_list_setlen(&parts, 0);\n \t\t/* break line into command and up to two parameters */\n -\t\tcmd = strtok(line.buf, DELIM);\n +\t\tstring_list_split_in_place_multi(&parts, line.buf, DELIM, 2);\n3: 135222d04e ! 4: a659431e9c t/helper/test-oidmap.c: avoid using `strtok()`\n @@ t/helper/test-oidmap.c: int cmd__oidmap(int argc UNUSED, const char **argv UNUSE\n \t\tstruct object_id oid;\n \n +\t\t/* see the comment in cmd__hashmap() */\n -+\t\tparts.nr = 0;\n ++\t\tstring_list_setlen(&parts, 0);\n \t\t/* break line into command and up to two parameters */\n -\t\tcmd = strtok(line.buf, DELIM);\n +\t\tstring_list_split_in_place_multi(&parts, line.buf, DELIM, 2);\n4: ae29d4d892 ! 5: fc6cd23698 t/helper/test-json-writer.c: avoid using `strtok()`\n @@ t/helper/test-json-writer.c: static int scripted(void)\n \n -\t\tverb = strtok(line, \" \");\n +\t\t/* see the comment in cmd__hashmap() */\n -+\t\tparts.nr = 0;\n ++\t\tstring_list_setlen(&parts, 0);\n +\t\t/* break line into command and zero or more tokens */\n +\t\tstring_list_split_in_place(&parts, line, ' ', -1);\n +\n5: 1d955f8bc6 ! 6: 56d2318a6d banned.h: mark `strtok()`, `strtok_r()` as banned\n @@ Metadata\n Author: Taylor Blau \n \n ## Commit message ##\n - banned.h: mark `strtok()`, `strtok_r()` as banned\n + banned.h: mark `strtok()` as banned\n \n `strtok_r()` is reentrant, but `strtok()` is not, meaning that using it\n is not thread-safe.\n \n - We could ban `strtok()` and force callers to use its reentrant\n - counterpart, but there are a few drawbacks to doing so:\n + `strtok()` has a couple of drawbacks that make it undesirable to have\n + any new instances. In addition to being thread-unsafe, it also\n + encourages confusing data flows, where `strtok()` may be called from\n + multiple functions with its first argument as NULL, making it unclear\n + from the immediate context which string is being tokenized.\n \n - - `strtok_r()` forces the caller to maintain an extra string pointer\n - to pass as its `saveptr` value\n + Now that we have removed all instances of `strtok()` from the tree,\n + let's ban `strtok()` to avoid introducing new ones in the future. If new\n + callers should arise, they can either use:\n \n - - `strtok_r()` also requires that its `saveptr` value be unmodified\n - between calls.\n + - `string_list_split_in_place()`,\n + - `string_list_split_in_place_multi()`, or\n + - `strtok_r()`.\n \n - - `strtok()` (and by extension, `strtok_r()`) is confusing when used\n - across multiple functions, since the caller is supposed to pass NULL\n - as its first argument after the first call. This makes it difficult\n - to determine what string is actually being tokenized without clear\n - dataflow.\n + Callers are encouraged to use either of the string_list functions when\n + appropriate over `strtok_r()`, since the latter suffers from the same\n + confusing data-flow problem as `strtok()` does.\n \n - So while we could ban only `strtok()`, there really is no good reason to\n - use either when callers could instead use the much friendlier\n - `string_list_split_in_place()` API, which avoids the above issues.\n + But callers may prefer `strtok_r()` when the number of tokens in a given\n + string is unknown, and they want to split and process them one at a\n + time, so `strtok_r()` is left off the banned.h list.\n \n Signed-off-by: Taylor Blau \n \n @@ banned.h\n #define strncpy(x,y,n) BANNED(strncpy)\n #undef strncat\n #define strncat(x,y,n) BANNED(strncat)\n -+#if 0\n +#undef strtok\n +#define strtok(x,y) BANNED(strtok)\n -+#undef strtok_r\n -+#define strtok_r(x,y,z) BANNED(strtok_r)\n -+#endif\n \n #undef sprintf\n #undef vsprintf\n-- \n2.40.0.358.g56d2318a6d\n"},{"id":"475645","messageId":"6658b231a906dde6acbe7ce156da693ef7dc40e6.1681845518.git.me@ttaylorr.com","threadId":"59592","inReplyTo":"cover.1681845518.git.me@ttaylorr.com","subject":"[PATCH v2 1/6] string-list: introduce `string_list_split_in_place_multi()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T19:18:43Z","receivedAt":"2023-04-18T19:20:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Introduce a variant of the `string_list_split_in_place()` function that\ntakes a string of accepted delimiters.\n\nBy contrast to its cousin `string_list_split_in_place()` which splits\nthe given string at every instance of the single character `delim`, the\n`_multi` variant splits the given string any any character appearing in\nthe string `delim`.\n\nLike `strtok()`, the `_multi` variant skips past sequential delimiting\ncharacters. For example:\n\n string_list_split_in_place(&xs, xstrdup(\"foo::bar::baz\"), \":\", -1);\n\nwould place in `xs` the elements \"foo\", \"bar\", and \"baz\".\n\nInstead of using `strchr(2)` to locate the first occurrence of the given\ndelimiter character, `string_list_split_in_place_multi()` uses\n`strcspn(2)` to move past the initial segment of characters comprised of\nany characters in the delimiting set.\n\nWhen only a single delimiting character is provided, `strcspn(2)` has\nequivalent performance to `strchr(2)`. Modern `strcspn(2)`\nimplementations treat an empty delimiter or the singleton delimiter as a\nspecial case and fall back to calling strchrnul(). Both glibc[1] and\nmusl[2] implement `strcspn(2)` this way.\n\nSince the `_multi` variant is a generalization of the original\nimplementation, reimplement `string_list_split_in_place()` in terms of\nthe more general function by providing a single-character string for the\nlist of accepted delimiters.\n\nTo avoid regressions, update t0063 in this patch as well. Any \"common\"\ntest cases (i.e., those that produce the same result whether you call\n`string_list_split()` or `string_list_split_in_place_multi()`) are\ngrouped into a loop which is parameterized over the function to test.\n\nAny cases which aren't common (of which there is one existing case, and\na handful of new ones added which are specific to the `_multi` variant)\nare tested independently.\n\n[1]: https://sourceware.org/git/?p=glibc.git;a=blob;f=string/strcspn.c;hb=glibc-2.37#l35\n[2]: https://git.musl-libc.org/cgit/musl/tree/src/string/strcspn.c?h=v1.2.3#n11\n\nSigned-off-by: Taylor Blau \n---\n string-list.c | 26 +++++++--\n string-list.h | 7 +++\n t/helper/test-string-list.c | 15 ++++++\n t/t0063-string-list.sh | 105 +++++++++++++++++++++++++-----------\n 4 files changed, 118 insertions(+), 35 deletions(-)\n\ndiff --git a/string-list.c b/string-list.c\nindex db473f273e..b27a53f2e1 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -300,8 +300,9 @@ int string_list_split(struct string_list *list, const char *string,\n \t}\n }\n \n-int string_list_split_in_place(struct string_list *list, char *string,\n-\t\t\t int delim, int maxsplit)\n+static int string_list_split_in_place_1(struct string_list *list, char *string,\n+\t\t\t\t\tconst char *delim, int maxsplit,\n+\t\t\t\t\tunsigned runs)\n {\n \tint count = 0;\n \tchar *p = string, *end;\n@@ -310,13 +311,16 @@ int string_list_split_in_place(struct string_list *list, char *string,\n \t\tdie(\"internal error in string_list_split_in_place(): \"\n \t\t \"list->strdup_strings must not be set\");\n \tfor (;;) {\n+\t\tif (runs)\n+\t\t\tp += strspn(p, delim);\n+\n \t\tcount++;\n \t\tif (maxsplit >= 0 && count > maxsplit) {\n \t\t\tstring_list_append(list, p);\n \t\t\treturn count;\n \t\t}\n-\t\tend = strchr(p, delim);\n-\t\tif (end) {\n+\t\tend = p + strcspn(p, delim);\n+\t\tif (end && *end) {\n \t\t\t*end = '\\0';\n \t\t\tstring_list_append(list, p);\n \t\t\tp = end + 1;\n@@ -326,3 +330,17 @@ int string_list_split_in_place(struct string_list *list, char *string,\n \t\t}\n \t}\n }\n+\n+int string_list_split_in_place_multi(struct string_list *list, char *string,\n+\t\t\t\t const char *delim, int maxsplit)\n+{\n+\treturn string_list_split_in_place_1(list, string, delim, maxsplit, 1);\n+}\n+\n+int string_list_split_in_place(struct string_list *list, char *string,\n+\t\t\t int delim, int maxsplit)\n+{\n+\tchar delim_s[2] = { delim, 0 };\n+\n+\treturn string_list_split_in_place_1(list, string, delim_s, maxsplit, 0);\n+}\ndiff --git a/string-list.h b/string-list.h\nindex c7b0d5d000..f01bbb0bb6 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -268,7 +268,14 @@ int string_list_split(struct string_list *list, const char *string,\n * new string_list_items point into string (which therefore must not\n * be modified or freed while the string_list is in use).\n * list->strdup_strings must *not* be set.\n+ *\n+ * The \"_multi\" variant splits the given string on any character\n+ * appearing in \"delim\", and the non-\"_multi\" variant splits only on the\n+ * given character. The \"_multi\" variant behaves like `strtok()` where\n+ * no element contains the delimiting byte(s).\n */\n+int string_list_split_in_place_multi(struct string_list *list, char *string,\n+\t\t\t\t const char *delim, int maxsplit);\n int string_list_split_in_place(struct string_list *list, char *string,\n \t\t\t int delim, int maxsplit);\n #endif /* STRING_LIST_H */\ndiff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c\nindex 2123dda85b..119bc9e1c9 100644\n--- a/t/helper/test-string-list.c\n+++ b/t/helper/test-string-list.c\n@@ -73,6 +73,21 @@ int cmd__string_list(int argc, const char **argv)\n \t\treturn 0;\n \t}\n \n+\tif (argc == 5 && !strcmp(argv[1], \"split_in_place_multi\")) {\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_multi(&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 46d4839194..9c5094616a 100755\n--- a/t/t0063-string-list.sh\n+++ b/t/t0063-string-list.sh\n@@ -18,42 +18,53 @@ test_split () {\n \t\"\n }\n \n-test_split \"foo:bar:baz\" \":\" \"-1\" <expected &&\n+\ttest_expect_success \"split_in_place_multi $1 at $2, max $3\" \"\n+\t\ttest-tool string-list split_in_place_multi '$1' '$2' '$3' >actual &&\n+\t\ttest_cmp expected actual\n+\t\"\n+}\n \n-test_split \"foo:bar:baz\" \":\" \"0\" <nr` to\n0 while leaving the list of elements allocated as-is.\n\nThis allows `string_list_split_in_place()` to overwrite any existing\nentries without needing to free and re-allocate them.\n\nHowever, setting `list->nr` manually is not safe in all instances. There\nare a couple of cases worth worrying about:\n\n - If the `string_list` is initialized with `strdup_strings`,\n truncating the list can lead to overwriting strings which are\n allocated elsewhere. If there aren't any other pointers to those\n strings other than the ones inside of the `items` array, they will\n become unreachable and leak.\n\n (We could ourselves free the truncated items between\n string_list->items[nr] and `list->nr`, but no present or future\n callers would benefit from this additional complexity).\n\n - If the given `nr` is larger than the current value of `list->nr`,\n we'll trick the `string_list` into a state where it thinks there are\n more items allocated than there actually are, which can lead to\n undefined behavior if we try to read or write those entries.\n\nGuard against both of these by introducing a helper function which\nguards assignment of `list->nr` against each of the above conditions.\n\nCo-authored-by: Jeff King \nSigned-off-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n string-list.c | 9 +++++++++\n string-list.h | 10 ++++++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/string-list.c b/string-list.c\nindex b27a53f2e1..f0b3cdae94 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -203,6 +203,15 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c\n \tlist->nr = list->alloc = 0;\n }\n \n+void string_list_setlen(struct string_list *list, size_t nr)\n+{\n+\tif (list->strdup_strings)\n+\t\tBUG(\"cannot setlen a string_list which owns its entries\");\n+\tif (nr > list->nr)\n+\t\tBUG(\"cannot grow a string_list with setlen\");\n+\tlist->nr = nr;\n+}\n+\n struct string_list_item *string_list_append_nodup(struct string_list *list,\n \t\t\t\t\t\t char *string)\n {\ndiff --git a/string-list.h b/string-list.h\nindex f01bbb0bb6..b41ecda6f4 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -134,6 +134,16 @@ typedef void (*string_list_clear_func_t)(void *p, const char *str);\n /** Call a custom clear function on each util pointer */\n void string_list_clear_func(struct string_list *list, string_list_clear_func_t clearfunc);\n \n+/*\n+ * Set the length of a string_list to `nr`, provided that (a) `list`\n+ * does not own its own storage, and (b) that `nr` is no larger than\n+ * `list->nr`.\n+ *\n+ * Useful when \"shrinking\" `list` to write over existing entries that\n+ * are no longer used without reallocating.\n+ */\n+void string_list_setlen(struct string_list *list, size_t nr);\n+\n /**\n * Apply `func` to each item. If `func` returns nonzero, the\n * iteration aborts and the return value is propagated.\n-- \n2.40.0.358.g56d2318a6d\n\n"},{"id":"475647","messageId":"0ae07dec3663d7cbb0f8662c47485c0667a879b9.1681845518.git.me@ttaylorr.com","threadId":"59592","inReplyTo":"cover.1681845518.git.me@ttaylorr.com","subject":"[PATCH v2 3/6] t/helper/test-hashmap.c: avoid using `strtok()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T19:18:49Z","receivedAt":"2023-04-18T19:20:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Avoid using the non-reentrant `strtok()` to separate the parts of each\nincoming command. Instead of replacing it with `strtok_r()`, let's\ninstead use the more friendly `string_list_split_in_place_multi()`.\n\nSigned-off-by: Taylor Blau \n---\n t/helper/test-hashmap.c | 30 +++++++++++++++++++++++-------\n 1 file changed, 23 insertions(+), 7 deletions(-)\n\ndiff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\nindex 36ff07bd4b..5a3e74a3e5 100644\n--- a/t/helper/test-hashmap.c\n+++ b/t/helper/test-hashmap.c\n@@ -2,6 +2,7 @@\n #include \"git-compat-util.h\"\n #include \"hashmap.h\"\n #include \"strbuf.h\"\n+#include \"string-list.h\"\n \n struct test_entry\n {\n@@ -150,6 +151,7 @@ static void perf_hashmap(unsigned int method, unsigned int rounds)\n */\n int cmd__hashmap(int argc, const char **argv)\n {\n+\tstruct string_list parts = STRING_LIST_INIT_NODUP;\n \tstruct strbuf line = STRBUF_INIT;\n \tint icase;\n \tstruct hashmap map = HASHMAP_INIT(test_entry_cmp, &icase);\n@@ -159,21 +161,34 @@ int cmd__hashmap(int argc, const char **argv)\n \n \t/* process commands from stdin */\n \twhile (strbuf_getline(&line, stdin) != EOF) {\n-\t\tchar *cmd, *p1 = NULL, *p2 = NULL;\n+\t\tchar *cmd, *p1, *p2;\n \t\tunsigned int hash = 0;\n \t\tstruct test_entry *entry;\n \n+\t\t/*\n+\t\t * Because we memdup() the arguments out of the\n+\t\t * string_list before inserting them into the hashmap,\n+\t\t * it's OK to set its length back to zero to avoid\n+\t\t * re-allocating the items array once per line.\n+\t\t *\n+\t\t * By doing so, we'll instead overwrite the existing\n+\t\t * entries and avoid re-allocating.\n+\t\t */\n+\t\tstring_list_setlen(&parts, 0);\n \t\t/* break line into command and up to two parameters */\n-\t\tcmd = strtok(line.buf, DELIM);\n+\t\tstring_list_split_in_place_multi(&parts, line.buf, DELIM, 2);\n+\n \t\t/* ignore empty lines */\n-\t\tif (!cmd || *cmd == '#')\n+\t\tif (!parts.nr)\n+\t\t\tcontinue;\n+\t\tif (!*parts.items[0].string || *parts.items[0].string == '#')\n \t\t\tcontinue;\n \n-\t\tp1 = strtok(NULL, DELIM);\n-\t\tif (p1) {\n+\t\tcmd = parts.items[0].string;\n+\t\tp1 = parts.nr >= 1 ? parts.items[1].string : NULL;\n+\t\tp2 = parts.nr >= 2 ? parts.items[2].string : NULL;\n+\t\tif (p1)\n \t\t\thash = icase ? strihash(p1) : strhash(p1);\n-\t\t\tp2 = strtok(NULL, DELIM);\n-\t\t}\n \n \t\tif (!strcmp(\"add\", cmd) && p1 && p2) {\n \n@@ -260,6 +275,7 @@ int cmd__hashmap(int argc, const char **argv)\n \t\t}\n \t}\n \n+\tstring_list_clear(&parts, 0);\n \tstrbuf_release(&line);\n \thashmap_clear_and_free(&map, struct test_entry, ent);\n \treturn 0;\n-- \n2.40.0.358.g56d2318a6d\n\n"},{"id":"475648","messageId":"a659431e9cf84831bf4a1480bbb0eb8c30389a65.1681845518.git.me@ttaylorr.com","threadId":"59592","inReplyTo":"cover.1681845518.git.me@ttaylorr.com","subject":"[PATCH v2 4/6] t/helper/test-oidmap.c: avoid using `strtok()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T19:18:52Z","receivedAt":"2023-04-18T19:20:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Apply similar treatment as in the previous commit to remove usage of\n`strtok()` from the \"oidmap\" test helper.\n\nSigned-off-by: Taylor Blau \n---\n t/helper/test-oidmap.c | 20 ++++++++++++++------\n 1 file changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c\nindex a7b7b38df1..bca2ca0e06 100644\n--- a/t/helper/test-oidmap.c\n+++ b/t/helper/test-oidmap.c\n@@ -4,6 +4,7 @@\n #include \"oidmap.h\"\n #include \"setup.h\"\n #include \"strbuf.h\"\n+#include \"string-list.h\"\n \n /* key is an oid and value is a name (could be a refname for example) */\n struct test_entry {\n@@ -25,6 +26,7 @@ struct test_entry {\n */\n int cmd__oidmap(int argc UNUSED, const char **argv UNUSED)\n {\n+\tstruct string_list parts = STRING_LIST_INIT_NODUP;\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct oidmap map = OIDMAP_INIT;\n \n@@ -35,19 +37,24 @@ int cmd__oidmap(int argc UNUSED, const char **argv UNUSED)\n \n \t/* process commands from stdin */\n \twhile (strbuf_getline(&line, stdin) != EOF) {\n-\t\tchar *cmd, *p1 = NULL, *p2 = NULL;\n+\t\tchar *cmd, *p1, *p2;\n \t\tstruct test_entry *entry;\n \t\tstruct object_id oid;\n \n+\t\t/* see the comment in cmd__hashmap() */\n+\t\tstring_list_setlen(&parts, 0);\n \t\t/* break line into command and up to two parameters */\n-\t\tcmd = strtok(line.buf, DELIM);\n+\t\tstring_list_split_in_place_multi(&parts, line.buf, DELIM, 2);\n+\n \t\t/* ignore empty lines */\n-\t\tif (!cmd || *cmd == '#')\n+\t\tif (!parts.nr)\n+\t\t\tcontinue;\n+\t\tif (!*parts.items[0].string || *parts.items[0].string == '#')\n \t\t\tcontinue;\n \n-\t\tp1 = strtok(NULL, DELIM);\n-\t\tif (p1)\n-\t\t\tp2 = strtok(NULL, DELIM);\n+\t\tcmd = parts.items[0].string;\n+\t\tp1 = parts.nr >= 1 ? parts.items[1].string : NULL;\n+\t\tp2 = parts.nr >= 2 ? parts.items[2].string : NULL;\n \n \t\tif (!strcmp(\"put\", cmd) && p1 && p2) {\n \n@@ -108,6 +115,7 @@ int cmd__oidmap(int argc UNUSED, const char **argv UNUSED)\n \t\t}\n \t}\n \n+\tstring_list_clear(&parts, 0);\n \tstrbuf_release(&line);\n \toidmap_free(&map, 1);\n \treturn 0;\n-- \n2.40.0.358.g56d2318a6d\n\n"},{"id":"475649","messageId":"fc6cd23698dae1d3af43efaeb7d1ef8332367e61.1681845518.git.me@ttaylorr.com","threadId":"59592","inReplyTo":"cover.1681845518.git.me@ttaylorr.com","subject":"[PATCH v2 5/6] t/helper/test-json-writer.c: avoid using `strtok()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T19:18:55Z","receivedAt":"2023-04-18T19:20:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Apply similar treatment as in the previous commit to remove usage of\n`strtok()` from the \"oidmap\" test helper.\n\nEach of the different commands that the \"json-writer\" helper accepts\npops the next space-delimited token from the current line and interprets\nit as a string, integer, or double (with the exception of the very first\ntoken, which is the command itself).\n\nTo accommodate this, split the line in place by the space character, and\npass the corresponding string_list to each of the specialized `get_s()`,\n`get_i()`, and `get_d()` functions.\n\n`get_i()` and `get_d()` are thin wrappers around `get_s()` that convert\ntheir result into the appropriate type by either calling `strtol()` or\n`strtod()`, respectively. In `get_s()`, we mark the token as \"consumed\"\nby incrementing the `consumed_nr` counter, indicating how many tokens we\nhave read up to that point.\n\nBecause each of these functions needs the string-list parts, the number\nof tokens consumed, and the line number, these three are wrapped up in\nto a struct representing the line state.\n\nSigned-off-by: Taylor Blau \n---\n t/helper/test-json-writer.c | 76 +++++++++++++++++++++++--------------\n 1 file changed, 48 insertions(+), 28 deletions(-)\n\ndiff --git a/t/helper/test-json-writer.c b/t/helper/test-json-writer.c\nindex 86887f5320..af0a34aa04 100644\n--- a/t/helper/test-json-writer.c\n+++ b/t/helper/test-json-writer.c\n@@ -1,5 +1,6 @@\n #include \"test-tool.h\"\n #include \"json-writer.h\"\n+#include \"string-list.h\"\n \n static const char *expect_obj1 = \"{\\\"a\\\":\\\"abc\\\",\\\"b\\\":42,\\\"c\\\":true}\";\n static const char *expect_obj2 = \"{\\\"a\\\":-1,\\\"b\\\":2147483647,\\\"c\\\":0}\";\n@@ -394,35 +395,41 @@ static int unit_tests(void)\n \treturn 0;\n }\n \n-static void get_s(int line_nr, char **s_in)\n+struct line {\n+\tstruct string_list *parts;\n+\tsize_t consumed_nr;\n+\tint nr;\n+};\n+\n+static void get_s(struct line *line, char **s_in)\n {\n-\t*s_in = strtok(NULL, \" \");\n-\tif (!*s_in)\n-\t\tdie(\"line[%d]: expected: \", line_nr);\n+\tif (line->consumed_nr > line->parts->nr)\n+\t\tdie(\"line[%d]: expected: \", line->nr);\n+\t*s_in = line->parts->items[line->consumed_nr++].string;\n }\n \n-static void get_i(int line_nr, intmax_t *s_in)\n+static void get_i(struct line *line, intmax_t *s_in)\n {\n \tchar *s;\n \tchar *endptr;\n \n-\tget_s(line_nr, &s);\n+\tget_s(line, &s);\n \n \t*s_in = strtol(s, &endptr, 10);\n \tif (*endptr || errno == ERANGE)\n-\t\tdie(\"line[%d]: invalid integer value\", line_nr);\n+\t\tdie(\"line[%d]: invalid integer value\", line->nr);\n }\n \n-static void get_d(int line_nr, double *s_in)\n+static void get_d(struct line *line, double *s_in)\n {\n \tchar *s;\n \tchar *endptr;\n \n-\tget_s(line_nr, &s);\n+\tget_s(line, &s);\n \n \t*s_in = strtod(s, &endptr);\n \tif (*endptr || errno == ERANGE)\n-\t\tdie(\"line[%d]: invalid float value\", line_nr);\n+\t\tdie(\"line[%d]: invalid float value\", line->nr);\n }\n \n static int pretty;\n@@ -453,6 +460,7 @@ static char *get_trimmed_line(char *buf, int buf_size)\n \n static int scripted(void)\n {\n+\tstruct string_list parts = STRING_LIST_INIT_NODUP;\n \tstruct json_writer jw = JSON_WRITER_INIT;\n \tchar buf[MAX_LINE_LENGTH];\n \tchar *line;\n@@ -470,66 +478,77 @@ static int scripted(void)\n \t\tdie(\"expected first line to be 'object' or 'array'\");\n \n \twhile ((line = get_trimmed_line(buf, MAX_LINE_LENGTH)) != NULL) {\n+\t\tstruct line state = { 0 };\n \t\tchar *verb;\n \t\tchar *key;\n \t\tchar *s_value;\n \t\tintmax_t i_value;\n \t\tdouble d_value;\n \n-\t\tline_nr++;\n+\t\tstate.parts = &parts;\n+\t\tstate.nr = ++line_nr;\n \n-\t\tverb = strtok(line, \" \");\n+\t\t/* see the comment in cmd__hashmap() */\n+\t\tstring_list_setlen(&parts, 0);\n+\t\t/* break line into command and zero or more tokens */\n+\t\tstring_list_split_in_place(&parts, line, ' ', -1);\n+\n+\t\t/* ignore empty lines */\n+\t\tif (!parts.nr || !*parts.items[0].string)\n+\t\t\tcontinue;\n+\n+\t\tverb = parts.items[state.consumed_nr++].string;\n \n \t\tif (!strcmp(verb, \"end\")) {\n \t\t\tjw_end(&jw);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-string\")) {\n-\t\t\tget_s(line_nr, &key);\n-\t\t\tget_s(line_nr, &s_value);\n+\t\t\tget_s(&state, &key);\n+\t\t\tget_s(&state, &s_value);\n \t\t\tjw_object_string(&jw, key, s_value);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-int\")) {\n-\t\t\tget_s(line_nr, &key);\n-\t\t\tget_i(line_nr, &i_value);\n+\t\t\tget_s(&state, &key);\n+\t\t\tget_i(&state, &i_value);\n \t\t\tjw_object_intmax(&jw, key, i_value);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-double\")) {\n-\t\t\tget_s(line_nr, &key);\n-\t\t\tget_i(line_nr, &i_value);\n-\t\t\tget_d(line_nr, &d_value);\n+\t\t\tget_s(&state, &key);\n+\t\t\tget_i(&state, &i_value);\n+\t\t\tget_d(&state, &d_value);\n \t\t\tjw_object_double(&jw, key, i_value, d_value);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-true\")) {\n-\t\t\tget_s(line_nr, &key);\n+\t\t\tget_s(&state, &key);\n \t\t\tjw_object_true(&jw, key);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-false\")) {\n-\t\t\tget_s(line_nr, &key);\n+\t\t\tget_s(&state, &key);\n \t\t\tjw_object_false(&jw, key);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-null\")) {\n-\t\t\tget_s(line_nr, &key);\n+\t\t\tget_s(&state, &key);\n \t\t\tjw_object_null(&jw, key);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-object\")) {\n-\t\t\tget_s(line_nr, &key);\n+\t\t\tget_s(&state, &key);\n \t\t\tjw_object_inline_begin_object(&jw, key);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-array\")) {\n-\t\t\tget_s(line_nr, &key);\n+\t\t\tget_s(&state, &key);\n \t\t\tjw_object_inline_begin_array(&jw, key);\n \t\t}\n \t\telse if (!strcmp(verb, \"array-string\")) {\n-\t\t\tget_s(line_nr, &s_value);\n+\t\t\tget_s(&state, &s_value);\n \t\t\tjw_array_string(&jw, s_value);\n \t\t}\n \t\telse if (!strcmp(verb, \"array-int\")) {\n-\t\t\tget_i(line_nr, &i_value);\n+\t\t\tget_i(&state, &i_value);\n \t\t\tjw_array_intmax(&jw, i_value);\n \t\t}\n \t\telse if (!strcmp(verb, \"array-double\")) {\n-\t\t\tget_i(line_nr, &i_value);\n-\t\t\tget_d(line_nr, &d_value);\n+\t\t\tget_i(&state, &i_value);\n+\t\t\tget_d(&state, &d_value);\n \t\t\tjw_array_double(&jw, i_value, d_value);\n \t\t}\n \t\telse if (!strcmp(verb, \"array-true\"))\n@@ -552,6 +571,7 @@ static int scripted(void)\n \tprintf(\"%s\\n\", jw.json.buf);\n \n \tjw_release(&jw);\n+\tstring_list_clear(&parts, 0);\n \treturn 0;\n }\n \n-- \n2.40.0.358.g56d2318a6d\n\n"},{"id":"475650","messageId":"56d2318a6d0aa17e2d56cd7d7e755adae89f8d99.1681845518.git.me@ttaylorr.com","threadId":"59592","inReplyTo":"cover.1681845518.git.me@ttaylorr.com","subject":"[PATCH v2 6/6] banned.h: mark `strtok()` as banned","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T19:18:59Z","receivedAt":"2023-04-18T19:20:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"`strtok_r()` is reentrant, but `strtok()` is not, meaning that using it\nis not thread-safe.\n\n`strtok()` has a couple of drawbacks that make it undesirable to have\nany new instances. In addition to being thread-unsafe, it also\nencourages confusing data flows, where `strtok()` may be called from\nmultiple functions with its first argument as NULL, making it unclear\nfrom the immediate context which string is being tokenized.\n\nNow that we have removed all instances of `strtok()` from the tree,\nlet's ban `strtok()` to avoid introducing new ones in the future. If new\ncallers should arise, they can either use:\n\n - `string_list_split_in_place()`,\n - `string_list_split_in_place_multi()`, or\n - `strtok_r()`.\n\nCallers are encouraged to use either of the string_list functions when\nappropriate over `strtok_r()`, since the latter suffers from the same\nconfusing data-flow problem as `strtok()` does.\n\nBut callers may prefer `strtok_r()` when the number of tokens in a given\nstring is unknown, and they want to split and process them one at a\ntime, so `strtok_r()` is left off the banned.h list.\n\nSigned-off-by: Taylor Blau \n---\n banned.h | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/banned.h b/banned.h\nindex 6ccf46bc19..dd43ab3178 100644\n--- a/banned.h\n+++ b/banned.h\n@@ -18,6 +18,8 @@\n #define strncpy(x,y,n) BANNED(strncpy)\n #undef strncat\n #define strncat(x,y,n) BANNED(strncat)\n+#undef strtok\n+#define strtok(x,y) BANNED(strtok)\n \n #undef sprintf\n #undef vsprintf\n-- \n2.40.0.358.g56d2318a6d\n"},{"id":"475653","messageId":"xmqqleipc73v.fsf@gitster.g","threadId":"59592","inReplyTo":"6658b231a906dde6acbe7ce156da693ef7dc40e6.1681845518.git.me@ttaylorr.com","subject":"Re: [PATCH v2 1/6] string-list: introduce `string_list_split_in_place_multi()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-18T19:39:48Z","receivedAt":"2023-04-18T19:39:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Introduce a variant of the `string_list_split_in_place()` function that\n> takes a string of accepted delimiters.\n>\n> By contrast to its cousin `string_list_split_in_place()` which splits\n> the given string at every instance of the single character `delim`, the\n> `_multi` variant splits the given string any any character appearing in\n> the string `delim`.\n>\n> Like `strtok()`, the `_multi` variant skips past sequential delimiting\n> characters. For example:\n>\n> string_list_split_in_place(&xs, xstrdup(\"foo::bar::baz\"), \":\", -1);\n>\n> would place in `xs` the elements \"foo\", \"bar\", and \"baz\".\n\nstrtok() also skips leading and trailing delimiters, i.e. the above\nwill give you identical result for \":foo:bar:baz:\".\n\nIt would be useful to test that here in addition to the existing ones.\n\n> +for test_fn in test_split test_split_in_place_multi\n> +do\n> +\t$test_fn \"foo:bar:baz\" \":\" \"-1\" <<-\\EOF\n> +\t3\n> +\t[0]: \"foo\"\n> +\t[1]: \"bar\"\n> +\t[2]: \"baz\"\n> +\tEOF\n> \n> +\t$test_fn \"foo:bar:baz\" \":\" \"0\" <<-\\EOF\n> +\t1\n> +\t[0]: \"foo:bar:baz\"\n> +\tEOF\n> \n> +\t$test_fn \"foo:bar:baz\" \":\" \"1\" <<-\\EOF\n> +\t2\n> +\t[0]: \"foo\"\n> +\t[1]: \"bar:baz\"\n> +\tEOF\n> \n> +\t$test_fn \"foo:bar:baz\" \":\" \"2\" <<-\\EOF\n> +\t3\n> +\t[0]: \"foo\"\n> +\t[1]: \"bar\"\n> +\t[2]: \"baz\"\n> +\tEOF\n"},{"id":"475672","messageId":"ZD8Df1iZaUGzodrg@nand.local","threadId":"59592","inReplyTo":"xmqqleipc73v.fsf@gitster.g","subject":"Re: [PATCH v2 1/6] string-list: introduce `string_list_split_in_place_multi()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T20:54:23Z","receivedAt":"2023-04-18T20:54:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 18, 2023 at 12:39:48PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > Introduce a variant of the `string_list_split_in_place()` function that\n> > takes a string of accepted delimiters.\n> >\n> > By contrast to its cousin `string_list_split_in_place()` which splits\n> > the given string at every instance of the single character `delim`, the\n> > `_multi` variant splits the given string any any character appearing in\n> > the string `delim`.\n> >\n> > Like `strtok()`, the `_multi` variant skips past sequential delimiting\n> > characters. For example:\n> >\n> > string_list_split_in_place(&xs, xstrdup(\"foo::bar::baz\"), \":\", -1);\n> >\n> > would place in `xs` the elements \"foo\", \"bar\", and \"baz\".\n>\n> strtok() also skips leading and trailing delimiters, i.e. the above\n> will give you identical result for \":foo:bar:baz:\".\n\nI'm not sure the results are identical. Adding this test case for\ntesting the behavior of string_list_split_in_place() passes before and\nafter this series:\n\n--- 8< ---\ndiff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\nindex 9c5094616a..dfe970a566 100755\n--- a/t/t0063-string-list.sh\n+++ b/t/t0063-string-list.sh\n@@ -72,6 +72,15 @@ test_split \":\" \":\" \"-1\" <8 ---\n\n> It would be useful to test that here in addition to the existing ones.\n\nSure. FWIW, the behavior for string_list_split_in_place_multi() is\nslightly different, since it will eat up all of the leading delimiter\ntokens and treat the first token as \"foo\".\n\nHere's a diff that could be squashed into this patch which captures both\ncases:\n\n--- 8< ---\ndiff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\nindex 9c5094616a..efc84dc124 100755\n--- a/t/t0063-string-list.sh\n+++ b/t/t0063-string-list.sh\n@@ -72,6 +72,15 @@ test_split \":\" \":\" \"-1\" <8 ---\n\n\nThanks,\nTaylor\n"},{"id":"475829","messageId":"20230422111213.GB2969939@coredump.intra.peff.net","threadId":"59592","inReplyTo":"6658b231a906dde6acbe7ce156da693ef7dc40e6.1681845518.git.me@ttaylorr.com","subject":"Re: [PATCH v2 1/6] string-list: introduce `string_list_split_in_place_multi()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-22T11:12:13Z","receivedAt":"2023-04-22T11:12:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 18, 2023 at 03:18:43PM -0400, Taylor Blau wrote:\n\n> Introduce a variant of the `string_list_split_in_place()` function that\n> takes a string of accepted delimiters.\n> \n> By contrast to its cousin `string_list_split_in_place()` which splits\n> the given string at every instance of the single character `delim`, the\n> `_multi` variant splits the given string any any character appearing in\n> the string `delim`.\n> \n> Like `strtok()`, the `_multi` variant skips past sequential delimiting\n> characters. For example:\n> \n> string_list_split_in_place(&xs, xstrdup(\"foo::bar::baz\"), \":\", -1);\n> \n> would place in `xs` the elements \"foo\", \"bar\", and \"baz\".\n\nI have mixed feelings on this.\n\nThere are multiple orthogonal options here: single/multiple delimiter,\nand how to deal with sequential delimiters (you call it \"runs\" here,\nthough I think of it inverted as \"allow empty fields\"). Plus existing\nones like maxsplit.\n\nYour underlying function, string_list_split_in_place_1(), handles these\nindependently. But it seems like a subtle gotcha that\nstring_list_split_in_place() and its _multi() variant, which purport to\ndiffer only in one dimension (representation of delimiter list), also\nsubtly differ in another dimension. Especially because it's a facet that\nmight not come up in simple tests, I can see somebody incorrectly\napplying one or the other.\n\nObviously one solution is to add the \"runs\" option to all variants. But\nI'd be hesitant to burden existing callers. So I'd propose one of:\n\n 1. Make your _1() function public, with a name like _with_options() or\n something (though the function name is sadly already quite long).\n Leave string_list_split_in_place() as a wrapper that behaves as\n now, and have the few new callers use the with_options() variant.\n\n 2. Don't bother implementing the \"runs\" version. The only users would\n be test programs, and nobody cares much either way for their cases.\n Document in the commit message (and possibly above the function)\n that this isn't a strict replacement for strtok(). That would\n hopefully be enough for a potential caller to think about the\n behavior, and we can add \"runs\" if and when somebody actually wants\n it.\n\n-Peff\n"},{"id":"475830","messageId":"20230422111433.GC2969939@coredump.intra.peff.net","threadId":"59592","inReplyTo":"2a20ad8bc5513aae912c53a294092ee5087e1873.1681845518.git.me@ttaylorr.com","subject":"Re: [PATCH v2 2/6] string-list: introduce `string_list_setlen()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-22T11:14:33Z","receivedAt":"2023-04-22T11:14:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 18, 2023 at 03:18:46PM -0400, Taylor Blau wrote:\n\n> This allows `string_list_split_in_place()` to overwrite any existing\n> entries without needing to free and re-allocate them.\n> \n> However, setting `list->nr` manually is not safe in all instances. There\n> are a couple of cases worth worrying about:\n> [...]\n\nThanks for adding this patch. Your explanation very nicely covers why\nthis is a reasonable pattern, and what pitfalls we're worried about by\ndoing it inline.\n\n> Co-authored-by: Jeff King \n> Signed-off-by: Jeff King \n\nAcking my forged sign-off. :)\n\n-Peff\n"},{"id":"475831","messageId":"20230422111657.GD2969939@coredump.intra.peff.net","threadId":"59592","inReplyTo":"0ae07dec3663d7cbb0f8662c47485c0667a879b9.1681845518.git.me@ttaylorr.com","subject":"Re: [PATCH v2 3/6] t/helper/test-hashmap.c: avoid using `strtok()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-22T11:16:57Z","receivedAt":"2023-04-22T11:17:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 18, 2023 at 03:18:49PM -0400, Taylor Blau wrote:\n\n> @@ -159,21 +161,34 @@ int cmd__hashmap(int argc, const char **argv)\n> \n> \t/* process commands from stdin */\n> \twhile (strbuf_getline(&line, stdin) != EOF) {\n> -\t\tchar *cmd, *p1 = NULL, *p2 = NULL;\n> +\t\tchar *cmd, *p1, *p2;\n> \t\tunsigned int hash = 0;\n> \t\tstruct test_entry *entry;\n> \n> +\t\t/*\n> +\t\t * Because we memdup() the arguments out of the\n> +\t\t * string_list before inserting them into the hashmap,\n> +\t\t * it's OK to set its length back to zero to avoid\n> +\t\t * re-allocating the items array once per line.\n> +\t\t *\n> +\t\t * By doing so, we'll instead overwrite the existing\n> +\t\t * entries and avoid re-allocating.\n> +\t\t */\n> +\t\tstring_list_setlen(&parts, 0);\n> \t\t/* break line into command and up to two parameters */\n> -\t\tcmd = strtok(line.buf, DELIM);\n> +\t\tstring_list_split_in_place_multi(&parts, line.buf, DELIM, 2);\n> +\n\nI'd argue we can drop this comment now. Having string_list_setlen()\nmakes it a blessed pattern, and I don't think there's anything special\nabout this caller that makes it more or less so. Obviously yes, the\nstring list items won't be valid as we enter a new loop iteration. But\nthat is always true of split_in_place(), not to mention strtok(),\nbecause we are overwriting the buffer in each loop.\n\nDitto for the later commits which have similar (if shorter) comments.\n\n-Peff\n"},{"id":"475844","messageId":"8d228041-d473-7f3f-87d7-543b350aab0e@web.de","threadId":"59592","inReplyTo":"20230422111213.GB2969939@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/6] string-list: introduce `string_list_split_in_place_multi()`","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-04-22T15:53:05Z","receivedAt":"2023-04-22T15:53:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 22.04.23 um 13:12 schrieb Jeff King:\n> On Tue, Apr 18, 2023 at 03:18:43PM -0400, Taylor Blau wrote:\n>\n>> Introduce a variant of the `string_list_split_in_place()` function that\n>> takes a string of accepted delimiters.\n>>\n>> By contrast to its cousin `string_list_split_in_place()` which splits\n>> the given string at every instance of the single character `delim`, the\n>> `_multi` variant splits the given string any any character appearing in\n>> the string `delim`.\n>>\n>> Like `strtok()`, the `_multi` variant skips past sequential delimiting\n>> characters. For example:\n>>\n>> string_list_split_in_place(&xs, xstrdup(\"foo::bar::baz\"), \":\", -1);\n>>\n>> would place in `xs` the elements \"foo\", \"bar\", and \"baz\".\n\nWhich one is it here really, string_list_split_in_place() or _multi()?\n\n> I have mixed feelings on this.\n>\n> There are multiple orthogonal options here: single/multiple delimiter,\n> and how to deal with sequential delimiters (you call it \"runs\" here,\n> though I think of it inverted as \"allow empty fields\"). Plus existing\n> ones like maxsplit.\n>\n> Your underlying function, string_list_split_in_place_1(), handles these\n> independently. But it seems like a subtle gotcha that\n> string_list_split_in_place() and its _multi() variant, which purport to\n> differ only in one dimension (representation of delimiter list), also\n> subtly differ in another dimension. Especially because it's a facet that\n> might not come up in simple tests, I can see somebody incorrectly\n> applying one or the other.\n>\n> Obviously one solution is to add the \"runs\" option to all variants. But\n> I'd be hesitant to burden existing callers. So I'd propose one of:\n>\n> 1. Make your _1() function public, with a name like _with_options() or\n> something (though the function name is sadly already quite long).\n> Leave string_list_split_in_place() as a wrapper that behaves as\n> now, and have the few new callers use the with_options() variant.\n>\n> 2. Don't bother implementing the \"runs\" version. The only users would\n> be test programs, and nobody cares much either way for their cases.\n> Document in the commit message (and possibly above the function)\n> that this isn't a strict replacement for strtok(). That would\n> hopefully be enough for a potential caller to think about the\n> behavior, and we can add \"runs\" if and when somebody actually wants\n> it.\n\nYou can call string_list_remove_empty_items() to get rid of empty\nstrings. And if the single-char version calls a multi-char version\nunder the hood then its only advantage is backward compatibility -- but\nconverting all callers isn't that hard. So the only string_list split\nfunction version you really need accepts multiple delimiters and\npreserves empty items and can be called string_list_split_in_place().\n\nRené\n\n"},{"id":"475886","messageId":"20230423003553.GA3953216@coredump.intra.peff.net","threadId":"59592","inReplyTo":"8d228041-d473-7f3f-87d7-543b350aab0e@web.de","subject":"Re: [PATCH v2 1/6] string-list: introduce `string_list_split_in_place_multi()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-23T00:35:53Z","receivedAt":"2023-04-23T00:35:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 22, 2023 at 05:53:05PM +0200, René Scharfe wrote:\n\n> > Obviously one solution is to add the \"runs\" option to all variants. But\n> > I'd be hesitant to burden existing callers. So I'd propose one of:\n> >\n> > 1. Make your _1() function public, with a name like _with_options() or\n> > something (though the function name is sadly already quite long).\n> > Leave string_list_split_in_place() as a wrapper that behaves as\n> > now, and have the few new callers use the with_options() variant.\n> >\n> > 2. Don't bother implementing the \"runs\" version. The only users would\n> > be test programs, and nobody cares much either way for their cases.\n> > Document in the commit message (and possibly above the function)\n> > that this isn't a strict replacement for strtok(). That would\n> > hopefully be enough for a potential caller to think about the\n> > behavior, and we can add \"runs\" if and when somebody actually wants\n> > it.\n> \n> You can call string_list_remove_empty_items() to get rid of empty\n> strings. And if the single-char version calls a multi-char version\n> under the hood then its only advantage is backward compatibility -- but\n> converting all callers isn't that hard. So the only string_list split\n> function version you really need accepts multiple delimiters and\n> preserves empty items and can be called string_list_split_in_place().\n\nOoh, I like that. I didn't think of processing after the fact. And\nindeed, we have several spots already that do the split/remove_empty\npair.\n\nAnd yes, it looks like there are only 7 callers which would need a\ntrivial update if we just switched to the multi-char version.\n\nThat's very compelling.\n\n-Peff\n"},{"id":"475892","messageId":"ZESaFXMcP6mhtBOv@nand.local","threadId":"59592","inReplyTo":"20230422111213.GB2969939@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/6] string-list: introduce `string_list_split_in_place_multi()`t","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-23T02:38:13Z","receivedAt":"2023-04-23T02:38:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Apr 22, 2023 at 07:12:13AM -0400, Jeff King wrote:\n> On Tue, Apr 18, 2023 at 03:18:43PM -0400, Taylor Blau wrote:\n>\n> > Introduce a variant of the `string_list_split_in_place()` function that\n> > takes a string of accepted delimiters.\n> >\n> > By contrast to its cousin `string_list_split_in_place()` which splits\n> > the given string at every instance of the single character `delim`, the\n> > `_multi` variant splits the given string any any character appearing in\n> > the string `delim`.\n> >\n> > Like `strtok()`, the `_multi` variant skips past sequential delimiting\n> > characters. For example:\n> >\n> > string_list_split_in_place(&xs, xstrdup(\"foo::bar::baz\"), \":\", -1);\n> >\n> > would place in `xs` the elements \"foo\", \"bar\", and \"baz\".\n>\n> I have mixed feelings on this.\n\nHmm. I implemented it this way partially after reading your suggestion\nin [1], but also to allow using `string_list_split_in_place()` as a\nstrict replacement for strtok().\n\nAnd I agree that for the existing users of strtok(), which are all in the\ntest helpers, this probably doesn't matter much either way. Though I\nfeel like since we are banning strtok() over the whole tree, that we\nshould provide a suitable replacement at the time we ban strtok(), not\nat the time somebody needs to rely on its non-empty fields behavior.\n\n[1]: https://lore.kernel.org/git/20230418102320.GB508219@coredump.intra.peff.net/\n\n> Obviously one solution is to add the \"runs\" option to all variants. But\n> I'd be hesitant to burden existing callers. So I'd propose one of:\n>\n> 1. Make your _1() function public, with a name like _with_options() or\n> something (though the function name is sadly already quite long).\n> Leave string_list_split_in_place() as a wrapper that behaves as\n> now, and have the few new callers use the with_options() variant.\n\nI think that in general I'd prefer (2) to avoid polluting the list of\ndeclarations in string-list.h, but in this case I think that in this\ncase it is the right thing to do.\n\nThanks,\nTaylor\n"},{"id":"475893","messageId":"ZESaryIvTOPiKWZK@nand.local","threadId":"59592","inReplyTo":"ZESaFXMcP6mhtBOv@nand.local","subject":"Re: [PATCH v2 1/6] string-list: introduce `string_list_split_in_place_multi()`t","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-23T02:40:47Z","receivedAt":"2023-04-23T02:40:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Apr 22, 2023 at 10:38:13PM -0400, Taylor Blau wrote:\n> > Obviously one solution is to add the \"runs\" option to all variants. But\n> > I'd be hesitant to burden existing callers. So I'd propose one of:\n> >\n> > 1. Make your _1() function public, with a name like _with_options() or\n> > something (though the function name is sadly already quite long).\n> > Leave string_list_split_in_place() as a wrapper that behaves as\n> > now, and have the few new callers use the with_options() variant.\n>\n> I think that in general I'd prefer (2) to avoid polluting the list of\n> declarations in string-list.h, but in this case I think that in this\n> case it is the right thing to do.\n\nOh, nevermind. I just read René's solution below and it is much cleaner.\nI take it back ;-).\n\nThanks,\nTaylor\n"},{"id":"475931","messageId":"xmqqwn21fdu0.fsf@gitster.g","threadId":"59592","inReplyTo":"20230423003553.GA3953216@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/6] string-list: introduce `string_list_split_in_place_multi()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-24T16:24:23Z","receivedAt":"2023-04-24T16:24:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> And yes, it looks like there are only 7 callers which would need a\n> trivial update if we just switched to the multi-char version.\n>\n> That's very compelling.\n\nSounds good. Thanks.\n"},{"id":"475974","messageId":"ZEbyVDWhSGAyKDJL@nand.local","threadId":"59592","inReplyTo":"20230422111657.GD2969939@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/6] t/helper/test-hashmap.c: avoid using `strtok()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-24T21:19:16Z","receivedAt":"2023-04-24T21:19:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Apr 22, 2023 at 07:16:57AM -0400, Jeff King wrote:\n> On Tue, Apr 18, 2023 at 03:18:49PM -0400, Taylor Blau wrote:\n>\n> > @@ -159,21 +161,34 @@ int cmd__hashmap(int argc, const char **argv)\n> >\n> > \t/* process commands from stdin */\n> > \twhile (strbuf_getline(&line, stdin) != EOF) {\n> > -\t\tchar *cmd, *p1 = NULL, *p2 = NULL;\n> > +\t\tchar *cmd, *p1, *p2;\n> > \t\tunsigned int hash = 0;\n> > \t\tstruct test_entry *entry;\n> >\n> > +\t\t/*\n> > +\t\t * Because we memdup() the arguments out of the\n> > +\t\t * string_list before inserting them into the hashmap,\n> > +\t\t * it's OK to set its length back to zero to avoid\n> > +\t\t * re-allocating the items array once per line.\n> > +\t\t *\n> > +\t\t * By doing so, we'll instead overwrite the existing\n> > +\t\t * entries and avoid re-allocating.\n> > +\t\t */\n> > +\t\tstring_list_setlen(&parts, 0);\n> > \t\t/* break line into command and up to two parameters */\n> > -\t\tcmd = strtok(line.buf, DELIM);\n> > +\t\tstring_list_split_in_place_multi(&parts, line.buf, DELIM, 2);\n> > +\n>\n> I'd argue we can drop this comment now. Having string_list_setlen()\n> makes it a blessed pattern, and I don't think there's anything special\n> about this caller that makes it more or less so. Obviously yes, the\n> string list items won't be valid as we enter a new loop iteration. But\n> that is always true of split_in_place(), not to mention strtok(),\n> because we are overwriting the buffer in each loop.\n\nAgreed, I think that part of the point of string_list_setlen() is that\nthis is a blessed pattern, so shouldn't need a comment.\n\nThanks,\nTaylor\n"},{"id":"475981","messageId":"cover.1682374789.git.me@ttaylorr.com","threadId":"59592","inReplyTo":"cover.1681428696.git.me@ttaylorr.com","subject":"[PATCH v3 0/6] banned: mark `strok()`, `strtok_r()` as banned","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-24T22:20:07Z","receivedAt":"2023-04-24T22:20:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is another medium-sized reroll of my series to add `strtok()` (and\n`strtok_r()`!) to the list of banned functions.\n\nNotable changes include:\n\n - Dropped `string_list_split_in_place_multi()` in favor of a\n combination of `string_list_split_in_place()` and\n `string_list_remove_empty_items()`.\n\n - `strtok_r()` is back on the banned list, with a more realistic sales\n pitch.\n\nAs always, a range-diff is included below for convenience. Thanks in\nadvance for your review!\n\n[1]: https://lore.kernel.org/git/cover.1681428696.git.me@ttaylorr.com/\n\nTaylor Blau (6):\n string-list: multi-delimiter `string_list_split_in_place()`\n string-list: introduce `string_list_setlen()`\n t/helper/test-hashmap.c: avoid using `strtok()`\n t/helper/test-oidmap.c: avoid using `strtok()`\n t/helper/test-json-writer.c: avoid using `strtok()`\n banned.h: mark `strtok()` and `strtok_r()` as banned\n\n banned.h | 4 ++\n builtin/gc.c | 4 +-\n diff.c | 2 +-\n notes.c | 2 +-\n refs/packed-backend.c | 2 +-\n string-list.c | 13 ++++++-\n string-list.h | 12 +++++-\n t/helper/test-hashmap.c | 22 +++++++----\n t/helper/test-json-writer.c | 76 +++++++++++++++++++++++--------------\n t/helper/test-oidmap.c | 20 +++++++---\n t/helper/test-string-list.c | 4 +-\n t/t0063-string-list.sh | 51 +++++++++++++++++++++++++\n 12 files changed, 161 insertions(+), 51 deletions(-)\n\nRange-diff against v2:\n1: 6658b231a9 ! 1: 59d3e778b6 string-list: introduce `string_list_split_in_place_multi()`\n @@ Metadata\n Author: Taylor Blau \n \n ## Commit message ##\n - string-list: introduce `string_list_split_in_place_multi()`\n + string-list: multi-delimiter `string_list_split_in_place()`\n \n - Introduce a variant of the `string_list_split_in_place()` function that\n - takes a string of accepted delimiters.\n -\n - By contrast to its cousin `string_list_split_in_place()` which splits\n - the given string at every instance of the single character `delim`, the\n - `_multi` variant splits the given string any any character appearing in\n - the string `delim`.\n -\n - Like `strtok()`, the `_multi` variant skips past sequential delimiting\n - characters. For example:\n -\n - string_list_split_in_place(&xs, xstrdup(\"foo::bar::baz\"), \":\", -1);\n -\n - would place in `xs` the elements \"foo\", \"bar\", and \"baz\".\n + Enhance `string_list_split_in_place()` to accept multiple characters as\n + delimiters instead of a single character.\n \n Instead of using `strchr(2)` to locate the first occurrence of the given\n delimiter character, `string_list_split_in_place_multi()` uses\n `strcspn(2)` to move past the initial segment of characters comprised of\n any characters in the delimiting set.\n \n - When only a single delimiting character is provided, `strcspn(2)` has\n - equivalent performance to `strchr(2)`. Modern `strcspn(2)`\n - implementations treat an empty delimiter or the singleton delimiter as a\n - special case and fall back to calling strchrnul(). Both glibc[1] and\n - musl[2] implement `strcspn(2)` this way.\n + When only a single delimiting character is provided, `strpbrk(2)` (which\n + is implemented with `strcspn(2)`) has equivalent performance to\n + `strchr(2)`. Modern `strcspn(2)` implementations treat an empty\n + delimiter or the singleton delimiter as a special case and fall back to\n + calling strchrnul(). Both glibc[1] and musl[2] implement `strcspn(2)`\n + this way.\n \n - Since the `_multi` variant is a generalization of the original\n - implementation, reimplement `string_list_split_in_place()` in terms of\n - the more general function by providing a single-character string for the\n - list of accepted delimiters.\n + This change is one step to removing `strtok(2)` from the tree. Note that\n + `string_list_split_in_place()` is not a strict replacement for\n + `strtok()`, since it will happily turn sequential delimiter characters\n + into empty entries in the resulting string_list. For example:\n \n - To avoid regressions, update t0063 in this patch as well. Any \"common\"\n - test cases (i.e., those that produce the same result whether you call\n - `string_list_split()` or `string_list_split_in_place_multi()`) are\n - grouped into a loop which is parameterized over the function to test.\n + string_list_split_in_place(&xs, \"foo:;:bar:;:baz\", \":;\", -1)\n \n - Any cases which aren't common (of which there is one existing case, and\n - a handful of new ones added which are specific to the `_multi` variant)\n - are tested independently.\n + would yield a string list of:\n +\n + [\"foo\", \"\", \"\", \"bar\", \"\", \"\", \"baz\"]\n +\n + Callers that wish to emulate the behavior of strtok(2) more directly\n + should call `string_list_remove_empty_items()` after splitting.\n +\n + To avoid regressions for the new multi-character delimter cases, update\n + t0063 in this patch as well.\n \n [1]: https://sourceware.org/git/?p=glibc.git;a=blob;f=string/strcspn.c;hb=glibc-2.37#l35\n [2]: https://git.musl-libc.org/cgit/musl/tree/src/string/strcspn.c?h=v1.2.3#n11\n \n Signed-off-by: Taylor Blau \n \n + ## builtin/gc.c ##\n +@@ builtin/gc.c: static int get_schedule_cmd(const char **cmd, int *is_available)\n + \tif (is_available)\n + \t\t*is_available = 0;\n + \n +-\tstring_list_split_in_place(&list, testing, ',', -1);\n ++\tstring_list_split_in_place(&list, testing, \",\", -1);\n + \tfor_each_string_list_item(item, &list) {\n + \t\tstruct string_list pair = STRING_LIST_INIT_NODUP;\n + \n +-\t\tif (string_list_split_in_place(&pair, item->string, ':', 2) != 2)\n ++\t\tif (string_list_split_in_place(&pair, item->string, \":\", 2) != 2)\n + \t\t\tcontinue;\n + \n + \t\tif (!strcmp(*cmd, pair.items[0].string)) {\n +\n + ## diff.c ##\n +@@ diff.c: static int parse_dirstat_params(struct diff_options *options, const char *params\n + \tint i;\n + \n + \tif (*params_copy)\n +-\t\tstring_list_split_in_place(¶ms, params_copy, ',', -1);\n ++\t\tstring_list_split_in_place(¶ms, params_copy, \",\", -1);\n + \tfor (i = 0; i < params.nr; i++) {\n + \t\tconst char *p = params.items[i].string;\n + \t\tif (!strcmp(p, \"changes\")) {\n +\n + ## notes.c ##\n +@@ notes.c: void string_list_add_refs_from_colon_sep(struct string_list *list,\n + \tchar *globs_copy = xstrdup(globs);\n + \tint i;\n + \n +-\tstring_list_split_in_place(&split, globs_copy, ':', -1);\n ++\tstring_list_split_in_place(&split, globs_copy, \":\", -1);\n + \tstring_list_remove_empty_items(&split, 0);\n + \n + \tfor (i = 0; i < split.nr; i++)\n +\n + ## refs/packed-backend.c ##\n +@@ refs/packed-backend.c: static struct snapshot *create_snapshot(struct packed_ref_store *refs)\n + \t\t\t\t\t snapshot->buf,\n + \t\t\t\t\t snapshot->eof - snapshot->buf);\n + \n +-\t\tstring_list_split_in_place(&traits, p, ' ', -1);\n ++\t\tstring_list_split_in_place(&traits, p, \" \", -1);\n + \n + \t\tif (unsorted_string_list_has_string(&traits, \"fully-peeled\"))\n + \t\t\tsnapshot->peeled = PEELED_FULLY;\n +\n ## string-list.c ##\n @@ string-list.c: int string_list_split(struct string_list *list, const char *string,\n - \t}\n }\n \n --int string_list_split_in_place(struct string_list *list, char *string,\n + int string_list_split_in_place(struct string_list *list, char *string,\n -\t\t\t int delim, int maxsplit)\n -+static int string_list_split_in_place_1(struct string_list *list, char *string,\n -+\t\t\t\t\tconst char *delim, int maxsplit,\n -+\t\t\t\t\tunsigned runs)\n ++\t\t\t const char *delim, int maxsplit)\n {\n \tint count = 0;\n \tchar *p = string, *end;\n @@ string-list.c: int string_list_split_in_place(struct string_list *list, char *string,\n - \t\tdie(\"internal error in string_list_split_in_place(): \"\n - \t\t \"list->strdup_strings must not be set\");\n - \tfor (;;) {\n -+\t\tif (runs)\n -+\t\t\tp += strspn(p, delim);\n -+\n - \t\tcount++;\n - \t\tif (maxsplit >= 0 && count > maxsplit) {\n \t\t\tstring_list_append(list, p);\n \t\t\treturn count;\n \t\t}\n -\t\tend = strchr(p, delim);\n --\t\tif (end) {\n -+\t\tend = p + strcspn(p, delim);\n -+\t\tif (end && *end) {\n ++\t\tend = strpbrk(p, delim);\n + \t\tif (end) {\n \t\t\t*end = '\\0';\n \t\t\tstring_list_append(list, p);\n - \t\t\tp = end + 1;\n -@@ string-list.c: int string_list_split_in_place(struct string_list *list, char *string,\n - \t\t}\n - \t}\n - }\n -+\n -+int string_list_split_in_place_multi(struct string_list *list, char *string,\n -+\t\t\t\t const char *delim, int maxsplit)\n -+{\n -+\treturn string_list_split_in_place_1(list, string, delim, maxsplit, 1);\n -+}\n -+\n -+int string_list_split_in_place(struct string_list *list, char *string,\n -+\t\t\t int delim, int maxsplit)\n -+{\n -+\tchar delim_s[2] = { delim, 0 };\n -+\n -+\treturn string_list_split_in_place_1(list, string, delim_s, maxsplit, 0);\n -+}\n \n ## string-list.h ##\n @@ string-list.h: int string_list_split(struct string_list *list, const char *string,\n - * new string_list_items point into string (which therefore must not\n - * be modified or freed while the string_list is in use).\n * list->strdup_strings must *not* be set.\n -+ *\n -+ * The \"_multi\" variant splits the given string on any character\n -+ * appearing in \"delim\", and the non-\"_multi\" variant splits only on the\n -+ * given character. The \"_multi\" variant behaves like `strtok()` where\n -+ * no element contains the delimiting byte(s).\n */\n -+int string_list_split_in_place_multi(struct string_list *list, char *string,\n -+\t\t\t\t const char *delim, int maxsplit);\n int string_list_split_in_place(struct string_list *list, char *string,\n - \t\t\t int delim, int maxsplit);\n +-\t\t\t int delim, int maxsplit);\n ++\t\t\t const char *delim, int maxsplit);\n #endif /* STRING_LIST_H */\n \n ## t/helper/test-string-list.c ##\n @@ t/helper/test-string-list.c: int cmd__string_list(int argc, const char **argv)\n - \t\treturn 0;\n - \t}\n - \n -+\tif (argc == 5 && !strcmp(argv[1], \"split_in_place_multi\")) {\n -+\t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n -+\t\tint i;\n -+\t\tchar *s = xstrdup(argv[2]);\n + \t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n + \t\tint i;\n + \t\tchar *s = xstrdup(argv[2]);\n +-\t\tint delim = *argv[3];\n +\t\tconst char *delim = argv[3];\n -+\t\tint maxsplit = atoi(argv[4]);\n -+\n -+\t\ti = string_list_split_in_place_multi(&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.\n + \t\tint maxsplit = atoi(argv[4]);\n + \n + \t\ti = string_list_split_in_place(&list, s, delim, maxsplit);\n +@@ t/helper/test-string-list.c: int cmd__string_list(int argc, const char **argv)\n + \t\t */\n + \t\tif (sb.len && sb.buf[sb.len - 1] == '\\n')\n + \t\t\tstrbuf_setlen(&sb, sb.len - 1);\n +-\t\tstring_list_split_in_place(&list, sb.buf, '\\n', -1);\n ++\t\tstring_list_split_in_place(&list, sb.buf, \"\\n\", -1);\n + \n + \t\tstring_list_sort(&list);\n + \n \n ## t/t0063-string-list.sh ##\n @@ t/t0063-string-list.sh: test_split () {\n \t\"\n }\n \n --test_split \"foo:bar:baz\" \":\" \"-1\" <expected &&\n -+\ttest_expect_success \"split_in_place_multi $1 at $2, max $3\" \"\n -+\t\ttest-tool string-list split_in_place_multi '$1' '$2' '$3' >actual &&\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 \"foo:bar:baz\" \":\" \"0\" <\n \n @@ t/helper/test-hashmap.c: int cmd__hashmap(int argc, const char **argv)\n \t\tunsigned int hash = 0;\n \t\tstruct test_entry *entry;\n \n -+\t\t/*\n -+\t\t * Because we memdup() the arguments out of the\n -+\t\t * string_list before inserting them into the hashmap,\n -+\t\t * it's OK to set its length back to zero to avoid\n -+\t\t * re-allocating the items array once per line.\n -+\t\t *\n -+\t\t * By doing so, we'll instead overwrite the existing\n -+\t\t * entries and avoid re-allocating.\n -+\t\t */\n -+\t\tstring_list_setlen(&parts, 0);\n \t\t/* break line into command and up to two parameters */\n -\t\tcmd = strtok(line.buf, DELIM);\n -+\t\tstring_list_split_in_place_multi(&parts, line.buf, DELIM, 2);\n ++\t\tstring_list_setlen(&parts, 0);\n ++\t\tstring_list_split_in_place(&parts, line.buf, DELIM, 2);\n ++\t\tstring_list_remove_empty_items(&parts, 0);\n +\n \t\t/* ignore empty lines */\n -\t\tif (!cmd || *cmd == '#')\n4: a659431e9c ! 4: c9b929406a t/helper/test-oidmap.c: avoid using `strtok()`\n @@ t/helper/test-oidmap.c: int cmd__oidmap(int argc UNUSED, const char **argv UNUSE\n \t\tstruct test_entry *entry;\n \t\tstruct object_id oid;\n \n -+\t\t/* see the comment in cmd__hashmap() */\n -+\t\tstring_list_setlen(&parts, 0);\n \t\t/* break line into command and up to two parameters */\n -\t\tcmd = strtok(line.buf, DELIM);\n -+\t\tstring_list_split_in_place_multi(&parts, line.buf, DELIM, 2);\n ++\t\tstring_list_setlen(&parts, 0);\n ++\t\tstring_list_split_in_place(&parts, line.buf, DELIM, 2);\n ++\t\tstring_list_remove_empty_items(&parts, 0);\n +\n \t\t/* ignore empty lines */\n -\t\tif (!cmd || *cmd == '#')\n5: fc6cd23698 ! 5: 201fcac6c4 t/helper/test-json-writer.c: avoid using `strtok()`\n @@ t/helper/test-json-writer.c: static int scripted(void)\n +\t\tstate.nr = ++line_nr;\n \n -\t\tverb = strtok(line, \" \");\n -+\t\t/* see the comment in cmd__hashmap() */\n -+\t\tstring_list_setlen(&parts, 0);\n +\t\t/* break line into command and zero or more tokens */\n -+\t\tstring_list_split_in_place(&parts, line, ' ', -1);\n ++\t\tstring_list_setlen(&parts, 0);\n ++\t\tstring_list_split_in_place(&parts, line, \" \", -1);\n ++\t\tstring_list_remove_empty_items(&parts, 0);\n +\n +\t\t/* ignore empty lines */\n +\t\tif (!parts.nr || !*parts.items[0].string)\n6: 56d2318a6d ! 6: da896aa358 banned.h: mark `strtok()` as banned\n @@ Metadata\n Author: Taylor Blau \n \n ## Commit message ##\n - banned.h: mark `strtok()` as banned\n -\n - `strtok_r()` is reentrant, but `strtok()` is not, meaning that using it\n - is not thread-safe.\n + banned.h: mark `strtok()` and `strtok_r()` as banned\n \n `strtok()` has a couple of drawbacks that make it undesirable to have\n any new instances. In addition to being thread-unsafe, it also\n @@ Commit message\n \n Now that we have removed all instances of `strtok()` from the tree,\n let's ban `strtok()` to avoid introducing new ones in the future. If new\n - callers should arise, they can either use:\n + callers should arise, they are encouraged to use\n + `string_list_split_in_place()` (and `string_list_remove_empty_items()`,\n + if applicable).\n \n - - `string_list_split_in_place()`,\n - - `string_list_split_in_place_multi()`, or\n - - `strtok_r()`.\n + string_list_split_in_place() is not a perfect drop-in replacement\n + for `strtok_r()`, particularly if the caller is processing a string with\n + an arbitrary number of tokens, and wants to process each token one at a\n + time.\n \n - Callers are encouraged to use either of the string_list functions when\n - appropriate over `strtok_r()`, since the latter suffers from the same\n - confusing data-flow problem as `strtok()` does.\n -\n - But callers may prefer `strtok_r()` when the number of tokens in a given\n - string is unknown, and they want to split and process them one at a\n - time, so `strtok_r()` is left off the banned.h list.\n + But there are no instances of this in Git's tree which are more\n + well-suited to `strtok_r()` than the friendlier\n + `string_list_in_place()`, so ban `strtok_r()`, too.\n \n Signed-off-by: Taylor Blau \n \n @@ banned.h\n #define strncat(x,y,n) BANNED(strncat)\n +#undef strtok\n +#define strtok(x,y) BANNED(strtok)\n ++#undef strtok_r\n ++#define strtok_r(x,y,z) BANNED(strtok_r)\n \n #undef sprintf\n #undef vsprintf\n-- \n2.40.0.380.gd2df7d2365\n"},{"id":"475982","messageId":"59d3e778b6c80b6bd10c1126cdc3f7e86558f3e7.1682374789.git.me@ttaylorr.com","threadId":"59592","inReplyTo":"cover.1682374789.git.me@ttaylorr.com","subject":"[PATCH v3 1/6] string-list: multi-delimiter `string_list_split_in_place()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-24T22:20:10Z","receivedAt":"2023-04-24T22:20:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Enhance `string_list_split_in_place()` to accept multiple characters as\ndelimiters instead of a single character.\n\nInstead of using `strchr(2)` to locate the first occurrence of the given\ndelimiter character, `string_list_split_in_place_multi()` uses\n`strcspn(2)` to move past the initial segment of characters comprised of\nany characters in the delimiting set.\n\nWhen only a single delimiting character is provided, `strpbrk(2)` (which\nis implemented with `strcspn(2)`) has equivalent performance to\n`strchr(2)`. Modern `strcspn(2)` implementations treat an empty\ndelimiter or the singleton delimiter as a special case and fall back to\ncalling strchrnul(). Both glibc[1] and musl[2] implement `strcspn(2)`\nthis way.\n\nThis change is one step to removing `strtok(2)` from the tree. Note that\n`string_list_split_in_place()` is not a strict replacement for\n`strtok()`, since it will happily turn sequential delimiter characters\ninto empty entries in the resulting string_list. For example:\n\n string_list_split_in_place(&xs, \"foo:;:bar:;:baz\", \":;\", -1)\n\nwould yield a string list of:\n\n [\"foo\", \"\", \"\", \"bar\", \"\", \"\", \"baz\"]\n\nCallers that wish to emulate the behavior of strtok(2) more directly\nshould call `string_list_remove_empty_items()` after splitting.\n\nTo avoid regressions for the new multi-character delimter cases, update\nt0063 in this patch as well.\n\n[1]: https://sourceware.org/git/?p=glibc.git;a=blob;f=string/strcspn.c;hb=glibc-2.37#l35\n[2]: https://git.musl-libc.org/cgit/musl/tree/src/string/strcspn.c?h=v1.2.3#n11\n\nSigned-off-by: Taylor Blau \n---\n builtin/gc.c | 4 +--\n diff.c | 2 +-\n notes.c | 2 +-\n refs/packed-backend.c | 2 +-\n string-list.c | 4 +--\n string-list.h | 2 +-\n t/helper/test-string-list.c | 4 +--\n t/t0063-string-list.sh | 51 +++++++++++++++++++++++++++++++++++++\n 8 files changed, 61 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex edd98d35a5..f68e976704 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1687,11 +1687,11 @@ static int get_schedule_cmd(const char **cmd, int *is_available)\n \tif (is_available)\n \t\t*is_available = 0;\n \n-\tstring_list_split_in_place(&list, testing, ',', -1);\n+\tstring_list_split_in_place(&list, testing, \",\", -1);\n \tfor_each_string_list_item(item, &list) {\n \t\tstruct string_list pair = STRING_LIST_INIT_NODUP;\n \n-\t\tif (string_list_split_in_place(&pair, item->string, ':', 2) != 2)\n+\t\tif (string_list_split_in_place(&pair, item->string, \":\", 2) != 2)\n \t\t\tcontinue;\n \n \t\tif (!strcmp(*cmd, pair.items[0].string)) {\ndiff --git a/diff.c b/diff.c\nindex 78b0fdd8ca..378a0248e1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -134,7 +134,7 @@ static int parse_dirstat_params(struct diff_options *options, const char *params\n \tint i;\n \n \tif (*params_copy)\n-\t\tstring_list_split_in_place(¶ms, params_copy, ',', -1);\n+\t\tstring_list_split_in_place(¶ms, params_copy, \",\", -1);\n \tfor (i = 0; i < params.nr; i++) {\n \t\tconst char *p = params.items[i].string;\n \t\tif (!strcmp(p, \"changes\")) {\ndiff --git a/notes.c b/notes.c\nindex 45fb7f22d1..eee806f626 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -963,7 +963,7 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,\n \tchar *globs_copy = xstrdup(globs);\n \tint i;\n \n-\tstring_list_split_in_place(&split, globs_copy, ':', -1);\n+\tstring_list_split_in_place(&split, globs_copy, \":\", -1);\n \tstring_list_remove_empty_items(&split, 0);\n \n \tfor (i = 0; i < split.nr; i++)\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 1eba1015dd..cc903baa7e 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -650,7 +650,7 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs)\n \t\t\t\t\t snapshot->buf,\n \t\t\t\t\t snapshot->eof - snapshot->buf);\n \n-\t\tstring_list_split_in_place(&traits, p, ' ', -1);\n+\t\tstring_list_split_in_place(&traits, p, \" \", -1);\n \n \t\tif (unsorted_string_list_has_string(&traits, \"fully-peeled\"))\n \t\t\tsnapshot->peeled = PEELED_FULLY;\ndiff --git a/string-list.c b/string-list.c\nindex db473f273e..5f5b60fe1c 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -301,7 +301,7 @@ int string_list_split(struct string_list *list, const char *string,\n }\n \n int string_list_split_in_place(struct string_list *list, char *string,\n-\t\t\t int delim, int maxsplit)\n+\t\t\t const char *delim, int maxsplit)\n {\n \tint count = 0;\n \tchar *p = string, *end;\n@@ -315,7 +315,7 @@ int string_list_split_in_place(struct string_list *list, char *string,\n \t\t\tstring_list_append(list, p);\n \t\t\treturn count;\n \t\t}\n-\t\tend = strchr(p, delim);\n+\t\tend = strpbrk(p, delim);\n \t\tif (end) {\n \t\t\t*end = '\\0';\n \t\t\tstring_list_append(list, p);\ndiff --git a/string-list.h b/string-list.h\nindex c7b0d5d000..77854840f7 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -270,5 +270,5 @@ int string_list_split(struct string_list *list, const char *string,\n * list->strdup_strings must *not* be set.\n */\n int string_list_split_in_place(struct string_list *list, char *string,\n-\t\t\t int delim, int maxsplit);\n+\t\t\t const char *delim, int maxsplit);\n #endif /* STRING_LIST_H */\ndiff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c\nindex 2123dda85b..63df88575c 100644\n--- a/t/helper/test-string-list.c\n+++ b/t/helper/test-string-list.c\n@@ -62,7 +62,7 @@ int cmd__string_list(int argc, const char **argv)\n \t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n \t\tint i;\n \t\tchar *s = xstrdup(argv[2]);\n-\t\tint delim = *argv[3];\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@@ -111,7 +111,7 @@ int cmd__string_list(int argc, const char **argv)\n \t\t */\n \t\tif (sb.len && sb.buf[sb.len - 1] == '\\n')\n \t\t\tstrbuf_setlen(&sb, sb.len - 1);\n-\t\tstring_list_split_in_place(&list, sb.buf, '\\n', -1);\n+\t\tstring_list_split_in_place(&list, sb.buf, \"\\n\", -1);\n \n \t\tstring_list_sort(&list);\n \ndiff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh\nindex 46d4839194..1fee6d9010 100755\n--- a/t/t0063-string-list.sh\n+++ b/t/t0063-string-list.sh\n@@ -18,6 +18,14 @@ test_split () {\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+\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 \"foo:bar:baz\" \":\" \"-1\" <nr` to\n0 while leaving the list of elements allocated as-is.\n\nThis allows `string_list_split_in_place()` to overwrite any existing\nentries without needing to free and re-allocate them.\n\nHowever, setting `list->nr` manually is not safe in all instances. There\nare a couple of cases worth worrying about:\n\n - If the `string_list` is initialized with `strdup_strings`,\n truncating the list can lead to overwriting strings which are\n allocated elsewhere. If there aren't any other pointers to those\n strings other than the ones inside of the `items` array, they will\n become unreachable and leak.\n\n (We could ourselves free the truncated items between\n string_list->items[nr] and `list->nr`, but no present or future\n callers would benefit from this additional complexity).\n\n - If the given `nr` is larger than the current value of `list->nr`,\n we'll trick the `string_list` into a state where it thinks there are\n more items allocated than there actually are, which can lead to\n undefined behavior if we try to read or write those entries.\n\nGuard against both of these by introducing a helper function which\nguards assignment of `list->nr` against each of the above conditions.\n\nCo-authored-by: Jeff King \nSigned-off-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n string-list.c | 9 +++++++++\n string-list.h | 10 ++++++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/string-list.c b/string-list.c\nindex 5f5b60fe1c..0f8ac117fd 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -203,6 +203,15 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c\n \tlist->nr = list->alloc = 0;\n }\n \n+void string_list_setlen(struct string_list *list, size_t nr)\n+{\n+\tif (list->strdup_strings)\n+\t\tBUG(\"cannot setlen a string_list which owns its entries\");\n+\tif (nr > list->nr)\n+\t\tBUG(\"cannot grow a string_list with setlen\");\n+\tlist->nr = nr;\n+}\n+\n struct string_list_item *string_list_append_nodup(struct string_list *list,\n \t\t\t\t\t\t char *string)\n {\ndiff --git a/string-list.h b/string-list.h\nindex 77854840f7..122b318641 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -134,6 +134,16 @@ typedef void (*string_list_clear_func_t)(void *p, const char *str);\n /** Call a custom clear function on each util pointer */\n void string_list_clear_func(struct string_list *list, string_list_clear_func_t clearfunc);\n \n+/*\n+ * Set the length of a string_list to `nr`, provided that (a) `list`\n+ * does not own its own storage, and (b) that `nr` is no larger than\n+ * `list->nr`.\n+ *\n+ * Useful when \"shrinking\" `list` to write over existing entries that\n+ * are no longer used without reallocating.\n+ */\n+void string_list_setlen(struct string_list *list, size_t nr);\n+\n /**\n * Apply `func` to each item. If `func` returns nonzero, the\n * iteration aborts and the return value is propagated.\n-- \n2.40.0.380.gd2df7d2365\n\n"},{"id":"475984","messageId":"78ecf13cb0a642c2591af20321245a77e9b0aadd.1682374789.git.me@ttaylorr.com","threadId":"59592","inReplyTo":"cover.1682374789.git.me@ttaylorr.com","subject":"[PATCH v3 3/6] t/helper/test-hashmap.c: avoid using `strtok()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-24T22:20:17Z","receivedAt":"2023-04-24T22:20:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Avoid using the non-reentrant `strtok()` to separate the parts of each\nincoming command. Instead of replacing it with `strtok_r()`, let's\ninstead use the more friendly pair of `string_list_split_in_place()` and\n`string_list_remove_empty_items()`.\n\nSigned-off-by: Taylor Blau \n---\n t/helper/test-hashmap.c | 22 +++++++++++++++-------\n 1 file changed, 15 insertions(+), 7 deletions(-)\n\ndiff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\nindex 36ff07bd4b..0eb0b3d49c 100644\n--- a/t/helper/test-hashmap.c\n+++ b/t/helper/test-hashmap.c\n@@ -2,6 +2,7 @@\n #include \"git-compat-util.h\"\n #include \"hashmap.h\"\n #include \"strbuf.h\"\n+#include \"string-list.h\"\n \n struct test_entry\n {\n@@ -150,6 +151,7 @@ static void perf_hashmap(unsigned int method, unsigned int rounds)\n */\n int cmd__hashmap(int argc, const char **argv)\n {\n+\tstruct string_list parts = STRING_LIST_INIT_NODUP;\n \tstruct strbuf line = STRBUF_INIT;\n \tint icase;\n \tstruct hashmap map = HASHMAP_INIT(test_entry_cmp, &icase);\n@@ -159,21 +161,26 @@ int cmd__hashmap(int argc, const char **argv)\n \n \t/* process commands from stdin */\n \twhile (strbuf_getline(&line, stdin) != EOF) {\n-\t\tchar *cmd, *p1 = NULL, *p2 = NULL;\n+\t\tchar *cmd, *p1, *p2;\n \t\tunsigned int hash = 0;\n \t\tstruct test_entry *entry;\n \n \t\t/* break line into command and up to two parameters */\n-\t\tcmd = strtok(line.buf, DELIM);\n+\t\tstring_list_setlen(&parts, 0);\n+\t\tstring_list_split_in_place(&parts, line.buf, DELIM, 2);\n+\t\tstring_list_remove_empty_items(&parts, 0);\n+\n \t\t/* ignore empty lines */\n-\t\tif (!cmd || *cmd == '#')\n+\t\tif (!parts.nr)\n+\t\t\tcontinue;\n+\t\tif (!*parts.items[0].string || *parts.items[0].string == '#')\n \t\t\tcontinue;\n \n-\t\tp1 = strtok(NULL, DELIM);\n-\t\tif (p1) {\n+\t\tcmd = parts.items[0].string;\n+\t\tp1 = parts.nr >= 1 ? parts.items[1].string : NULL;\n+\t\tp2 = parts.nr >= 2 ? parts.items[2].string : NULL;\n+\t\tif (p1)\n \t\t\thash = icase ? strihash(p1) : strhash(p1);\n-\t\t\tp2 = strtok(NULL, DELIM);\n-\t\t}\n \n \t\tif (!strcmp(\"add\", cmd) && p1 && p2) {\n \n@@ -260,6 +267,7 @@ int cmd__hashmap(int argc, const char **argv)\n \t\t}\n \t}\n \n+\tstring_list_clear(&parts, 0);\n \tstrbuf_release(&line);\n \thashmap_clear_and_free(&map, struct test_entry, ent);\n \treturn 0;\n-- \n2.40.0.380.gd2df7d2365\n\n"},{"id":"475985","messageId":"c9b929406a4e895e5148cf292af672ead2b5b5dc.1682374789.git.me@ttaylorr.com","threadId":"59592","inReplyTo":"cover.1682374789.git.me@ttaylorr.com","subject":"[PATCH v3 4/6] t/helper/test-oidmap.c: avoid using `strtok()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-24T22:20:20Z","receivedAt":"2023-04-24T22:20:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Apply similar treatment as in the previous commit to remove usage of\n`strtok()` from the \"oidmap\" test helper.\n\nSigned-off-by: Taylor Blau \n---\n t/helper/test-oidmap.c | 20 ++++++++++++++------\n 1 file changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c\nindex a7b7b38df1..5ce9eb3334 100644\n--- a/t/helper/test-oidmap.c\n+++ b/t/helper/test-oidmap.c\n@@ -4,6 +4,7 @@\n #include \"oidmap.h\"\n #include \"setup.h\"\n #include \"strbuf.h\"\n+#include \"string-list.h\"\n \n /* key is an oid and value is a name (could be a refname for example) */\n struct test_entry {\n@@ -25,6 +26,7 @@ struct test_entry {\n */\n int cmd__oidmap(int argc UNUSED, const char **argv UNUSED)\n {\n+\tstruct string_list parts = STRING_LIST_INIT_NODUP;\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct oidmap map = OIDMAP_INIT;\n \n@@ -35,19 +37,24 @@ int cmd__oidmap(int argc UNUSED, const char **argv UNUSED)\n \n \t/* process commands from stdin */\n \twhile (strbuf_getline(&line, stdin) != EOF) {\n-\t\tchar *cmd, *p1 = NULL, *p2 = NULL;\n+\t\tchar *cmd, *p1, *p2;\n \t\tstruct test_entry *entry;\n \t\tstruct object_id oid;\n \n \t\t/* break line into command and up to two parameters */\n-\t\tcmd = strtok(line.buf, DELIM);\n+\t\tstring_list_setlen(&parts, 0);\n+\t\tstring_list_split_in_place(&parts, line.buf, DELIM, 2);\n+\t\tstring_list_remove_empty_items(&parts, 0);\n+\n \t\t/* ignore empty lines */\n-\t\tif (!cmd || *cmd == '#')\n+\t\tif (!parts.nr)\n+\t\t\tcontinue;\n+\t\tif (!*parts.items[0].string || *parts.items[0].string == '#')\n \t\t\tcontinue;\n \n-\t\tp1 = strtok(NULL, DELIM);\n-\t\tif (p1)\n-\t\t\tp2 = strtok(NULL, DELIM);\n+\t\tcmd = parts.items[0].string;\n+\t\tp1 = parts.nr >= 1 ? parts.items[1].string : NULL;\n+\t\tp2 = parts.nr >= 2 ? parts.items[2].string : NULL;\n \n \t\tif (!strcmp(\"put\", cmd) && p1 && p2) {\n \n@@ -108,6 +115,7 @@ int cmd__oidmap(int argc UNUSED, const char **argv UNUSED)\n \t\t}\n \t}\n \n+\tstring_list_clear(&parts, 0);\n \tstrbuf_release(&line);\n \toidmap_free(&map, 1);\n \treturn 0;\n-- \n2.40.0.380.gd2df7d2365\n\n"},{"id":"475986","messageId":"201fcac6c41a80df78e58edd0d4dfda5f3944b8a.1682374789.git.me@ttaylorr.com","threadId":"59592","inReplyTo":"cover.1682374789.git.me@ttaylorr.com","subject":"[PATCH v3 5/6] t/helper/test-json-writer.c: avoid using `strtok()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-24T22:20:23Z","receivedAt":"2023-04-24T22:20:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Apply similar treatment as in the previous commit to remove usage of\n`strtok()` from the \"oidmap\" test helper.\n\nEach of the different commands that the \"json-writer\" helper accepts\npops the next space-delimited token from the current line and interprets\nit as a string, integer, or double (with the exception of the very first\ntoken, which is the command itself).\n\nTo accommodate this, split the line in place by the space character, and\npass the corresponding string_list to each of the specialized `get_s()`,\n`get_i()`, and `get_d()` functions.\n\n`get_i()` and `get_d()` are thin wrappers around `get_s()` that convert\ntheir result into the appropriate type by either calling `strtol()` or\n`strtod()`, respectively. In `get_s()`, we mark the token as \"consumed\"\nby incrementing the `consumed_nr` counter, indicating how many tokens we\nhave read up to that point.\n\nBecause each of these functions needs the string-list parts, the number\nof tokens consumed, and the line number, these three are wrapped up in\nto a struct representing the line state.\n\nSigned-off-by: Taylor Blau \n---\n t/helper/test-json-writer.c | 76 +++++++++++++++++++++++--------------\n 1 file changed, 48 insertions(+), 28 deletions(-)\n\ndiff --git a/t/helper/test-json-writer.c b/t/helper/test-json-writer.c\nindex 86887f5320..afe393f597 100644\n--- a/t/helper/test-json-writer.c\n+++ b/t/helper/test-json-writer.c\n@@ -1,5 +1,6 @@\n #include \"test-tool.h\"\n #include \"json-writer.h\"\n+#include \"string-list.h\"\n \n static const char *expect_obj1 = \"{\\\"a\\\":\\\"abc\\\",\\\"b\\\":42,\\\"c\\\":true}\";\n static const char *expect_obj2 = \"{\\\"a\\\":-1,\\\"b\\\":2147483647,\\\"c\\\":0}\";\n@@ -394,35 +395,41 @@ static int unit_tests(void)\n \treturn 0;\n }\n \n-static void get_s(int line_nr, char **s_in)\n+struct line {\n+\tstruct string_list *parts;\n+\tsize_t consumed_nr;\n+\tint nr;\n+};\n+\n+static void get_s(struct line *line, char **s_in)\n {\n-\t*s_in = strtok(NULL, \" \");\n-\tif (!*s_in)\n-\t\tdie(\"line[%d]: expected: \", line_nr);\n+\tif (line->consumed_nr > line->parts->nr)\n+\t\tdie(\"line[%d]: expected: \", line->nr);\n+\t*s_in = line->parts->items[line->consumed_nr++].string;\n }\n \n-static void get_i(int line_nr, intmax_t *s_in)\n+static void get_i(struct line *line, intmax_t *s_in)\n {\n \tchar *s;\n \tchar *endptr;\n \n-\tget_s(line_nr, &s);\n+\tget_s(line, &s);\n \n \t*s_in = strtol(s, &endptr, 10);\n \tif (*endptr || errno == ERANGE)\n-\t\tdie(\"line[%d]: invalid integer value\", line_nr);\n+\t\tdie(\"line[%d]: invalid integer value\", line->nr);\n }\n \n-static void get_d(int line_nr, double *s_in)\n+static void get_d(struct line *line, double *s_in)\n {\n \tchar *s;\n \tchar *endptr;\n \n-\tget_s(line_nr, &s);\n+\tget_s(line, &s);\n \n \t*s_in = strtod(s, &endptr);\n \tif (*endptr || errno == ERANGE)\n-\t\tdie(\"line[%d]: invalid float value\", line_nr);\n+\t\tdie(\"line[%d]: invalid float value\", line->nr);\n }\n \n static int pretty;\n@@ -453,6 +460,7 @@ static char *get_trimmed_line(char *buf, int buf_size)\n \n static int scripted(void)\n {\n+\tstruct string_list parts = STRING_LIST_INIT_NODUP;\n \tstruct json_writer jw = JSON_WRITER_INIT;\n \tchar buf[MAX_LINE_LENGTH];\n \tchar *line;\n@@ -470,66 +478,77 @@ static int scripted(void)\n \t\tdie(\"expected first line to be 'object' or 'array'\");\n \n \twhile ((line = get_trimmed_line(buf, MAX_LINE_LENGTH)) != NULL) {\n+\t\tstruct line state = { 0 };\n \t\tchar *verb;\n \t\tchar *key;\n \t\tchar *s_value;\n \t\tintmax_t i_value;\n \t\tdouble d_value;\n \n-\t\tline_nr++;\n+\t\tstate.parts = &parts;\n+\t\tstate.nr = ++line_nr;\n \n-\t\tverb = strtok(line, \" \");\n+\t\t/* break line into command and zero or more tokens */\n+\t\tstring_list_setlen(&parts, 0);\n+\t\tstring_list_split_in_place(&parts, line, \" \", -1);\n+\t\tstring_list_remove_empty_items(&parts, 0);\n+\n+\t\t/* ignore empty lines */\n+\t\tif (!parts.nr || !*parts.items[0].string)\n+\t\t\tcontinue;\n+\n+\t\tverb = parts.items[state.consumed_nr++].string;\n \n \t\tif (!strcmp(verb, \"end\")) {\n \t\t\tjw_end(&jw);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-string\")) {\n-\t\t\tget_s(line_nr, &key);\n-\t\t\tget_s(line_nr, &s_value);\n+\t\t\tget_s(&state, &key);\n+\t\t\tget_s(&state, &s_value);\n \t\t\tjw_object_string(&jw, key, s_value);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-int\")) {\n-\t\t\tget_s(line_nr, &key);\n-\t\t\tget_i(line_nr, &i_value);\n+\t\t\tget_s(&state, &key);\n+\t\t\tget_i(&state, &i_value);\n \t\t\tjw_object_intmax(&jw, key, i_value);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-double\")) {\n-\t\t\tget_s(line_nr, &key);\n-\t\t\tget_i(line_nr, &i_value);\n-\t\t\tget_d(line_nr, &d_value);\n+\t\t\tget_s(&state, &key);\n+\t\t\tget_i(&state, &i_value);\n+\t\t\tget_d(&state, &d_value);\n \t\t\tjw_object_double(&jw, key, i_value, d_value);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-true\")) {\n-\t\t\tget_s(line_nr, &key);\n+\t\t\tget_s(&state, &key);\n \t\t\tjw_object_true(&jw, key);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-false\")) {\n-\t\t\tget_s(line_nr, &key);\n+\t\t\tget_s(&state, &key);\n \t\t\tjw_object_false(&jw, key);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-null\")) {\n-\t\t\tget_s(line_nr, &key);\n+\t\t\tget_s(&state, &key);\n \t\t\tjw_object_null(&jw, key);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-object\")) {\n-\t\t\tget_s(line_nr, &key);\n+\t\t\tget_s(&state, &key);\n \t\t\tjw_object_inline_begin_object(&jw, key);\n \t\t}\n \t\telse if (!strcmp(verb, \"object-array\")) {\n-\t\t\tget_s(line_nr, &key);\n+\t\t\tget_s(&state, &key);\n \t\t\tjw_object_inline_begin_array(&jw, key);\n \t\t}\n \t\telse if (!strcmp(verb, \"array-string\")) {\n-\t\t\tget_s(line_nr, &s_value);\n+\t\t\tget_s(&state, &s_value);\n \t\t\tjw_array_string(&jw, s_value);\n \t\t}\n \t\telse if (!strcmp(verb, \"array-int\")) {\n-\t\t\tget_i(line_nr, &i_value);\n+\t\t\tget_i(&state, &i_value);\n \t\t\tjw_array_intmax(&jw, i_value);\n \t\t}\n \t\telse if (!strcmp(verb, \"array-double\")) {\n-\t\t\tget_i(line_nr, &i_value);\n-\t\t\tget_d(line_nr, &d_value);\n+\t\t\tget_i(&state, &i_value);\n+\t\t\tget_d(&state, &d_value);\n \t\t\tjw_array_double(&jw, i_value, d_value);\n \t\t}\n \t\telse if (!strcmp(verb, \"array-true\"))\n@@ -552,6 +571,7 @@ static int scripted(void)\n \tprintf(\"%s\\n\", jw.json.buf);\n \n \tjw_release(&jw);\n+\tstring_list_clear(&parts, 0);\n \treturn 0;\n }\n \n-- \n2.40.0.380.gd2df7d2365\n\n"},{"id":"475987","messageId":"da896aa358eab65f2629f85189c5be4ad9cec635.1682374789.git.me@ttaylorr.com","threadId":"59592","inReplyTo":"cover.1682374789.git.me@ttaylorr.com","subject":"[PATCH v3 6/6] banned.h: mark `strtok()` and `strtok_r()` as banned","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-24T22:20:26Z","receivedAt":"2023-04-24T22:20:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"`strtok()` has a couple of drawbacks that make it undesirable to have\nany new instances. In addition to being thread-unsafe, it also\nencourages confusing data flows, where `strtok()` may be called from\nmultiple functions with its first argument as NULL, making it unclear\nfrom the immediate context which string is being tokenized.\n\nNow that we have removed all instances of `strtok()` from the tree,\nlet's ban `strtok()` to avoid introducing new ones in the future. If new\ncallers should arise, they are encouraged to use\n`string_list_split_in_place()` (and `string_list_remove_empty_items()`,\nif applicable).\n\nstring_list_split_in_place() is not a perfect drop-in replacement\nfor `strtok_r()`, particularly if the caller is processing a string with\nan arbitrary number of tokens, and wants to process each token one at a\ntime.\n\nBut there are no instances of this in Git's tree which are more\nwell-suited to `strtok_r()` than the friendlier\n`string_list_in_place()`, so ban `strtok_r()`, too.\n\nSigned-off-by: Taylor Blau \n---\n banned.h | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/banned.h b/banned.h\nindex 6ccf46bc19..44e76bd90a 100644\n--- a/banned.h\n+++ b/banned.h\n@@ -18,6 +18,10 @@\n #define strncpy(x,y,n) BANNED(strncpy)\n #undef strncat\n #define strncat(x,y,n) BANNED(strncat)\n+#undef strtok\n+#define strtok(x,y) BANNED(strtok)\n+#undef strtok_r\n+#define strtok_r(x,y,z) BANNED(strtok_r)\n \n #undef sprintf\n #undef vsprintf\n-- \n2.40.0.380.gd2df7d2365\n"},{"id":"475988","messageId":"CAPx1GvdHWBdA8XZu8ZHf3PO3QC5qVpb9UdNMG9Xegyk_9ZtHwg@mail.gmail.com","threadId":"59592","inReplyTo":"da896aa358eab65f2629f85189c5be4ad9cec635.1682374789.git.me@ttaylorr.com","subject":"Re: [PATCH v3 6/6] banned.h: mark `strtok()` and `strtok_r()` as banned","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2023-04-24T22:25:55Z","receivedAt":"2023-04-24T22:26:49Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"I eyeballed the whole thing (not the same as real tests of course)\nand it looks good, but there's one typo (missing-word-o) here of note:\n\nOn Mon, Apr 24, 2023 at 3:20 PM Taylor Blau wrote:\n...\n> But there are no instances of this in Git's tree which are more\n> well-suited to `strtok_r()` than the friendlier\n> `string_list_in_place()`, so ban `strtok_r()`, too.\n>\n> Signed-off-by: Taylor Blau \n\nMissing `split_`. Probably not worth a re-roll...\n\nChris\n"},{"id":"475989","messageId":"ZEcKAzwtOUKF14nV@nand.local","threadId":"59592","inReplyTo":"CAPx1GvdHWBdA8XZu8ZHf3PO3QC5qVpb9UdNMG9Xegyk_9ZtHwg@mail.gmail.com","subject":"Re: [PATCH v3 6/6] banned.h: mark `strtok()` and `strtok_r()` as banned","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-24T23:00:19Z","receivedAt":"2023-04-24T23:00:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Apr 24, 2023 at 03:25:55PM -0700, Chris Torek wrote:\n> I eyeballed the whole thing (not the same as real tests of course)\n> and it looks good, but there's one typo (missing-word-o) here of note:\n>\n> On Mon, Apr 24, 2023 at 3:20 PM Taylor Blau wrote:\n> ...\n> > But there are no instances of this in Git's tree which are more\n> > well-suited to `strtok_r()` than the friendlier\n> > `string_list_in_place()`, so ban `strtok_r()`, too.\n> >\n> > Signed-off-by: Taylor Blau \n>\n> Missing `split_`. Probably not worth a re-roll...\n\nOops, thanks for noticing.\n\nThanks,\nTaylor\n"},{"id":"476012","messageId":"20230425062107.GA4061254@coredump.intra.peff.net","threadId":"59592","inReplyTo":"ae8d0ce1f25f26da09f2e3f5bc68f85cc162ce64.1682374789.git.me@ttaylorr.com","subject":"Re: [PATCH v3 2/6] string-list: introduce `string_list_setlen()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-25T06:21:07Z","receivedAt":"2023-04-25T06:21:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 24, 2023 at 06:20:14PM -0400, Taylor Blau wrote:\n\n> However, setting `list->nr` manually is not safe in all instances. There\n> are a couple of cases worth worrying about:\n> \n> - If the `string_list` is initialized with `strdup_strings`,\n> truncating the list can lead to overwriting strings which are\n> allocated elsewhere. If there aren't any other pointers to those\n> strings other than the ones inside of the `items` array, they will\n> become unreachable and leak.\n> \n> (We could ourselves free the truncated items between\n> string_list->items[nr] and `list->nr`, but no present or future\n> callers would benefit from this additional complexity).\n\nI wondered how bad it would be to just free those truncated entries when\nstrdup_strings is set. But that led me to another interesting point: the\nutil fields. The regular string_list_clear() will optionally free the\nutil entries, too. We'd potentially need to deal with those, too.\n\nWe don't do anything with them here. So code like:\n\n struct string_list foo = STRING_LIST_INIT_NODUP;\n\n string_list_append(&foo, \"bar\")->util = xstrdup(\"something else\");\n string_list_setlen(&foo, 0);\n\nwould leak that util field. To be clear, to me this definitely falls\nunder \"if it hurts, don't do it\", and I think code like above is pretty\nunlikely. But since the point of our function is to prevent mistakes, I\nthought it was worth mentioning.\n\nI think we _could_ do something like:\n\n for (i = nr; i < list->nr; i++) {\n\tif (list->items[i].util)\n\t\tBUG(\"truncated string list item has non-NULL util field\");\n }\n\nthough that is technically tighter than we need to be (it could be an\nunowned util field, after all; we don't know what it means here). So I'm\ninclined to leave your patch as-is.\n\nThis would all be easier if the string_list had a field for \"we own the\nutil fields, too\" just like it has strdup_strings. Or even a free-ing\nfunction. But instead we have ad-hoc solutions like \"free_util\" and\nstring_list_clear_func(). But that's really outside the scope of your\nseries. :)\n\n-Peff\n"},{"id":"476013","messageId":"20230425062617.GB4061254@coredump.intra.peff.net","threadId":"59592","inReplyTo":"da896aa358eab65f2629f85189c5be4ad9cec635.1682374789.git.me@ttaylorr.com","subject":"Re: [PATCH v3 6/6] banned.h: mark `strtok()` and `strtok_r()` as banned","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-25T06:26:17Z","receivedAt":"2023-04-25T06:26:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 24, 2023 at 06:20:26PM -0400, Taylor Blau wrote:\n\n> string_list_split_in_place() is not a perfect drop-in replacement\n> for `strtok_r()`, particularly if the caller is processing a string with\n> an arbitrary number of tokens, and wants to process each token one at a\n> time.\n> \n> But there are no instances of this in Git's tree which are more\n> well-suited to `strtok_r()` than the friendlier\n> `string_list_in_place()`, so ban `strtok_r()`, too.\n\nFor true incremental left-to-right parsing, strcspn() is probably a\nbetter solution. We could mention that here in case anybody digs up the\ncommit after getting a \"banned function\" error.\n\nI'm tempted to say that this thread could serve the same function, but\nI'm not sure where people turn to for answers (I find searching the list\nabout as easy as \"git log -S\", but then I've invested a lot of effort in\nmy list archive tooling :) ).\n\nI'm happy with it either the way, though.\n\n-Peff\n"},{"id":"476014","messageId":"20230425062708.GC4061254@coredump.intra.peff.net","threadId":"59592","inReplyTo":"cover.1682374789.git.me@ttaylorr.com","subject":"Re: [PATCH v3 0/6] banned: mark `strok()`, `strtok_r()` as banned","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-25T06:27:08Z","receivedAt":"2023-04-25T06:27:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 24, 2023 at 06:20:07PM -0400, Taylor Blau wrote:\n\n> Here is another medium-sized reroll of my series to add `strtok()` (and\n> `strtok_r()`!) to the list of banned functions.\n> \n> Notable changes include:\n> \n> - Dropped `string_list_split_in_place_multi()` in favor of a\n> combination of `string_list_split_in_place()` and\n> `string_list_remove_empty_items()`.\n> \n> - `strtok_r()` is back on the banned list, with a more realistic sales\n> pitch.\n\nThis all looks good to me. I left two comments which are on the border\nbetween \"minor\" and \"philosophizing\", so I'd be happy to see the series\ngo in as-is.\n\n-Peff\n"},{"id":"476028","messageId":"e7dba6a6-b0dd-061d-ee94-06c66f955c0f@jeffhostetler.com","threadId":"59592","inReplyTo":"201fcac6c41a80df78e58edd0d4dfda5f3944b8a.1682374789.git.me@ttaylorr.com","subject":"Re: [PATCH v3 5/6] t/helper/test-json-writer.c: avoid using `strtok()`","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-04-25T13:57:49Z","receivedAt":"2023-04-25T13:57:54Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 4/24/23 6:20 PM, Taylor Blau wrote:\n> Apply similar treatment as in the previous commit to remove usage of\n> `strtok()` from the \"oidmap\" test helper.\n> \n> Each of the different commands that the \"json-writer\" helper accepts\n> pops the next space-delimited token from the current line and interprets\n> it as a string, integer, or double (with the exception of the very first\n> token, which is the command itself).\n> \n> To accommodate this, split the line in place by the space character, and\n> pass the corresponding string_list to each of the specialized `get_s()`,\n> `get_i()`, and `get_d()` functions.\n> \n> `get_i()` and `get_d()` are thin wrappers around `get_s()` that convert\n> their result into the appropriate type by either calling `strtol()` or\n> `strtod()`, respectively. In `get_s()`, we mark the token as \"consumed\"\n> by incrementing the `consumed_nr` counter, indicating how many tokens we\n> have read up to that point.\n> \n> Because each of these functions needs the string-list parts, the number\n> of tokens consumed, and the line number, these three are wrapped up in\n> to a struct representing the line state.\n> \n> Signed-off-by: Taylor Blau \n\nLGTM\n\nThanks,\nJeff\n"},{"id":"476069","messageId":"ZEg/XPbtaAx0tPzK@nand.local","threadId":"59592","inReplyTo":"20230425062107.GA4061254@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/6] string-list: introduce `string_list_setlen()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-25T21:00:12Z","receivedAt":"2023-04-25T21:00:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 25, 2023 at 02:21:07AM -0400, Jeff King wrote:\n> I think we _could_ do something like:\n>\n> for (i = nr; i < list->nr; i++) {\n> \tif (list->items[i].util)\n> \t\tBUG(\"truncated string list item has non-NULL util field\");\n> }\n>\n> though that is technically tighter than we need to be (it could be an\n> unowned util field, after all; we don't know what it means here). So I'm\n> inclined to leave your patch as-is.\n\nI think there are two ways to do it, either:\n\n - something like what you wrote above, perhaps with an additional\n `free_util` bit on the string_list itself (which might make it\n convenient to drop all of the `free_util` parameters that permeate\n its API)\n\n - have string_list_setlen() take a `free_util` argument itself, in\n which case your code would change to:\n\n if (free_util) {\n for (i = nr; i < list->nr; i++)\n free(list->items[i].util)\n }\n\n> This would all be easier if the string_list had a field for \"we own the\n> util fields, too\" just like it has strdup_strings. Or even a free-ing\n> function. But instead we have ad-hoc solutions like \"free_util\" and\n> string_list_clear_func(). But that's really outside the scope of your\n> series. :)\n\n;-).\n\nThanks,\nTaylor\n"},{"id":"476070","messageId":"ZEg/8klMmHyJrdrj@nand.local","threadId":"59592","inReplyTo":"20230425062617.GB4061254@coredump.intra.peff.net","subject":"Re: [PATCH v3 6/6] banned.h: mark `strtok()` and `strtok_r()` as banned","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-25T21:02:42Z","receivedAt":"2023-04-25T21:02:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 25, 2023 at 02:26:17AM -0400, Jeff King wrote:\n> On Mon, Apr 24, 2023 at 06:20:26PM -0400, Taylor Blau wrote:\n>\n> > string_list_split_in_place() is not a perfect drop-in replacement\n> > for `strtok_r()`, particularly if the caller is processing a string with\n> > an arbitrary number of tokens, and wants to process each token one at a\n> > time.\n> >\n> > But there are no instances of this in Git's tree which are more\n> > well-suited to `strtok_r()` than the friendlier\n> > `string_list_in_place()`, so ban `strtok_r()`, too.\n>\n> For true incremental left-to-right parsing, strcspn() is probably a\n> better solution. We could mention that here in case anybody digs up the\n> commit after getting a \"banned function\" error.\n>\n> I'm tempted to say that this thread could serve the same function, but\n> I'm not sure where people turn to for answers (I find searching the list\n> about as easy as \"git log -S\", but then I've invested a lot of effort in\n> my list archive tooling :) ).\n\nPersonally, between what's already in the patch message and this\ndiscussion on the list, I think that folks would have enough guidance on\nhow to do things right.\n\nBut if others feel like we could or should be more rigid here and update\nthe commit message to say something like \"if you're scanning from\nleft-to-right, you could use strtok_r(), or strcspn() instead\", but TBH\nI think there are a gazillion different ways to do this task, so I don't\nknow that adding another item to that list substantially changes things.\n\nThanks,\nTaylor\n"},{"id":"476071","messageId":"ZEhADEFE9cq/fjSV@nand.local","threadId":"59592","inReplyTo":"20230425062708.GC4061254@coredump.intra.peff.net","subject":"Re: [PATCH v3 0/6] banned: mark `strok()`, `strtok_r()` as banned","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-25T21:03:08Z","receivedAt":"2023-04-25T21:03:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 25, 2023 at 02:27:08AM -0400, Jeff King wrote:\n> On Mon, Apr 24, 2023 at 06:20:07PM -0400, Taylor Blau wrote:\n>\n> > Here is another medium-sized reroll of my series to add `strtok()` (and\n> > `strtok_r()`!) to the list of banned functions.\n> >\n> > Notable changes include:\n> >\n> > - Dropped `string_list_split_in_place_multi()` in favor of a\n> > combination of `string_list_split_in_place()` and\n> > `string_list_remove_empty_items()`.\n> >\n> > - `strtok_r()` is back on the banned list, with a more realistic sales\n> > pitch.\n>\n> This all looks good to me. I left two comments which are on the border\n> between \"minor\" and \"philosophizing\", so I'd be happy to see the series\n> go in as-is.\n\nThanks, I agree that this version is ready to go, absent any other\nshow-stopping reviews.\n\nThanks,\nTaylor\n"}]}