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

[Patrick Steinhardt <ps@pks.im>]

On Fri, Aug 16, 2024 at 12:06:47AM +0530, Chandra Pratap wrote:
> On Thu, 15 Aug 2024 at 15:11, Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Wed, Aug 14, 2024 at 05:33:16PM +0530, Chandra Pratap wrote:
> > > @@ -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.

> > > +     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?
> >
> > > +             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