From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 09 Mar 2026 15:38:16 GMT
Subject: Re: [PATCH v1 2/2] list-objects-filter-options: avoid strbuf_split_str()
Message-ID: <xmqqjyvl57yv.fsf@gitster.g>
In-Reply-To: <20260308180359.31188-3-deveshigurgaon@gmail.com>

Deveshi Dwivedi <deveshigurgaon@gmail.com> writes:

> 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 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 *.

Makes sense.  Instead of finding '+' and making many small copies
piecemeal, you could make a single copy of "const char *arg" once,
walk that string using strchr() looking for the next '+', and
replace '+' with '\0' before processing the current piece and
iterate, which may reduce the need for many small allocations and
deallocations, but I do not know if it is worth it.  Benchmarking
it would not yield measurable difference, I suspect.

> +	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';
> +
> +		result = parse_combine_subfilter(filter_options, sub, errbuf);
> +		free(sub);
> +		if (!sep)
> +			break;
> +		p = sep + 1;
>  	}

Hmph, would this loop handle a trailing '+' the same way as before,
e.g., "combine:tree:2+"?  The original would have split the string
into ["tree:2+", ""] and the last call to parse_combine_subfilter()
would have been made with an empty string.  The new code does not
make that last call with an empty string.  Perhaps the differences
do not matter?  I dunno.

Other than that, nice to see one fewer use of "splitting into an
array of strbuf" pattern.

Thanks.