From: Jeff King <peff@peff.net>
Date: Mon, 09 Mar 2026 19:01:12 GMT
Subject: Re: [PATCH v1 2/2] list-objects-filter-options: avoid strbuf_split_str()
Message-ID: <20260309190112.GA309867@coredump.intra.peff.net>
In-Reply-To: <xmqqjyvl57yv.fsf@gitster.g>

On Mon, Mar 09, 2026 at 08:38:16AM -0700, Junio C Hamano wrote:

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

I think the original was wrong in its parsing. The first entry should be
"tree:2", without the trailing "+". And that would almost always result
in rejecting the string, because the "+" doesn't make any sense for most
filters. The exception is:

  blob=$(echo foo | git hash-object -w --stdin)
  git tag foo+ $blob
  git rev-list --filter=combine:sparse:oid=foo+

which happens to work. But it is wrong according to the documentation,
which says that "+" needs to be escaped if you want it passed along to
the sub-filter.

So flagging a trailing "+" as an error would probably be OK, and match
what happens now. But quietly ignoring it is perhaps friendlier (or less
friendly, if you think it might let a typo'd input go unnoticed).

-Peff