{"thread":{"id":"63882","subject":"[PATCH 0/9] do not overuse strbuf_split*()","startedAt":"2025-07-31T07:41:57Z","lastAt":"2025-08-02T18:47:16Z","messageCount":33,"participants":["Junio C Hamano","Christian Couder","Eric Sunshine","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"523064","messageId":"20250731074154.2835370-1-gitster@pobox.com","threadId":"63882","inReplyTo":null,"subject":"[PATCH 0/9] do not overuse strbuf_split*()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T07:41:45Z","receivedAt":"2025-07-31T07:41:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"strbuf is a very good data structure to work with string data\nwithout having to worry about running past the end of the string.\n\nBut an array of strbuf is often a wrong data structure. You rarely\nhave need to be able to edit multiple strings represented by such an\narray simultaneously. And strbuf_split*() that produces result in\nsuch a shape is a misdesigned API function.\n\nThe most common use case of strbuf_split*() family of functions\nseems to be to trim away the whitespaces around each piece of split\nstring. With modern string_list_split*(), it is often no longer\nnecessary.\n\nThis series builds on top of the other series that extends string\nlist API to allow string_list_split() to take more than one delimiter\nbytes, and to optionally trim the resulting string pieces.\n\nJunio C Hamano (9):\n wt-status: avoid strbuf_split*()\n clean: do not pass strbuf by value\n clean: do not use strbuf_split*() [part 1]\n clean: do not use strbuf_split*() [part 2]\n merge-tree: do not use strbuf_split*()\n notes: do not use strbuf_split*()\n config: do not use strbuf_split()\n environment: do not use strbuf_split*()\n sub-process: do not use strbuf_split*()\n\n builtin/clean.c | 74 +++++++++++++++++++++-----------------------\n builtin/merge-tree.c | 30 +++++++++---------\n builtin/notes.c | 23 +++++++-------\n config.c | 23 ++++++--------\n environment.c | 19 +++++++-----\n sub-process.c | 16 +++++-----\n wt-status.c | 31 ++++++-------------\n 7 files changed, 103 insertions(+), 113 deletions(-)\n\n-- \n2.50.1-612-g4756c59422\n\n"},{"id":"523065","messageId":"20250731074154.2835370-2-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731074154.2835370-1-gitster@pobox.com","subject":"[PATCH 1/9] wt-status: avoid strbuf_split*()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T07:41:46Z","receivedAt":"2025-07-31T07:41:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"strbuf is a very good data structure to work with string data\nwithout having to worry about running past the end of the string,\nbut strbuf_split() is a wrong API and an array of strbuf that the\nfunction produces is a wrong thing to use in general. You do not\nedit these N strings split out of a single strbuf simultaneously.\nOften it is much better off to split a string into string_list and\nwork with the resulting strings.\n\nwt-status.c:abbrev_oid_in_line() takes one line of rebase todo list\n(like \"pick e813a0200a7121b97fec535f0d0b460b0a33356c title\"), and\nfor instructions that has an object name as the second token on the\nline, replace the object name with its unique abbreviation. After\nsplitting these tokens out of a single line, no simultaneous edit on\nany of these pieces of string that takes advantage of strbuf API\ntakes place. The final string is composed with strbuf API, but\nthese split pieces are merely used as pieces of strings and there is\nno need for them to be stored in individual strbuf.\n\nInstead, split the line into a string_list, and compose the final\nstring using these pieces.\n\nSigned-off-by: Junio C Hamano \n---\n wt-status.c | 31 ++++++++++---------------------\n 1 file changed, 10 insertions(+), 21 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 454601afa1..a34dc144ee 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1351,8 +1351,8 @@ static int split_commit_in_progress(struct wt_status *s)\n */\n static void abbrev_oid_in_line(struct strbuf *line)\n {\n-\tstruct strbuf **split;\n-\tint i;\n+\tstruct string_list split = STRING_LIST_INIT_DUP;\n+\tstruct object_id oid;\n \n \tif (starts_with(line->buf, \"exec \") ||\n \t starts_with(line->buf, \"x \") ||\n@@ -1360,26 +1360,15 @@ static void abbrev_oid_in_line(struct strbuf *line)\n \t starts_with(line->buf, \"l \"))\n \t\treturn;\n \n-\tsplit = strbuf_split_max(line, ' ', 3);\n-\tif (split[0] && split[1]) {\n-\t\tstruct object_id oid;\n-\n-\t\t/*\n-\t\t * strbuf_split_max left a space. Trim it and re-add\n-\t\t * it after abbreviation.\n-\t\t */\n-\t\tstrbuf_trim(split[1]);\n-\t\tif (!repo_get_oid(the_repository, split[1]->buf, &oid)) {\n-\t\t\tstrbuf_reset(split[1]);\n-\t\t\tstrbuf_add_unique_abbrev(split[1], &oid,\n-\t\t\t\t\t\t DEFAULT_ABBREV);\n-\t\t\tstrbuf_addch(split[1], ' ');\n-\t\t\tstrbuf_reset(line);\n-\t\t\tfor (i = 0; split[i]; i++)\n-\t\t\t\tstrbuf_addbuf(line, split[i]);\n-\t\t}\n+\tif ((2 <= string_list_split(&split, line->buf, \" \", 2)) &&\n+\t !repo_get_oid(the_repository, split.items[1].string, &oid)) {\n+\t\tstrbuf_reset(line);\n+\t\tstrbuf_addf(line, \"%s \", split.items[0].string);\n+\t\tstrbuf_add_unique_abbrev(line, &oid, DEFAULT_ABBREV);\n+\t\tfor (size_t i = 2; i < split.nr; i++)\n+\t\t\tstrbuf_addf(line, \" %s\", split.items[i].string);\n \t}\n-\tstrbuf_list_free(split);\n+\tstring_list_clear(&split, 0);\n }\n \n static int read_rebase_todolist(const char *fname, struct string_list *lines)\n-- \n2.50.1-612-g4756c59422\n\n"},{"id":"523066","messageId":"20250731074154.2835370-3-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731074154.2835370-1-gitster@pobox.com","subject":"[PATCH 2/9] clean: do not pass strbuf by value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T07:41:47Z","receivedAt":"2025-07-31T07:42:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When you pass a structure by value, the callee can modify the\ncontents of the structure that was passed in without having to worry\nabout changing the structure the caller has. Passing structure by\nvalue sometimes (but not very often) can be a valid way to give\ncallee a temporary variable it can freely modify.\n\nBut not a structure with members that are pointers, like a strbuf.\n\nbuiltin/clean.c:list_and_choose() reads a line interactively from\nthe user, and passes the line (in a strbuf) to parse_choice() by\nvalue, which then munges by replacing ',' with ' ' (to accept both\ncomma and space separated list of choices). But because the strbuf\npassed by value still shares the underlying character array buf[],\nthis ends up munging the caller's strbuf contents.\n\nThis is a catastrophe waiting to happen. If the callee causes the\nstrbuf to be reallocated, the buf[] the caller has will become\ndangling, and when the caller does strbuf_release(), it would result\nin double-free.\n\nStop calling the function with misleading call-by-value with strbuf.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/clean.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 053c94fc6b..224551537e 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -477,7 +477,7 @@ static int find_unique(const char *choice, struct menu_stuff *menu_stuff)\n */\n static int parse_choice(struct menu_stuff *menu_stuff,\n \t\t\tint is_single,\n-\t\t\tstruct strbuf input,\n+\t\t\tstruct strbuf *input,\n \t\t\tint **chosen)\n {\n \tstruct strbuf **choice_list, **ptr;\n@@ -485,14 +485,14 @@ static int parse_choice(struct menu_stuff *menu_stuff,\n \tint i;\n \n \tif (is_single) {\n-\t\tchoice_list = strbuf_split_max(&input, '\\n', 0);\n+\t\tchoice_list = strbuf_split_max(input, '\\n', 0);\n \t} else {\n-\t\tchar *p = input.buf;\n+\t\tchar *p = input->buf;\n \t\tdo {\n \t\t\tif (*p == ',')\n \t\t\t\t*p = ' ';\n \t\t} while (*p++);\n-\t\tchoice_list = strbuf_split_max(&input, ' ', 0);\n+\t\tchoice_list = strbuf_split_max(input, ' ', 0);\n \t}\n \n \tfor (ptr = choice_list; *ptr; ptr++) {\n@@ -630,7 +630,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)\n \n \t\tnr = parse_choice(stuff,\n \t\t\t\t opts->flags & MENU_OPTS_SINGLETON,\n-\t\t\t\t choice,\n+\t\t\t\t &choice,\n \t\t\t\t &chosen);\n \n \t\tif (opts->flags & MENU_OPTS_SINGLETON) {\n-- \n2.50.1-612-g4756c59422\n\n"},{"id":"523067","messageId":"20250731074154.2835370-4-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731074154.2835370-1-gitster@pobox.com","subject":"[PATCH 3/9] clean: do not use strbuf_split*() [part 1]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T07:41:48Z","receivedAt":"2025-07-31T07:42:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"builtin/clean.c:parse_choice() is fed a single line of input, which\nis space or comma separated list of tokens, and a list of menu\nitems. It parses the tokens into number ranges (e.g. 1-3 that means\nthe first three items) or string prefix (e.g. 's' to choose the menu\nitem \"(s)elect\") that specify the elements in the menu item list,\nand tells the caller which ones are chosen.\n\nFor parsing the input string, it uses strbuf_split() to split it\ninto bunch of strbufs. Instead use string_list_split_in_place(),\nfor a few reasons.\n\n * strbuf_split() is a bad API function to use, that yields an array\n of strbuf that is a bad data structure to use in general.\n\n * string_list_split_in_place() allows you to split with \"comma or\n space\"; the current code has to preprocess the input string to\n replace comma with space because strbuf_split() does not allow\n this.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/clean.c | 50 +++++++++++++++++++++++--------------------------\n 1 file changed, 23 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 224551537e..708cd9344c 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -480,40 +480,36 @@ static int parse_choice(struct menu_stuff *menu_stuff,\n \t\t\tstruct strbuf *input,\n \t\t\tint **chosen)\n {\n-\tstruct strbuf **choice_list, **ptr;\n+\tstruct string_list choice = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *item;\n \tint nr = 0;\n \tint i;\n \n-\tif (is_single) {\n-\t\tchoice_list = strbuf_split_max(input, '\\n', 0);\n-\t} else {\n-\t\tchar *p = input->buf;\n-\t\tdo {\n-\t\t\tif (*p == ',')\n-\t\t\t\t*p = ' ';\n-\t\t} while (*p++);\n-\t\tchoice_list = strbuf_split_max(input, ' ', 0);\n-\t}\n+\tstring_list_split_in_place_f(&choice, input->buf,\n+\t\t\t\t is_single ? \"\\n\" : \", \", -1,\n+\t\t\t\t STRING_LIST_SPLIT_TRIM);\n \n-\tfor (ptr = choice_list; *ptr; ptr++) {\n-\t\tchar *p;\n-\t\tint choose = 1;\n+\tfor_each_string_list_item(item, &choice) {\n+\t\tconst char *string;\n+\t\tint choose;\n \t\tint bottom = 0, top = 0;\n \t\tint is_range, is_number;\n \n-\t\tstrbuf_trim(*ptr);\n-\t\tif (!(*ptr)->len)\n+\t\tstring = item->string;\n+\t\tif (!*string)\n \t\t\tcontinue;\n \n \t\t/* Input that begins with '-'; unchoose */\n-\t\tif (*(*ptr)->buf == '-') {\n+\t\tif (string[0] == '-') {\n \t\t\tchoose = 0;\n-\t\t\tstrbuf_remove((*ptr), 0, 1);\n+\t\t\tstring++;\n+\t\t} else {\n+\t\t\tchoose = 1;\n \t\t}\n \n \t\tis_range = 0;\n \t\tis_number = 1;\n-\t\tfor (p = (*ptr)->buf; *p; p++) {\n+\t\tfor (const char *p = string; *p; p++) {\n \t\t\tif ('-' == *p) {\n \t\t\t\tif (!is_range) {\n \t\t\t\t\tis_range = 1;\n@@ -531,27 +527,27 @@ static int parse_choice(struct menu_stuff *menu_stuff,\n \t\t}\n \n \t\tif (is_number) {\n-\t\t\tbottom = atoi((*ptr)->buf);\n+\t\t\tbottom = atoi(string);\n \t\t\ttop = bottom;\n \t\t} else if (is_range) {\n-\t\t\tbottom = atoi((*ptr)->buf);\n+\t\t\tbottom = atoi(string);\n \t\t\t/* a range can be specified like 5-7 or 5- */\n-\t\t\tif (!*(strchr((*ptr)->buf, '-') + 1))\n+\t\t\tif (!*(strchr(string, '-') + 1))\n \t\t\t\ttop = menu_stuff->nr;\n \t\t\telse\n-\t\t\t\ttop = atoi(strchr((*ptr)->buf, '-') + 1);\n-\t\t} else if (!strcmp((*ptr)->buf, \"*\")) {\n+\t\t\t\ttop = atoi(strchr(string, '-') + 1);\n+\t\t} else if (!strcmp(string, \"*\")) {\n \t\t\tbottom = 1;\n \t\t\ttop = menu_stuff->nr;\n \t\t} else {\n-\t\t\tbottom = find_unique((*ptr)->buf, menu_stuff);\n+\t\t\tbottom = find_unique(string, menu_stuff);\n \t\t\ttop = bottom;\n \t\t}\n \n \t\tif (top <= 0 || bottom <= 0 || top > menu_stuff->nr || bottom > top ||\n \t\t (is_single && bottom != top)) {\n \t\t\tclean_print_color(CLEAN_COLOR_ERROR);\n-\t\t\tprintf(_(\"Huh (%s)?\\n\"), (*ptr)->buf);\n+\t\t\tprintf(_(\"Huh (%s)?\\n\"), string);\n \t\t\tclean_print_color(CLEAN_COLOR_RESET);\n \t\t\tcontinue;\n \t\t}\n@@ -560,7 +556,7 @@ static int parse_choice(struct menu_stuff *menu_stuff,\n \t\t\t(*chosen)[i-1] = choose;\n \t}\n \n-\tstrbuf_list_free(choice_list);\n+\tstring_list_clear(&choice, 0);\n \n \tfor (i = 0; i < menu_stuff->nr; i++)\n \t\tnr += (*chosen)[i];\n-- \n2.50.1-612-g4756c59422\n\n"},{"id":"523068","messageId":"20250731074154.2835370-5-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731074154.2835370-1-gitster@pobox.com","subject":"[PATCH 4/9] clean: do not use strbuf_split*() [part 2]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T07:41:49Z","receivedAt":"2025-07-31T07:42:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"builtin/clean.c:filter_by_patterns_cmd() interactively reads a line\nthat has exclude patterns from the user and splits the line into a\nlist of patterns. It uses the strbuf_split() so that each split\npiece can then trimmed.\n\nThere is no need to use strbuf anymore, thanks to the recent\nenhancement to string_list_split*() family that allows us to trim\nthe pieces split into a string_list.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/clean.c | 20 +++++++++++---------\n 1 file changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 708cd9344c..56dabf7e03 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -674,12 +674,13 @@ static int filter_by_patterns_cmd(void)\n {\n \tstruct dir_struct dir = DIR_INIT;\n \tstruct strbuf confirm = STRBUF_INIT;\n-\tstruct strbuf **ignore_list;\n-\tstruct string_list_item *item;\n \tstruct pattern_list *pl;\n \tint changed = -1, i;\n \n \tfor (;;) {\n+\t\tstruct string_list ignore_list = STRING_LIST_INIT_NODUP;\n+\t\tstruct string_list_item *item;\n+\n \t\tif (!del_list.nr)\n \t\t\tbreak;\n \n@@ -697,14 +698,15 @@ static int filter_by_patterns_cmd(void)\n \t\t\tbreak;\n \n \t\tpl = add_pattern_list(&dir, EXC_CMDL, \"manual exclude\");\n-\t\tignore_list = strbuf_split_max(&confirm, ' ', 0);\n \n-\t\tfor (i = 0; ignore_list[i]; i++) {\n-\t\t\tstrbuf_trim(ignore_list[i]);\n-\t\t\tif (!ignore_list[i]->len)\n-\t\t\t\tcontinue;\n+\t\tstring_list_split_in_place_f(&ignore_list, confirm.buf, \" \", -1,\n+\t\t\t\t\t STRING_LIST_SPLIT_TRIM);\n \n-\t\t\tadd_pattern(ignore_list[i]->buf, \"\", 0, pl, -(i+1));\n+\t\tfor (i = 0; i < ignore_list.nr; i++) {\n+\t\t\titem = &ignore_list.items[i];\n+\t\t\tif (!*item->string)\n+\t\t\t\tcontinue;\n+\t\t\tadd_pattern(item->string, \"\", 0, pl, -(i+1));\n \t\t}\n \n \t\tchanged = 0;\n@@ -725,7 +727,7 @@ static int filter_by_patterns_cmd(void)\n \t\t\tclean_print_color(CLEAN_COLOR_RESET);\n \t\t}\n \n-\t\tstrbuf_list_free(ignore_list);\n+\t\tstring_list_clear(&ignore_list, 0);\n \t\tdir_clear(&dir);\n \t}\n \n-- \n2.50.1-612-g4756c59422\n\n"},{"id":"523069","messageId":"20250731074154.2835370-6-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731074154.2835370-1-gitster@pobox.com","subject":"[PATCH 5/9] merge-tree: do not use strbuf_split*()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T07:41:50Z","receivedAt":"2025-07-31T07:42:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When reading merge instructions from the standard input, the program\nreads from the standard input, splits the line into tokens at\nwhitespace, and trims each of them before using. We no longer need\nto use strbuf just for trimming, as string_list_split*() family can\ntrim while splitting a string.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/merge-tree.c | 30 ++++++++++++++++--------------\n 1 file changed, 16 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex cf8b06cadc..70235856d7 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -618,32 +618,34 @@ int cmd_merge_tree(int argc,\n \t\t\t \"--merge-base\", \"--stdin\");\n \t\tline_termination = '\\0';\n \t\twhile (strbuf_getline_lf(&buf, stdin) != EOF) {\n-\t\t\tstruct strbuf **split;\n+\t\t\tstruct string_list split = STRING_LIST_INIT_NODUP;\n \t\t\tconst char *input_merge_base = NULL;\n \n-\t\t\tsplit = strbuf_split(&buf, ' ');\n-\t\t\tif (!split[0] || !split[1])\n+\t\t\tstring_list_split_in_place_f(&split, buf.buf, \" \", -1,\n+\t\t\t\t\t\t STRING_LIST_SPLIT_TRIM);\n+\n+\t\t\tif (split.nr < 2)\n \t\t\t\tdie(_(\"malformed input line: '%s'.\"), buf.buf);\n-\t\t\tstrbuf_rtrim(split[0]);\n-\t\t\tstrbuf_rtrim(split[1]);\n \n \t\t\t/* parse the merge-base */\n-\t\t\tif (!strcmp(split[1]->buf, \"--\")) {\n-\t\t\t\tinput_merge_base = split[0]->buf;\n+\t\t\tif (!strcmp(split.items[1].string, \"--\")) {\n+\t\t\t\tinput_merge_base = split.items[0].string;\n \t\t\t}\n \n-\t\t\tif (input_merge_base && split[2] && split[3] && !split[4]) {\n-\t\t\t\tstrbuf_rtrim(split[2]);\n-\t\t\t\tstrbuf_rtrim(split[3]);\n-\t\t\t\treal_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix);\n-\t\t\t} else if (!input_merge_base && !split[2]) {\n-\t\t\t\treal_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix);\n+\t\t\tif (input_merge_base && split.nr == 4) {\n+\t\t\t\treal_merge(&o, input_merge_base,\n+\t\t\t\t\t split.items[2].string, split.items[3].string,\n+\t\t\t\t\t prefix);\n+\t\t\t} else if (!input_merge_base && split.nr == 2) {\n+\t\t\t\treal_merge(&o, NULL,\n+\t\t\t\t\t split.items[0].string, split.items[1].string,\n+\t\t\t\t\t prefix);\n \t\t\t} else {\n \t\t\t\tdie(_(\"malformed input line: '%s'.\"), buf.buf);\n \t\t\t}\n \t\t\tmaybe_flush_or_die(stdout, \"stdout\");\n \n-\t\t\tstrbuf_list_free(split);\n+\t\t\tstring_list_clear(&split, 0);\n \t\t}\n \t\tstrbuf_release(&buf);\n \n-- \n2.50.1-612-g4756c59422\n\n"},{"id":"523070","messageId":"20250731074154.2835370-7-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731074154.2835370-1-gitster@pobox.com","subject":"[PATCH 6/9] notes: do not use strbuf_split*()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T07:41:51Z","receivedAt":"2025-07-31T07:42:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When reading the copy instruction from the standard input, the\nprogram reads a line, splits it into tokens at whitespace, and trims\neach of the tokens before using. We no longer need to use strbuf\njust to be able to trimming, as string_list_split*() family now can\ntrim while splitting a string.\n\nRetire the use of strbuf_split().\n\nNote that this loop is a bit sloppy in that it ensures at least\nthere are two tokens on each line, but ignores if there are extra\ntokens on the line. Tightening it is outside the scope of this\nseries.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/notes.c | 23 ++++++++++++-----------\n 1 file changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex a9529b1696..4fb36a743c 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -375,18 +375,19 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \n \twhile (strbuf_getline_lf(&buf, stdin) != EOF) {\n \t\tstruct object_id from_obj, to_obj;\n-\t\tstruct strbuf **split;\n+\t\tstruct string_list split = STRING_LIST_INIT_NODUP;\n \t\tint err;\n \n-\t\tsplit = strbuf_split(&buf, ' ');\n-\t\tif (!split[0] || !split[1])\n+\t\tstring_list_split_in_place_f(&split, buf.buf, \" \", -1,\n+\t\t\t\t\t STRING_LIST_SPLIT_TRIM);\n+\t\tif (split.nr < 2)\n \t\t\tdie(_(\"malformed input line: '%s'.\"), buf.buf);\n-\t\tstrbuf_rtrim(split[0]);\n-\t\tstrbuf_rtrim(split[1]);\n-\t\tif (repo_get_oid(the_repository, split[0]->buf, &from_obj))\n-\t\t\tdie(_(\"failed to resolve '%s' as a valid ref.\"), split[0]->buf);\n-\t\tif (repo_get_oid(the_repository, split[1]->buf, &to_obj))\n-\t\t\tdie(_(\"failed to resolve '%s' as a valid ref.\"), split[1]->buf);\n+\t\tif (repo_get_oid(the_repository, split.items[0].string, &from_obj))\n+\t\t\tdie(_(\"failed to resolve '%s' as a valid ref.\"),\n+\t\t\t split.items[0].string);\n+\t\tif (repo_get_oid(the_repository, split.items[1].string, &to_obj))\n+\t\t\tdie(_(\"failed to resolve '%s' as a valid ref.\"),\n+\t\t\t split.items[1].string);\n \n \t\tif (rewrite_cmd)\n \t\t\terr = copy_note_for_rewrite(c, &from_obj, &to_obj);\n@@ -396,11 +397,11 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \n \t\tif (err) {\n \t\t\terror(_(\"failed to copy notes from '%s' to '%s'\"),\n-\t\t\t split[0]->buf, split[1]->buf);\n+\t\t\t split.items[0].string, split.items[1].string);\n \t\t\tret = 1;\n \t\t}\n \n-\t\tstrbuf_list_free(split);\n+\t\tstring_list_clear(&split, 0);\n \t}\n \n \tif (!rewrite_cmd) {\n-- \n2.50.1-612-g4756c59422\n\n"},{"id":"523071","messageId":"20250731074154.2835370-8-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731074154.2835370-1-gitster@pobox.com","subject":"[PATCH 7/9] config: do not use strbuf_split()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T07:41:52Z","receivedAt":"2025-07-31T07:42:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When parsing an old-style GIT_CONFIG_PARAMETERS environment\nvariable, the code parses the key=value pair by spliting them at '='\ninto an array of strbuf's. As strbuf_split() leafes the delimiter\nat the end of the split piece, the code has to manually trim it.\n\nIf we split with string_list_split(), that becomes unnecessary.\nRetire the use of strbuf_split().\n\nNote that the max parameter of string_list_split() is of\nan ergonomically iffy design---it specifies the maximum number of\ntimes the function is allowed to split, which means that in order to\nsplit a text into up to 2 pieces, you have to pass 1, not 2.\n\nSigned-off-by: Junio C Hamano \n---\n config.c | 23 ++++++++++-------------\n 1 file changed, 10 insertions(+), 13 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 8a2d0b7916..1769f15ee3 100644\n--- a/config.c\n+++ b/config.c\n@@ -638,31 +638,28 @@ int git_config_parse_parameter(const char *text,\n \t\t\t config_fn_t fn, void *data)\n {\n \tconst char *value;\n-\tstruct strbuf **pair;\n+\tstruct string_list pair = STRING_LIST_INIT_DUP;\n \tint ret;\n \tstruct key_value_info kvi = KVI_INIT;\n \n \tkvi_from_param(&kvi);\n \n-\tpair = strbuf_split_str(text, '=', 2);\n-\tif (!pair[0])\n+\tstring_list_split(&pair, text, \"=\", 1);\n+\tif (!pair.nr)\n \t\treturn error(_(\"bogus config parameter: %s\"), text);\n \n-\tif (pair[0]->len && pair[0]->buf[pair[0]->len - 1] == '=') {\n-\t\tstrbuf_setlen(pair[0], pair[0]->len - 1);\n-\t\tvalue = pair[1] ? pair[1]->buf : \"\";\n-\t} else {\n+\tif (pair.nr == 1)\n \t\tvalue = NULL;\n-\t}\n+\telse\n+\t\tvalue = pair.items[1].string;\n \n-\tstrbuf_trim(pair[0]);\n-\tif (!pair[0]->len) {\n-\t\tstrbuf_list_free(pair);\n+\tif (!*pair.items[0].string) {\n+\t\tstring_list_clear(&pair, 0);\n \t\treturn error(_(\"bogus config parameter: %s\"), text);\n \t}\n \n-\tret = config_parse_pair(pair[0]->buf, value, &kvi, fn, data);\n-\tstrbuf_list_free(pair);\n+\tret = config_parse_pair(pair.items[0].string, value, &kvi, fn, data);\n+\tstring_list_clear(&pair, 0);\n \treturn ret;\n }\n \n-- \n2.50.1-612-g4756c59422\n\n"},{"id":"523072","messageId":"20250731074154.2835370-9-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731074154.2835370-1-gitster@pobox.com","subject":"[PATCH 8/9] environment: do not use strbuf_split*()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T07:41:53Z","receivedAt":"2025-07-31T07:42:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"environment.c:get_git_namespace() learns the raw namespace from an\nenvironment variable, splits it at \"/\", and appends them after\n\"refs/namespaces/\"; the reason why it splits first is so that an\nempty string resulting from double slashes can be omitted.\n\nThe split pieces do not need to be edited in any way, so an array of\nstrbufs is a wrong data structure to use. Instead split into a\nstring list and use the pieces from there.\n\nSigned-off-by: Junio C Hamano \n---\n environment.c | 19 ++++++++++++-------\n 1 file changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex 7c2480b22e..ab3ed08433 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -163,10 +163,10 @@ int have_git_dir(void)\n const char *get_git_namespace(void)\n {\n \tstatic const char *namespace;\n-\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct strbuf **components, **c;\n \tconst char *raw_namespace;\n+\tstruct string_list components = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *item;\n \n \tif (namespace)\n \t\treturn namespace;\n@@ -178,12 +178,17 @@ const char *get_git_namespace(void)\n \t}\n \n \tstrbuf_addstr(&buf, raw_namespace);\n-\tcomponents = strbuf_split(&buf, '/');\n+\n+\tstring_list_split(&components, buf.buf, \"/\", -1);\n \tstrbuf_reset(&buf);\n-\tfor (c = components; *c; c++)\n-\t\tif (strcmp((*c)->buf, \"/\") != 0)\n-\t\t\tstrbuf_addf(&buf, \"refs/namespaces/%s\", (*c)->buf);\n-\tstrbuf_list_free(components);\n+\n+\tfor_each_string_list_item(item, &components) {\n+\t\tif (item->string[0])\n+\t\t\tstrbuf_addf(&buf, \"refs/namespaces/%s/\", item->string);\n+\t}\n+\tstring_list_clear(&components, 0);\n+\n+\tstrbuf_trim_trailing_dir_sep(&buf);\n \tif (check_refname_format(buf.buf, 0))\n \t\tdie(_(\"bad git namespace path \\\"%s\\\"\"), raw_namespace);\n \tstrbuf_addch(&buf, '/');\n-- \n2.50.1-612-g4756c59422\n\n"},{"id":"523073","messageId":"20250731074154.2835370-10-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731074154.2835370-1-gitster@pobox.com","subject":"[PATCH 9/9] sub-process: do not use strbuf_split*()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T07:41:54Z","receivedAt":"2025-07-31T07:42:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The code to read status from subprocess reads one packet line and\ntries to find \"status=\". It is way overkill to split the line\ninto an array of two strbufs to extract .\n\nSigned-off-by: Junio C Hamano \n---\n sub-process.c | 16 +++++++---------\n 1 file changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/sub-process.c b/sub-process.c\nindex 1daf5a9752..de3235c15a 100644\n--- a/sub-process.c\n+++ b/sub-process.c\n@@ -5,6 +5,7 @@\n #include \"sub-process.h\"\n #include \"sigchain.h\"\n #include \"pkt-line.h\"\n+#include \"string-list.h\"\n \n int cmd2process_cmp(const void *cmp_data UNUSED,\n \t\t const struct hashmap_entry *eptr,\n@@ -30,23 +31,20 @@ struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const ch\n \n int subprocess_read_status(int fd, struct strbuf *status)\n {\n-\tstruct strbuf **pair;\n-\tchar *line;\n \tint len;\n \n \tfor (;;) {\n+\t\tchar *line;\n+\t\tconst char *value;\n+\n \t\tlen = packet_read_line_gently(fd, NULL, &line);\n \t\tif ((len < 0) || !line)\n \t\t\tbreak;\n-\t\tpair = strbuf_split_str(line, '=', 2);\n-\t\tif (pair[0] && pair[0]->len && pair[1]) {\n+\t\tif (skip_prefix(line, \"status=\", &value)) {\n \t\t\t/* the last \"status=\" line wins */\n-\t\t\tif (!strcmp(pair[0]->buf, \"status=\")) {\n-\t\t\t\tstrbuf_reset(status);\n-\t\t\t\tstrbuf_addbuf(status, pair[1]);\n-\t\t\t}\n+\t\t\tstrbuf_reset(status);\n+\t\t\tstrbuf_addstr(status, value);\n \t\t}\n-\t\tstrbuf_list_free(pair);\n \t}\n \n \treturn (len < 0) ? len : 0;\n-- \n2.50.1-612-g4756c59422\n\n"},{"id":"523083","messageId":"CAP8UFD2qAOhRN=b9PB_eXQS1PntZKX95ZK=V4G=10n3iU2xtbw@mail.gmail.com","threadId":"63882","inReplyTo":"20250731074154.2835370-10-gitster@pobox.com","subject":"Re: [PATCH 9/9] sub-process: do not use strbuf_split*()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2025-07-31T08:50:50Z","receivedAt":"2025-07-31T08:51:04Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Jul 31, 2025 at 9:44 AM Junio C Hamano wrote:\n>\n> The code to read status from subprocess reads one packet line and\n> tries to find \"status=\". It is way overkill to split the line\n> into an array of two strbufs to extract .\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> sub-process.c | 16 +++++++---------\n> 1 file changed, 7 insertions(+), 9 deletions(-)\n>\n> diff --git a/sub-process.c b/sub-process.c\n> index 1daf5a9752..de3235c15a 100644\n> --- a/sub-process.c\n> +++ b/sub-process.c\n> @@ -5,6 +5,7 @@\n> #include \"sub-process.h\"\n> #include \"sigchain.h\"\n> #include \"pkt-line.h\"\n> +#include \"string-list.h\"\n\nIs this needed? It looks like skip_prefix() (not any string_list\nfunction) is used instead of strbuf_split_str().\n\nThanks.\n"},{"id":"523095","messageId":"xmqqikj8touu.fsf@gitster.g","threadId":"63882","inReplyTo":"CAP8UFD2qAOhRN=b9PB_eXQS1PntZKX95ZK=V4G=10n3iU2xtbw@mail.gmail.com","subject":"Re: [PATCH 9/9] sub-process: do not use strbuf_split*()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T14:30:01Z","receivedAt":"2025-07-31T14:30:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Thu, Jul 31, 2025 at 9:44 AM Junio C Hamano wrote:\n>>\n>> The code to read status from subprocess reads one packet line and\n>> tries to find \"status=\". It is way overkill to split the line\n>> into an array of two strbufs to extract .\n>>\n>> Signed-off-by: Junio C Hamano \n>> ---\n>> sub-process.c | 16 +++++++---------\n>> 1 file changed, 7 insertions(+), 9 deletions(-)\n>>\n>> diff --git a/sub-process.c b/sub-process.c\n>> index 1daf5a9752..de3235c15a 100644\n>> --- a/sub-process.c\n>> +++ b/sub-process.c\n>> @@ -5,6 +5,7 @@\n>> #include \"sub-process.h\"\n>> #include \"sigchain.h\"\n>> #include \"pkt-line.h\"\n>> +#include \"string-list.h\"\n>\n> Is this needed? It looks like skip_prefix() (not any string_list\n> function) is used instead of strbuf_split_str().\n\nYes, as you suspected, I initially rewrote strbuf_split*() with\nstring_list_split*(), but then realized there is no need to split\nthe string into tokens in the first place.\n\nWill remove the unused include.\nThanks for carefully reading.\n"},{"id":"523122","messageId":"CAPig+cQVo9diuwxqJZ-ippc65B2e_aFq6vEVZ2j-8ZYrVByZ9A@mail.gmail.com","threadId":"63882","inReplyTo":"20250731074154.2835370-7-gitster@pobox.com","subject":"Re: [PATCH 6/9] notes: do not use strbuf_split*()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-07-31T20:14:09Z","receivedAt":"2025-07-31T20:14:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 31, 2025 at 3:42 AM Junio C Hamano wrote:\n> When reading the copy instruction from the standard input, the\n> program reads a line, splits it into tokens at whitespace, and trims\n> each of the tokens before using. We no longer need to use strbuf\n> just to be able to trimming, as string_list_split*() family now can\n> trim while splitting a string.\n\ns/trimming/trim/\n\n> Retire the use of strbuf_split().\n>\n> Note that this loop is a bit sloppy in that it ensures at least\n> there are two tokens on each line, but ignores if there are extra\n> tokens on the line. Tightening it is outside the scope of this\n> series.\n>\n> Signed-off-by: Junio C Hamano \n"},{"id":"523123","messageId":"CAPig+cTnNeZ9gtQbKdL6P2KBn7jzGJT6BC+tPMnzBNaR0tyrvQ@mail.gmail.com","threadId":"63882","inReplyTo":"20250731074154.2835370-8-gitster@pobox.com","subject":"Re: [PATCH 7/9] config: do not use strbuf_split()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-07-31T20:15:49Z","receivedAt":"2025-07-31T20:16:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 31, 2025 at 3:42 AM Junio C Hamano wrote:\n> When parsing an old-style GIT_CONFIG_PARAMETERS environment\n> variable, the code parses the key=value pair by spliting them at '='\n> into an array of strbuf's. As strbuf_split() leafes the delimiter\n> at the end of the split piece, the code has to manually trim it.\n\ns/spliting/splitting/\ns/leafes/leaves/\n\n> If we split with string_list_split(), that becomes unnecessary.\n> Retire the use of strbuf_split().\n>\n> Note that the max parameter of string_list_split() is of\n> an ergonomically iffy design---it specifies the maximum number of\n> times the function is allowed to split, which means that in order to\n> split a text into up to 2 pieces, you have to pass 1, not 2.\n>\n> Signed-off-by: Junio C Hamano \n"},{"id":"523141","messageId":"20250731225433.4028872-1-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731074154.2835370-1-gitster@pobox.com","subject":"[PATCH v2 00/11] do not overuse strbuf_split*()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T22:54:22Z","receivedAt":"2025-07-31T22:54:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"strbuf is a very good data structure to work with string data\nwithout having to worry about running past the end of the string.\n\nBut an array of strbuf is often a wrong data structure. You rarely\nhave need to be able to edit multiple strings represented by such an\narray simultaneously. And strbuf_split*() that produces result in\nsuch a shape is a misdesigned API function.\n\nThe most common use case of strbuf_split*() family of functions\nseems to be to trim away the whitespaces around each piece of split\nstring. With modern string_list_split*(), it is often no longer\nnecessary.\n\nThis series builds on top of the other series that extends string\nlist API to allow string_list_split() to take more than one delimiter\nbytes, and to optionally trim the resulting string pieces.\n\nI do not plan to eradicate all the uses of strbuf_split*() myself,\nnot because I found some valid use cases in the existing code (I\nhaven't yet), but these patches would give interested others enough\nmaterial to study and mimic to continue the effort and I can safely\nleave it as #leftoverbits to rewrite them.\n\nJunio C Hamano (11):\n wt-status: avoid strbuf_split*()\n clean: do not pass strbuf by value\n clean: do not use strbuf_split*() [part 1]\n clean: do not use strbuf_split*() [part 2]\n merge-tree: do not use strbuf_split*()\n notes: do not use strbuf_split*()\n config: do not use strbuf_split()\n environment: do not use strbuf_split*()\n sub-process: do not use strbuf_split*()\n trace2: trim_trailing_newline followed by trim is a no-op\n trace2: do not use strbuf_split*()\n\n builtin/clean.c | 74 ++++++++++++++++++++--------------------\n builtin/merge-tree.c | 30 +++++++++--------\n builtin/notes.c | 23 +++++++------\n config.c | 23 ++++++-------\n environment.c | 19 +++++++----\n sub-process.c | 15 ++++-----\n trace2/tr2_cfg.c | 80 +++++++++++++++-----------------------------\n wt-status.c | 31 ++++++-----------\n 8 files changed, 129 insertions(+), 166 deletions(-)\n\n 1: 79164fa6de = 1: e254c0b462 wt-status: avoid strbuf_split*()\n 2: 85006a11ff = 2: 07306ec99d clean: do not pass strbuf by value\n 3: 633dd871f7 = 3: 1dc849eb0b clean: do not use strbuf_split*() [part 1]\n 4: 60dd58af7e = 4: 69b885a579 clean: do not use strbuf_split*() [part 2]\n 5: 49a6606d76 = 5: ac90eb1b57 merge-tree: do not use strbuf_split*()\n 6: 3813cc0e4d ! 6: 68dd2be7ae notes: do not use strbuf_split*()\n @@ Metadata\n ## Commit message ##\n notes: do not use strbuf_split*()\n \n - When reading the copy instruction from the standard input, the\n - program reads a line, splits it into tokens at whitespace, and trims\n - each of the tokens before using. We no longer need to use strbuf\n - just to be able to trimming, as string_list_split*() family now can\n - trim while splitting a string.\n + When reading copy instructions from the standard input, the program\n + reads a line, splits it into tokens at whitespace, and trims each of\n + the tokens before using. We no longer need to use strbuf just to be\n + able to trim, as string_list_split*() family now can trim while\n + splitting a string.\n \n - Retire the use of strbuf_split().\n + Retire the use of strbuf_split() from this code path.\n \n Note that this loop is a bit sloppy in that it ensures at least\n there are two tokens on each line, but ignores if there are extra\n 7: 2bd08bde36 ! 7: 1f8c86ad0a config: do not use strbuf_split()\n @@ Commit message\n config: do not use strbuf_split()\n \n When parsing an old-style GIT_CONFIG_PARAMETERS environment\n - variable, the code parses the key=value pair by spliting them at '='\n - into an array of strbuf's. As strbuf_split() leafes the delimiter\n + variable, the code parses key=value pairs by splitting them at '='\n + into an array of strbuf's. As strbuf_split() leaves the delimiter\n at the end of the split piece, the code has to manually trim it.\n \n If we split with string_list_split(), that becomes unnecessary.\n - Retire the use of strbuf_split().\n + Retire the use of strbuf_split() from this code path.\n \n Note that the max parameter of string_list_split() is of\n an ergonomically iffy design---it specifies the maximum number of\n 8: ed35491fb1 = 8: 26c782fd7e environment: do not use strbuf_split*()\n 9: 5b8fe54684 ! 9: 95316b62fe sub-process: do not use strbuf_split*()\n @@ Commit message\n Signed-off-by: Junio C Hamano \n \n ## sub-process.c ##\n -@@\n - #include \"sub-process.h\"\n - #include \"sigchain.h\"\n - #include \"pkt-line.h\"\n -+#include \"string-list.h\"\n - \n - int cmd2process_cmp(const void *cmp_data UNUSED,\n - \t\t const struct hashmap_entry *eptr,\n @@ sub-process.c: struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const ch\n \n int subprocess_read_status(int fd, struct strbuf *status)\n -: ---------- > 10: 09e83741d2 trace2: trim_trailing_newline followed by trim is a no-op\n -: ---------- > 11: be9c9cb420 trace2: do not use strbuf_split*()\n"},{"id":"523140","messageId":"20250731225433.4028872-2-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731225433.4028872-1-gitster@pobox.com","subject":"[PATCH v2 01/11] wt-status: avoid strbuf_split*()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T22:54:23Z","receivedAt":"2025-07-31T22:54:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"strbuf is a very good data structure to work with string data\nwithout having to worry about running past the end of the string,\nbut strbuf_split() is a wrong API and an array of strbuf that the\nfunction produces is a wrong thing to use in general. You do not\nedit these N strings split out of a single strbuf simultaneously.\nOften it is much better off to split a string into string_list and\nwork with the resulting strings.\n\nwt-status.c:abbrev_oid_in_line() takes one line of rebase todo list\n(like \"pick e813a0200a7121b97fec535f0d0b460b0a33356c title\"), and\nfor instructions that has an object name as the second token on the\nline, replace the object name with its unique abbreviation. After\nsplitting these tokens out of a single line, no simultaneous edit on\nany of these pieces of string that takes advantage of strbuf API\ntakes place. The final string is composed with strbuf API, but\nthese split pieces are merely used as pieces of strings and there is\nno need for them to be stored in individual strbuf.\n\nInstead, split the line into a string_list, and compose the final\nstring using these pieces.\n\nSigned-off-by: Junio C Hamano \n---\n wt-status.c | 31 ++++++++++---------------------\n 1 file changed, 10 insertions(+), 21 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 454601afa1..a34dc144ee 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1351,8 +1351,8 @@ static int split_commit_in_progress(struct wt_status *s)\n */\n static void abbrev_oid_in_line(struct strbuf *line)\n {\n-\tstruct strbuf **split;\n-\tint i;\n+\tstruct string_list split = STRING_LIST_INIT_DUP;\n+\tstruct object_id oid;\n \n \tif (starts_with(line->buf, \"exec \") ||\n \t starts_with(line->buf, \"x \") ||\n@@ -1360,26 +1360,15 @@ static void abbrev_oid_in_line(struct strbuf *line)\n \t starts_with(line->buf, \"l \"))\n \t\treturn;\n \n-\tsplit = strbuf_split_max(line, ' ', 3);\n-\tif (split[0] && split[1]) {\n-\t\tstruct object_id oid;\n-\n-\t\t/*\n-\t\t * strbuf_split_max left a space. Trim it and re-add\n-\t\t * it after abbreviation.\n-\t\t */\n-\t\tstrbuf_trim(split[1]);\n-\t\tif (!repo_get_oid(the_repository, split[1]->buf, &oid)) {\n-\t\t\tstrbuf_reset(split[1]);\n-\t\t\tstrbuf_add_unique_abbrev(split[1], &oid,\n-\t\t\t\t\t\t DEFAULT_ABBREV);\n-\t\t\tstrbuf_addch(split[1], ' ');\n-\t\t\tstrbuf_reset(line);\n-\t\t\tfor (i = 0; split[i]; i++)\n-\t\t\t\tstrbuf_addbuf(line, split[i]);\n-\t\t}\n+\tif ((2 <= string_list_split(&split, line->buf, \" \", 2)) &&\n+\t !repo_get_oid(the_repository, split.items[1].string, &oid)) {\n+\t\tstrbuf_reset(line);\n+\t\tstrbuf_addf(line, \"%s \", split.items[0].string);\n+\t\tstrbuf_add_unique_abbrev(line, &oid, DEFAULT_ABBREV);\n+\t\tfor (size_t i = 2; i < split.nr; i++)\n+\t\t\tstrbuf_addf(line, \" %s\", split.items[i].string);\n \t}\n-\tstrbuf_list_free(split);\n+\tstring_list_clear(&split, 0);\n }\n \n static int read_rebase_todolist(const char *fname, struct string_list *lines)\n-- \n2.50.1-618-g45d530d26b\n\n"},{"id":"523142","messageId":"20250731225433.4028872-3-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731225433.4028872-1-gitster@pobox.com","subject":"[PATCH v2 02/11] clean: do not pass strbuf by value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T22:54:24Z","receivedAt":"2025-07-31T22:54:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When you pass a structure by value, the callee can modify the\ncontents of the structure that was passed in without having to worry\nabout changing the structure the caller has. Passing structure by\nvalue sometimes (but not very often) can be a valid way to give\ncallee a temporary variable it can freely modify.\n\nBut not a structure with members that are pointers, like a strbuf.\n\nbuiltin/clean.c:list_and_choose() reads a line interactively from\nthe user, and passes the line (in a strbuf) to parse_choice() by\nvalue, which then munges by replacing ',' with ' ' (to accept both\ncomma and space separated list of choices). But because the strbuf\npassed by value still shares the underlying character array buf[],\nthis ends up munging the caller's strbuf contents.\n\nThis is a catastrophe waiting to happen. If the callee causes the\nstrbuf to be reallocated, the buf[] the caller has will become\ndangling, and when the caller does strbuf_release(), it would result\nin double-free.\n\nStop calling the function with misleading call-by-value with strbuf.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/clean.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 053c94fc6b..224551537e 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -477,7 +477,7 @@ static int find_unique(const char *choice, struct menu_stuff *menu_stuff)\n */\n static int parse_choice(struct menu_stuff *menu_stuff,\n \t\t\tint is_single,\n-\t\t\tstruct strbuf input,\n+\t\t\tstruct strbuf *input,\n \t\t\tint **chosen)\n {\n \tstruct strbuf **choice_list, **ptr;\n@@ -485,14 +485,14 @@ static int parse_choice(struct menu_stuff *menu_stuff,\n \tint i;\n \n \tif (is_single) {\n-\t\tchoice_list = strbuf_split_max(&input, '\\n', 0);\n+\t\tchoice_list = strbuf_split_max(input, '\\n', 0);\n \t} else {\n-\t\tchar *p = input.buf;\n+\t\tchar *p = input->buf;\n \t\tdo {\n \t\t\tif (*p == ',')\n \t\t\t\t*p = ' ';\n \t\t} while (*p++);\n-\t\tchoice_list = strbuf_split_max(&input, ' ', 0);\n+\t\tchoice_list = strbuf_split_max(input, ' ', 0);\n \t}\n \n \tfor (ptr = choice_list; *ptr; ptr++) {\n@@ -630,7 +630,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)\n \n \t\tnr = parse_choice(stuff,\n \t\t\t\t opts->flags & MENU_OPTS_SINGLETON,\n-\t\t\t\t choice,\n+\t\t\t\t &choice,\n \t\t\t\t &chosen);\n \n \t\tif (opts->flags & MENU_OPTS_SINGLETON) {\n-- \n2.50.1-618-g45d530d26b\n\n"},{"id":"523143","messageId":"20250731225433.4028872-4-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731225433.4028872-1-gitster@pobox.com","subject":"[PATCH v2 03/11] clean: do not use strbuf_split*() [part 1]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T22:54:25Z","receivedAt":"2025-07-31T22:54:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"builtin/clean.c:parse_choice() is fed a single line of input, which\nis space or comma separated list of tokens, and a list of menu\nitems. It parses the tokens into number ranges (e.g. 1-3 that means\nthe first three items) or string prefix (e.g. 's' to choose the menu\nitem \"(s)elect\") that specify the elements in the menu item list,\nand tells the caller which ones are chosen.\n\nFor parsing the input string, it uses strbuf_split() to split it\ninto bunch of strbufs. Instead use string_list_split_in_place(),\nfor a few reasons.\n\n * strbuf_split() is a bad API function to use, that yields an array\n of strbuf that is a bad data structure to use in general.\n\n * string_list_split_in_place() allows you to split with \"comma or\n space\"; the current code has to preprocess the input string to\n replace comma with space because strbuf_split() does not allow\n this.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/clean.c | 50 +++++++++++++++++++++++--------------------------\n 1 file changed, 23 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 224551537e..708cd9344c 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -480,40 +480,36 @@ static int parse_choice(struct menu_stuff *menu_stuff,\n \t\t\tstruct strbuf *input,\n \t\t\tint **chosen)\n {\n-\tstruct strbuf **choice_list, **ptr;\n+\tstruct string_list choice = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *item;\n \tint nr = 0;\n \tint i;\n \n-\tif (is_single) {\n-\t\tchoice_list = strbuf_split_max(input, '\\n', 0);\n-\t} else {\n-\t\tchar *p = input->buf;\n-\t\tdo {\n-\t\t\tif (*p == ',')\n-\t\t\t\t*p = ' ';\n-\t\t} while (*p++);\n-\t\tchoice_list = strbuf_split_max(input, ' ', 0);\n-\t}\n+\tstring_list_split_in_place_f(&choice, input->buf,\n+\t\t\t\t is_single ? \"\\n\" : \", \", -1,\n+\t\t\t\t STRING_LIST_SPLIT_TRIM);\n \n-\tfor (ptr = choice_list; *ptr; ptr++) {\n-\t\tchar *p;\n-\t\tint choose = 1;\n+\tfor_each_string_list_item(item, &choice) {\n+\t\tconst char *string;\n+\t\tint choose;\n \t\tint bottom = 0, top = 0;\n \t\tint is_range, is_number;\n \n-\t\tstrbuf_trim(*ptr);\n-\t\tif (!(*ptr)->len)\n+\t\tstring = item->string;\n+\t\tif (!*string)\n \t\t\tcontinue;\n \n \t\t/* Input that begins with '-'; unchoose */\n-\t\tif (*(*ptr)->buf == '-') {\n+\t\tif (string[0] == '-') {\n \t\t\tchoose = 0;\n-\t\t\tstrbuf_remove((*ptr), 0, 1);\n+\t\t\tstring++;\n+\t\t} else {\n+\t\t\tchoose = 1;\n \t\t}\n \n \t\tis_range = 0;\n \t\tis_number = 1;\n-\t\tfor (p = (*ptr)->buf; *p; p++) {\n+\t\tfor (const char *p = string; *p; p++) {\n \t\t\tif ('-' == *p) {\n \t\t\t\tif (!is_range) {\n \t\t\t\t\tis_range = 1;\n@@ -531,27 +527,27 @@ static int parse_choice(struct menu_stuff *menu_stuff,\n \t\t}\n \n \t\tif (is_number) {\n-\t\t\tbottom = atoi((*ptr)->buf);\n+\t\t\tbottom = atoi(string);\n \t\t\ttop = bottom;\n \t\t} else if (is_range) {\n-\t\t\tbottom = atoi((*ptr)->buf);\n+\t\t\tbottom = atoi(string);\n \t\t\t/* a range can be specified like 5-7 or 5- */\n-\t\t\tif (!*(strchr((*ptr)->buf, '-') + 1))\n+\t\t\tif (!*(strchr(string, '-') + 1))\n \t\t\t\ttop = menu_stuff->nr;\n \t\t\telse\n-\t\t\t\ttop = atoi(strchr((*ptr)->buf, '-') + 1);\n-\t\t} else if (!strcmp((*ptr)->buf, \"*\")) {\n+\t\t\t\ttop = atoi(strchr(string, '-') + 1);\n+\t\t} else if (!strcmp(string, \"*\")) {\n \t\t\tbottom = 1;\n \t\t\ttop = menu_stuff->nr;\n \t\t} else {\n-\t\t\tbottom = find_unique((*ptr)->buf, menu_stuff);\n+\t\t\tbottom = find_unique(string, menu_stuff);\n \t\t\ttop = bottom;\n \t\t}\n \n \t\tif (top <= 0 || bottom <= 0 || top > menu_stuff->nr || bottom > top ||\n \t\t (is_single && bottom != top)) {\n \t\t\tclean_print_color(CLEAN_COLOR_ERROR);\n-\t\t\tprintf(_(\"Huh (%s)?\\n\"), (*ptr)->buf);\n+\t\t\tprintf(_(\"Huh (%s)?\\n\"), string);\n \t\t\tclean_print_color(CLEAN_COLOR_RESET);\n \t\t\tcontinue;\n \t\t}\n@@ -560,7 +556,7 @@ static int parse_choice(struct menu_stuff *menu_stuff,\n \t\t\t(*chosen)[i-1] = choose;\n \t}\n \n-\tstrbuf_list_free(choice_list);\n+\tstring_list_clear(&choice, 0);\n \n \tfor (i = 0; i < menu_stuff->nr; i++)\n \t\tnr += (*chosen)[i];\n-- \n2.50.1-618-g45d530d26b\n\n"},{"id":"523144","messageId":"20250731225433.4028872-5-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731225433.4028872-1-gitster@pobox.com","subject":"[PATCH v2 04/11] clean: do not use strbuf_split*() [part 2]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T22:54:26Z","receivedAt":"2025-07-31T22:54:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"builtin/clean.c:filter_by_patterns_cmd() interactively reads a line\nthat has exclude patterns from the user and splits the line into a\nlist of patterns. It uses the strbuf_split() so that each split\npiece can then trimmed.\n\nThere is no need to use strbuf anymore, thanks to the recent\nenhancement to string_list_split*() family that allows us to trim\nthe pieces split into a string_list.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/clean.c | 20 +++++++++++---------\n 1 file changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 708cd9344c..56dabf7e03 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -674,12 +674,13 @@ static int filter_by_patterns_cmd(void)\n {\n \tstruct dir_struct dir = DIR_INIT;\n \tstruct strbuf confirm = STRBUF_INIT;\n-\tstruct strbuf **ignore_list;\n-\tstruct string_list_item *item;\n \tstruct pattern_list *pl;\n \tint changed = -1, i;\n \n \tfor (;;) {\n+\t\tstruct string_list ignore_list = STRING_LIST_INIT_NODUP;\n+\t\tstruct string_list_item *item;\n+\n \t\tif (!del_list.nr)\n \t\t\tbreak;\n \n@@ -697,14 +698,15 @@ static int filter_by_patterns_cmd(void)\n \t\t\tbreak;\n \n \t\tpl = add_pattern_list(&dir, EXC_CMDL, \"manual exclude\");\n-\t\tignore_list = strbuf_split_max(&confirm, ' ', 0);\n \n-\t\tfor (i = 0; ignore_list[i]; i++) {\n-\t\t\tstrbuf_trim(ignore_list[i]);\n-\t\t\tif (!ignore_list[i]->len)\n-\t\t\t\tcontinue;\n+\t\tstring_list_split_in_place_f(&ignore_list, confirm.buf, \" \", -1,\n+\t\t\t\t\t STRING_LIST_SPLIT_TRIM);\n \n-\t\t\tadd_pattern(ignore_list[i]->buf, \"\", 0, pl, -(i+1));\n+\t\tfor (i = 0; i < ignore_list.nr; i++) {\n+\t\t\titem = &ignore_list.items[i];\n+\t\t\tif (!*item->string)\n+\t\t\t\tcontinue;\n+\t\t\tadd_pattern(item->string, \"\", 0, pl, -(i+1));\n \t\t}\n \n \t\tchanged = 0;\n@@ -725,7 +727,7 @@ static int filter_by_patterns_cmd(void)\n \t\t\tclean_print_color(CLEAN_COLOR_RESET);\n \t\t}\n \n-\t\tstrbuf_list_free(ignore_list);\n+\t\tstring_list_clear(&ignore_list, 0);\n \t\tdir_clear(&dir);\n \t}\n \n-- \n2.50.1-618-g45d530d26b\n\n"},{"id":"523145","messageId":"20250731225433.4028872-6-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731225433.4028872-1-gitster@pobox.com","subject":"[PATCH v2 05/11] merge-tree: do not use strbuf_split*()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T22:54:27Z","receivedAt":"2025-07-31T22:54:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When reading merge instructions from the standard input, the program\nreads from the standard input, splits the line into tokens at\nwhitespace, and trims each of them before using. We no longer need\nto use strbuf just for trimming, as string_list_split*() family can\ntrim while splitting a string.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/merge-tree.c | 30 ++++++++++++++++--------------\n 1 file changed, 16 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex cf8b06cadc..70235856d7 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -618,32 +618,34 @@ int cmd_merge_tree(int argc,\n \t\t\t \"--merge-base\", \"--stdin\");\n \t\tline_termination = '\\0';\n \t\twhile (strbuf_getline_lf(&buf, stdin) != EOF) {\n-\t\t\tstruct strbuf **split;\n+\t\t\tstruct string_list split = STRING_LIST_INIT_NODUP;\n \t\t\tconst char *input_merge_base = NULL;\n \n-\t\t\tsplit = strbuf_split(&buf, ' ');\n-\t\t\tif (!split[0] || !split[1])\n+\t\t\tstring_list_split_in_place_f(&split, buf.buf, \" \", -1,\n+\t\t\t\t\t\t STRING_LIST_SPLIT_TRIM);\n+\n+\t\t\tif (split.nr < 2)\n \t\t\t\tdie(_(\"malformed input line: '%s'.\"), buf.buf);\n-\t\t\tstrbuf_rtrim(split[0]);\n-\t\t\tstrbuf_rtrim(split[1]);\n \n \t\t\t/* parse the merge-base */\n-\t\t\tif (!strcmp(split[1]->buf, \"--\")) {\n-\t\t\t\tinput_merge_base = split[0]->buf;\n+\t\t\tif (!strcmp(split.items[1].string, \"--\")) {\n+\t\t\t\tinput_merge_base = split.items[0].string;\n \t\t\t}\n \n-\t\t\tif (input_merge_base && split[2] && split[3] && !split[4]) {\n-\t\t\t\tstrbuf_rtrim(split[2]);\n-\t\t\t\tstrbuf_rtrim(split[3]);\n-\t\t\t\treal_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix);\n-\t\t\t} else if (!input_merge_base && !split[2]) {\n-\t\t\t\treal_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix);\n+\t\t\tif (input_merge_base && split.nr == 4) {\n+\t\t\t\treal_merge(&o, input_merge_base,\n+\t\t\t\t\t split.items[2].string, split.items[3].string,\n+\t\t\t\t\t prefix);\n+\t\t\t} else if (!input_merge_base && split.nr == 2) {\n+\t\t\t\treal_merge(&o, NULL,\n+\t\t\t\t\t split.items[0].string, split.items[1].string,\n+\t\t\t\t\t prefix);\n \t\t\t} else {\n \t\t\t\tdie(_(\"malformed input line: '%s'.\"), buf.buf);\n \t\t\t}\n \t\t\tmaybe_flush_or_die(stdout, \"stdout\");\n \n-\t\t\tstrbuf_list_free(split);\n+\t\t\tstring_list_clear(&split, 0);\n \t\t}\n \t\tstrbuf_release(&buf);\n \n-- \n2.50.1-618-g45d530d26b\n\n"},{"id":"523146","messageId":"20250731225433.4028872-7-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731225433.4028872-1-gitster@pobox.com","subject":"[PATCH v2 06/11] notes: do not use strbuf_split*()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T22:54:28Z","receivedAt":"2025-07-31T22:54:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When reading copy instructions from the standard input, the program\nreads a line, splits it into tokens at whitespace, and trims each of\nthe tokens before using. We no longer need to use strbuf just to be\nable to trim, as string_list_split*() family now can trim while\nsplitting a string.\n\nRetire the use of strbuf_split() from this code path.\n\nNote that this loop is a bit sloppy in that it ensures at least\nthere are two tokens on each line, but ignores if there are extra\ntokens on the line. Tightening it is outside the scope of this\nseries.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/notes.c | 23 ++++++++++++-----------\n 1 file changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex a9529b1696..4fb36a743c 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -375,18 +375,19 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \n \twhile (strbuf_getline_lf(&buf, stdin) != EOF) {\n \t\tstruct object_id from_obj, to_obj;\n-\t\tstruct strbuf **split;\n+\t\tstruct string_list split = STRING_LIST_INIT_NODUP;\n \t\tint err;\n \n-\t\tsplit = strbuf_split(&buf, ' ');\n-\t\tif (!split[0] || !split[1])\n+\t\tstring_list_split_in_place_f(&split, buf.buf, \" \", -1,\n+\t\t\t\t\t STRING_LIST_SPLIT_TRIM);\n+\t\tif (split.nr < 2)\n \t\t\tdie(_(\"malformed input line: '%s'.\"), buf.buf);\n-\t\tstrbuf_rtrim(split[0]);\n-\t\tstrbuf_rtrim(split[1]);\n-\t\tif (repo_get_oid(the_repository, split[0]->buf, &from_obj))\n-\t\t\tdie(_(\"failed to resolve '%s' as a valid ref.\"), split[0]->buf);\n-\t\tif (repo_get_oid(the_repository, split[1]->buf, &to_obj))\n-\t\t\tdie(_(\"failed to resolve '%s' as a valid ref.\"), split[1]->buf);\n+\t\tif (repo_get_oid(the_repository, split.items[0].string, &from_obj))\n+\t\t\tdie(_(\"failed to resolve '%s' as a valid ref.\"),\n+\t\t\t split.items[0].string);\n+\t\tif (repo_get_oid(the_repository, split.items[1].string, &to_obj))\n+\t\t\tdie(_(\"failed to resolve '%s' as a valid ref.\"),\n+\t\t\t split.items[1].string);\n \n \t\tif (rewrite_cmd)\n \t\t\terr = copy_note_for_rewrite(c, &from_obj, &to_obj);\n@@ -396,11 +397,11 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \n \t\tif (err) {\n \t\t\terror(_(\"failed to copy notes from '%s' to '%s'\"),\n-\t\t\t split[0]->buf, split[1]->buf);\n+\t\t\t split.items[0].string, split.items[1].string);\n \t\t\tret = 1;\n \t\t}\n \n-\t\tstrbuf_list_free(split);\n+\t\tstring_list_clear(&split, 0);\n \t}\n \n \tif (!rewrite_cmd) {\n-- \n2.50.1-618-g45d530d26b\n\n"},{"id":"523147","messageId":"20250731225433.4028872-8-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731225433.4028872-1-gitster@pobox.com","subject":"[PATCH v2 07/11] config: do not use strbuf_split()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T22:54:29Z","receivedAt":"2025-07-31T22:54:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When parsing an old-style GIT_CONFIG_PARAMETERS environment\nvariable, the code parses key=value pairs by splitting them at '='\ninto an array of strbuf's. As strbuf_split() leaves the delimiter\nat the end of the split piece, the code has to manually trim it.\n\nIf we split with string_list_split(), that becomes unnecessary.\nRetire the use of strbuf_split() from this code path.\n\nNote that the max parameter of string_list_split() is of\nan ergonomically iffy design---it specifies the maximum number of\ntimes the function is allowed to split, which means that in order to\nsplit a text into up to 2 pieces, you have to pass 1, not 2.\n\nSigned-off-by: Junio C Hamano \n---\n config.c | 23 ++++++++++-------------\n 1 file changed, 10 insertions(+), 13 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 8a2d0b7916..1769f15ee3 100644\n--- a/config.c\n+++ b/config.c\n@@ -638,31 +638,28 @@ int git_config_parse_parameter(const char *text,\n \t\t\t config_fn_t fn, void *data)\n {\n \tconst char *value;\n-\tstruct strbuf **pair;\n+\tstruct string_list pair = STRING_LIST_INIT_DUP;\n \tint ret;\n \tstruct key_value_info kvi = KVI_INIT;\n \n \tkvi_from_param(&kvi);\n \n-\tpair = strbuf_split_str(text, '=', 2);\n-\tif (!pair[0])\n+\tstring_list_split(&pair, text, \"=\", 1);\n+\tif (!pair.nr)\n \t\treturn error(_(\"bogus config parameter: %s\"), text);\n \n-\tif (pair[0]->len && pair[0]->buf[pair[0]->len - 1] == '=') {\n-\t\tstrbuf_setlen(pair[0], pair[0]->len - 1);\n-\t\tvalue = pair[1] ? pair[1]->buf : \"\";\n-\t} else {\n+\tif (pair.nr == 1)\n \t\tvalue = NULL;\n-\t}\n+\telse\n+\t\tvalue = pair.items[1].string;\n \n-\tstrbuf_trim(pair[0]);\n-\tif (!pair[0]->len) {\n-\t\tstrbuf_list_free(pair);\n+\tif (!*pair.items[0].string) {\n+\t\tstring_list_clear(&pair, 0);\n \t\treturn error(_(\"bogus config parameter: %s\"), text);\n \t}\n \n-\tret = config_parse_pair(pair[0]->buf, value, &kvi, fn, data);\n-\tstrbuf_list_free(pair);\n+\tret = config_parse_pair(pair.items[0].string, value, &kvi, fn, data);\n+\tstring_list_clear(&pair, 0);\n \treturn ret;\n }\n \n-- \n2.50.1-618-g45d530d26b\n\n"},{"id":"523148","messageId":"20250731225433.4028872-9-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731225433.4028872-1-gitster@pobox.com","subject":"[PATCH v2 08/11] environment: do not use strbuf_split*()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T22:54:30Z","receivedAt":"2025-07-31T22:54:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"environment.c:get_git_namespace() learns the raw namespace from an\nenvironment variable, splits it at \"/\", and appends them after\n\"refs/namespaces/\"; the reason why it splits first is so that an\nempty string resulting from double slashes can be omitted.\n\nThe split pieces do not need to be edited in any way, so an array of\nstrbufs is a wrong data structure to use. Instead split into a\nstring list and use the pieces from there.\n\nSigned-off-by: Junio C Hamano \n---\n environment.c | 19 ++++++++++++-------\n 1 file changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex 7c2480b22e..ab3ed08433 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -163,10 +163,10 @@ int have_git_dir(void)\n const char *get_git_namespace(void)\n {\n \tstatic const char *namespace;\n-\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct strbuf **components, **c;\n \tconst char *raw_namespace;\n+\tstruct string_list components = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *item;\n \n \tif (namespace)\n \t\treturn namespace;\n@@ -178,12 +178,17 @@ const char *get_git_namespace(void)\n \t}\n \n \tstrbuf_addstr(&buf, raw_namespace);\n-\tcomponents = strbuf_split(&buf, '/');\n+\n+\tstring_list_split(&components, buf.buf, \"/\", -1);\n \tstrbuf_reset(&buf);\n-\tfor (c = components; *c; c++)\n-\t\tif (strcmp((*c)->buf, \"/\") != 0)\n-\t\t\tstrbuf_addf(&buf, \"refs/namespaces/%s\", (*c)->buf);\n-\tstrbuf_list_free(components);\n+\n+\tfor_each_string_list_item(item, &components) {\n+\t\tif (item->string[0])\n+\t\t\tstrbuf_addf(&buf, \"refs/namespaces/%s/\", item->string);\n+\t}\n+\tstring_list_clear(&components, 0);\n+\n+\tstrbuf_trim_trailing_dir_sep(&buf);\n \tif (check_refname_format(buf.buf, 0))\n \t\tdie(_(\"bad git namespace path \\\"%s\\\"\"), raw_namespace);\n \tstrbuf_addch(&buf, '/');\n-- \n2.50.1-618-g45d530d26b\n\n"},{"id":"523149","messageId":"20250731225433.4028872-10-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731225433.4028872-1-gitster@pobox.com","subject":"[PATCH v2 09/11] sub-process: do not use strbuf_split*()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T22:54:31Z","receivedAt":"2025-07-31T22:54:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The code to read status from subprocess reads one packet line and\ntries to find \"status=\". It is way overkill to split the line\ninto an array of two strbufs to extract .\n\nSigned-off-by: Junio C Hamano \n---\n sub-process.c | 15 ++++++---------\n 1 file changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/sub-process.c b/sub-process.c\nindex 1daf5a9752..83bf0a0e82 100644\n--- a/sub-process.c\n+++ b/sub-process.c\n@@ -30,23 +30,20 @@ struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const ch\n \n int subprocess_read_status(int fd, struct strbuf *status)\n {\n-\tstruct strbuf **pair;\n-\tchar *line;\n \tint len;\n \n \tfor (;;) {\n+\t\tchar *line;\n+\t\tconst char *value;\n+\n \t\tlen = packet_read_line_gently(fd, NULL, &line);\n \t\tif ((len < 0) || !line)\n \t\t\tbreak;\n-\t\tpair = strbuf_split_str(line, '=', 2);\n-\t\tif (pair[0] && pair[0]->len && pair[1]) {\n+\t\tif (skip_prefix(line, \"status=\", &value)) {\n \t\t\t/* the last \"status=\" line wins */\n-\t\t\tif (!strcmp(pair[0]->buf, \"status=\")) {\n-\t\t\t\tstrbuf_reset(status);\n-\t\t\t\tstrbuf_addbuf(status, pair[1]);\n-\t\t\t}\n+\t\t\tstrbuf_reset(status);\n+\t\t\tstrbuf_addstr(status, value);\n \t\t}\n-\t\tstrbuf_list_free(pair);\n \t}\n \n \treturn (len < 0) ? len : 0;\n-- \n2.50.1-618-g45d530d26b\n\n"},{"id":"523150","messageId":"20250731225433.4028872-11-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731225433.4028872-1-gitster@pobox.com","subject":"[PATCH v2 10/11] trace2: trim_trailing_newline followed by trim is a no-op","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T22:54:32Z","receivedAt":"2025-07-31T22:54:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"strbuf_trim_trailing_newline() removes a LF or a CRLF from the tail\nof a string. If the code plans to call strbuf_trim() immediately\nafter doing so, the code is better off skipping the EOL trimming in\nthe first place. After all, LF/CRLF at the end is a mere special\ncase of whitespaces at the end of the string, which will be removed\nby strbuf_rtrim() anyway.\n\nSigned-off-by: Junio C Hamano \n---\n trace2/tr2_cfg.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/trace2/tr2_cfg.c b/trace2/tr2_cfg.c\nindex 22a99a0682..2b7cfcd10c 100644\n--- a/trace2/tr2_cfg.c\n+++ b/trace2/tr2_cfg.c\n@@ -39,7 +39,6 @@ static int tr2_cfg_load_patterns(void)\n \n \t\tif (buf->len && buf->buf[buf->len - 1] == ',')\n \t\t\tstrbuf_setlen(buf, buf->len - 1);\n-\t\tstrbuf_trim_trailing_newline(*s);\n \t\tstrbuf_trim(*s);\n \t}\n \n@@ -78,7 +77,6 @@ static int tr2_load_env_vars(void)\n \n \t\tif (buf->len && buf->buf[buf->len - 1] == ',')\n \t\t\tstrbuf_setlen(buf, buf->len - 1);\n-\t\tstrbuf_trim_trailing_newline(*s);\n \t\tstrbuf_trim(*s);\n \t}\n \n-- \n2.50.1-618-g45d530d26b\n\n"},{"id":"523151","messageId":"20250731225433.4028872-12-gitster@pobox.com","threadId":"63882","inReplyTo":"20250731225433.4028872-1-gitster@pobox.com","subject":"[PATCH v2 11/11] trace2: do not use strbuf_split*()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T22:54:33Z","receivedAt":"2025-07-31T22:54:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"tr2_cfg_load_patterns() and tr2_load_env_vars() functions are\nfunctions with very similar structure that each reads an environment\nvariable, splits its value at the ',' boundaries, and trims the\nresulting string pieces into an array of strbufs.\n\nBut the code paths that later use these strbufs take no advantage of\nthe strbuf-ness of the result (they do not benefit from \nrepresentation to avoid having to run strlen(), for example).\n\nSimplify the code by teaching these functions to split into a string\nlist instead; even the trimming comes for free ;-).\n\nSigned-off-by: Junio C Hamano \n---\n trace2/tr2_cfg.c | 78 +++++++++++++++++-------------------------------\n 1 file changed, 27 insertions(+), 51 deletions(-)\n\ndiff --git a/trace2/tr2_cfg.c b/trace2/tr2_cfg.c\nindex 2b7cfcd10c..bbcfeda60a 100644\n--- a/trace2/tr2_cfg.c\n+++ b/trace2/tr2_cfg.c\n@@ -8,87 +8,65 @@\n #include \"trace2/tr2_sysenv.h\"\n #include \"wildmatch.h\"\n \n-static struct strbuf **tr2_cfg_patterns;\n-static int tr2_cfg_count_patterns;\n+static struct string_list tr2_cfg_patterns = STRING_LIST_INIT_DUP;\n static int tr2_cfg_loaded;\n \n-static struct strbuf **tr2_cfg_env_vars;\n-static int tr2_cfg_env_vars_count;\n+static struct string_list tr2_cfg_env_vars = STRING_LIST_INIT_DUP;\n static int tr2_cfg_env_vars_loaded;\n \n /*\n * Parse a string containing a comma-delimited list of config keys\n- * or wildcard patterns into a list of strbufs.\n+ * or wildcard patterns into a string list.\n */\n-static int tr2_cfg_load_patterns(void)\n+static size_t tr2_cfg_load_patterns(void)\n {\n-\tstruct strbuf **s;\n \tconst char *envvar;\n \n \tif (tr2_cfg_loaded)\n-\t\treturn tr2_cfg_count_patterns;\n+\t\treturn tr2_cfg_patterns.nr;\n \ttr2_cfg_loaded = 1;\n \n \tenvvar = tr2_sysenv_get(TR2_SYSENV_CFG_PARAM);\n \tif (!envvar || !*envvar)\n-\t\treturn tr2_cfg_count_patterns;\n+\t\treturn tr2_cfg_patterns.nr;\n \n-\ttr2_cfg_patterns = strbuf_split_buf(envvar, strlen(envvar), ',', -1);\n-\tfor (s = tr2_cfg_patterns; *s; s++) {\n-\t\tstruct strbuf *buf = *s;\n-\n-\t\tif (buf->len && buf->buf[buf->len - 1] == ',')\n-\t\t\tstrbuf_setlen(buf, buf->len - 1);\n-\t\tstrbuf_trim(*s);\n-\t}\n-\n-\ttr2_cfg_count_patterns = s - tr2_cfg_patterns;\n-\treturn tr2_cfg_count_patterns;\n+\tstring_list_split_f(&tr2_cfg_patterns, envvar, \",\", -1,\n+\t\t\t STRING_LIST_SPLIT_TRIM);\n+\treturn tr2_cfg_patterns.nr;\n }\n \n void tr2_cfg_free_patterns(void)\n {\n-\tif (tr2_cfg_patterns)\n-\t\tstrbuf_list_free(tr2_cfg_patterns);\n-\ttr2_cfg_count_patterns = 0;\n+\tif (tr2_cfg_patterns.nr)\n+\t\tstring_list_clear(&tr2_cfg_patterns, 0);\n \ttr2_cfg_loaded = 0;\n }\n \n /*\n * Parse a string containing a comma-delimited list of environment variable\n- * names into a list of strbufs.\n+ * names into a string list.\n */\n-static int tr2_load_env_vars(void)\n+static size_t tr2_load_env_vars(void)\n {\n-\tstruct strbuf **s;\n \tconst char *varlist;\n \n \tif (tr2_cfg_env_vars_loaded)\n-\t\treturn tr2_cfg_env_vars_count;\n+\t\treturn tr2_cfg_env_vars.nr;\n \ttr2_cfg_env_vars_loaded = 1;\n \n \tvarlist = tr2_sysenv_get(TR2_SYSENV_ENV_VARS);\n \tif (!varlist || !*varlist)\n-\t\treturn tr2_cfg_env_vars_count;\n-\n-\ttr2_cfg_env_vars = strbuf_split_buf(varlist, strlen(varlist), ',', -1);\n-\tfor (s = tr2_cfg_env_vars; *s; s++) {\n-\t\tstruct strbuf *buf = *s;\n-\n-\t\tif (buf->len && buf->buf[buf->len - 1] == ',')\n-\t\t\tstrbuf_setlen(buf, buf->len - 1);\n-\t\tstrbuf_trim(*s);\n-\t}\n+\t\treturn tr2_cfg_env_vars.nr;\n \n-\ttr2_cfg_env_vars_count = s - tr2_cfg_env_vars;\n-\treturn tr2_cfg_env_vars_count;\n+\tstring_list_split_f(&tr2_cfg_env_vars, varlist, \",\", -1,\n+\t\t\t STRING_LIST_SPLIT_TRIM);\n+\treturn tr2_cfg_env_vars.nr;\n }\n \n void tr2_cfg_free_env_vars(void)\n {\n-\tif (tr2_cfg_env_vars)\n-\t\tstrbuf_list_free(tr2_cfg_env_vars);\n-\ttr2_cfg_env_vars_count = 0;\n+\tif (tr2_cfg_env_vars.nr)\n+\t\tstring_list_clear(&tr2_cfg_env_vars, 0);\n \ttr2_cfg_env_vars_loaded = 0;\n }\n \n@@ -103,12 +81,11 @@ struct tr2_cfg_data {\n static int tr2_cfg_cb(const char *key, const char *value,\n \t\t const struct config_context *ctx, void *d)\n {\n-\tstruct strbuf **s;\n+\tstruct string_list_item *item;\n \tstruct tr2_cfg_data *data = (struct tr2_cfg_data *)d;\n \n-\tfor (s = tr2_cfg_patterns; *s; s++) {\n-\t\tstruct strbuf *buf = *s;\n-\t\tint wm = wildmatch(buf->buf, key, WM_CASEFOLD);\n+\tfor_each_string_list_item(item, &tr2_cfg_patterns) {\n+\t\tint wm = wildmatch(item->string, key, WM_CASEFOLD);\n \t\tif (wm == WM_MATCH) {\n \t\t\ttrace2_def_param_fl(data->file, data->line, key, value,\n \t\t\t\t\t ctx->kvi);\n@@ -130,17 +107,16 @@ void tr2_cfg_list_config_fl(const char *file, int line)\n void tr2_list_env_vars_fl(const char *file, int line)\n {\n \tstruct key_value_info kvi = KVI_INIT;\n-\tstruct strbuf **s;\n+\tstruct string_list_item *item;\n \n \tkvi_from_param(&kvi);\n \tif (tr2_load_env_vars() <= 0)\n \t\treturn;\n \n-\tfor (s = tr2_cfg_env_vars; *s; s++) {\n-\t\tstruct strbuf *buf = *s;\n-\t\tconst char *val = getenv(buf->buf);\n+\tfor_each_string_list_item(item, &tr2_cfg_env_vars) {\n+\t\tconst char *val = getenv(item->string);\n \t\tif (val && *val)\n-\t\t\ttrace2_def_param_fl(file, line, buf->buf, val, &kvi);\n+\t\t\ttrace2_def_param_fl(file, line, item->string, val, &kvi);\n \t}\n }\n \n-- \n2.50.1-618-g45d530d26b\n\n"},{"id":"523261","messageId":"20250802083846.GC3711639@coredump.intra.peff.net","threadId":"63882","inReplyTo":"20250731225433.4028872-3-gitster@pobox.com","subject":"Re: [PATCH v2 02/11] clean: do not pass strbuf by value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-02T08:38:46Z","receivedAt":"2025-08-02T08:38:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 31, 2025 at 03:54:24PM -0700, Junio C Hamano wrote:\n\n> This is a catastrophe waiting to happen. If the callee causes the\n> strbuf to be reallocated, the buf[] the caller has will become\n> dangling, and when the caller does strbuf_release(), it would result\n> in double-free.\n> \n> Stop calling the function with misleading call-by-value with strbuf.\n\nThis is definitely an improvement, though I wonder if we could go\nfurther.\n\nWhen I saw the original version of this patch, my first thought was: if\nwe are passing a strbuf that is not modified, then why not just pass a\nconst string pointer? It is more flexible for the caller and makes our\nintention more clear. (A careful reader will note that this does not\npass in the length, so it is different if we expect embedded NULs, but I\ndon't think that is the case here).\n\nBut it does not quite work in this case because of this:\n\n> \tif (is_single) {\n> -\t\tchoice_list = strbuf_split_max(&input, '\\n', 0);\n> +\t\tchoice_list = strbuf_split_max(input, '\\n', 0);\n> \t} else {\n> -\t\tchar *p = input.buf;\n> +\t\tchar *p = input->buf;\n> \t\tdo {\n> \t\t\tif (*p == ',')\n> \t\t\t\t*p = ' ';\n> \t\t} while (*p++);\n> -\t\tchoice_list = strbuf_split_max(&input, ' ', 0);\n> +\t\tchoice_list = strbuf_split_max(input, ' ', 0);\n> \t}\n\nWe do modify the string! However, that code goes away in your next patch\nwhen we are able to switch to a split function that handles multiple\ndelimiters. You do still do an in-place split, but isn't strictly\nneeded.\n\nIf we swapped patches 2 and 3 in this series, then the strbuf cleanup\ncan just become:\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex a01d130f62..e4b075df70 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -477,17 +477,17 @@ static int find_unique(const char *choice, struct menu_stuff *menu_stuff)\n */\n static int parse_choice(struct menu_stuff *menu_stuff,\n \t\t\tint is_single,\n-\t\t\tstruct strbuf input,\n+\t\t\tconst char *input,\n \t\t\tint **chosen)\n {\n-\tstruct string_list choice = STRING_LIST_INIT_NODUP;\n+\tstruct string_list choice = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *item;\n \tint nr = 0;\n \tint i;\n \n-\tstring_list_split_in_place_f(&choice, input.buf,\n-\t\t\t\t is_single ? \"\\n\" : \", \", -1,\n-\t\t\t\t STRING_LIST_SPLIT_TRIM);\n+\tstring_list_split_f(&choice, input,\n+\t\t\t is_single ? \"\\n\" : \", \", -1,\n+\t\t\t STRING_LIST_SPLIT_TRIM);\n \n \tfor_each_string_list_item(item, &choice) {\n \t\tconst char *string;\n@@ -626,7 +626,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)\n \n \t\tnr = parse_choice(stuff,\n \t\t\t\t opts->flags & MENU_OPTS_SINGLETON,\n-\t\t\t\t choice,\n+\t\t\t\t choice.buf,\n \t\t\t\t &chosen);\n \n \t\tif (opts->flags & MENU_OPTS_SINGLETON) {\n\nI dunno. Maybe it is nitpicking, but I think \"don't take a strbuf if you\nonly need a string\" is a good general rule. Of course there is only one\ncaller here, so flexibility is probably not that important.\n\n-Peff\n"},{"id":"523262","messageId":"20250802085513.GD3711639@coredump.intra.peff.net","threadId":"63882","inReplyTo":"20250731225433.4028872-6-gitster@pobox.com","subject":"Re: [PATCH v2 05/11] merge-tree: do not use strbuf_split*()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-02T08:55:13Z","receivedAt":"2025-08-02T08:55:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 31, 2025 at 03:54:27PM -0700, Junio C Hamano wrote:\n\n> -\t\t\tif (input_merge_base && split[2] && split[3] && !split[4]) {\n> [...]\n> +\t\t\tif (input_merge_base && split.nr == 4) {\n\nI always hated strbuf_split() because the array-of-strbufs was awkward\nto work with. But I never realized how inconvenient it was not to have\nthe count of split items until seeing that monstrosity in the pre-image. ;)\n\n-Peff\n"},{"id":"523263","messageId":"20250802090825.GE3711639@coredump.intra.peff.net","threadId":"63882","inReplyTo":"20250731225433.4028872-1-gitster@pobox.com","subject":"Re: [PATCH v2 00/11] do not overuse strbuf_split*()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-02T09:08:25Z","receivedAt":"2025-08-02T09:08:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 31, 2025 at 03:54:22PM -0700, Junio C Hamano wrote:\n\n> strbuf is a very good data structure to work with string data\n> without having to worry about running past the end of the string.\n> \n> But an array of strbuf is often a wrong data structure. You rarely\n> have need to be able to edit multiple strings represented by such an\n> array simultaneously. And strbuf_split*() that produces result in\n> such a shape is a misdesigned API function.\n\nYeah, I agree it's a bad API, and I'd be happy if we could eventually\nget rid of it.\n\nConversion to string_list mostly makes sense (though I especially like\nthe case in sub-process.c where we can just parse with skip_prefix()).\n\nI looked over the patches and didn't see anything objectionable. But I\ntried to think of subtle incompatibilities we might run into:\n\n 1. I saw the different handling of \"max\" you had to deal with in one\n patch. Yuck. It might be worth tweaking string_list_split() to\n count \"pieces\" (which is how every other split function I've seen,\n like the one in perl, works). But that can be done later (and is\n tricky to do safely, since we wouldn't be changing the function\n signature).\n\n 2. Is the handling of repeated delimiters the same? E.g., if I split\n on \"/\" and we see \"foo//bar\", do both split implementations yield\n the same output. I could see either of (\"foo\", \"\", \"bar\") and\n (\"foo\", \"bar\") being produced.\n\nAnd...\n\n> The most common use case of strbuf_split*() family of functions\n> seems to be to trim away the whitespaces around each piece of split\n> string. With modern string_list_split*(), it is often no longer\n> necessary.\n\nSome of these sites do strbuf_rtrim() on the split pieces. But\nSTRING_LIST_SPLIT_TRIM does both left and right. Is this OK? I think so\nbecause in both cases we are already splitting on space, so we wouldn't\nexpect left-hand spaces (of course you could have a stray tab or\nsomething, but I suspect the new code matches the intent more closely).\n\n-Peff\n"},{"id":"523280","messageId":"xmqqtt2pisg0.fsf@gitster.g","threadId":"63882","inReplyTo":"20250802083846.GC3711639@coredump.intra.peff.net","subject":"Re: [PATCH v2 02/11] clean: do not pass strbuf by value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-02T16:44:47Z","receivedAt":"2025-08-02T16:44:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jul 31, 2025 at 03:54:24PM -0700, Junio C Hamano wrote:\n>\n>> This is a catastrophe waiting to happen. If the callee causes the\n>> strbuf to be reallocated, the buf[] the caller has will become\n>> dangling, and when the caller does strbuf_release(), it would result\n>> in double-free.\n>> \n>> Stop calling the function with misleading call-by-value with strbuf.\n>\n> This is definitely an improvement, though I wonder if we could go\n> further.\n\nYes, but in short, between these two\n\n - think twice before you pass struct by value\n - do not insist taking the whole struct, take only what you need\n\nlessons, I happened to pick the former one more important to carve\nin stone than the latter one. Both are valuable guidance we should\ngive to our developers, though.\n\n> \t\tnr = parse_choice(stuff,\n> \t\t\t\t opts->flags & MENU_OPTS_SINGLETON,\n> -\t\t\t\t choice,\n> +\t\t\t\t choice.buf,\n> \t\t\t\t &chosen);\n> \n> \t\tif (opts->flags & MENU_OPTS_SINGLETON) {\n>\n> I dunno. Maybe it is nitpicking, but I think \"don't take a strbuf if you\n> only need a string\" is a good general rule. Of course there is only one\n> caller here, so flexibility is probably not that important.\n\nBut I think we engrave both lessons in the history by keeping this\nstep as-is, do the string_list_split_in_place_f() thing, and then\nadd a new patch to pass just the .buf member to parse_choice().\n"},{"id":"523282","messageId":"xmqq4iupira0.fsf@gitster.g","threadId":"63882","inReplyTo":"20250802090825.GE3711639@coredump.intra.peff.net","subject":"Re: [PATCH v2 00/11] do not overuse strbuf_split*()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-02T17:09:59Z","receivedAt":"2025-08-02T17:10:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jul 31, 2025 at 03:54:22PM -0700, Junio C Hamano wrote:\n>\n>> strbuf is a very good data structure to work with string data\n>> without having to worry about running past the end of the string.\n>> \n>> But an array of strbuf is often a wrong data structure. You rarely\n>> have need to be able to edit multiple strings represented by such an\n>> array simultaneously. And strbuf_split*() that produces result in\n>> such a shape is a misdesigned API function.\n>\n> Yeah, I agree it's a bad API, and I'd be happy if we could eventually\n> get rid of it.\n\nI'd be happy to mark it #leftoverbits for aspiring Git hackers to\nlook at. Perhaps a good microproject material (there are still\nseveral instances left---I talked Christian out of adding more in\nthe series he is iterating right now).\n\n> Conversion to string_list mostly makes sense (though I especially like\n> the case in sub-process.c where we can just parse with skip_prefix()).\n\nYes, for that one, strbuf_split() was really killing a butterfly\nwith a musket or cracking a nut with a sledgehammer; or whatever\ntranslation of the idiom you'd like ;-).\n\n> I looked over the patches and didn't see anything objectionable. But I\n> tried to think of subtle incompatibilities we might run into:\n>\n> 1. I saw the different handling of \"max\" you had to deal with in one\n> patch. Yuck. It might be worth tweaking string_list_split() to\n> count \"pieces\" (which is how every other split function I've seen,\n> like the one in perl, works). But that can be done later (and is\n> tricky to do safely, since we wouldn't be changing the function\n> signature).\n\nYes, I think I whined about it in one of the commit log messages.\nThat part is one of the things string_list API gets wrong, which we\nmay want to fix.\n\n> 2. Is the handling of repeated delimiters the same? E.g., if I split\n> on \"/\" and we see \"foo//bar\", do both split implementations yield\n> the same output. I could see either of (\"foo\", \"\", \"bar\") and\n> (\"foo\", \"bar\") being produced.\n\nSame between? strbuf_split*() seems to be happy to produce an empty\npiece in the result (and there is no \"omit empty ones\" feature).\nYou can choose with STRING_LIST_SPLIT_NOEMPTY both results, but for\na straight conversion, the vanilla one without that flag would do\nwhat we want, I think.\n\n> And...\n>\n>> The most common use case of strbuf_split*() family of functions\n>> seems to be to trim away the whitespaces around each piece of split\n>> string. With modern string_list_split*(), it is often no longer\n>> necessary.\n>\n> Some of these sites do strbuf_rtrim() on the split pieces. But\n> STRING_LIST_SPLIT_TRIM does both left and right. Is this OK? I think so\n> because in both cases we are already splitting on space, so we wouldn't\n> expect left-hand spaces (of course you could have a stray tab or\n> something, but I suspect the new code matches the intent more closely).\n\nI did wonder about these \"rtrim-only\" callsites strbuf_split(), but\nI did not think they needed ltrim, since they were splitting at SP\nand rejected when the element were empty (remember, strbuf_split()\nhappily creates an empty element in the result).\n\nTheir use of rtrim() is primarily when they split at SP they want to\nremove that SP (remember, another misdesign of strbuf_split() is\nthat it leaves the delimiter itself at the end of each split piece).\nBoth 05/11 step for merge-tree and 06/11 step for notes are good\nexamples.\n\nSo in short, yes, the new code is being a bit more lenient (the\noriginal parsers were more strict and insisted on non-trimming\nbehaviour). We might consider tightening them by removing the\nSTRING_LIST_SPLIT_TRIM bit to be more faithful to the original, but\nas you may have noticed, doing so would make it more strict at the\nright end. The original that split at SP and then rtrimmed allowed\ntrailing HT after the data to be removed, but I do not think it was\npart of the designed behaviour of the original anyway. So I am\ninclined to say we should probably drop STRING_LIST_SPLIT_TRIM;\nnobody would scream.\n\nThanks for carefully reading.\n"},{"id":"523289","messageId":"20250802184044.GC1773585@coredump.intra.peff.net","threadId":"63882","inReplyTo":"xmqqtt2pisg0.fsf@gitster.g","subject":"Re: [PATCH v2 02/11] clean: do not pass strbuf by value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-02T18:40:44Z","receivedAt":"2025-08-02T18:40:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 02, 2025 at 09:44:47AM -0700, Junio C Hamano wrote:\n\n> > I dunno. Maybe it is nitpicking, but I think \"don't take a strbuf if you\n> > only need a string\" is a good general rule. Of course there is only one\n> > caller here, so flexibility is probably not that important.\n> \n> But I think we engrave both lessons in the history by keeping this\n> step as-is, do the string_list_split_in_place_f() thing, and then\n> add a new patch to pass just the .buf member to parse_choice().\n\nI would certainly be happy with that.\n\nThe other strbuf-by-value in write_worktree_linking_files() could also\njust turn into a \"const char *\".\n\n-Peff\n"},{"id":"523290","messageId":"20250802184715.GD1773585@coredump.intra.peff.net","threadId":"63882","inReplyTo":"xmqq4iupira0.fsf@gitster.g","subject":"Re: [PATCH v2 00/11] do not overuse strbuf_split*()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-02T18:47:15Z","receivedAt":"2025-08-02T18:47:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 02, 2025 at 10:09:59AM -0700, Junio C Hamano wrote:\n\n> > 2. Is the handling of repeated delimiters the same? E.g., if I split\n> > on \"/\" and we see \"foo//bar\", do both split implementations yield\n> > the same output. I could see either of (\"foo\", \"\", \"bar\") and\n> > (\"foo\", \"bar\") being produced.\n> \n> Same between? strbuf_split*() seems to be happy to produce an empty\n> piece in the result (and there is no \"omit empty ones\" feature).\n> You can choose with STRING_LIST_SPLIT_NOEMPTY both results, but for\n> a straight conversion, the vanilla one without that flag would do\n> what we want, I think.\n\nYeah, I meant same between strbuf_split and string_list_split. I didn't\nactually peek into the behavior of either. It was more of a \"did you\ncheck this it?\" question. It sounds like you did. Good.\n\n> > Some of these sites do strbuf_rtrim() on the split pieces. But\n> > STRING_LIST_SPLIT_TRIM does both left and right. Is this OK? I think so\n> > because in both cases we are already splitting on space, so we wouldn't\n> > expect left-hand spaces (of course you could have a stray tab or\n> > something, but I suspect the new code matches the intent more closely).\n> \n> I did wonder about these \"rtrim-only\" callsites strbuf_split(), but\n> I did not think they needed ltrim, since they were splitting at SP\n> and rejected when the element were empty (remember, strbuf_split()\n> happily creates an empty element in the result).\n> \n> Their use of rtrim() is primarily when they split at SP they want to\n> remove that SP (remember, another misdesign of strbuf_split() is\n> that it leaves the delimiter itself at the end of each split piece).\n> Both 05/11 step for merge-tree and 06/11 step for notes are good\n> examples.\n\nMakes sense. I did not even realize all of the horrible gotchas of\nstrbuf_split(). :)\n\n> So in short, yes, the new code is being a bit more lenient (the\n> original parsers were more strict and insisted on non-trimming\n> behaviour). We might consider tightening them by removing the\n> STRING_LIST_SPLIT_TRIM bit to be more faithful to the original, but\n> as you may have noticed, doing so would make it more strict at the\n> right end. The original that split at SP and then rtrimmed allowed\n> trailing HT after the data to be removed, but I do not think it was\n> part of the designed behaviour of the original anyway. So I am\n> inclined to say we should probably drop STRING_LIST_SPLIT_TRIM;\n> nobody would scream.\n\nHmm, yeah, I see what you mean. The documentation for merge-tree is\nquite vague about whether it is a single space or if multiple spaces are\nallowed. I'd be inclined to leave it on the lenient side, since I don't\nthink there is any ambiguity.\n\nThe git-notes one does specifically say \"SP\", so anybody who would be\nbroken by losing the extra trim is already violating the docs. But\nagain, I'd be tempted to just err on the friendly side.\n\nI doubt either case matters too much either way, though.\n\n-Peff\n"}]}