Re: [PATCH v2 00/10] reftable: stop using `struct strbuf`

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 15, 2024, 10:44 UTC
Message-ID: <Zw5HntIyk_DLWESC@pks.im>
In-Reply-To: <Zw5E8d3AotDBYKSA@ArchLinux>

On Tue, Oct 15, 2024 at 06:33:21PM +0800, shejialuo wrote:

Show 41 quoted lines

> On Tue, Oct 15, 2024 at 06:37:37AM +0200, Patrick Steinhardt wrote:
> > On Mon, Oct 14, 2024 at 06:44:35PM -0400, Taylor Blau wrote:
> > > On Mon, Oct 14, 2024 at 03:02:16PM +0200, Patrick Steinhardt wrote:
> > > > Hi,
> > > >
> > > > this is the second part of my patch series that stop using `struct
> > > > strbuf` in the reftable library. This is done such that the reftable
> > > > library becomes standalone again and so that we can use the pluggable
> > > > allocators part of the library.
> > > 
> > > I reviewed this round, and it looks generally good to me. I feel
> > > somewhat unhappy to have to force the reftable backend to implement its
> > > own strbuf-like functionality.
> > > 
> > > So I think it may be worth considering whether or not we can reuse Git's
> > > strbuf implementation through a vtable or similar. But it may not be
> > > immediately possible since that implementation just die()s on error,
> > > can't easily swap out the allocator, etc. So perhaps this is the best
> > > path forward, it just feels somewhat unsatisfying to me.
> > 
> > It's not perfect, I agree. I initially tried to do something like a
> > vtable or to even compile this into code with something like a wrapper
> > structure. But that approach in the end fell flat. So I decided to be
> > pragmatic about this whole issue and just duplicate some code --
> > overall, we are talking about ~200 lines of code to completely detangle
> > the reftable library from libgit.a.
> > 
> 
> I have read some patches yesterday, I feel quite strange that we need to
> make repetition. Could we provide a header file which requires the users
> who need to use the reftable library to implement the interfaces?
> 
>     reftable_strbuf_addf(void *buf, char *fmt, va_list ap);
> 
> Thus, we could reuse "strbuf_addf" to implement this interface in Git.
> As for libgit2, could we let it implement these interfaces? Although I
> have never read the source code of libgit2, I think there should be some
> code which could be reuse to implement these interfaces?
> 
> However, I do not know the context. Maybe the above is totally wrong. If
> so, please ignore.

The thing is that we'll have repetition regardless of what we end up doing:

  - We could either have repetition once in the reftable library,
    reimplementing `struct strbuf`. This can then be reused by every
    single user of the reftable library.

  - Or we can have repetition for every single user of the reftable
    library. For now that'd only be Git and libgit2, but we'd still have
    repetition.

The second kind of repetition is way worse though, because now every user of the reftable library has a different implementation of a type that is as basic as a buffer. These _must_ behave the exact same across implementations or we will hit issues. So I'd rather have the repetition a single time in the reftable library such that all users of the library will behave the same rather than having downstream users copy the implementation of `struct strbuf` and making it work for their library.

Also, due to the nature of `struct strbuf` not allowing for allocation failures we'd already have diverging behaviour. In Git you would never hit error code paths for allocation failures, whereas every library user potentially can.

So we really have to treat the reftable code base special. If we want to be a good citizen and be a proper upstream for projects like libgit2 we don't really have much of a choice than to detangle it from libgit.a. If we don't we may be saving 20 lines of code, but we make everybody elses life harder.

Patrick