Subject: Re: [PATCH v2 04/22] reftable/basics: handle allocation failures in `reftable_calloc()`

From: Junio C Hamano <[email-masked]>
Date: Thu, 26 Sep 2024 09:13:23 -0700

Patrick Steinhardt <[email-masked]> writes:

>> But it illustrates why open coding is not necessarily an excellent
>> idea in the longer term, doesn't it?  When unsigned_mult_overflows()
>> is updated to avoid such a false positive, how would we remember
>> that we need to update this copy we?
>
> I agree in general, but with the reftable library I'm stuck between a
> rock and a hard place. My goal is to make it fully reusable by other
> projects without first having to do surgery on their side. While having
> something like `st_mult()` is simple enough to port over, the biggest
> problem I have is that over time we sneak in more and more code from the
> Git codebase. The result is death by a thousand cuts.

> Now if we had a single header that exposes a small set of functions
> without _anything_ else it could work alright. But I rather doubt that
> we really want to have a standalone header for `st_mult()` that we can
> include. But without such a standalone header it is simply too easy to
> start building on top of Git features by accident.
>
> So I'm instead aiming to go a different way and fully cut the reftable
> code loose from Git. So even if we e.g. eventually want `struct strbuf`
> return errors on failure, it would only address one part of my problem.

The dependency to "strbuf" (just as an example) was added initially
fairly early.  Soon after 27f7ed2a (reftable: add LICENSE,
2021-10-07) added the reftable/ hierarchy, e303bf22 (reftable:
(de)serialization for the polymorphic record type., 2021-10-07).  I
somehow had an impression that reftable "library" started without
any Git dependency and then use of our helper functions seemed
through from the shim layer, but it was pretty much part of the
library from day one, it seems.

> A couple months ago I said that I'll try to write something like shims
> that wrap our types in reftable types such that other projects can
> provide implementations for such shims themselves. I tried to make that
> work, but the result was an unholy mess that really didn't make any
> sense whatsoever. Most of the features that I need from the Git codebase
> can be provided in a couple of lines of code (`struct strbuf` is roughly
> 50 lines for example), plus maybe a callback function that can be
> provided to wire things up on the user side (`register_tempfiles()` for
> example). So once I saw that those wrappers are harder to use and result
> in roughly the same lines of code I decided to scrap that approach and
> instead try to convert it fully.
>
> So yeah, overall we shouldn't open-code things like this. But I don't
> really see another way to do this for the reftable library.

But isn't all of the above what Libification ought to be about?  I
was hoping that the reftable polishing would not have to be done by
you alone, and you would recruit those who are into libification of
other parts of Git codebase to help cleaning up these fringe (from
the point of view of reftable) interfaces.

What do the libification folks feel about this (folks involved in
libgit-sys CC'ed; of course all others who are interested in the
libification topic are welcome to comment)?

Thanks.