Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()

From: Jeff King <peff@peff.net>
Date: Feb 16, 2016, 22:34 UTC
Message-ID: <20160216223451.GB9014@sigill.intra.peff.net>
In-Reply-To: <CAPig+cQDs35Uirm5cG552tR8iCFOstNJoOzLCZiXCgnq+g7MRQ@mail.gmail.com>

On Tue, Feb 16, 2016 at 04:09:53PM -0500, Eric Sunshine wrote:

Show 17 quoted lines

> My initial reaction was negative due to the heavy review burden this
> series has demanded thus far, however, my mind was changing even as I
> composed the above response. In retrospect, I think I'd be okay seeing
> a v6, for the following reasons:
> 
> - I already ended up reviewing the the suggested new changes pretty
> closely as a side-effect of reading your proposal.
> 
> - It would indeed be nice to avoid introducing
> strbuf_split_str_omit_term() in the first place; thus one less thing
> to worry about if someone ever takes on the task of retiring the
> strbuf_split interface.
> 
> - It should be only a minimal amount of work for Karthik, thus
> turnaround time should be short.
> 
> So, I think I'm fine with it, if Karthik is game.

I started to write up a commit message for my proposed change. But it did make me think of a counter-argument. Right now we parse "%(align:10,middle)" but do not allow "%(align: 10, middle)".

Should we? Or perhaps: might we? If the answer is yes, we are likely better off with strbuf_split, because then we are only a strbuf_trim() away from making that work.

-Peff