Re: [PATCH v2 04/11] reftable/writer: improve error when passed an invalid block size

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1863 bytes --]

On Fri, May 10, 2024 at 02:25:28PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > The reftable format only supports block sizes up to 16MB. When the
> > writer is being passed a value bigger than that it simply calls
> > abort(3P), which isn't all that helpful due to the lack of a proper
> > error message.
> >
> > Improve this by calling `BUG()` instead.
> 
> As a "git" person, I do not mind this at all.
> 
> But doesn't the reftable/ library codebase want to avoid things like
> BUG() that are very much tied to our codebase, for the same reason
> as it avoids things like xmalloc(), xcalloc(), and ALLOC_GROW()?
> 
> We may have crossed the bridge long time ago, though.  We see a
> handful calls to BUG() already inside reftable/ directory.

Exactly. No matter what, once there will be a second user of the
reftable library we will have to figure out a maintainable way to ensure
that the library can be used by other projects, too. And that will
require some larger refactorings anyway.

I think initially, the intent was to have a "system.h" header that
contains a bunch of wrappers that bridge the gap between reftables and
Git. I feel like this abstraction does not make any sense though in its
current form as it is simply being included by the reftable code, which
then uses the Git functions directly.

I think eventually, we will have to adapt this such that the Git
includes do not leak into the reftable code at all. Instead, we should
have a shim "system.c" that carries the Git-specific includes and then
implements a couple of wrapper functions. "system.h" would then only be
carrying function declarations of those wrappers.

That's a larger topic though, and I think that tackling this now would
be premature without any potential users of the reftable library.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]