Web lists-archives.com

Re: [PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf




Hi Hannes,

On Tue, 5 Feb 2019, Johannes Sixt wrote:

> Am 05.02.19 um 12:06 schrieb Johannes Schindelin:
> > The real examples are much more mundane, and very different from what you
> > suspected, e.g. inserting the tag header before the tag message in
> > `create_tag()` in `builtin/tag.c`. Basically, it is building up a strbuf
> > for the sake of calling `strbuf_insert()` to insert that string elsewhere.
> 
> I had a look, and this example does not convince me at all. If
> separation of concerns were applied well around the launch_editor APIs,
> you would not need strbuf_insert() in the first place. But, alas, these
> functions focus more on DRY, and that is often the opposite of
> separation of concerns.

So you actually are convinced that it is needed in this instance. Good.

> > In any case, the mere existence, and use, of `strbuf_insert()` is a
> > pretty clear counter case to the idea that `strbuf_vinsertf()` would
> > encourage invalid code flows.
> 
> Nobody wants to get rid of strbuf_insert(). I have worked with at least
> 3 string APIs. All have insert operations, and some have string
> formatting capabilities, but none of them conflate the two operations
> into one. Maybe, there is a plan behind it? strbuf is the first (my 4th)
> API that does, and it was non-trivial to get it right...

Sorry, by that token `strbuf_vaddf()` would already be a violation of the
separation of concerns: it also makes space first, and then it also
formats a string.

Render me unconvinced.

I am still glad you found and fixed the bug,
Dscho