| Message ID | 5e7cbb7b193c578f7c946a5077a79421b0ac57f2.1715336798.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reftable: expose write options as config | expand |
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.
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
diff --git a/reftable/writer.c b/reftable/writer.c index 7df6e53699..374b7d15ed 100644 --- a/reftable/writer.c +++ b/reftable/writer.c @@ -125,10 +125,8 @@ reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t), struct reftable_writer *wp = reftable_calloc(1, sizeof(*wp)); options_set_defaults(&opts); - if (opts.block_size >= (1 << 24)) { - /* TODO - error return? */ - abort(); - } + if (opts.block_size >= (1 << 24)) + BUG("configured block size exceeds 16MB"); strbuf_init(&wp->block_writer_data.last_key, 0); strbuf_init(&wp->last_key, 0);
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. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- reftable/writer.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)