On Fri, 2 May 2025 at 10:57, Patrick Steinhardt <ps@xxxxxx> wrote: > > On Tue, Apr 29, 2025 at 06:52:55PM +0100, Seyi Kuforiji wrote: > > diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c > > deleted file mode 100644 > > index 22040aeefa..0000000000 > > --- a/t/unit-tests/t-reftable-block.c > > +++ /dev/null > > Hm, why is this recorded as a delete and creation? Weird, inspecting the > diff locally properly shows it as a rename, which makes it a ton easier > to review. It would be great if you could try to play around with the > `--find-renames` option in the next iteration of this series and double > check that these are shown as a rename. > I used `--find-renames=90` for this patch series. Is there any recommended number to set it to? :/ > > diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c > > new file mode 100644 > > index 0000000000..af24901230 > > --- /dev/null > > +++ b/t/unit-tests/u-reftable-block.c > > @@ -0,0 +1,373 @@ > > +/* > > +Copyright 2020 Google LLC > > + > > +Use of this source code is governed by a BSD-style > > +license that can be found in the LICENSE file or at > > +https://developers.google.com/open-source/licenses/bsd > > +*/ > > + > > +#include "unit-test.h" > > +#include "reftable/block.h" > > +#include "reftable/blocksource.h" > > +#include "reftable/constants.h" > > +#include "reftable/reftable-error.h" > > +#include "strbuf.h" > > + > > +void test_reftable_block__index_read_write(void) > > This doesn't got to do anything with indices but with refs, so I'd > rename this to `__ref_read_write()`. > > > +{ > > + const int header_off = 21; /* random */ > > + struct reftable_record recs[30]; > > + const size_t N = ARRAY_SIZE(recs); > > + const size_t block_size = 1024; > > + struct reftable_block block = { 0 }; > > + struct block_writer bw = { > > + .last_key = REFTABLE_BUF_INIT, > > + }; > > + struct reftable_record rec = { > > + .type = BLOCK_TYPE_REF, > > + }; > > + size_t i = 0; > > + int ret; > > + struct block_reader br = { 0 }; > > + struct block_iter it = BLOCK_ITER_INIT; > > + struct reftable_buf want = REFTABLE_BUF_INIT, buf = REFTABLE_BUF_INIT; > > + > > + REFTABLE_CALLOC_ARRAY(block.data, block_size); > > + cl_assert(block.data != NULL); > > + block.len = block_size; > > + block_source_from_buf(&block.source ,&buf); > > + ret = block_writer_init(&bw, BLOCK_TYPE_REF, block.data, block_size, > > + header_off, hash_size(REFTABLE_HASH_SHA1)); > > + cl_assert(ret == 0); > > Same comment here, asserts like this can be retained as > `cl_assert(!ret)`. > ohh, I didn't know this was possible. > > + rec.u.ref.refname = (char *) ""; > > + rec.u.ref.value_type = REFTABLE_REF_DELETION; > > + ret = block_writer_add(&bw, &rec); > > + cl_assert_equal_i(ret, REFTABLE_API_ERROR); > > + > > + for (i = 0; i < N; i++) { > > + rec.u.ref.refname = xstrfmt("branch%02"PRIuMAX, (uintmax_t)i); > > + rec.u.ref.value_type = REFTABLE_REF_VAL1; > > + memset(rec.u.ref.value.val1, i, REFTABLE_HASH_SIZE_SHA1); > > + > > + recs[i] = rec; > > + ret = block_writer_add(&bw, &rec); > > + rec.u.ref.refname = NULL; > > + rec.u.ref.value_type = REFTABLE_REF_DELETION; > > + cl_assert_equal_i(ret, 0); > > + } > > + > > + ret = block_writer_finish(&bw); > > + cl_assert(ret > 0); > > It's a bit unfortunate that we have to use `cl_assert()` here, but that > isn't the fault of this series. I do have a pull request pending > upstream that introduces integer comparisons. Once we've updated to that > version I'll go through our unit tests and adapt callsites accordingly. > > [snip] > > +void test_reftable_block__ref_read_write(void) > > This one here should be called `__index_read_write()`. I guess you > confused the first and and this test name with one another. > > Patrick Ah, okay, Thanks! Best Seyi
