{"thread":{"id":"65208","subject":"[PATCH v2 0/2] avoid unnecessary strbuf_split*() and strbuf-by-value usage","startedAt":"2026-03-11T13:20:54Z","lastAt":"2026-03-11T18:13:40Z","messageCount":11,"participants":["Deveshi Dwivedi","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"538596","messageId":"20260311132041.12044-1-deveshigurgaon@gmail.com","threadId":"65208","inReplyTo":null,"subject":"[PATCH v2 0/2] avoid unnecessary strbuf_split*() and strbuf-by-value usage","fromName":"Deveshi Dwivedi","fromEmail":"deveshigurgaon@gmail.com","sentAt":"2026-03-11T13:20:39Z","receivedAt":"2026-03-11T13:20:54Z","isPatch":true,"sender":{"key":"deveshigurgaon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/120312681?v=4"},"body":"Junio's \"do not overuse strbuf_split*()\" series calls out remaining\nuses of strbuf_split*() as leftover bits for others to continue.\nThis series picks up two of them.\n\nwrite_worktree_linking_files() takes two struct strbuf parameters by\nvalue even though it only needs plain path strings.\n\nparse_combine_filter() in list-objects-filter-options.c uses\nstrbuf_split_str() to split a combine: filter spec at '+'.  An array\nof strbufs is unnecessary; walking the string directly with\nstrchrnul() is simpler and cleaner.\n\nChanges since v1:\n\n * Patch 1/2: no changes.\n\n * Patch 2/2: Incorporate review feedback from Jeff King.\n   - Use strchrnul() instead of strchr() so the loop needs no\n     separate \"found separator?\" branch or strlen() fallback.\n   - Drop the unnecessary (size_t) cast on the length expression.\n   - Always exclude '+' from the sub-spec via end - p, removing the\n     incorrect conditional stripping.  A trailing '+' now causes the\n     while (*p) condition to fail cleanly on the next iteration\n     rather than passing an empty string to the parser.\n   - Remove the test that expected an error on trailing '+', since\n     that behavior was incorrect.\n\nDeveshi Dwivedi (2):\n  worktree: do not pass strbuf by value\n  list-objects-filter-options: avoid strbuf_split_str()\n\n builtin/worktree.c                  |  2 +-\n list-objects-filter-options.c       | 35 +++++++++++++----------------\n t/t6112-rev-list-filters-objects.sh |  4 ----\n worktree.c                          | 22 +++++++++---------\n worktree.h                          |  2 +-\n 5 files changed, 28 insertions(+), 37 deletions(-)\n\nRange-diff against v1:\n1:  ee6b7d1e6a = 1:  ee6b7d1e6a worktree: do not pass strbuf by value\n2:  386aed0adf ! 2:  c04ddaeb95 list-objects-filter-options: avoid strbuf_split_str()\n    @@ Commit message\n         parse_combine_subfilter(), only read the string content of the strbuf\n         they receive.\n     \n    -    Walk the input string directly with strchr() to find each '+'.  Copy\n    -    each sub-spec into a temporary buffer and strip the '+' only when\n    -    another sub-spec follows.  Change the helpers to take const char *\n    -    instead of struct strbuf *.\n    +    Walk the input string directly with strchrnul() to find each '+'.\n    +    strchrnul() returns a pointer to the terminating '\\0' when the\n    +    delimiter is not found, so no separate \"found separator?\" branch is\n    +    needed.  Copy each sub-spec into a temporary buffer using end - p,\n    +    which naturally excludes the '+', so the separator is always stripped\n    +    cleanly.  A trailing '+' causes the outer while (*p) test to fail on\n    +    the next iteration rather than passing an empty string to the parser.\n    +    Change the helpers to take const char * instead of struct strbuf *.\n    +\n    +    The test that expected an error on a trailing '+' is removed, since\n    +    that behavior was incorrect.\n     \n         Signed-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>\n     \n    @@ list-objects-filter-options.c: static int parse_combine_filter(\n     -\t\tresult = parse_combine_subfilter(\n     -\t\t\tfilter_options, subspecs[sub], errbuf);\n     +\twhile (*p && !result) {\n    -+\t\tconst char *sep = strchr(p, '+');\n    -+\t\tsize_t len = sep ? (size_t)(sep - p + 1) : strlen(p);\n    -+\t\tchar *sub = xmemdupz(p, len);\n    -+\n    -+\t\t/* strip '+' separator, but only when more sub-specs follow */\n    -+\t\tif (sep && *(sep + 1))\n    -+\t\t\tsub[len - 1] = '\\0';\n    ++\t\tconst char *end = strchrnul(p, '+');\n    ++\t\tchar *sub = xmemdupz(p, end - p);\n     +\n     +\t\tresult = parse_combine_subfilter(filter_options, sub, errbuf);\n     +\t\tfree(sub);\n    -+\t\tif (!sep)\n    ++\t\tif (!*end)\n     +\t\t\tbreak;\n    -+\t\tp = sep + 1;\n    ++\t\tp = end + 1;\n      \t}\n      \n      \tfilter_options->choice = LOFC_COMBINE;\n    @@ list-objects-filter-options.c: static int parse_combine_filter(\n      \tif (result)\n      \t\tlist_objects_filter_release(filter_options);\n      \treturn result;\n    +\n    + ## t/t6112-rev-list-filters-objects.sh ##\n    +@@ t/t6112-rev-list-filters-objects.sh: test_expect_success 'combine:... with non-encoded reserved chars' '\n    + \t\t\"must escape char in sub-filter-spec: .\\~.\"\n    + '\n    + \n    +-test_expect_success 'validate err msg for \"combine:<valid-filter>+\"' '\n    +-\texpect_invalid_filter_spec combine:tree:2+ \"expected .tree:<depth>.\"\n    +-'\n    +-\n    + test_expect_success 'combine:... with edge-case hex digits: Ff Aa 0 9' '\n    + \tgit -C r3 rev-list --objects --filter=\"combine:tree:2+bl%6Fb:n%6fne\" \\\n    + \t\tHEAD >actual &&\n-- \n2.52.0.230.gd8af7cadaa\n\n"},{"id":"538597","messageId":"20260311132041.12044-2-deveshigurgaon@gmail.com","threadId":"65208","inReplyTo":"20260311132041.12044-1-deveshigurgaon@gmail.com","subject":"[PATCH v2 1/2] worktree: do not pass strbuf by value","fromName":"Deveshi Dwivedi","fromEmail":"deveshigurgaon@gmail.com","sentAt":"2026-03-11T13:20:40Z","receivedAt":"2026-03-11T13:20:57Z","isPatch":true,"sender":{"key":"deveshigurgaon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/120312681?v=4"},"body":"write_worktree_linking_files() takes two struct strbuf parameters by\nvalue, even though it only reads path strings from them.\n\nPassing a strbuf by value is misleading and dangerous. The structure\ncarries a pointer to its underlying character array; caller and callee\nend up sharing that storage.  If the callee ever causes the strbuf to\nbe reallocated, the caller's copy becomes a dangling pointer, which\nresults in a double-free when the caller does strbuf_release().\n\nThe function only needs the string values, not the strbuf machinery.\nSwitch it to take const char * and update all callers to pass .buf.\n\nSigned-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>\n---\n builtin/worktree.c |  2 +-\n worktree.c         | 22 +++++++++++-----------\n worktree.h         |  2 +-\n 3 files changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex bc2d0d645b..4035b1cb06 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -539,7 +539,7 @@ static int add_worktree(const char *path, const char *refname,\n \n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/gitdir\", sb_repo.buf);\n-\twrite_worktree_linking_files(sb_git, sb, opts->relative_paths);\n+\twrite_worktree_linking_files(sb_git.buf, sb.buf, opts->relative_paths);\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/commondir\", sb_repo.buf);\n \twrite_file(sb.buf, \"../..\");\ndiff --git a/worktree.c b/worktree.c\nindex 6e2f0f7828..7eba12c6ed 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -445,7 +445,7 @@ void update_worktree_location(struct worktree *wt, const char *path_,\n \tstrbuf_realpath(&path, path_, 1);\n \tstrbuf_addf(&dotgit, \"%s/.git\", path.buf);\n \tif (fspathcmp(wt->path, path.buf)) {\n-\t\twrite_worktree_linking_files(dotgit, gitdir, use_relative_paths);\n+\t\twrite_worktree_linking_files(dotgit.buf, gitdir.buf, use_relative_paths);\n \n \t\tfree(wt->path);\n \t\twt->path = strbuf_detach(&path, NULL);\n@@ -684,7 +684,7 @@ static void repair_gitfile(struct worktree *wt,\n \n \tif (repair) {\n \t\tfn(0, wt->path, repair, cb_data);\n-\t\twrite_worktree_linking_files(dotgit, gitdir, use_relative_paths);\n+\t\twrite_worktree_linking_files(dotgit.buf, gitdir.buf, use_relative_paths);\n \t}\n \n done:\n@@ -742,7 +742,7 @@ void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path\n \tif (!file_exists(dotgit.buf))\n \t\tgoto done;\n \n-\twrite_worktree_linking_files(dotgit, gitdir, is_relative_path);\n+\twrite_worktree_linking_files(dotgit.buf, gitdir.buf, is_relative_path);\n done:\n \tstrbuf_release(&gitdir);\n \tstrbuf_release(&dotgit);\n@@ -913,7 +913,7 @@ void repair_worktree_at_path(const char *path,\n \n \tif (repair) {\n \t\tfn(0, gitdir.buf, repair, cb_data);\n-\t\twrite_worktree_linking_files(dotgit, gitdir, use_relative_paths);\n+\t\twrite_worktree_linking_files(dotgit.buf, gitdir.buf, use_relative_paths);\n \t}\n done:\n \tfree(dotgit_contents);\n@@ -1087,17 +1087,17 @@ int init_worktree_config(struct repository *r)\n \treturn res;\n }\n \n-void write_worktree_linking_files(struct strbuf dotgit, struct strbuf gitdir,\n+void write_worktree_linking_files(const char *dotgit, const char *gitdir,\n \t\t\t\t  int use_relative_paths)\n {\n \tstruct strbuf path = STRBUF_INIT;\n \tstruct strbuf repo = STRBUF_INIT;\n \tstruct strbuf tmp = STRBUF_INIT;\n \n-\tstrbuf_addbuf(&path, &dotgit);\n+\tstrbuf_addstr(&path, dotgit);\n \tstrbuf_strip_suffix(&path, \"/.git\");\n \tstrbuf_realpath(&path, path.buf, 1);\n-\tstrbuf_addbuf(&repo, &gitdir);\n+\tstrbuf_addstr(&repo, gitdir);\n \tstrbuf_strip_suffix(&repo, \"/gitdir\");\n \tstrbuf_realpath(&repo, repo.buf, 1);\n \n@@ -1110,11 +1110,11 @@ void write_worktree_linking_files(struct strbuf dotgit, struct strbuf gitdir,\n \t}\n \n \tif (use_relative_paths) {\n-\t\twrite_file(gitdir.buf, \"%s/.git\", relative_path(path.buf, repo.buf, &tmp));\n-\t\twrite_file(dotgit.buf, \"gitdir: %s\", relative_path(repo.buf, path.buf, &tmp));\n+\t\twrite_file(gitdir, \"%s/.git\", relative_path(path.buf, repo.buf, &tmp));\n+\t\twrite_file(dotgit, \"gitdir: %s\", relative_path(repo.buf, path.buf, &tmp));\n \t} else {\n-\t\twrite_file(gitdir.buf, \"%s/.git\", path.buf);\n-\t\twrite_file(dotgit.buf, \"gitdir: %s\", repo.buf);\n+\t\twrite_file(gitdir, \"%s/.git\", path.buf);\n+\t\twrite_file(dotgit, \"gitdir: %s\", repo.buf);\n \t}\n \n \tstrbuf_release(&path);\ndiff --git a/worktree.h b/worktree.h\nindex 06efe26b83..f4e46be385 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -240,7 +240,7 @@ int init_worktree_config(struct repository *r);\n  *  dotgit: \"/path/to/foo/.git\"\n  *  gitdir: \"/path/to/repo/worktrees/foo/gitdir\"\n  */\n-void write_worktree_linking_files(struct strbuf dotgit, struct strbuf gitdir,\n+void write_worktree_linking_files(const char *dotgit, const char *gitdir,\n \t\t\t\t  int use_relative_paths);\n \n #endif\n-- \n2.52.0.230.gd8af7cadaa\n\n"},{"id":"538598","messageId":"20260311132041.12044-3-deveshigurgaon@gmail.com","threadId":"65208","inReplyTo":"20260311132041.12044-1-deveshigurgaon@gmail.com","subject":"[PATCH v2 2/2] list-objects-filter-options: avoid strbuf_split_str()","fromName":"Deveshi Dwivedi","fromEmail":"deveshigurgaon@gmail.com","sentAt":"2026-03-11T13:20:41Z","receivedAt":"2026-03-11T13:21:03Z","isPatch":true,"sender":{"key":"deveshigurgaon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/120312681?v=4"},"body":"parse_combine_filter() splits a combine: filter spec at '+' using\nstrbuf_split_str(), which yields an array of strbufs with the\ndelimiter left at the end of each non-final piece.  The code then\nmutates each non-final piece to strip the trailing '+' before parsing.\n\nAllocating an array of strbufs is unnecessary.  The function processes\none sub-spec at a time and does not use strbuf editing on the pieces.\nThe two helpers it calls, has_reserved_character() and\nparse_combine_subfilter(), only read the string content of the strbuf\nthey receive.\n\nWalk the input string directly with strchrnul() to find each '+'.\nstrchrnul() returns a pointer to the terminating '\\0' when the\ndelimiter is not found, so no separate \"found separator?\" branch is\nneeded.  Copy each sub-spec into a temporary buffer using end - p,\nwhich naturally excludes the '+', so the separator is always stripped\ncleanly.  A trailing '+' causes the outer while (*p) test to fail on\nthe next iteration rather than passing an empty string to the parser.\nChange the helpers to take const char * instead of struct strbuf *.\n\nThe test that expected an error on a trailing '+' is removed, since\nthat behavior was incorrect.\n\nSigned-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>\n---\n list-objects-filter-options.c       | 35 +++++++++++++----------------\n t/t6112-rev-list-filters-objects.sh |  4 ----\n 2 files changed, 15 insertions(+), 24 deletions(-)\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 7f3e7b8f50..616c6c7faa 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -125,9 +125,9 @@ int gently_parse_list_objects_filter(\n static const char *RESERVED_NON_WS = \"~`!@#$^&*()[]{}\\\\;'\\\",<>?\";\n \n static int has_reserved_character(\n-\tstruct strbuf *sub_spec, struct strbuf *errbuf)\n+\tconst char *sub_spec, struct strbuf *errbuf)\n {\n-\tconst char *c = sub_spec->buf;\n+\tconst char *c = sub_spec;\n \twhile (*c) {\n \t\tif (*c <= ' ' || strchr(RESERVED_NON_WS, *c)) {\n \t\t\tstrbuf_addf(\n@@ -144,7 +144,7 @@ static int has_reserved_character(\n \n static int parse_combine_subfilter(\n \tstruct list_objects_filter_options *filter_options,\n-\tstruct strbuf *subspec,\n+\tconst char *subspec,\n \tstruct strbuf *errbuf)\n {\n \tsize_t new_index = filter_options->sub_nr;\n@@ -155,7 +155,7 @@ static int parse_combine_subfilter(\n \t\t      filter_options->sub_alloc);\n \tlist_objects_filter_init(&filter_options->sub[new_index]);\n \n-\tdecoded = url_percent_decode(subspec->buf);\n+\tdecoded = url_percent_decode(subspec);\n \n \tresult = has_reserved_character(subspec, errbuf);\n \tif (result)\n@@ -182,34 +182,29 @@ static int parse_combine_filter(\n \tconst char *arg,\n \tstruct strbuf *errbuf)\n {\n-\tstruct strbuf **subspecs = strbuf_split_str(arg, '+', 0);\n-\tsize_t sub;\n+\tconst char *p = arg;\n \tint result = 0;\n \n-\tif (!subspecs[0]) {\n+\tif (!*p) {\n \t\tstrbuf_addstr(errbuf, _(\"expected something after combine:\"));\n \t\tresult = 1;\n \t\tgoto cleanup;\n \t}\n \n-\tfor (sub = 0; subspecs[sub] && !result; sub++) {\n-\t\tif (subspecs[sub + 1]) {\n-\t\t\t/*\n-\t\t\t * This is not the last subspec. Remove trailing \"+\" so\n-\t\t\t * we can parse it.\n-\t\t\t */\n-\t\t\tsize_t last = subspecs[sub]->len - 1;\n-\t\t\tassert(subspecs[sub]->buf[last] == '+');\n-\t\t\tstrbuf_remove(subspecs[sub], last, 1);\n-\t\t}\n-\t\tresult = parse_combine_subfilter(\n-\t\t\tfilter_options, subspecs[sub], errbuf);\n+\twhile (*p && !result) {\n+\t\tconst char *end = strchrnul(p, '+');\n+\t\tchar *sub = xmemdupz(p, end - p);\n+\n+\t\tresult = parse_combine_subfilter(filter_options, sub, errbuf);\n+\t\tfree(sub);\n+\t\tif (!*end)\n+\t\t\tbreak;\n+\t\tp = end + 1;\n \t}\n \n \tfilter_options->choice = LOFC_COMBINE;\n \n cleanup:\n-\tstrbuf_list_free(subspecs);\n \tif (result)\n \t\tlist_objects_filter_release(filter_options);\n \treturn result;\ndiff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh\nindex 0387f35a32..39211ef989 100755\n--- a/t/t6112-rev-list-filters-objects.sh\n+++ b/t/t6112-rev-list-filters-objects.sh\n@@ -483,10 +483,6 @@ test_expect_success 'combine:... with non-encoded reserved chars' '\n \t\t\"must escape char in sub-filter-spec: .\\~.\"\n '\n \n-test_expect_success 'validate err msg for \"combine:<valid-filter>+\"' '\n-\texpect_invalid_filter_spec combine:tree:2+ \"expected .tree:<depth>.\"\n-'\n-\n test_expect_success 'combine:... with edge-case hex digits: Ff Aa 0 9' '\n \tgit -C r3 rev-list --objects --filter=\"combine:tree:2+bl%6Fb:n%6fne\" \\\n \t\tHEAD >actual &&\n-- \n2.52.0.230.gd8af7cadaa\n\n"},{"id":"538620","messageId":"xmqqo6kuqqje.fsf@gitster.g","threadId":"65208","inReplyTo":"20260311132041.12044-3-deveshigurgaon@gmail.com","subject":"Re: [PATCH v2 2/2] list-objects-filter-options: avoid strbuf_split_str()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-11T16:28:21Z","receivedAt":"2026-03-11T16:28:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Deveshi Dwivedi <deveshigurgaon@gmail.com> writes:\n\n> Walk the input string directly with strchrnul() to find each '+'.\n> strchrnul() returns a pointer to the terminating '\\0' when the\n> delimiter is not found, so no separate \"found separator?\" branch is\n> needed.  Copy each sub-spec into a temporary buffer using end - p,\n> which naturally excludes the '+', so the separator is always stripped\n> cleanly.  A trailing '+' causes the outer while (*p) test to fail on\n> the next iteration rather than passing an empty string to the parser.\n\nOK.  The above description is a bit more excessively focused on the\nimplementation (which readers can find in the patch text) than the\nlevel of detail we usually aim for, but let's let it pass.\n\n> +\tif (!*p) {\n>  \t\tstrbuf_addstr(errbuf, _(\"expected something after combine:\"));\n>  \t\tresult = 1;\n>  \t\tgoto cleanup;\n>  \t}\n\nThis complains when \"combine:\" is not followed by anything.\n\n> +\twhile (*p && !result) {\n> +\t\tconst char *end = strchrnul(p, '+');\n> +\t\tchar *sub = xmemdupz(p, end - p);\n> +\n> +\t\tresult = parse_combine_subfilter(filter_options, sub, errbuf);\n> +\t\tfree(sub);\n> +\t\tif (!*end)\n> +\t\t\tbreak;\n> +\t\tp = end + 1;\n>  \t}\n\nThe usual \"process up to the next '+' and skip over that '+' to find the\nnext piece\" pattern is here.  There is nothing surprising.  Good.\n\nThe \"trailing '+' problem\" the proposed log message talks about\nhappens when the input is \"combine:foo+\" (this loop starts scanning\nfrom 'f').  We find the '+' at the end of the string in \"end\", make\na temporary copy of 'foo' and feed it to parse_combine_subfilter(),\nand move on to the NUL after '+', at which the loop control notices\nthat we are at the end.\n\nIt is curious what would happen when the input were \"combine:foo++\",\nthough.  What happens is that the loop begins with p pointing at 'f'\nin the initial iteration, \"end\" points at the first '+', and a\ntemporary copy of 'foo' is fed to parse_combine_subfilter(), and we\nmove on to the second '+'.  Then the second iteration finds NUL\nafter that '+' in \"end\", and we end up calling the helper function\nwith a temporary copy of '+'; gently_parse_list_objects_fiter() will\nreject it as an invalid filter-spec.\n\nLogically, \"foo+\" would be a combination of \"foo\" and \"\" (an empty\nstring) and we ignore the empty string, and \"foo++\" would be a\ncombination of \"foo\", \"\" and \"\" (two empty strings), but we barf at\nthe empty string if it appears in the middle.  And recall that \"\" we\nsaw earlier at the beginning of this function was also triggered an\nerror.\n\nAdmittedly the original wasn't much better.  It ignored an empty\nstring in the middle (e.g., \"foo++bar\" would have fed 'foo', '', and\n'bar' to parse_combine_subfilter() and an empty string would have\nbecome a no-op) but barfed at the trailing one \"foo+\".  This new\nimplementation swaps where it barfs, complaining an empty string in\nthe middle and ignoring an empty string at the end.\n\nIn any case, the error behaviour against an empty filter-spec feels\na bit uneven.\n\nTightening to reject empty string in the middle may appear to\nexisting users as a regression if they are using \"combine:foo++bar\"\nas they are forced to update it to lose the extra '+'.\n\nBy the way, instead of making a temporary copy and discarding it\nrepeatedly in a loop, it might be cheaper to reuse an allocated\ntemporary with the common pattern:\n\n\tstruct strbuf temp = STRBUF_INIT;\n\twhile (... loop ...) {\n\t\tconst char *end = ...;\n\t\tstrbuf_reset(&temp);\n\t\tstrbuf_add(&temp, p, end - p);\n\t\t... use temp.buf ...\n\t}\n\tstrbuf_release(&temp);\n\nbecause _reset() only resets the len member of the strbuf without\nreleasing the resource, if the next piece of memory you need a\ntemporary copy for is shorter than the pieces you have ever used the\nstrbuf for, you can make the copy without a new allocation.\n\n> -test_expect_success 'validate err msg for \"combine:<valid-filter>+\"' '\n> -\texpect_invalid_filter_spec combine:tree:2+ \"expected .tree:<depth>.\"\n> -'\n\n"},{"id":"538633","messageId":"20260311173336.8395-1-deveshigurgaon@gmail.com","threadId":"65208","inReplyTo":"20260311132041.12044-1-deveshigurgaon@gmail.com","subject":"[PATCH v3 0/2] avoid unnecessary strbuf_split*() and strbuf-by-value usage","fromName":"Deveshi Dwivedi","fromEmail":"deveshigurgaon@gmail.com","sentAt":"2026-03-11T17:33:34Z","receivedAt":"2026-03-11T17:33:47Z","isPatch":true,"sender":{"key":"deveshigurgaon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/120312681?v=4"},"body":"Junio's \"do not overuse strbuf_split*()\" series calls out remaining\nuses of strbuf_split*() as leftover bits for others to continue.\nThis series picks up two of them.\n\nwrite_worktree_linking_files() takes two struct strbuf parameters by\nvalue even though it only needs plain path strings.\n\nparse_combine_filter() in list-objects-filter-options.c uses\nstrbuf_split_str() to split a combine: filter spec at '+'.  An array\nof strbufs is unnecessary; walking the string directly with\nstrchrnul() is simpler and cleaner.\n\nChanges since v2:\n\n * Patch 1/2: no changes.\n * Patch 2/2: Incorporate review feedback from Junio C Hamano.\n   - Reuse a single strbuf instead of xmemdupz()/free() per\n     iteration to avoid repeated allocations.\n   - Skip empty sub-specs consistently (e.g. \"foo++bar\" or \"foo+\")\n     instead of only handling the trailing case.\n   - Trim commit message to focus less on implementation details.\n\nDeveshi Dwivedi (2):\n  worktree: do not pass strbuf by value\n  list-objects-filter-options: avoid strbuf_split_str()\n\n builtin/worktree.c                  |  2 +-\n list-objects-filter-options.c       | 40 ++++++++++++++---------------\n t/t6112-rev-list-filters-objects.sh |  4 ---\n worktree.c                          | 22 ++++++++--------\n worktree.h                          |  2 +-\n 5 files changed, 33 insertions(+), 37 deletions(-)\n\nRange-diff against v2:\n1:  ee6b7d1e6a = 1:  ee6b7d1e6a worktree: do not pass strbuf by value\n2:  c04ddaeb95 ! 2:  9f8690b9c7 list-objects-filter-options: avoid strbuf_split_str()\n    @@ Commit message\n         parse_combine_subfilter(), only read the string content of the strbuf\n         they receive.\n     \n    -    Walk the input string directly with strchrnul() to find each '+'.\n    -    strchrnul() returns a pointer to the terminating '\\0' when the\n    -    delimiter is not found, so no separate \"found separator?\" branch is\n    -    needed.  Copy each sub-spec into a temporary buffer using end - p,\n    -    which naturally excludes the '+', so the separator is always stripped\n    -    cleanly.  A trailing '+' causes the outer while (*p) test to fail on\n    -    the next iteration rather than passing an empty string to the parser.\n    -    Change the helpers to take const char * instead of struct strbuf *.\n    +    Walk the input string directly with strchrnul() to find each '+',\n    +    copying each sub-spec into a reusable temporary buffer.  The '+'\n    +    delimiter is naturally excluded.  Empty sub-specs (e.g. from a\n    +    trailing '+') are silently skipped for consistency.  Change the\n    +    helpers to take const char * instead of struct strbuf *.\n     \n         The test that expected an error on a trailing '+' is removed, since\n         that behavior was incorrect.\n    @@ list-objects-filter-options.c: static int parse_combine_filter(\n     -\tstruct strbuf **subspecs = strbuf_split_str(arg, '+', 0);\n     -\tsize_t sub;\n     +\tconst char *p = arg;\n    ++\tstruct strbuf sub = STRBUF_INIT;\n      \tint result = 0;\n      \n     -\tif (!subspecs[0]) {\n    @@ list-objects-filter-options.c: static int parse_combine_filter(\n     -\t\t\tfilter_options, subspecs[sub], errbuf);\n     +\twhile (*p && !result) {\n     +\t\tconst char *end = strchrnul(p, '+');\n    -+\t\tchar *sub = xmemdupz(p, end - p);\n     +\n    -+\t\tresult = parse_combine_subfilter(filter_options, sub, errbuf);\n    -+\t\tfree(sub);\n    ++\t\tstrbuf_reset(&sub);\n    ++\t\tstrbuf_add(&sub, p, end - p);\n    ++\n    ++\t\tif (sub.len)\n    ++\t\t\tresult = parse_combine_subfilter(filter_options, sub.buf, errbuf);\n    ++\n     +\t\tif (!*end)\n     +\t\t\tbreak;\n     +\t\tp = end + 1;\n      \t}\n    ++\tstrbuf_release(&sub);\n      \n      \tfilter_options->choice = LOFC_COMBINE;\n      \n-- \n2.52.0.230.gd8af7cadaa\n\n"},{"id":"538634","messageId":"20260311173336.8395-2-deveshigurgaon@gmail.com","threadId":"65208","inReplyTo":"20260311173336.8395-1-deveshigurgaon@gmail.com","subject":"[PATCH v3 1/2] worktree: do not pass strbuf by value","fromName":"Deveshi Dwivedi","fromEmail":"deveshigurgaon@gmail.com","sentAt":"2026-03-11T17:33:35Z","receivedAt":"2026-03-11T17:33:50Z","isPatch":true,"sender":{"key":"deveshigurgaon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/120312681?v=4"},"body":"write_worktree_linking_files() takes two struct strbuf parameters by\nvalue, even though it only reads path strings from them.\n\nPassing a strbuf by value is misleading and dangerous. The structure\ncarries a pointer to its underlying character array; caller and callee\nend up sharing that storage.  If the callee ever causes the strbuf to\nbe reallocated, the caller's copy becomes a dangling pointer, which\nresults in a double-free when the caller does strbuf_release().\n\nThe function only needs the string values, not the strbuf machinery.\nSwitch it to take const char * and update all callers to pass .buf.\n\nSigned-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>\n---\n builtin/worktree.c |  2 +-\n worktree.c         | 22 +++++++++++-----------\n worktree.h         |  2 +-\n 3 files changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex bc2d0d645b..4035b1cb06 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -539,7 +539,7 @@ static int add_worktree(const char *path, const char *refname,\n \n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/gitdir\", sb_repo.buf);\n-\twrite_worktree_linking_files(sb_git, sb, opts->relative_paths);\n+\twrite_worktree_linking_files(sb_git.buf, sb.buf, opts->relative_paths);\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/commondir\", sb_repo.buf);\n \twrite_file(sb.buf, \"../..\");\ndiff --git a/worktree.c b/worktree.c\nindex 6e2f0f7828..7eba12c6ed 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -445,7 +445,7 @@ void update_worktree_location(struct worktree *wt, const char *path_,\n \tstrbuf_realpath(&path, path_, 1);\n \tstrbuf_addf(&dotgit, \"%s/.git\", path.buf);\n \tif (fspathcmp(wt->path, path.buf)) {\n-\t\twrite_worktree_linking_files(dotgit, gitdir, use_relative_paths);\n+\t\twrite_worktree_linking_files(dotgit.buf, gitdir.buf, use_relative_paths);\n \n \t\tfree(wt->path);\n \t\twt->path = strbuf_detach(&path, NULL);\n@@ -684,7 +684,7 @@ static void repair_gitfile(struct worktree *wt,\n \n \tif (repair) {\n \t\tfn(0, wt->path, repair, cb_data);\n-\t\twrite_worktree_linking_files(dotgit, gitdir, use_relative_paths);\n+\t\twrite_worktree_linking_files(dotgit.buf, gitdir.buf, use_relative_paths);\n \t}\n \n done:\n@@ -742,7 +742,7 @@ void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path\n \tif (!file_exists(dotgit.buf))\n \t\tgoto done;\n \n-\twrite_worktree_linking_files(dotgit, gitdir, is_relative_path);\n+\twrite_worktree_linking_files(dotgit.buf, gitdir.buf, is_relative_path);\n done:\n \tstrbuf_release(&gitdir);\n \tstrbuf_release(&dotgit);\n@@ -913,7 +913,7 @@ void repair_worktree_at_path(const char *path,\n \n \tif (repair) {\n \t\tfn(0, gitdir.buf, repair, cb_data);\n-\t\twrite_worktree_linking_files(dotgit, gitdir, use_relative_paths);\n+\t\twrite_worktree_linking_files(dotgit.buf, gitdir.buf, use_relative_paths);\n \t}\n done:\n \tfree(dotgit_contents);\n@@ -1087,17 +1087,17 @@ int init_worktree_config(struct repository *r)\n \treturn res;\n }\n \n-void write_worktree_linking_files(struct strbuf dotgit, struct strbuf gitdir,\n+void write_worktree_linking_files(const char *dotgit, const char *gitdir,\n \t\t\t\t  int use_relative_paths)\n {\n \tstruct strbuf path = STRBUF_INIT;\n \tstruct strbuf repo = STRBUF_INIT;\n \tstruct strbuf tmp = STRBUF_INIT;\n \n-\tstrbuf_addbuf(&path, &dotgit);\n+\tstrbuf_addstr(&path, dotgit);\n \tstrbuf_strip_suffix(&path, \"/.git\");\n \tstrbuf_realpath(&path, path.buf, 1);\n-\tstrbuf_addbuf(&repo, &gitdir);\n+\tstrbuf_addstr(&repo, gitdir);\n \tstrbuf_strip_suffix(&repo, \"/gitdir\");\n \tstrbuf_realpath(&repo, repo.buf, 1);\n \n@@ -1110,11 +1110,11 @@ void write_worktree_linking_files(struct strbuf dotgit, struct strbuf gitdir,\n \t}\n \n \tif (use_relative_paths) {\n-\t\twrite_file(gitdir.buf, \"%s/.git\", relative_path(path.buf, repo.buf, &tmp));\n-\t\twrite_file(dotgit.buf, \"gitdir: %s\", relative_path(repo.buf, path.buf, &tmp));\n+\t\twrite_file(gitdir, \"%s/.git\", relative_path(path.buf, repo.buf, &tmp));\n+\t\twrite_file(dotgit, \"gitdir: %s\", relative_path(repo.buf, path.buf, &tmp));\n \t} else {\n-\t\twrite_file(gitdir.buf, \"%s/.git\", path.buf);\n-\t\twrite_file(dotgit.buf, \"gitdir: %s\", repo.buf);\n+\t\twrite_file(gitdir, \"%s/.git\", path.buf);\n+\t\twrite_file(dotgit, \"gitdir: %s\", repo.buf);\n \t}\n \n \tstrbuf_release(&path);\ndiff --git a/worktree.h b/worktree.h\nindex 06efe26b83..f4e46be385 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -240,7 +240,7 @@ int init_worktree_config(struct repository *r);\n  *  dotgit: \"/path/to/foo/.git\"\n  *  gitdir: \"/path/to/repo/worktrees/foo/gitdir\"\n  */\n-void write_worktree_linking_files(struct strbuf dotgit, struct strbuf gitdir,\n+void write_worktree_linking_files(const char *dotgit, const char *gitdir,\n \t\t\t\t  int use_relative_paths);\n \n #endif\n-- \n2.52.0.230.gd8af7cadaa\n\n"},{"id":"538635","messageId":"20260311173336.8395-3-deveshigurgaon@gmail.com","threadId":"65208","inReplyTo":"20260311173336.8395-1-deveshigurgaon@gmail.com","subject":"[PATCH v3 2/2] list-objects-filter-options: avoid strbuf_split_str()","fromName":"Deveshi Dwivedi","fromEmail":"deveshigurgaon@gmail.com","sentAt":"2026-03-11T17:33:36Z","receivedAt":"2026-03-11T17:33:54Z","isPatch":true,"sender":{"key":"deveshigurgaon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/120312681?v=4"},"body":"parse_combine_filter() splits a combine: filter spec at '+' using\nstrbuf_split_str(), which yields an array of strbufs with the\ndelimiter left at the end of each non-final piece.  The code then\nmutates each non-final piece to strip the trailing '+' before parsing.\n\nAllocating an array of strbufs is unnecessary.  The function processes\none sub-spec at a time and does not use strbuf editing on the pieces.\nThe two helpers it calls, has_reserved_character() and\nparse_combine_subfilter(), only read the string content of the strbuf\nthey receive.\n\nWalk the input string directly with strchrnul() to find each '+',\ncopying each sub-spec into a reusable temporary buffer.  The '+'\ndelimiter is naturally excluded.  Empty sub-specs (e.g. from a\ntrailing '+') are silently skipped for consistency.  Change the\nhelpers to take const char * instead of struct strbuf *.\n\nThe test that expected an error on a trailing '+' is removed, since\nthat behavior was incorrect.\n\nSigned-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>\n---\n list-objects-filter-options.c       | 40 ++++++++++++++---------------\n t/t6112-rev-list-filters-objects.sh |  4 ---\n 2 files changed, 20 insertions(+), 24 deletions(-)\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 7f3e7b8f50..cef67e5919 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -125,9 +125,9 @@ int gently_parse_list_objects_filter(\n static const char *RESERVED_NON_WS = \"~`!@#$^&*()[]{}\\\\;'\\\",<>?\";\n \n static int has_reserved_character(\n-\tstruct strbuf *sub_spec, struct strbuf *errbuf)\n+\tconst char *sub_spec, struct strbuf *errbuf)\n {\n-\tconst char *c = sub_spec->buf;\n+\tconst char *c = sub_spec;\n \twhile (*c) {\n \t\tif (*c <= ' ' || strchr(RESERVED_NON_WS, *c)) {\n \t\t\tstrbuf_addf(\n@@ -144,7 +144,7 @@ static int has_reserved_character(\n \n static int parse_combine_subfilter(\n \tstruct list_objects_filter_options *filter_options,\n-\tstruct strbuf *subspec,\n+\tconst char *subspec,\n \tstruct strbuf *errbuf)\n {\n \tsize_t new_index = filter_options->sub_nr;\n@@ -155,7 +155,7 @@ static int parse_combine_subfilter(\n \t\t      filter_options->sub_alloc);\n \tlist_objects_filter_init(&filter_options->sub[new_index]);\n \n-\tdecoded = url_percent_decode(subspec->buf);\n+\tdecoded = url_percent_decode(subspec);\n \n \tresult = has_reserved_character(subspec, errbuf);\n \tif (result)\n@@ -182,34 +182,34 @@ static int parse_combine_filter(\n \tconst char *arg,\n \tstruct strbuf *errbuf)\n {\n-\tstruct strbuf **subspecs = strbuf_split_str(arg, '+', 0);\n-\tsize_t sub;\n+\tconst char *p = arg;\n+\tstruct strbuf sub = STRBUF_INIT;\n \tint result = 0;\n \n-\tif (!subspecs[0]) {\n+\tif (!*p) {\n \t\tstrbuf_addstr(errbuf, _(\"expected something after combine:\"));\n \t\tresult = 1;\n \t\tgoto cleanup;\n \t}\n \n-\tfor (sub = 0; subspecs[sub] && !result; sub++) {\n-\t\tif (subspecs[sub + 1]) {\n-\t\t\t/*\n-\t\t\t * This is not the last subspec. Remove trailing \"+\" so\n-\t\t\t * we can parse it.\n-\t\t\t */\n-\t\t\tsize_t last = subspecs[sub]->len - 1;\n-\t\t\tassert(subspecs[sub]->buf[last] == '+');\n-\t\t\tstrbuf_remove(subspecs[sub], last, 1);\n-\t\t}\n-\t\tresult = parse_combine_subfilter(\n-\t\t\tfilter_options, subspecs[sub], errbuf);\n+\twhile (*p && !result) {\n+\t\tconst char *end = strchrnul(p, '+');\n+\n+\t\tstrbuf_reset(&sub);\n+\t\tstrbuf_add(&sub, p, end - p);\n+\n+\t\tif (sub.len)\n+\t\t\tresult = parse_combine_subfilter(filter_options, sub.buf, errbuf);\n+\n+\t\tif (!*end)\n+\t\t\tbreak;\n+\t\tp = end + 1;\n \t}\n+\tstrbuf_release(&sub);\n \n \tfilter_options->choice = LOFC_COMBINE;\n \n cleanup:\n-\tstrbuf_list_free(subspecs);\n \tif (result)\n \t\tlist_objects_filter_release(filter_options);\n \treturn result;\ndiff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh\nindex 0387f35a32..39211ef989 100755\n--- a/t/t6112-rev-list-filters-objects.sh\n+++ b/t/t6112-rev-list-filters-objects.sh\n@@ -483,10 +483,6 @@ test_expect_success 'combine:... with non-encoded reserved chars' '\n \t\t\"must escape char in sub-filter-spec: .\\~.\"\n '\n \n-test_expect_success 'validate err msg for \"combine:<valid-filter>+\"' '\n-\texpect_invalid_filter_spec combine:tree:2+ \"expected .tree:<depth>.\"\n-'\n-\n test_expect_success 'combine:... with edge-case hex digits: Ff Aa 0 9' '\n \tgit -C r3 rev-list --objects --filter=\"combine:tree:2+bl%6Fb:n%6fne\" \\\n \t\tHEAD >actual &&\n-- \n2.52.0.230.gd8af7cadaa\n\n"},{"id":"538638","messageId":"20260311174548.GA1900488@coredump.intra.peff.net","threadId":"65208","inReplyTo":"xmqqo6kuqqje.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] list-objects-filter-options: avoid strbuf_split_str()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-11T17:45:48Z","receivedAt":"2026-03-11T17:45:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 11, 2026 at 09:28:21AM -0700, Junio C Hamano wrote:\n\n> > +\twhile (*p && !result) {\n> > +\t\tconst char *end = strchrnul(p, '+');\n> > +\t\tchar *sub = xmemdupz(p, end - p);\n> > +\n> > +\t\tresult = parse_combine_subfilter(filter_options, sub, errbuf);\n> > +\t\tfree(sub);\n> > +\t\tif (!*end)\n> > +\t\t\tbreak;\n> > +\t\tp = end + 1;\n> >  \t}\n> [...]\n> It is curious what would happen when the input were \"combine:foo++\",\n> though.  What happens is that the loop begins with p pointing at 'f'\n> in the initial iteration, \"end\" points at the first '+', and a\n> temporary copy of 'foo' is fed to parse_combine_subfilter(), and we\n> move on to the second '+'.  Then the second iteration finds NUL\n> after that '+' in \"end\", and we end up calling the helper function\n> with a temporary copy of '+'; gently_parse_list_objects_fiter() will\n> reject it as an invalid filter-spec.\n\nI don't think this is quite right. After we skip the first \"+\" and \"p\"\npoints to the second one, then strchrnul() will find that second \"+\",\nnot NUL. And so we have a 0-length spec, and feed the empty string to\nparse_combine_subfilter(), which complains.\n\nIt is the same behavior when we see \"++\" in the middle of the string.\n\n> Logically, \"foo+\" would be a combination of \"foo\" and \"\" (an empty\n> string) and we ignore the empty string, and \"foo++\" would be a\n> combination of \"foo\", \"\" and \"\" (two empty strings), but we barf at\n> the empty string if it appears in the middle.  And recall that \"\" we\n> saw earlier at the beginning of this function was also triggered an\n> error.\n> \n> Admittedly the original wasn't much better.  It ignored an empty\n> string in the middle (e.g., \"foo++bar\" would have fed 'foo', '', and\n> 'bar' to parse_combine_subfilter() and an empty string would have\n> become a no-op) but barfed at the trailing one \"foo+\".  This new\n> implementation swaps where it barfs, complaining an empty string in\n> the middle and ignoring an empty string at the end.\n> \n> In any case, the error behaviour against an empty filter-spec feels\n> a bit uneven.\n> \n> Tightening to reject empty string in the middle may appear to\n> existing users as a regression if they are using \"combine:foo++bar\"\n> as they are forced to update it to lose the extra '+'.\n\nBut yeah, it is somewhat inconsistent that we complain about an empty\nspec in the middle, but not at the end. If we were starting from\nscratch, I'd probably forbid it everywhere. But since we allow it in\nsome cases now, it may be worth being more permissive.\n\nIt is easy to check in the loop, or even just teach the helper to make\nempty specs a noop:\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 616c6c7faa..56e1c651f6 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -151,6 +151,9 @@ static int parse_combine_subfilter(\n \tchar *decoded;\n \tint result;\n \n+\tif (!*subspec)\n+\t\treturn 0;\n+\n \tALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,\n \t\t      filter_options->sub_alloc);\n \tlist_objects_filter_init(&filter_options->sub[new_index]);\n\n> By the way, instead of making a temporary copy and discarding it\n> repeatedly in a loop, it might be cheaper to reuse an allocated\n> temporary with the common pattern:\n> \n> \tstruct strbuf temp = STRBUF_INIT;\n> \twhile (... loop ...) {\n> \t\tconst char *end = ...;\n> \t\tstrbuf_reset(&temp);\n> \t\tstrbuf_add(&temp, p, end - p);\n> \t\t... use temp.buf ...\n> \t}\n> \tstrbuf_release(&temp);\n> \n> because _reset() only resets the len member of the strbuf without\n> releasing the resource, if the next piece of memory you need a\n> temporary copy for is shorter than the pieces you have ever used the\n> strbuf for, you can make the copy without a new allocation.\n\nAs a general strategy, I agree this is a good one. But I'd be quite\nsurprised if it ever made a measurable difference for this loop, which\nwe'd expect to trigger a handful of times (and which allocates in the\nsub-function anyway).\n\n-Peff\n"},{"id":"538639","messageId":"20260311174816.GB1900488@coredump.intra.peff.net","threadId":"65208","inReplyTo":"20260311173336.8395-3-deveshigurgaon@gmail.com","subject":"Re: [PATCH v3 2/2] list-objects-filter-options: avoid strbuf_split_str()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-11T17:48:16Z","receivedAt":"2026-03-11T17:48:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 11, 2026 at 05:33:36PM +0000, Deveshi Dwivedi wrote:\n\n> +\twhile (*p && !result) {\n> +\t\tconst char *end = strchrnul(p, '+');\n> +\n> +\t\tstrbuf_reset(&sub);\n> +\t\tstrbuf_add(&sub, p, end - p);\n> +\n> +\t\tif (sub.len)\n> +\t\t\tresult = parse_combine_subfilter(filter_options, sub.buf, errbuf);\n> +\n> +\t\tif (!*end)\n> +\t\t\tbreak;\n> +\t\tp = end + 1;\n>  \t}\n> +\tstrbuf_release(&sub);\n\nThis version of the loop looks good to me.\n\n-Peff\n"},{"id":"538646","messageId":"xmqqo6kup7cw.fsf@gitster.g","threadId":"65208","inReplyTo":"20260311174548.GA1900488@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] list-objects-filter-options: avoid strbuf_split_str()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-11T18:07:59Z","receivedAt":"2026-03-11T18:08:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I don't think this is quite right. After we skip the first \"+\" and \"p\"\n> points to the second one, then strchrnul() will find that second \"+\",\n> not NUL. And so we have a 0-length spec, and feed the empty string to\n> parse_combine_subfilter(), which complains.\n\nAh, I misread gently_parse_list_objects_fiter(), which makes a NULL\narg a silent no-op, but fully complains on an empty string.  Thanks.\n\n> But yeah, it is somewhat inconsistent that we complain about an empty\n> spec in the middle, but not at the end. If we were starting from\n> scratch, I'd probably forbid it everywhere. But since we allow it in\n> some cases now, it may be worth being more permissive.\n>\n> It is easy to check in the loop, or even just teach the helper to make\n> empty specs a noop:\n\nYup.\n"},{"id":"538648","messageId":"xmqqjyvip73i.fsf@gitster.g","threadId":"65208","inReplyTo":"20260311174816.GB1900488@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] list-objects-filter-options: avoid strbuf_split_str()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-11T18:13:37Z","receivedAt":"2026-03-11T18:13:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Mar 11, 2026 at 05:33:36PM +0000, Deveshi Dwivedi wrote:\n>\n>> +\twhile (*p && !result) {\n>> +\t\tconst char *end = strchrnul(p, '+');\n>> +\n>> +\t\tstrbuf_reset(&sub);\n>> +\t\tstrbuf_add(&sub, p, end - p);\n>> +\n>> +\t\tif (sub.len)\n>> +\t\t\tresult = parse_combine_subfilter(filter_options, sub.buf, errbuf);\n>> +\n>> +\t\tif (!*end)\n>> +\t\t\tbreak;\n>> +\t\tp = end + 1;\n>>  \t}\n>> +\tstrbuf_release(&sub);\n>\n> This version of the loop looks good to me.\n>\n> -Peff\n\nThanks, both.  Will queue.\n"}]}