On Thu, Dec 05, 2024 at 04:49:57PM +0100, Karthik Nayak wrote:
> Each reftable addition has an associated update_index. While writing
> refs, the update_index is verified to be within the range of the
> reftable writer, i.e. `writer.min_update_index < ref.update_index` and
> `writer.max_update_index > ref.update_index`.
These should probably be `<=` and `>=`, respectively.
> diff --git a/reftable/writer.c b/reftable/writer.c
> index fd136794d5a27b33b5017f36fbd6b095ab8dac5b..f87086777cd20a9890a63f10c5d6932310dd5610 100644
> --- a/reftable/writer.c
> +++ b/reftable/writer.c
> @@ -412,6 +412,18 @@ int reftable_writer_add_log(struct reftable_writer *w,
> if (log->value_type == REFTABLE_LOG_DELETION)
> return reftable_writer_add_log_verbatim(w, log);
>
> + /*
> + * Verify only the upper limit of the update_index. Each reflog entry
> + * is tied to a specific update_index. Entries in the reflog can be
> + * replaced by adding a new entry with the same update_index,
> + * effectively canceling the old one.
> + *
> + * Consequently, reflog updates may include update_index values lower
> + * than the writer's min_update_index.
> + */
> + if (log->update_index > w->max_update_index)
> + return REFTABLE_API_ERROR;
Yup, looks sensible.
> if (!log->refname)
> return REFTABLE_API_ERROR;
>
> diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c
> index d279b86df0aeda11b3fb4d2c15803760ae394941..5ad1c72f6901abcfe7fdc6c3e69e26b58d0013a6 100644
> --- a/t/unit-tests/t-reftable-readwrite.c
> +++ b/t/unit-tests/t-reftable-readwrite.c
> @@ -151,6 +151,45 @@ static void t_log_overflow(void)
> reftable_buf_release(&buf);
> }
>
> +static void t_log_write_limits(void)
> +{
> + struct reftable_write_options opts = { 0 };
> + struct reftable_buf buf = REFTABLE_BUF_INIT;
> + struct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);
> + struct reftable_log_record log = {
> + .refname = (char *)"refs/head/master",
> + .update_index = 1,
> + .value_type = REFTABLE_LOG_UPDATE,
> + .value = {
> + .update = {
> + .old_hash = { 1 },
> + .new_hash = { 2 },
> + .name = (char *)"Han-Wen Nienhuys",
> + .email = (char *)"hanwen@xxxxxxxxxx",
> + .tz_offset = 100,
> + .time = 0x5e430672,
> + },
> + },
> + };
> + int err;
> +
> + reftable_writer_set_limits(w, 1, 2);
> +
> + err = reftable_writer_add_log(w, &log);
> + check_int(err, ==, 0);
> +
> + log.update_index = 2;
> + err = reftable_writer_add_log(w, &log);
> + check_int(err, ==, 0);
> +
> + log.update_index = 3;
> + err = reftable_writer_add_log(w, &log);
> + check_int(err, ==, REFTABLE_API_ERROR);
> +
> + reftable_writer_free(w);
> + reftable_buf_release(&buf);
> +}
Makes sense, as well. We could trivially extend this test to also assert
that we can successfully write a log record with update index 0, which
would be smaller than the lower bound.
Patrick
