Junio's "do not overuse strbuf_split*()" series calls out remaining uses of strbuf_split*() as leftover bits for others to continue. This series picks up two of them.

write_worktree_linking_files() takes two struct strbuf parameters by value even though it only needs plain path strings.

parse_combine_filter() in list-objects-filter-options.c uses strbuf_split_str() to split a combine: filter spec at '+'. An array of strbufs is unnecessary; walking the string directly with strchrnul() is simpler and cleaner.

Changes since v1:

 * Patch 1/2: no changes.

 * Patch 2/2: Incorporate review feedback from Jeff King.
   - Use strchrnul() instead of strchr() so the loop needs no
     separate "found separator?" branch or strlen() fallback.
   - Drop the unnecessary (size_t) cast on the length expression.
   - Always exclude '+' from the sub-spec via end - p, removing the
     incorrect conditional stripping.  A trailing '+' now causes the
     while (*p) condition to fail cleanly on the next iteration
     rather than passing an empty string to the parser.
   - Remove the test that expected an error on trailing '+', since
     that behavior was incorrect.

Deveshi Dwivedi (2):
  worktree: do not pass strbuf by value
  list-objects-filter-options: avoid strbuf_split_str()

 builtin/worktree.c                  |  2 +-
 list-objects-filter-options.c       | 35 +++++++++++++----------------
 t/t6112-rev-list-filters-objects.sh |  4 ----
 worktree.c                          | 22 +++++++++---------
 worktree.h                          |  2 +-
 5 files changed, 28 insertions(+), 37 deletions(-)

