Patrick Steinhardt <ps@xxxxxx> writes:
> 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.
>
Indeed, good catch, will fix.
>> 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
Yeah, that does make sense, will add and send in a new version. Thanks
for the quick review!
Karthik
Attachment:
signature.asc
Description: PGP signature
