Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Feb 16, 2016, 22:49 UTC
- Message-ID
- <CAPig+cS+i5QfpUbs8T+CqcDkC4ybaTygE9bguiqQMNgV4JhDOQ@mail.gmail.com>
- In-Reply-To
- <20160216223451.GB9014@sigill.intra.peff.net>
On Tue, Feb 16, 2016 at 5:34 PM, Jeff King <peff@peff.net> wrote:
Show 26 quoted lines
> On Tue, Feb 16, 2016 at 04:09:53PM -0500, Eric Sunshine wrote: >> 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.
I also considered the issue of embedded whitespace very early on when reading your initial proposal, but didn't mention anything about it due to a vague recollection from one of the early reviews (or possibly a review of one of Karthik's other patch series) of someone (possibly Junio) saying or implying that embedded whitespace would not be supported. Unfortunately, I can't locate that message (assuming it even exists and wasn't a figment of my imagination).