Re: [PATCH 1/7] strbuf: add "include_delim" parameter to "strbuf_split"

From: Christian Couder <chriscool@tuxfamily.org>
Date: Mar 13, 2009, 06:02 UTC
Message-ID: <200903130702.01039.chriscool@tuxfamily.org>
In-Reply-To: <7v1vt2j91x.fsf@gitster.siamese.dyndns.org>

Le vendredi 13 mars 2009, Junio C Hamano a écrit :

Show 17 quoted lines

> Christian Couder <chriscool@tuxfamily.org> writes:
> > Yes, my patch does not do that, because I think including the delimiter
> > is a special case of the more general and useful behavior of not
> > including it.
>
> You got it backwards.
>
> With the way the returned string is used by the single caller that your
> patch adds (which splits at ","), I would agree that lack of delimiter
> allows the result to get used directly in the further processing.
>
> But even in that codepath, I have to say that it is just lazy programming
> that the caller does not postprocess the returned value from the splitter
> function.  If it wants not just accept input such as "a,b,c" but also
> wants to tolerate things like "a, b, c", it will have to look at the
> resulting string, and ignoring the delimiter at the end becomes just a
> small part of the general clean-up process [*1*].

I think talking about "lazy programming" in this case is a bit strong, because first "git rev-list --bisect-skip" is plumbing and will be used mostly by porcelain and second because there are much more common shell utilities that don't tolerate things like "a, b, c". Try using "cut" with -f'1, 2' (instead of -f1,2) for example.

> Once you start allowing "split at one of these characters" and/or "split
> at delimiter that matches this pattern", you cannot just discard the
> delimiter if you want to support non-simplistic callers, because they
> would want to know what the delimiter was.

But I let non simplistic callers use "1" as the last parameter if they want the delimiter. I just give one more way to use strbuf_split. I don't remove anything.

> Stripping out the delimiter is the special case for simplistic callers
> (think "gets()" that strips, and "fgets()" that doesn't).

Aren't gets and fgets an example that having the choice to strip out the delimiter or not is good?

Show 5 quoted lines

> A more general 
> solution should be by default not to strip it, and I do not think your
> new caller, if it were written correctly, needs stripping behaviour
> either. That means there is no need for the "optionally strip" flag to
> the function in order to support the rest of the series [*2*].

I think my patch 8/7, that I just sent, is a good solution and it still uses the new behavior of strbuf_split introduced in 1/7.

> Also comparing this with Perl/Python split() forgets that you are working
> in C, where truncating an existing string is quite cheap (just assign
> '\0'). There is a different trade-off to be made in these language
> environments.

Sorry but I think the goal of the strbuf API is to be quite high level, so I think comparing this with Perl/Python is ok.

Best regards, Christian.