git/list[1] front-page[2] threads[3] people[4] search[5] about
wed 2026-10-07 16:52 UTC

[PATCH v3 2/2] list-objects-filter-options: avoid strbuf_split_str()

From
Deveshi Dwivedi <deveshigurgaon@gmail.com>
Date
Mar 11, 2026, 17:33 UTC
Message-ID
<20260311173336.8395-3-deveshigurgaon@gmail.com>
In-Reply-To
<20260311173336.8395-1-deveshigurgaon@gmail.com>

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(-)
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 7f3e7b8f50..cef67e5919 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -125,9 +125,9 @@ int gently_parse_list_objects_filter(
 static const char *RESERVED_NON_WS = "~`!@#$^&*()[]{}\\;'\",<>?";
 
 static int has_reserved_character(
-	struct strbuf *sub_spec, struct strbuf *errbuf)
+	const char *sub_spec, struct strbuf *errbuf)
 {
-	const char *c = sub_spec->buf;
+	const char *c = sub_spec;
 	while (*c) {
 		if (*c <= ' ' || strchr(RESERVED_NON_WS, *c)) {
 			strbuf_addf(
@@ -144,7 +144,7 @@ static int has_reserved_character(
 
 static int parse_combine_subfilter(
 	struct list_objects_filter_options *filter_options,
-	struct strbuf *subspec,
+	const char *subspec,
 	struct strbuf *errbuf)
 {
 	size_t new_index = filter_options->sub_nr;
@@ -155,7 +155,7 @@ static int parse_combine_subfilter(
 		      filter_options->sub_alloc);
 	list_objects_filter_init(&filter_options->sub[new_index]);
 
-	decoded = url_percent_decode(subspec->buf);
+	decoded = url_percent_decode(subspec);
 
 	result = has_reserved_character(subspec, errbuf);
 	if (result)
@@ -182,34 +182,34 @@ static int parse_combine_filter(
 	const char *arg,
 	struct strbuf *errbuf)
 {
-	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]) {
+	if (!*p) {
 		strbuf_addstr(errbuf, _("expected something after combine:"));
 		result = 1;
 		goto cleanup;
 	}
 
-	for (sub = 0; subspecs[sub] && !result; sub++) {
-		if (subspecs[sub + 1]) {
-			/*
-			 * This is not the last subspec. Remove trailing "+" so
-			 * we can parse it.
-			 */
-			size_t last = subspecs[sub]->len - 1;
-			assert(subspecs[sub]->buf[last] == '+');
-			strbuf_remove(subspecs[sub], last, 1);
-		}
-		result = parse_combine_subfilter(
-			filter_options, subspecs[sub], errbuf);
+	while (*p && !result) {
+		const char *end = strchrnul(p, '+');
+
+		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;
 
 cleanup:
-	strbuf_list_free(subspecs);
 	if (result)
 		list_objects_filter_release(filter_options);
 	return result;
diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
index 0387f35a32..39211ef989 100755
--- a/t/t6112-rev-list-filters-objects.sh
+++ b/t/t6112-rev-list-filters-objects.sh
@@ -483,10 +483,6 @@ 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 &&
-- 
2.52.0.230.gd8af7cadaa
Previous: Deveshi DwivediNext: Jeff King
Message 7 of 11 in “avoid unnecessary strbuf_split*() and strbuf-by-value usage”
  1. 0/2 avoid unnecessary strbuf_split*() and strbuf-by-value usageDeveshi Dwivedi, Mar 11, 2026
  2. 1/2 worktree: do not pass strbuf by valueDeveshi Dwivedi, Mar 11, 2026
  3. 2/2 list-objects-filter-options: avoid strbuf_split_str()Deveshi Dwivedi, Mar 11, 2026
  4. Junio C HamanoMar 11, 2026
  5. 0/2 avoid unnecessary strbuf_split*() and strbuf-by-value usageDeveshi Dwivedi, Mar 11, 2026
  6. 1/2 worktree: do not pass strbuf by valueDeveshi Dwivedi, Mar 11, 2026
  7. 2/2 list-objects-filter-options: avoid strbuf_split_str()Deveshi Dwivedi, Mar 11, 2026
  8. Jeff KingMar 11, 2026
  9. Jeff KingMar 11, 2026
  10. Junio C HamanoMar 11, 2026
  11. Junio C HamanoMar 11, 2026

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.