Re: [PATCH 3/3] reftable: prevent 'update_index' changes after header write

[Patrick Steinhardt <ps@pks.im>]

On Fri, Jan 17, 2025 at 08:59:14AM +0100, Karthik Nayak wrote:
> 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.

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.

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

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

> +	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".

Patrick