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

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 27, 2024, 05:28 UTC
Message-ID: <ZvZCdcpifMpmKajx@pks.im>
In-Reply-To: <xmqqzfnul7fg.fsf@gitster.g>

On Thu, Sep 26, 2024 at 09:13:23AM -0700, Junio C Hamano wrote:

Show 32 quoted lines

> Patrick Steinhardt <ps@pks.im> 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.

I think the history goes a bit different. Initially, the reftable library was developed completely outside of the Git tree in [1]. It did not have any external dependencies and didn't use any of the Git code.

During the upstreaming process the discussion rather quickly swayed into the direction of making Git the canonical upstream instead of using a separate repository that hosts the code. See e.g. [2], which was still tracking a VERSION file to keep track of the upstream version that the code was pulled from. In [3] the discussion started about upstream requiring a CLA, which was an (understandable) non-starter. I cannot find the statement, but eventually we decided that canonical upstream should be Git, and the VERSION file was never committed.

But with that decision, now that the reftable library was part of Git, the reviews also started to note that it really should use functions provided by Git. The initial version of the patch series didn't yet have any of that. From my point of view that was a big mistake, as the result is that Git is _not_ reusable by other projects in its current state.

[1]: https://github.com/google/reftable/tree/master/c [2]: <2106ff286b1135f9428529d9fc392edc127e960c.1580134944.git.gitgitgadget@gmail.com> [3]: <20200207001612.GF6573@camp.crustytoothpaste.net>

Show 24 quoted lines

> > 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)?

I think it's orthogonal to that. The libification effort doesn't really care about exposing low-level details like `st_mult()` as a library, or even the `strbuf` interface. It cares about making Git functionality available, which is on a much higher conceptual level.

Furthermore, to the best of my knowledge the intent wasn't ever to provide the ability to take a couple of C files and have them be easy to build as part of another project. The goal is to have proper interfaces to our code, but whether the code itself has internal interdependencies is not really much of a concern (globals are a different story).

My goal is different: I want to `cp -r` the "reftable/" directory into libgit2 or any other project that wants to implement a reftable backend without having to care about all the different dependencies that it may pull in or having to carefully massage all of the includes.

Now if it was a single standalone file that one would have to copy in addition to that I'd be fine. But "strbuf.c' isn't that at all: it ropes in "gettext.h", "hex-ll.h", "string-list.h", "utf-8.h", "date.h". All of which is not needed at all, but the person who does the porting now has to carefully figure out what can and cannot be removed from both "strbuf.h" and "strbuf.c".

So... it gets out of hand really fast. We would have to significantly change the way we develop the Git codebase if we wanted to make this a good experience. But enforcing very strict coding guidelines onto all of our codebase doesn't feel reasonable to me, which is why I am instead trying to reinstate the state where the reftable library was decoupled from the rest of the Git codebase.

The difference here is roughly 100 to 200 lines of code, which I don't think is much of a maintenance burden by itself. In fact, we'll even start to share the maintenance burden with libgit2, because they would be able to use the exact same copy as we do and thus contribute back bugfixes and improvements for discoveries that their additional test coverage brings us.

I hope that this all clarifies my point of view :) I've also Cc'd Han-Wen in case I've made any mistakes regarding the original intent.

Patrick