Re: [PATCH v2 00/11] do not overuse strbuf_split*()

From: Jeff King <peff@peff.net>
Date: Aug 2, 2025, 09:08 UTC
Message-ID: <20250802090825.GE3711639@coredump.intra.peff.net>
In-Reply-To: <20250731225433.4028872-1-gitster@pobox.com>

On Thu, Jul 31, 2025 at 03:54:22PM -0700, Junio C Hamano wrote:

Show 7 quoted lines

> strbuf is a very good data structure to work with string data
> without having to worry about running past the end of the string.
> 
> But an array of strbuf is often a wrong data structure.  You rarely
> have need to be able to edit multiple strings represented by such an
> array simultaneously.  And strbuf_split*() that produces result in
> such a shape is a misdesigned API function.

Yeah, I agree it's a bad API, and I'd be happy if we could eventually get rid of it.

Conversion to string_list mostly makes sense (though I especially like the case in sub-process.c where we can just parse with skip_prefix()).

I looked over the patches and didn't see anything objectionable. But I tried to think of subtle incompatibilities we might run into:

  1. I saw the different handling of "max" you had to deal with in one
     patch. Yuck. It might be worth tweaking string_list_split() to
     count "pieces" (which is how every other split function I've seen,
     like the one in perl, works). But that can be done later (and is
     tricky to do safely, since we wouldn't be changing the function
     signature).

  2. Is the handling of repeated delimiters the same? E.g., if I split
     on "/" and we see "foo//bar", do both split implementations yield
     the same output. I could see either of ("foo", "", "bar") and
     ("foo", "bar") being produced.

And...

> The most common use case of strbuf_split*() family of functions
> seems to be to trim away the whitespaces around each piece of split
> string.  With modern string_list_split*(), it is often no longer
> necessary.

Some of these sites do strbuf_rtrim() on the split pieces. But STRING_LIST_SPLIT_TRIM does both left and right. Is this OK? I think so because in both cases we are already splitting on space, so we wouldn't expect left-hand spaces (of course you could have a stray tab or something, but I suspect the new code matches the intent more closely).

-Peff