Range-diff against v1:
1:  ee6b7d1e6a = 1:  ee6b7d1e6a worktree: do not pass strbuf by value
2:  386aed0adf ! 2:  c04ddaeb95 list-objects-filter-options: avoid strbuf_split_str()
    @@ Commit message
         parse_combine_subfilter(), only read the string content of the strbuf
         they receive.
     
    -    Walk the input string directly with strchr() to find each '+'.  Copy
    -    each sub-spec into a temporary buffer and strip the '+' only when
    -    another sub-spec follows.  Change the helpers to take const char *
    -    instead of struct strbuf *.
    +    Walk the input string directly with strchrnul() to find each '+'.
    +    strchrnul() returns a pointer to the terminating '\0' when the
    +    delimiter is not found, so no separate "found separator?" branch is
    +    needed.  Copy each sub-spec into a temporary buffer using end - p,
    +    which naturally excludes the '+', so the separator is always stripped
    +    cleanly.  A trailing '+' causes the outer while (*p) test to fail on
    +    the next iteration rather than passing an empty string to the parser.
    +    Change the helpers to take const char * instead of struct strbuf *.
    +
    +    The test that expected an error on a trailing '+' is removed, since
    +    that behavior was incorrect.
     
         Signed-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>
     
    @@ list-objects-filter-options.c: static int parse_combine_filter(
     -		result = parse_combine_subfilter(
     -			filter_options, subspecs[sub], errbuf);
     +	while (*p && !result) {
    -+		const char *sep = strchr(p, '+');
    -+		size_t len = sep ? (size_t)(sep - p + 1) : strlen(p);
    -+		char *sub = xmemdupz(p, len);
    -+
    -+		/* strip '+' separator, but only when more sub-specs follow */
    -+		if (sep && *(sep + 1))
    -+			sub[len - 1] = '\0';
    ++		const char *end = strchrnul(p, '+');
    ++		char *sub = xmemdupz(p, end - p);
     +
     +		result = parse_combine_subfilter(filter_options, sub, errbuf);
     +		free(sub);
    -+		if (!sep)
    ++		if (!*end)
     +			break;
    -+		p = sep + 1;
    ++		p = end + 1;
      	}
      
      	filter_options->choice = LOFC_COMBINE;
    @@ list-objects-filter-options.c: static int parse_combine_filter(
      	if (result)
      		list_objects_filter_release(filter_options);
      	return result;
    +
    + ## t/t6112-rev-list-filters-objects.sh ##
    +@@ t/t6112-rev-list-filters-objects.sh: test_expect_success 'combine:... with non-encoded reserved chars' '
    + 		"must escape char in sub-filter-spec: .\~."
    + '
    + 
    +-test_expect_success 'validate err msg for "combine:<valid-filter>+"' '
    +-	expect_invalid_filter_spec combine:tree:2+ "expected .tree:<depth>."
    +-'
    +-
    + test_expect_success 'combine:... with edge-case hex digits: Ff Aa 0 9' '
    + 	git -C r3 rev-list --objects --filter="combine:tree:2+bl%6Fb:n%6fne" \
    + 		HEAD >actual &&

write_worktree_linking_files() takes two struct strbuf parameters by value, even though it only reads path strings from them.

Passing a strbuf by value is misleading and dangerous. The structure carries a pointer to its underlying character array; caller and callee end up sharing that storage. If the callee ever causes the strbuf to be reallocated, the caller's copy becomes a dangling pointer, which results in a double-free when the caller does strbuf_release().

The function only needs the string values, not the strbuf machinery. Switch it to take const char * and update all callers to pass .buf.

Signed-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>
---
 builtin/worktree.c |  2 +-
 worktree.c         | 22 +++++++++++-----------
 worktree.h         |  2 +-
 3 files changed, 13 insertions(+), 13 deletions(-)

parse_combine_filter() splits a combine: filter spec at '+' using strbuf_split_str(), which yields an array of strbufs with the delimiter left at the end of each non-final piece. The code then mutates each non-final piece to strip the trailing '+' before parsing.

Allocating an array of strbufs is unnecessary. The function processes one sub-spec at a time and does not use strbuf editing on the pieces. The two helpers it calls, has_reserved_character() and parse_combine_subfilter(), only read the string content of the strbuf they receive.

Walk the input string directly with strchrnul() to find each '+'. strchrnul() returns a pointer to the terminating '\0' when the delimiter is not found, so no separate "found separator?" branch is needed. Copy each sub-spec into a temporary buffer using end - p, which naturally excludes the '+', so the separator is always stripped cleanly. A trailing '+' causes the outer while (*p) test to fail on the next iteration rather than passing an empty string to the parser. Change the helpers to take const char * instead of struct strbuf *.

The test that expected an error on a trailing '+' is removed, since that behavior was incorrect.

Signed-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>
---
 list-objects-filter-options.c       | 35 +++++++++++++----------------
 t/t6112-rev-list-filters-objects.sh |  4 ----
 2 files changed, 15 insertions(+), 24 deletions(-)

Deveshi Dwivedi <deveshigurgaon@gmail.com> writes:

OK. The above description is a bit more excessively focused on the implementation (which readers can find in the patch text) than the level of detail we usually aim for, but let's let it pass.

This complains when "combine:" is not followed by anything.

The usual "process up to the next '+' and skip over that '+' to find the next piece" pattern is here. There is nothing surprising. Good.

The "trailing '+' problem" the proposed log message talks about happens when the input is "combine:foo+" (this loop starts scanning from 'f'). We find the '+' at the end of the string in "end", make a temporary copy of 'foo' and feed it to parse_combine_subfilter(), and move on to the NUL after '+', at which the loop control notices that we are at the end.

It is curious what would happen when the input were "combine:foo++", though. What happens is that the loop begins with p pointing at 'f' in the initial iteration, "end" points at the first '+', and a temporary copy of 'foo' is fed to parse_combine_subfilter(), and we move on to the second '+'. Then the second iteration finds NUL after that '+' in "end", and we end up calling the helper function with a temporary copy of '+'; gently_parse_list_objects_fiter() will reject it as an invalid filter-spec.

Logically, "foo+" would be a combination of "foo" and "" (an empty string) and we ignore the empty string, and "foo++" would be a combination of "foo", "" and "" (two empty strings), but we barf at the empty string if it appears in the middle. And recall that "" we saw earlier at the beginning of this function was also triggered an error.

Admittedly the original wasn't much better. It ignored an empty string in the middle (e.g., "foo++bar" would have fed 'foo', '', and 'bar' to parse_combine_subfilter() and an empty string would have become a no-op) but barfed at the trailing one "foo+". This new implementation swaps where it barfs, complaining an empty string in the middle and ignoring an empty string at the end.

In any case, the error behaviour against an empty filter-spec feels a bit uneven.

Tightening to reject empty string in the middle may appear to existing users as a regression if they are using "combine:foo++bar" as they are forced to update it to lose the extra '+'.

By the way, instead of making a temporary copy and discarding it repeatedly in a loop, it might be cheaper to reuse an allocated temporary with the common pattern:

	struct strbuf temp = STRBUF_INIT;
	while (... loop ...) {
		const char *end = ...;
		strbuf_reset(&temp);
		strbuf_add(&temp, p, end - p);
		... use temp.buf ...
	}
	strbuf_release(&temp);

because _reset() only resets the len member of the strbuf without releasing the resource, if the next piece of memory you need a temporary copy for is shorter than the pieces you have ever used the strbuf for, you can make the copy without a new allocation.

Junio's "do not overuse strbuf_split*()" series calls out remaining uses of strbuf_split*() as leftover bits for others to continue. This series picks up two of them.

write_worktree_linking_files() takes two struct strbuf parameters by value even though it only needs plain path strings.

parse_combine_filter() in list-objects-filter-options.c uses strbuf_split_str() to split a combine: filter spec at '+'. An array of strbufs is unnecessary; walking the string directly with strchrnul() is simpler and cleaner.

Changes since v2:

 * Patch 1/2: no changes.
 * Patch 2/2: Incorporate review feedback from Junio C Hamano.
   - Reuse a single strbuf instead of xmemdupz()/free() per
     iteration to avoid repeated allocations.
   - Skip empty sub-specs consistently (e.g. "foo++bar" or "foo+")
     instead of only handling the trailing case.
   - Trim commit message to focus less on implementation details.

Deveshi Dwivedi (2):
  worktree: do not pass strbuf by value
  list-objects-filter-options: avoid strbuf_split_str()

 builtin/worktree.c                  |  2 +-
 list-objects-filter-options.c       | 40 ++++++++++++++---------------
 t/t6112-rev-list-filters-objects.sh |  4 ---
 worktree.c                          | 22 ++++++++--------
 worktree.h                          |  2 +-
 5 files changed, 33 insertions(+), 37 deletions(-)

Range-diff against v2:
1:  ee6b7d1e6a = 1:  ee6b7d1e6a worktree: do not pass strbuf by value
2:  c04ddaeb95 ! 2:  9f8690b9c7 list-objects-filter-options: avoid strbuf_split_str()
    @@ Commit message
         parse_combine_subfilter(), only read the string content of the strbuf
         they receive.
     
    -    Walk the input string directly with strchrnul() to find each '+'.
    -    strchrnul() returns a pointer to the terminating '\0' when the
    -    delimiter is not found, so no separate "found separator?" branch is
    -    needed.  Copy each sub-spec into a temporary buffer using end - p,
    -    which naturally excludes the '+', so the separator is always stripped
    -    cleanly.  A trailing '+' causes the outer while (*p) test to fail on
    -    the next iteration rather than passing an empty string to the parser.
    -    Change the helpers to take const char * instead of struct strbuf *.
    +    Walk the input string directly with strchrnul() to find each '+',
    +    copying each sub-spec into a reusable temporary buffer.  The '+'
    +    delimiter is naturally excluded.  Empty sub-specs (e.g. from a
    +    trailing '+') are silently skipped for consistency.  Change the
    +    helpers to take const char * instead of struct strbuf *.
     
         The test that expected an error on a trailing '+' is removed, since
         that behavior was incorrect.
    @@ list-objects-filter-options.c: static int parse_combine_filter(
     -	struct strbuf **subspecs = strbuf_split_str(arg, '+', 0);
     -	size_t sub;
     +	const char *p = arg;
    ++	struct strbuf sub = STRBUF_INIT;
      	int result = 0;
      
     -	if (!subspecs[0]) {
    @@ list-objects-filter-options.c: static int parse_combine_filter(
     -			filter_options, subspecs[sub], errbuf);
     +	while (*p && !result) {
     +		const char *end = strchrnul(p, '+');
    -+		char *sub = xmemdupz(p, end - p);
     +
    -+		result = parse_combine_subfilter(filter_options, sub, errbuf);
    -+		free(sub);
    ++		strbuf_reset(&sub);
    ++		strbuf_add(&sub, p, end - p);
    ++
    ++		if (sub.len)
    ++			result = parse_combine_subfilter(filter_options, sub.buf, errbuf);
    ++
     +		if (!*end)
     +			break;
     +		p = end + 1;
      	}
    ++	strbuf_release(&sub);
      
      	filter_options->choice = LOFC_COMBINE;
      

write_worktree_linking_files() takes two struct strbuf parameters by value, even though it only reads path strings from them.

Passing a strbuf by value is misleading and dangerous. The structure carries a pointer to its underlying character array; caller and callee end up sharing that storage. If the callee ever causes the strbuf to be reallocated, the caller's copy becomes a dangling pointer, which results in a double-free when the caller does strbuf_release().

The function only needs the string values, not the strbuf machinery. Switch it to take const char * and update all callers to pass .buf.

Signed-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>
---
 builtin/worktree.c |  2 +-
 worktree.c         | 22 +++++++++++-----------
 worktree.h         |  2 +-
 3 files changed, 13 insertions(+), 13 deletions(-)

parse_combine_filter() splits a combine: filter spec at '+' using strbuf_split_str(), which yields an array of strbufs with the delimiter left at the end of each non-final piece. The code then mutates each non-final piece to strip the trailing '+' before parsing.

Allocating an array of strbufs is unnecessary. The function processes one sub-spec at a time and does not use strbuf editing on the pieces. The two helpers it calls, has_reserved_character() and parse_combine_subfilter(), only read the string content of the strbuf they receive.

Walk the input string directly with strchrnul() to find each '+', copying each sub-spec into a reusable temporary buffer. The '+' delimiter is naturally excluded. Empty sub-specs (e.g. from a trailing '+') are silently skipped for consistency. Change the helpers to take const char * instead of struct strbuf *.

The test that expected an error on a trailing '+' is removed, since that behavior was incorrect.

Signed-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>
---
 list-objects-filter-options.c       | 40 ++++++++++++++---------------
 t/t6112-rev-list-filters-objects.sh |  4 ---
 2 files changed, 20 insertions(+), 24 deletions(-)

On Wed, Mar 11, 2026 at 09:28:21AM -0700, Junio C Hamano wrote:

I don't think this is quite right. After we skip the first "+" and "p" points to the second one, then strchrnul() will find that second "+", not NUL. And so we have a 0-length spec, and feed the empty string to parse_combine_subfilter(), which complains.

It is the same behavior when we see "++" in the middle of the string.

But yeah, it is somewhat inconsistent that we complain about an empty spec in the middle, but not at the end. If we were starting from scratch, I'd probably forbid it everywhere. But since we allow it in some cases now, it may be worth being more permissive.

It is easy to check in the loop, or even just teach the helper to make empty specs a noop:

On Wed, Mar 11, 2026 at 05:33:36PM +0000, Deveshi Dwivedi wrote:

This version of the loop looks good to me.

-Peff

Jeff King <peff@peff.net> writes:

Ah, I misread gently_parse_list_objects_fiter(), which makes a NULL arg a silent no-op, but fully complains on an empty string. Thanks.

Yup.

Jeff King <peff@peff.net> writes:

Thanks, both.  Will queue.