From: Karthik Nayak <karthik.188@gmail.com>
Date: Wed, 17 Feb 2016 16:58:24 GMT
Subject: Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()
Message-ID: <CAOLa=ZSgt=+OnSQsDvp0S5yKNekZ0XF4D1OncfyH4152Nvf6QQ@mail.gmail.com>
In-Reply-To: <20160216192231.GA16567@sigill.intra.peff.net>

On Wed, Feb 17, 2016 at 12:52 AM, Jeff King <peff@peff.net> wrote:
> On Wed, Feb 17, 2016 at 12:30:05AM +0530, Karthik Nayak wrote:
>
>> Use the newly introduced strbuf_split_str_omit_term() rather than
>> using strbuf_split_str() and manually removing the ',' terminator.
>>
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>>  ref-filter.c | 9 +--------
>>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> Did you consider just using string_list_split for this? AFAICT, you
> don't care about the results being strbufs themselves, and it would do
> what you want without having to bother with patch 1. The result would
> look something like the patch below.
>

I haven't, thanks for bringing it up :)

> Sorry to waltz into a review of v5 with a suggestion to throw out all
> the work done in previous iterations. :-/ I just think the strbuf_split
> interface is kind of clunky and I'd be happy if we could slowly get rid
> of it rather than growing it. Maybe that's not realistic, though (some
> of the callsites _do_ want to do things like strbuf_trim() after
> splitting).
>
> -Peff

That's fine, as I see it, it's better to wait a while and get a better version
of something.

-- 
Regards,
Karthik Nayak