Re: [PATCH v2] strbuf: add and use strbuf_insertstr()

From: Jeff King <peff@peff.net>
Date: Feb 11, 2020, 17:13 UTC
Message-ID: <20200211171357.GA2118476@coredump.intra.peff.net>
In-Reply-To: <a5622196-1978-2fe5-144f-99edc5516dd6@web.de>

On Tue, Feb 11, 2020 at 05:18:02PM +0100, René Scharfe wrote:

> And you could rename it to skip_header() to fix the issue that its name
> starts with cmp but its return value is the inverse of a cmp-style
> function.

Good suggestion.

> And it could take a char pointer instead of a strbuf, to reduce its
> dependencies and make it more widely useful -- but that might also be
> a case of YAGNI.

I didn't take this one because all of the callers have strbuf, and it saves them from dereferencing.

> > -			strbuf_add(&sb, line->buf + len + 2, line->len - len - 2);
> > +			strbuf_addstr(&sb, val);
> 
> That assumes the header value never contains NULs.  Valid?

I think so, but I pulled it out to a separate patch so that it could be argued for explicitly.

> The repeated sequence cmp_header()+strbuf_add{,str}()+decode_header()
> makes me itchy.

Yeah, it's ugly and I factored it out in a new patch. But the boilerplate and docstring ends up longer than the savings. ;)

-Peff