From: Taylor Blau <me@ttaylorr.com>
Date: Tue, 18 Apr 2023 20:54:23 GMT
Subject: Re: [PATCH v2 1/6] string-list: introduce `string_list_split_in_place_multi()`
Message-ID: <ZD8Df1iZaUGzodrg@nand.local>
In-Reply-To: <xmqqleipc73v.fsf@gitster.g>

On Tue, Apr 18, 2023 at 12:39:48PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > Introduce a variant of the `string_list_split_in_place()` function that
> > takes a string of accepted delimiters.
> >
> > By contrast to its cousin `string_list_split_in_place()` which splits
> > the given string at every instance of the single character `delim`, the
> > `_multi` variant splits the given string any any character appearing in
> > the string `delim`.
> >
> > Like `strtok()`, the `_multi` variant skips past sequential delimiting
> > characters. For example:
> >
> >     string_list_split_in_place(&xs, xstrdup("foo::bar::baz"), ":", -1);
> >
> > would place in `xs` the elements "foo", "bar", and "baz".
>
> strtok() also skips leading and trailing delimiters, i.e. the above
> will give you identical result for ":foo:bar:baz:".

I'm not sure the results are identical. Adding this test case for
testing the behavior of string_list_split_in_place() passes before and
after this series:

--- 8< ---
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
index 9c5094616a..dfe970a566 100755
--- a/t/t0063-string-list.sh
+++ b/t/t0063-string-list.sh
@@ -72,6 +72,15 @@ test_split ":" ":" "-1" <<EOF
 [1]: ""
 EOF

+test_split ":foo:bar:baz:" ":" "-1" <<-\EOF
+5
+[0]: ""
+[1]: "foo"
+[2]: "bar"
+[3]: "baz"
+[4]: ""
+EOF
+
 test_split_in_place_multi "foo:;:bar:;:baz" ":;" "-1" <<-\EOF
 3
 [0]: "foo"
--- >8 ---

> It would be useful to test that here in addition to the existing ones.

Sure. FWIW, the behavior for string_list_split_in_place_multi() is
slightly different, since it will eat up all of the leading delimiter
tokens and treat the first token as "foo".

Here's a diff that could be squashed into this patch which captures both
cases:

--- 8< ---
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
index 9c5094616a..efc84dc124 100755
--- a/t/t0063-string-list.sh
+++ b/t/t0063-string-list.sh
@@ -72,6 +72,15 @@ test_split ":" ":" "-1" <<EOF
 [1]: ""
 EOF

+test_split ":foo:bar:baz:" ":" "-1" <<-\EOF
+5
+[0]: ""
+[1]: "foo"
+[2]: "bar"
+[3]: "baz"
+[4]: ""
+EOF
+
 test_split_in_place_multi "foo:;:bar:;:baz" ":;" "-1" <<-\EOF
 3
 [0]: "foo"
@@ -104,6 +113,14 @@ test_split_in_place_multi "foo:;:bar:;:" ":;" "-1" <<-\EOF
 [2]: ""
 EOF

+test_split_in_place_multi ":;:foo:;:bar:;:baz:;:" ":;" "-1" <<-\EOF
+4
+[0]: "foo"
+[1]: "bar"
+[2]: "baz"
+[3]: ""
+EOF
+
 test_expect_success "test filter_string_list" '
 	test "x-" = "x$(test-tool string-list filter - y)" &&
 	test "x-" = "x$(test-tool string-list filter no y)" &&
--- >8 ---


Thanks,
Taylor