Re: [PATCH 08/10] t-reftable-block: add tests for log blocks

On Fri, Aug 16, 2024 at 12:06:47AM +0530, Chandra Pratap wrote:

On Thu, 15 Aug 2024 at 15:11, Patrick Steinhardt [off-list ref] wrote:

quoted

On Wed, Aug 14, 2024 at 05:33:16PM +0530, Chandra Pratap wrote:

quoted

@@ -101,9 +101,95 @@ static void t_block_read_write(void)
              reftable_record_release(&recs[i]);
 }

+static void t_log_block_read_write(void)
+{
+     const int header_off = 21;
+     struct reftable_record recs[30];
+     const size_t N = ARRAY_SIZE(recs);
+     const size_t block_size = 2048;
+     struct reftable_block block = { 0 };
+     struct block_writer bw = {
+             .last_key = STRBUF_INIT,
+     };
+     struct reftable_record rec = {
+             .type = BLOCK_TYPE_LOG,
+     };
+     size_t i = 0;
+     int n;
+     struct block_reader br = { 0 };
+     struct block_iter it = BLOCK_ITER_INIT;
+     struct strbuf want = STRBUF_INIT;
+
+     REFTABLE_CALLOC_ARRAY(block.data, block_size);
+     block.len = block_size;
+     block.source = malloc_block_source();
+     block_writer_init(&bw, BLOCK_TYPE_LOG, block.data, block_size,
+                       header_off, hash_size(GIT_SHA1_FORMAT_ID));
+
+     for (i = 0; i < N; i++) {
+             rec.u.log.refname = xstrfmt("branch%02"PRIuMAX , (uintmax_t)i);
+             rec.u.log.update_index = i;
+             rec.u.log.value_type = REFTABLE_LOG_UPDATE;
+
+             recs[i] = rec;
+             n = block_writer_add(&bw, &rec);
+             rec.u.log.refname = NULL;
+             rec.u.log.value_type = REFTABLE_LOG_DELETION;
+             check_int(n, ==, 0);
+     }
+
+     n = block_writer_finish(&bw);
+     check_int(n, >, 0);

Do we maybe want to rename `n` to `ret`? That's way more customary in
our codebase.

Sure thing, but then I would want to change the existing test (which gets
renamed as t_ref_block_read_write) and I'm unsure of which patch would
be the most suitable for that change. Would it be fine to include that
change as a part of this patch?

The way I'd do it is to first do the minimum required changes to port
the old test to the new testing framework, without any of the cleanups
to align code style. Then I'd put another commit on top that does all
the changes like removing braces, converting types and also adapting
names.

quoted

quoted

+     block_writer_release(&bw);
+
+     block_reader_init(&br, &block, header_off, block_size, GIT_SHA1_RAWSZ);
+
+     block_iter_seek_start(&it, &br);
+
+     for (i = 0; ; i++) {
+             int r = block_iter_next(&it, &rec);
+             check_int(r, >=, 0);
+             if (r > 0)
+                     break;

We can also reuse `n` (or `ret`) here, right?

quoted

+             check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+     }

One thing that this loop doesn't verify is whether we actually got the
expected number of log records. It could be that the first iteration
already returns `r > 0`, which is not our expectation. So we should
likely add a check for `i == N` after the loop.

What about something like
if (r > 0) {
    check_int(i, ==, N);
    break;
}
That should achieve the same results if I'm not wrong.

Yup, looks good to me.

Patrick