Re: [PATCH v2 02/10] reftable: stop using `strbuf_addf()`

[Patrick Steinhardt <ps@pks.im>]

On Mon, Oct 14, 2024 at 06:32:07PM -0400, Taylor Blau wrote:
> On Mon, Oct 14, 2024 at 03:02:21PM +0200, Patrick Steinhardt wrote:
> > We're about to introduce our own `reftable_buf` type to replace
> > `strbuf`. Get rid of the seldomly-used `strbuf_addf()` function such
> > that we have to reimplement one less function.
> 
> Hmm. I count twelve calls to strbuf_addf() here in this patch that were
> rewritten in terms of snprintf()ing to a temporary buffer. So I am not
> sure that I agree that it is "seldomly-used".
> 
> Sure, implementing fewer functions is nice, but I am not sure that
> forcing the caller to use snprintf() directly is necessarily a
> worthwhile trade-off.

Another problem here is that snprintf() isn't exactly the most portable
interface. Some systems don't have it at all, others have broken return
code handling. So avoiding it completely makes that issue go away
entirely.

I can add that to the commit message if this needs a reroll.

> Part of me wishes that we didn't have to write our own `reftable_buf` in
> the first place. Could we use `strbuf` as-is and expose it through a
> generic reftable-specific interface that users of reftable fill in with
> a vtable or something?

I tried that, and it felt way worse. The amount of code you have to
write is roughly in the same ballpark, you don't have pluggable
allocators, you don't have allocation error handling and every consumer
would have to implement their own type.

So overall it's only losses from my point of view.

Patrick