On Fri, 2 May 2025 at 10:57, Patrick Steinhardt [off-list ref] wrote:

On Tue, Apr 29, 2025 at 06:52:55PM +0100, Seyi Kuforiji wrote:

quoted

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? :/

quoted

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()`.

quoted

+{
+     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.

quoted

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

quoted

+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