Patrick Steinhardt [off-list ref] writes:

On Fri, Jan 17, 2025 at 08:59:14AM +0100, Karthik Nayak wrote:

quoted

diff --git a/reftable/writer.c b/reftable/writer.c
index 740c98038eaf883258bef4988f78977ac7e4a75a..c602b873543790e36178f797ed9f98112671f97f 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c

@@ -182,6 +182,13 @@ int reftable_writer_new(struct reftable_writer **out,
 void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,
 				uint64_t max)
 {
+	/*
+	 * The limits shouldn't be modified post writing the first block, else
+	 * it would cause a mismatch between the header and the footer.
+	 */

Can we make this *even* stricter? I think that this is something that is
easy to do wrong, and the fact that it only triggers in some situations
of misuse may easily make tests miss this issue. So ideally, we should
assert that `set_limits()` is always called before queueing any records
to the writer. This would make us error out in all situations where the
calling order is wrong.

I agree here, it makes sense to make this stricter. Like you mentioned,
currently they are independent. The only way to enforce the limits is to
ensure that they are dependent.

There are two ways I can see us doing that:

  - Detect any state written by `writer_add_record()` and error out if
    it's set when `reftable_writer_set_limits()` is called.

Yeah I think this would be simple to do. I guess we can check
`w->last_key` is set, since any record write would modify that.

  - Adapt `reftable_writer_new()` so that it takes the update indices as
    input and drop `reftable_writer_set_limits()` altogether.

This one is a bit harder to do because of our flow. Generally the writer
is provided to callers via a callback function passed to
`reftable_addition_add()`. I guess I could simply pass the data:

  caller -> reftable_addition_add() -> reftable_writer_new()

Any direct users of `reftable_writer_new()` would simply pass the data
directly.

I'll play around and see if this is doable without too much refactoring
and have something in the next version.

The latter might be preferable as you basically want to set limits in
all (most?) situations anyway.

quoted

+	if (w->next)
+		BUG("update index modified after writing first block");

Let's not use BUG, but rather return a `REFTABLE_API_ERROR` error. It
requires a bit more plumbing because we'll also hvae to adapt all
callers to handle errors. But on the one hand we don't want to die in
library code. And on the other hand we don't want to keep on adding more
dependencies on "git-compat-util.h".

Fair enough, thanks for explaining.

Patrick