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

On Tue, Apr 29, 2025 at 06:53:02PM +0100, Seyi Kuforiji wrote:

quoted

Helper functions defined in `t/unit-tests/lib-reftable.{c,h}` are
required for the reftable-related test files to run efficeintly. In the
current implementation these functions are designed to conform with our
homegrown unit-testing structure. So in other to convert the reftable
test files, there is need for a clar specific implementation of these
helper functions.

type cast `for (size_t i = 0; i < (size_t)stats->ref_stats.blocks; i++)`
Adapt functions in lib-reftable.{c,h} to use clar. These functions
conform with the clar testing framework and become available for all
reftable-related test files implemented using the clar testing
framework, which requires them. This change migrates the helper
functions back into `lib-reftable.{c,h}`.

Signed-off-by: Seyi Kuforiji <redacted>
---
 Makefile                    |  4 +-
 t/meson.build               |  4 +-
 t/unit-tests/lib-reftable.c | 26 +++++------
 t/unit-tests/lib-reftable.h |  6 +--
 t/unit-tests/unit-test.c    | 93 -------------------------------------
 t/unit-tests/unit-test.h    | 16 -------
 6 files changed, 20 insertions(+), 129 deletions(-)

diff --git a/Makefile b/Makefile
index 0b42893611..7e646e16ee 100644
--- a/Makefile
+++ b/Makefile

@@ -1379,12 +1379,12 @@ CLAR_TEST_SUITES += u-urlmatch-normalization
 CLAR_TEST_PROG = $(UNIT_TEST_BIN)/unit-tests$(X)
 CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES))
 CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/clar/clar.o
-CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/unit-test.o
 CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/lib-oid.o
+CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/lib-reftable.o
+CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/unit-test.o

 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
-UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-reftable.o

`UNIT_TEST_PROGS` only contains the "unit-test" binary now, right? Does
this mean that we can simplify build rules in our Makefile now?

quoted

diff --git a/t/meson.build b/t/meson.build
index 8fa00fc9ef..7c305a90b5 100644
--- a/t/meson.build
+++ b/t/meson.build

@@ -26,8 +26,9 @@ clar_test_suites = [

 clar_sources = [
   'unit-tests/clar/clar.c',
+  'unit-tests/lib-oid.c',
+  'unit-tests/lib-reftable.c',
   'unit-tests/unit-test.c',
-  'unit-tests/lib-oid.c'
 ]

 clar_decls_h = custom_target(

@@ -69,7 +70,6 @@ foreach unit_test_program : unit_test_programs
   unit_test = executable(unit_test_name,
     sources: [
       'unit-tests/test-lib.c',
-      'unit-tests/lib-reftable.c',
       unit_test_program,
     ],
     dependencies: [libgit_commonmain],

Can't we remove this completely? `unit_test_programs` is empty now, so
this loop is a no-op now.

quoted

diff --git a/t/unit-tests/lib-reftable.h b/t/unit-tests/lib-reftable.h
index e4c360fa7e..2958db5dc0 100644
--- a/t/unit-tests/lib-reftable.h
+++ b/t/unit-tests/lib-reftable.h

@@ -6,12 +6,12 @@

 struct reftable_buf;

-void t_reftable_set_hash(uint8_t *p, int i, enum reftable_hash id);
+void cl_reftable_set_hash(uint8_t *p, int i, enum reftable_hash id);

-struct reftable_writer *t_reftable_strbuf_writer(struct reftable_buf *buf,
+struct reftable_writer *cl_reftable_strbuf_writer(struct reftable_buf *buf,
                                               struct reftable_write_options *opts);

-void t_reftable_write_to_buf(struct reftable_buf *buf,
+void cl_reftable_write_to_buf(struct reftable_buf *buf,
                           struct reftable_ref_record *refs,
                           size_t nrecords,
                           struct reftable_log_record *logs,

It is quite weird that we declare the replacement functions in
"unit-test.h" in the first commit only to remove them at a later point.
It would make way more sense if we introduced the functions in
"t/unit/lib-reftable.{c,h}" right from the start and then only remove
the unused functions in the last step.

Patrick

If I get it correctly, you're suggesting I have both the original
functions and the clar-based variant in `t/unit/lib-reftable.{c,h}`
right from the first commit, right?

Thanks
Seyi