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

On Tue, Oct 15, 2024 at 06:37:37AM +0200, Patrick Steinhardt wrote:

quoted

On Mon, Oct 14, 2024 at 06:44:35PM -0400, Taylor Blau wrote:

quoted

On Mon, Oct 14, 2024 at 03:02:16PM +0200, Patrick Steinhardt wrote:

quoted

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