[GSoC][PATCH 0/5] t: port reftable/merged_test.c to the unit testing framework

[GSoC][PATCH 0/5] t: port reftable/merged_test.c to the unit testing framework

[Chandra Pratap]

In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit
testing framework written entirely in C was introduced to the Git project
aimed at simplifying testing and reducing test run times.
Currently, tests for the reftable refs-backend are performed by a custom
testing framework defined by reftable/test_framework.{c, h}. Port
reftable/merged_test.c to the unit testing framework and improve upon
the ported test.

The first patch in the series moves the test to the unit testing framework,
and the rest of the patches improve upon the ported test.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>

---
CI/PR: https://github.com/gitgitgadget/git/pull/1755

Chandra Pratap (5):
[PATCH 1/5] t: move reftable/merged_test.c to the unit testing framework
[PATCH 2/5] t: harmonize t-reftable-merged.c with coding guidelines
[PATCH 3/5] t-reftable-merged: add test for reftable_merged_table_max_update_index()
[PATCH 4/5] t-reftable-merged: use reftable_ref_record_equal to compare ref records
[PATCH 5/5] t-reftable-merged: add test for REFTABLE_FORMAT_ERROR

Makefile                                                   |   2 +-
t/helper/test-reftable.c                                   |   1 -
reftable/merged_test.c => t/unit-tests/t-reftable-merged.c | 170 +++++++++++++++----------------
3 files changed, 86 insertions(+), 87 deletions(-)

[PATCH 1/5] t: move reftable/merged_test.c to the unit testing framework

[Chandra Pratap]

reftable/merged_test.c exercises the functions defined in
reftable/merged.{c, h}. Migrate reftable/merged_test.c to the unit
testing framework. Migration involves refactoring the tests
to use the unit testing framework instead of reftable's test
framework and renaming the tests according to unit-tests' naming
conventions.

Also, move strbuf_add_void() and noop_flush() from
reftable/test_framework.c to the ported test. This is because
both these functions are used in the merged tests and
reftable/test_framework.{c, h} is not #included in the ported test.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 Makefile                                      |   2 +-
 t/helper/test-reftable.c                      |   1 -
 .../unit-tests/t-reftable-merged.c            | 112 +++++++++---------
 3 files changed, 60 insertions(+), 55 deletions(-)
 rename reftable/merged_test.c => t/unit-tests/t-reftable-merged.c (84%)

diff --git a/Makefile b/Makefile
index 3eab701b10..e5d1b53991 100644
--- a/Makefile
+++ b/Makefile
@@ -1340,6 +1340,7 @@ UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-oidtree
 UNIT_TEST_PROGRAMS += t-prio-queue
 UNIT_TEST_PROGRAMS += t-reftable-basics
+UNIT_TEST_PROGRAMS += t-reftable-merged
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-strcmp-offset
 UNIT_TEST_PROGRAMS += t-strvec
@@ -2679,7 +2680,6 @@ REFTABLE_OBJS += reftable/writer.o
 
 REFTABLE_TEST_OBJS += reftable/block_test.o
 REFTABLE_TEST_OBJS += reftable/dump.o
-REFTABLE_TEST_OBJS += reftable/merged_test.o
 REFTABLE_TEST_OBJS += reftable/pq_test.o
 REFTABLE_TEST_OBJS += reftable/record_test.o
 REFTABLE_TEST_OBJS += reftable/readwrite_test.o
diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
index 9160bc5da6..0357718fa8 100644
--- a/t/helper/test-reftable.c
+++ b/t/helper/test-reftable.c
@@ -10,7 +10,6 @@ int cmd__reftable(int argc, const char **argv)
 	tree_test_main(argc, argv);
 	pq_test_main(argc, argv);
 	readwrite_test_main(argc, argv);
-	merged_test_main(argc, argv);
 	stack_test_main(argc, argv);
 	return 0;
 }
diff --git a/reftable/merged_test.c b/t/unit-tests/t-reftable-merged.c
similarity index 84%
rename from reftable/merged_test.c
rename to t/unit-tests/t-reftable-merged.c
index a9d6661c13..1718489f06 100644
--- a/reftable/merged_test.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -6,20 +6,25 @@ license that can be found in the LICENSE file or at
 https://developers.google.com/open-source/licenses/bsd
 */
 
-#include "merged.h"
-
-#include "system.h"
+#include "test-lib.h"
+#include "reftable/blocksource.h"
+#include "reftable/constants.h"
+#include "reftable/merged.h"
+#include "reftable/reader.h"
+#include "reftable/reftable-generic.h"
+#include "reftable/reftable-merged.h"
+#include "reftable/reftable-writer.h"
+
+static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)
+{
+	strbuf_add(b, data, sz);
+	return sz;
+}
 
-#include "basics.h"
-#include "blocksource.h"
-#include "constants.h"
-#include "reader.h"
-#include "record.h"
-#include "test_framework.h"
-#include "reftable-merged.h"
-#include "reftable-tests.h"
-#include "reftable-generic.h"
-#include "reftable-writer.h"
+static int noop_flush(void *arg)
+{
+	return 0;
+}
 
 static void write_test_table(struct strbuf *buf,
 			     struct reftable_ref_record refs[], int n)
@@ -49,12 +54,12 @@ static void write_test_table(struct strbuf *buf,
 	for (i = 0; i < n; i++) {
 		uint64_t before = refs[i].update_index;
 		int n = reftable_writer_add_ref(w, &refs[i]);
-		EXPECT(n == 0);
-		EXPECT(before == refs[i].update_index);
+		check_int(n, ==, 0);
+		check_int(before, ==, refs[i].update_index);
 	}
 
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_writer_free(w);
 }
@@ -76,11 +81,11 @@ static void write_test_log_table(struct strbuf *buf,
 
 	for (i = 0; i < n; i++) {
 		int err = reftable_writer_add_log(w, &logs[i]);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_writer_free(w);
 }
@@ -105,12 +110,12 @@ merged_table_from_records(struct reftable_ref_record **refs,
 
 		err = reftable_new_reader(&(*readers)[i], &(*source)[i],
 					  "name");
-		EXPECT_ERR(err);
+		check(!err);
 		reftable_table_from_reader(&tabs[i], (*readers)[i]);
 	}
 
 	err = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
-	EXPECT_ERR(err);
+	check(!err);
 	return mt;
 }
 
@@ -122,7 +127,7 @@ static void readers_destroy(struct reftable_reader **readers, size_t n)
 	reftable_free(readers);
 }
 
-static void test_merged_between(void)
+static void t_merged_between(void)
 {
 	struct reftable_ref_record r1[] = { {
 		.refname = (char *) "b",
@@ -150,11 +155,11 @@ static void test_merged_between(void)
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
 	err = reftable_iterator_seek_ref(&it, "a");
-	EXPECT_ERR(err);
+	check(!err);
 
 	err = reftable_iterator_next_ref(&it, &ref);
-	EXPECT_ERR(err);
-	EXPECT(ref.update_index == 2);
+	check(!err);
+	check_int(ref.update_index, ==, 2);
 	reftable_ref_record_release(&ref);
 	reftable_iterator_destroy(&it);
 	readers_destroy(readers, 2);
@@ -165,7 +170,7 @@ static void test_merged_between(void)
 	reftable_free(bs);
 }
 
-static void test_merged(void)
+static void t_merged(void)
 {
 	struct reftable_ref_record r1[] = {
 		{
@@ -230,9 +235,9 @@ static void test_merged(void)
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
 	err = reftable_iterator_seek_ref(&it, "a");
-	EXPECT_ERR(err);
-	EXPECT(reftable_merged_table_hash_id(mt) == GIT_SHA1_FORMAT_ID);
-	EXPECT(reftable_merged_table_min_update_index(mt) == 1);
+	check(!err);
+	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
+	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
 
 	while (len < 100) { /* cap loops/recursion. */
 		struct reftable_ref_record ref = { NULL };
@@ -245,9 +250,9 @@ static void test_merged(void)
 	}
 	reftable_iterator_destroy(&it);
 
-	EXPECT(ARRAY_SIZE(want) == len);
+	check_int(ARRAY_SIZE(want), ==, len);
 	for (i = 0; i < len; i++) {
-		EXPECT(reftable_ref_record_equal(want[i], &out[i],
+		check(reftable_ref_record_equal(want[i], &out[i],
 						 GIT_SHA1_RAWSZ));
 	}
 	for (i = 0; i < len; i++) {
@@ -283,16 +288,16 @@ merged_table_from_log_records(struct reftable_log_record **logs,
 
 		err = reftable_new_reader(&(*readers)[i], &(*source)[i],
 					  "name");
-		EXPECT_ERR(err);
+		check(!err);
 		reftable_table_from_reader(&tabs[i], (*readers)[i]);
 	}
 
 	err = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
-	EXPECT_ERR(err);
+	check(!err);
 	return mt;
 }
 
-static void test_merged_logs(void)
+static void t_merged_logs(void)
 {
 	struct reftable_log_record r1[] = {
 		{
@@ -362,9 +367,9 @@ static void test_merged_logs(void)
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
 	err = reftable_iterator_seek_log(&it, "a");
-	EXPECT_ERR(err);
-	EXPECT(reftable_merged_table_hash_id(mt) == GIT_SHA1_FORMAT_ID);
-	EXPECT(reftable_merged_table_min_update_index(mt) == 1);
+	check(!err);
+	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
+	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
 
 	while (len < 100) { /* cap loops/recursion. */
 		struct reftable_log_record log = { NULL };
@@ -377,19 +382,19 @@ static void test_merged_logs(void)
 	}
 	reftable_iterator_destroy(&it);
 
-	EXPECT(ARRAY_SIZE(want) == len);
+	check_int(ARRAY_SIZE(want), ==, len);
 	for (i = 0; i < len; i++) {
-		EXPECT(reftable_log_record_equal(want[i], &out[i],
+		check(reftable_log_record_equal(want[i], &out[i],
 						 GIT_SHA1_RAWSZ));
 	}
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
 	err = reftable_iterator_seek_log_at(&it, "a", 2);
-	EXPECT_ERR(err);
+	check(!err);
 	reftable_log_record_release(&out[0]);
 	err = reftable_iterator_next_log(&it, &out[0]);
-	EXPECT_ERR(err);
-	EXPECT(reftable_log_record_equal(&out[0], &r3[0], GIT_SHA1_RAWSZ));
+	check(!err);
+	check(reftable_log_record_equal(&out[0], &r3[0], GIT_SHA1_RAWSZ));
 	reftable_iterator_destroy(&it);
 
 	for (i = 0; i < len; i++) {
@@ -405,7 +410,7 @@ static void test_merged_logs(void)
 	reftable_free(bs);
 }
 
-static void test_default_write_opts(void)
+static void t_default_write_opts(void)
 {
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
@@ -426,23 +431,23 @@ static void test_default_write_opts(void)
 	reftable_writer_set_limits(w, 1, 1);
 
 	err = reftable_writer_add_ref(w, &rec);
-	EXPECT_ERR(err);
+	check(!err);
 
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
+	check(!err);
 	reftable_writer_free(w);
 
 	block_source_from_strbuf(&source, &buf);
 
 	err = reftable_new_reader(&rd, &source, "filename");
-	EXPECT_ERR(err);
+	check(!err);
 
 	hash_id = reftable_reader_hash_id(rd);
-	EXPECT(hash_id == GIT_SHA1_FORMAT_ID);
+	check_int(hash_id, ==, GIT_SHA1_FORMAT_ID);
 
 	reftable_table_from_reader(&tab[0], rd);
 	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA1_FORMAT_ID);
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_free(rd);
 	reftable_merged_table_free(merged);
@@ -451,11 +456,12 @@ static void test_default_write_opts(void)
 
 /* XXX test refs_for(oid) */
 
-int merged_test_main(int argc, const char *argv[])
+int cmd_main(int argc, const char *argv[])
 {
-	RUN_TEST(test_merged_logs);
-	RUN_TEST(test_merged_between);
-	RUN_TEST(test_merged);
-	RUN_TEST(test_default_write_opts);
-	return 0;
+	TEST(t_merged_logs(), "merged table with log records");
+	TEST(t_merged_between(), "seek ref in a merged table");
+	TEST(t_merged(), "merged table with multiple updates to same ref");
+	TEST(t_default_write_opts(), "merged table with default write opts");
+
+	return test_done();
 }
-- 
2.45.2.404.g9eaef5822c

Re: [PATCH 1/5] t: move reftable/merged_test.c to the unit testing framework

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 11509 bytes --]

Chandra Pratap <chandrapratap3519@gmail.com> writes:

> reftable/merged_test.c exercises the functions defined in
> reftable/merged.{c, h}. Migrate reftable/merged_test.c to the unit
> testing framework. Migration involves refactoring the tests
> to use the unit testing framework instead of reftable's test
> framework and renaming the tests according to unit-tests' naming
> conventions.
>
> Also, move strbuf_add_void() and noop_flush() from
> reftable/test_framework.c to the ported test. This is because
> both these functions are used in the merged tests and
> reftable/test_framework.{c, h} is not #included in the ported test.
>
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
>  Makefile                                      |   2 +-
>  t/helper/test-reftable.c                      |   1 -
>  .../unit-tests/t-reftable-merged.c            | 112 +++++++++---------
>  3 files changed, 60 insertions(+), 55 deletions(-)
>  rename reftable/merged_test.c => t/unit-tests/t-reftable-merged.c (84%)
>
> diff --git a/Makefile b/Makefile
> index 3eab701b10..e5d1b53991 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1340,6 +1340,7 @@ UNIT_TEST_PROGRAMS += t-mem-pool
>  UNIT_TEST_PROGRAMS += t-oidtree
>  UNIT_TEST_PROGRAMS += t-prio-queue
>  UNIT_TEST_PROGRAMS += t-reftable-basics
> +UNIT_TEST_PROGRAMS += t-reftable-merged
>  UNIT_TEST_PROGRAMS += t-strbuf
>  UNIT_TEST_PROGRAMS += t-strcmp-offset
>  UNIT_TEST_PROGRAMS += t-strvec
> @@ -2679,7 +2680,6 @@ REFTABLE_OBJS += reftable/writer.o
>
>  REFTABLE_TEST_OBJS += reftable/block_test.o
>  REFTABLE_TEST_OBJS += reftable/dump.o
> -REFTABLE_TEST_OBJS += reftable/merged_test.o
>  REFTABLE_TEST_OBJS += reftable/pq_test.o
>  REFTABLE_TEST_OBJS += reftable/record_test.o
>  REFTABLE_TEST_OBJS += reftable/readwrite_test.o
> diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
> index 9160bc5da6..0357718fa8 100644
> --- a/t/helper/test-reftable.c
> +++ b/t/helper/test-reftable.c
> @@ -10,7 +10,6 @@ int cmd__reftable(int argc, const char **argv)
>  	tree_test_main(argc, argv);
>  	pq_test_main(argc, argv);
>  	readwrite_test_main(argc, argv);
> -	merged_test_main(argc, argv);
>  	stack_test_main(argc, argv);
>  	return 0;
>  }
> diff --git a/reftable/merged_test.c b/t/unit-tests/t-reftable-merged.c
> similarity index 84%
> rename from reftable/merged_test.c
> rename to t/unit-tests/t-reftable-merged.c
> index a9d6661c13..1718489f06 100644
> --- a/reftable/merged_test.c
> +++ b/t/unit-tests/t-reftable-merged.c
> @@ -6,20 +6,25 @@ license that can be found in the LICENSE file or at
>  https://developers.google.com/open-source/licenses/bsd
>  */
>
> -#include "merged.h"
> -
> -#include "system.h"
> +#include "test-lib.h"
> +#include "reftable/blocksource.h"
> +#include "reftable/constants.h"
> +#include "reftable/merged.h"
> +#include "reftable/reader.h"
> +#include "reftable/reftable-generic.h"
> +#include "reftable/reftable-merged.h"
> +#include "reftable/reftable-writer.h"
> +
> +static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)
> +{
> +	strbuf_add(b, data, sz);
> +	return sz;
> +}
>
> -#include "basics.h"
> -#include "blocksource.h"
> -#include "constants.h"
> -#include "reader.h"
> -#include "record.h"
> -#include "test_framework.h"
> -#include "reftable-merged.h"
> -#include "reftable-tests.h"
> -#include "reftable-generic.h"
> -#include "reftable-writer.h"
> +static int noop_flush(void *arg)
> +{
> +	return 0;
> +}
>
>  static void write_test_table(struct strbuf *buf,
>  			     struct reftable_ref_record refs[], int n)
> @@ -49,12 +54,12 @@ static void write_test_table(struct strbuf *buf,
>  	for (i = 0; i < n; i++) {
>  		uint64_t before = refs[i].update_index;
>  		int n = reftable_writer_add_ref(w, &refs[i]);
> -		EXPECT(n == 0);
> -		EXPECT(before == refs[i].update_index);
> +		check_int(n, ==, 0);
> +		check_int(before, ==, refs[i].update_index);
>  	}
>
>  	err = reftable_writer_close(w);
> -	EXPECT_ERR(err);
> +	check(!err);
>

Nit: Couldn't we just do `check(!reftable_writer_close(w))` here and in
a lot of places below?

>  	reftable_writer_free(w);
>  }
> @@ -76,11 +81,11 @@ static void write_test_log_table(struct strbuf *buf,
>
>  	for (i = 0; i < n; i++) {
>  		int err = reftable_writer_add_log(w, &logs[i]);
> -		EXPECT_ERR(err);
> +		check(!err);
>  	}
>
>  	err = reftable_writer_close(w);
> -	EXPECT_ERR(err);
> +	check(!err);
>
>  	reftable_writer_free(w);
>  }
> @@ -105,12 +110,12 @@ merged_table_from_records(struct reftable_ref_record **refs,
>
>  		err = reftable_new_reader(&(*readers)[i], &(*source)[i],
>  					  "name");
> -		EXPECT_ERR(err);
> +		check(!err);
>  		reftable_table_from_reader(&tabs[i], (*readers)[i]);
>  	}
>
>  	err = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
> -	EXPECT_ERR(err);
> +	check(!err);
>  	return mt;
>  }
>
> @@ -122,7 +127,7 @@ static void readers_destroy(struct reftable_reader **readers, size_t n)
>  	reftable_free(readers);
>  }
>
> -static void test_merged_between(void)
> +static void t_merged_between(void)
>  {
>  	struct reftable_ref_record r1[] = { {
>  		.refname = (char *) "b",
> @@ -150,11 +155,11 @@ static void test_merged_between(void)
>
>  	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
>  	err = reftable_iterator_seek_ref(&it, "a");
> -	EXPECT_ERR(err);
> +	check(!err);
>
>  	err = reftable_iterator_next_ref(&it, &ref);
> -	EXPECT_ERR(err);
> -	EXPECT(ref.update_index == 2);
> +	check(!err);
> +	check_int(ref.update_index, ==, 2);
>  	reftable_ref_record_release(&ref);
>  	reftable_iterator_destroy(&it);
>  	readers_destroy(readers, 2);
> @@ -165,7 +170,7 @@ static void test_merged_between(void)
>  	reftable_free(bs);
>  }
>
> -static void test_merged(void)
> +static void t_merged(void)
>  {
>  	struct reftable_ref_record r1[] = {
>  		{
> @@ -230,9 +235,9 @@ static void test_merged(void)
>
>  	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
>  	err = reftable_iterator_seek_ref(&it, "a");
> -	EXPECT_ERR(err);
> -	EXPECT(reftable_merged_table_hash_id(mt) == GIT_SHA1_FORMAT_ID);
> -	EXPECT(reftable_merged_table_min_update_index(mt) == 1);
> +	check(!err);
> +	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
> +	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
>
>  	while (len < 100) { /* cap loops/recursion. */
>  		struct reftable_ref_record ref = { NULL };
> @@ -245,9 +250,9 @@ static void test_merged(void)
>  	}
>  	reftable_iterator_destroy(&it);
>
> -	EXPECT(ARRAY_SIZE(want) == len);
> +	check_int(ARRAY_SIZE(want), ==, len);
>  	for (i = 0; i < len; i++) {
> -		EXPECT(reftable_ref_record_equal(want[i], &out[i],
> +		check(reftable_ref_record_equal(want[i], &out[i],
>  						 GIT_SHA1_RAWSZ));
>  	}
>  	for (i = 0; i < len; i++) {
> @@ -283,16 +288,16 @@ merged_table_from_log_records(struct reftable_log_record **logs,
>
>  		err = reftable_new_reader(&(*readers)[i], &(*source)[i],
>  					  "name");
> -		EXPECT_ERR(err);
> +		check(!err);
>  		reftable_table_from_reader(&tabs[i], (*readers)[i]);
>  	}
>
>  	err = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
> -	EXPECT_ERR(err);
> +	check(!err);
>  	return mt;
>  }
>
> -static void test_merged_logs(void)
> +static void t_merged_logs(void)
>  {
>  	struct reftable_log_record r1[] = {
>  		{
> @@ -362,9 +367,9 @@ static void test_merged_logs(void)
>
>  	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
>  	err = reftable_iterator_seek_log(&it, "a");
> -	EXPECT_ERR(err);
> -	EXPECT(reftable_merged_table_hash_id(mt) == GIT_SHA1_FORMAT_ID);
> -	EXPECT(reftable_merged_table_min_update_index(mt) == 1);
> +	check(!err);
> +	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
> +	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
>
>  	while (len < 100) { /* cap loops/recursion. */
>  		struct reftable_log_record log = { NULL };
> @@ -377,19 +382,19 @@ static void test_merged_logs(void)
>  	}
>  	reftable_iterator_destroy(&it);
>
> -	EXPECT(ARRAY_SIZE(want) == len);
> +	check_int(ARRAY_SIZE(want), ==, len);
>  	for (i = 0; i < len; i++) {
> -		EXPECT(reftable_log_record_equal(want[i], &out[i],
> +		check(reftable_log_record_equal(want[i], &out[i],
>  						 GIT_SHA1_RAWSZ));
>  	}
>
>  	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
>  	err = reftable_iterator_seek_log_at(&it, "a", 2);
> -	EXPECT_ERR(err);
> +	check(!err);
>  	reftable_log_record_release(&out[0]);
>  	err = reftable_iterator_next_log(&it, &out[0]);
> -	EXPECT_ERR(err);
> -	EXPECT(reftable_log_record_equal(&out[0], &r3[0], GIT_SHA1_RAWSZ));
> +	check(!err);
> +	check(reftable_log_record_equal(&out[0], &r3[0], GIT_SHA1_RAWSZ));
>  	reftable_iterator_destroy(&it);
>
>  	for (i = 0; i < len; i++) {
> @@ -405,7 +410,7 @@ static void test_merged_logs(void)
>  	reftable_free(bs);
>  }
>
> -static void test_default_write_opts(void)
> +static void t_default_write_opts(void)
>  {
>  	struct reftable_write_options opts = { 0 };
>  	struct strbuf buf = STRBUF_INIT;
> @@ -426,23 +431,23 @@ static void test_default_write_opts(void)
>  	reftable_writer_set_limits(w, 1, 1);
>
>  	err = reftable_writer_add_ref(w, &rec);
> -	EXPECT_ERR(err);
> +	check(!err);
>
>  	err = reftable_writer_close(w);
> -	EXPECT_ERR(err);
> +	check(!err);
>  	reftable_writer_free(w);
>
>  	block_source_from_strbuf(&source, &buf);
>
>  	err = reftable_new_reader(&rd, &source, "filename");
> -	EXPECT_ERR(err);
> +	check(!err);
>
>  	hash_id = reftable_reader_hash_id(rd);
> -	EXPECT(hash_id == GIT_SHA1_FORMAT_ID);
> +	check_int(hash_id, ==, GIT_SHA1_FORMAT_ID);
>
>  	reftable_table_from_reader(&tab[0], rd);
>  	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA1_FORMAT_ID);
> -	EXPECT_ERR(err);
> +	check(!err);
>
>  	reftable_reader_free(rd);
>  	reftable_merged_table_free(merged);
> @@ -451,11 +456,12 @@ static void test_default_write_opts(void)
>
>  /* XXX test refs_for(oid) */

Not sure what this comment is for, let's remove it.

> -int merged_test_main(int argc, const char *argv[])
> +int cmd_main(int argc, const char *argv[])
>  {
> -	RUN_TEST(test_merged_logs);
> -	RUN_TEST(test_merged_between);
> -	RUN_TEST(test_merged);
> -	RUN_TEST(test_default_write_opts);
> -	return 0;
> +	TEST(t_merged_logs(), "merged table with log records");
> +	TEST(t_merged_between(), "seek ref in a merged table");
> +	TEST(t_merged(), "merged table with multiple updates to same ref");
> +	TEST(t_default_write_opts(), "merged table with default write opts");

Nit: Could we order these alphabetically?

> +	return test_done();
>  }
> --

The diff only shows parts of the file which were renamed, some generic
comments though:
* Some of the parameters passed to functions can be made 'const', this
makes it clearer if/not it is read-only.
* The functions names could definitely be a little more verbose and
clearer. e.g. `t_merged_between` doesn't even signify what we're doing.
Even the test descriptions could definitely be improved.
* Some single lined for loops have '{}' which can be removed.

Now coming to some individual tests:
- t_merged_between: This test is to check and ensure that a ref which
only occurds in one of the records can be retrieved. This is fine, It
would be nice if we could however add another record 'r3' and ensure the
ref 'a' only occurs in 'r2', this would also ensure that merged tables
don't simply read the last record.
- t_default_write_opts: What is this test trying to achieve?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 1/5] t: move reftable/merged_test.c to the unit testing framework

[Chandra Pratap]

On Fri, 5 Jul 2024 at 23:10, Karthik Nayak <karthik.188@gmail.com> wrote:
>
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>
> > reftable/merged_test.c exercises the functions defined in
> > reftable/merged.{c, h}. Migrate reftable/merged_test.c to the unit
> > testing framework. Migration involves refactoring the tests
> > to use the unit testing framework instead of reftable's test
> > framework and renaming the tests according to unit-tests' naming
> > conventions.
> >
> > Also, move strbuf_add_void() and noop_flush() from
> > reftable/test_framework.c to the ported test. This is because
> > both these functions are used in the merged tests and
> > reftable/test_framework.{c, h} is not #included in the ported test.
> >
> > Mentored-by: Patrick Steinhardt <ps@pks.im>
> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> > ---
> >  Makefile                                      |   2 +-
> >  t/helper/test-reftable.c                      |   1 -
> >  .../unit-tests/t-reftable-merged.c            | 112 +++++++++---------
> >  3 files changed, 60 insertions(+), 55 deletions(-)
> >  rename reftable/merged_test.c => t/unit-tests/t-reftable-merged.c (84%)
> >
> > diff --git a/Makefile b/Makefile
> > index 3eab701b10..e5d1b53991 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1340,6 +1340,7 @@ UNIT_TEST_PROGRAMS += t-mem-pool
> >  UNIT_TEST_PROGRAMS += t-oidtree
> >  UNIT_TEST_PROGRAMS += t-prio-queue
> >  UNIT_TEST_PROGRAMS += t-reftable-basics
> > +UNIT_TEST_PROGRAMS += t-reftable-merged
> >  UNIT_TEST_PROGRAMS += t-strbuf
> >  UNIT_TEST_PROGRAMS += t-strcmp-offset
> >  UNIT_TEST_PROGRAMS += t-strvec
> > @@ -2679,7 +2680,6 @@ REFTABLE_OBJS += reftable/writer.o
> >
> >  REFTABLE_TEST_OBJS += reftable/block_test.o
> >  REFTABLE_TEST_OBJS += reftable/dump.o
> > -REFTABLE_TEST_OBJS += reftable/merged_test.o
> >  REFTABLE_TEST_OBJS += reftable/pq_test.o
> >  REFTABLE_TEST_OBJS += reftable/record_test.o
> >  REFTABLE_TEST_OBJS += reftable/readwrite_test.o
> > diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
> > index 9160bc5da6..0357718fa8 100644
> > --- a/t/helper/test-reftable.c
> > +++ b/t/helper/test-reftable.c
> > @@ -10,7 +10,6 @@ int cmd__reftable(int argc, const char **argv)
> >       tree_test_main(argc, argv);
> >       pq_test_main(argc, argv);
> >       readwrite_test_main(argc, argv);
> > -     merged_test_main(argc, argv);
> >       stack_test_main(argc, argv);
> >       return 0;
> >  }
> > diff --git a/reftable/merged_test.c b/t/unit-tests/t-reftable-merged.c
> > similarity index 84%
> > rename from reftable/merged_test.c
> > rename to t/unit-tests/t-reftable-merged.c
> > index a9d6661c13..1718489f06 100644
> > --- a/reftable/merged_test.c
> > +++ b/t/unit-tests/t-reftable-merged.c
> > @@ -6,20 +6,25 @@ license that can be found in the LICENSE file or at
> >  https://developers.google.com/open-source/licenses/bsd
> >  */
> >
> > -#include "merged.h"
> > -
> > -#include "system.h"
> > +#include "test-lib.h"
> > +#include "reftable/blocksource.h"
> > +#include "reftable/constants.h"
> > +#include "reftable/merged.h"
> > +#include "reftable/reader.h"
> > +#include "reftable/reftable-generic.h"
> > +#include "reftable/reftable-merged.h"
> > +#include "reftable/reftable-writer.h"
> > +
> > +static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)
> > +{
> > +     strbuf_add(b, data, sz);
> > +     return sz;
> > +}
> >
> > -#include "basics.h"
> > -#include "blocksource.h"
> > -#include "constants.h"
> > -#include "reader.h"
> > -#include "record.h"
> > -#include "test_framework.h"
> > -#include "reftable-merged.h"
> > -#include "reftable-tests.h"
> > -#include "reftable-generic.h"
> > -#include "reftable-writer.h"
> > +static int noop_flush(void *arg)
> > +{
> > +     return 0;
> > +}
> >
> >  static void write_test_table(struct strbuf *buf,
> >                            struct reftable_ref_record refs[], int n)
> > @@ -49,12 +54,12 @@ static void write_test_table(struct strbuf *buf,
> >       for (i = 0; i < n; i++) {
> >               uint64_t before = refs[i].update_index;
> >               int n = reftable_writer_add_ref(w, &refs[i]);
> > -             EXPECT(n == 0);
> > -             EXPECT(before == refs[i].update_index);
> > +             check_int(n, ==, 0);
> > +             check_int(before, ==, refs[i].update_index);
> >       }
> >
> >       err = reftable_writer_close(w);
> > -     EXPECT_ERR(err);
> > +     check(!err);
> >
>
> Nit: Couldn't we just do `check(!reftable_writer_close(w))` here and in
> a lot of places below?

I guess we could do that here because 'err' is only used once, but in
most of the other tests, 'err' is used multiple times and with more
complicated functions, so doing a change like this there would probably
make the code less readable.
If we aren't doing this change in any of the other tests, I think we're
better off not changing things here as well to preserve uniformity across
tests.

> >       reftable_writer_free(w);
> >  }
> > @@ -76,11 +81,11 @@ static void write_test_log_table(struct strbuf *buf,
> >
> >       for (i = 0; i < n; i++) {
> >               int err = reftable_writer_add_log(w, &logs[i]);
> > -             EXPECT_ERR(err);
> > +             check(!err);
> >       }
> >
> >       err = reftable_writer_close(w);
> > -     EXPECT_ERR(err);
> > +     check(!err);
> >
> >       reftable_writer_free(w);
> >  }
> > @@ -105,12 +110,12 @@ merged_table_from_records(struct reftable_ref_record **refs,
> >
> >               err = reftable_new_reader(&(*readers)[i], &(*source)[i],
> >                                         "name");
> > -             EXPECT_ERR(err);
> > +             check(!err);
> >               reftable_table_from_reader(&tabs[i], (*readers)[i]);
> >       }
> >
> >       err = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
> > -     EXPECT_ERR(err);
> > +     check(!err);
> >       return mt;
> >  }
> >
> > @@ -122,7 +127,7 @@ static void readers_destroy(struct reftable_reader **readers, size_t n)
> >       reftable_free(readers);
> >  }
> >
> > -static void test_merged_between(void)
> > +static void t_merged_between(void)
> >  {
> >       struct reftable_ref_record r1[] = { {
> >               .refname = (char *) "b",
> > @@ -150,11 +155,11 @@ static void test_merged_between(void)
> >
> >       merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
> >       err = reftable_iterator_seek_ref(&it, "a");
> > -     EXPECT_ERR(err);
> > +     check(!err);
> >
> >       err = reftable_iterator_next_ref(&it, &ref);
> > -     EXPECT_ERR(err);
> > -     EXPECT(ref.update_index == 2);
> > +     check(!err);
> > +     check_int(ref.update_index, ==, 2);
> >       reftable_ref_record_release(&ref);
> >       reftable_iterator_destroy(&it);
> >       readers_destroy(readers, 2);
> > @@ -165,7 +170,7 @@ static void test_merged_between(void)
> >       reftable_free(bs);
> >  }
> >
> > -static void test_merged(void)
> > +static void t_merged(void)
> >  {
> >       struct reftable_ref_record r1[] = {
> >               {
> > @@ -230,9 +235,9 @@ static void test_merged(void)
> >
> >       merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
> >       err = reftable_iterator_seek_ref(&it, "a");
> > -     EXPECT_ERR(err);
> > -     EXPECT(reftable_merged_table_hash_id(mt) == GIT_SHA1_FORMAT_ID);
> > -     EXPECT(reftable_merged_table_min_update_index(mt) == 1);
> > +     check(!err);
> > +     check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
> > +     check_int(reftable_merged_table_min_update_index(mt), ==, 1);
> >
> >       while (len < 100) { /* cap loops/recursion. */
> >               struct reftable_ref_record ref = { NULL };
> > @@ -245,9 +250,9 @@ static void test_merged(void)
> >       }
> >       reftable_iterator_destroy(&it);
> >
> > -     EXPECT(ARRAY_SIZE(want) == len);
> > +     check_int(ARRAY_SIZE(want), ==, len);
> >       for (i = 0; i < len; i++) {
> > -             EXPECT(reftable_ref_record_equal(want[i], &out[i],
> > +             check(reftable_ref_record_equal(want[i], &out[i],
> >                                                GIT_SHA1_RAWSZ));
> >       }
> >       for (i = 0; i < len; i++) {
> > @@ -283,16 +288,16 @@ merged_table_from_log_records(struct reftable_log_record **logs,
> >
> >               err = reftable_new_reader(&(*readers)[i], &(*source)[i],
> >                                         "name");
> > -             EXPECT_ERR(err);
> > +             check(!err);
> >               reftable_table_from_reader(&tabs[i], (*readers)[i]);
> >       }
> >
> >       err = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
> > -     EXPECT_ERR(err);
> > +     check(!err);
> >       return mt;
> >  }
> >
> > -static void test_merged_logs(void)
> > +static void t_merged_logs(void)
> >  {
> >       struct reftable_log_record r1[] = {
> >               {
> > @@ -362,9 +367,9 @@ static void test_merged_logs(void)
> >
> >       merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
> >       err = reftable_iterator_seek_log(&it, "a");
> > -     EXPECT_ERR(err);
> > -     EXPECT(reftable_merged_table_hash_id(mt) == GIT_SHA1_FORMAT_ID);
> > -     EXPECT(reftable_merged_table_min_update_index(mt) == 1);
> > +     check(!err);
> > +     check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
> > +     check_int(reftable_merged_table_min_update_index(mt), ==, 1);
> >
> >       while (len < 100) { /* cap loops/recursion. */
> >               struct reftable_log_record log = { NULL };
> > @@ -377,19 +382,19 @@ static void test_merged_logs(void)
> >       }
> >       reftable_iterator_destroy(&it);
> >
> > -     EXPECT(ARRAY_SIZE(want) == len);
> > +     check_int(ARRAY_SIZE(want), ==, len);
> >       for (i = 0; i < len; i++) {
> > -             EXPECT(reftable_log_record_equal(want[i], &out[i],
> > +             check(reftable_log_record_equal(want[i], &out[i],
> >                                                GIT_SHA1_RAWSZ));
> >       }
> >
> >       merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
> >       err = reftable_iterator_seek_log_at(&it, "a", 2);
> > -     EXPECT_ERR(err);
> > +     check(!err);
> >       reftable_log_record_release(&out[0]);
> >       err = reftable_iterator_next_log(&it, &out[0]);
> > -     EXPECT_ERR(err);
> > -     EXPECT(reftable_log_record_equal(&out[0], &r3[0], GIT_SHA1_RAWSZ));
> > +     check(!err);
> > +     check(reftable_log_record_equal(&out[0], &r3[0], GIT_SHA1_RAWSZ));
> >       reftable_iterator_destroy(&it);
> >
> >       for (i = 0; i < len; i++) {
> > @@ -405,7 +410,7 @@ static void test_merged_logs(void)
> >       reftable_free(bs);
> >  }
> >
> > -static void test_default_write_opts(void)
> > +static void t_default_write_opts(void)
> >  {
> >       struct reftable_write_options opts = { 0 };
> >       struct strbuf buf = STRBUF_INIT;
> > @@ -426,23 +431,23 @@ static void test_default_write_opts(void)
> >       reftable_writer_set_limits(w, 1, 1);
> >
> >       err = reftable_writer_add_ref(w, &rec);
> > -     EXPECT_ERR(err);
> > +     check(!err);
> >
> >       err = reftable_writer_close(w);
> > -     EXPECT_ERR(err);
> > +     check(!err);
> >       reftable_writer_free(w);
> >
> >       block_source_from_strbuf(&source, &buf);
> >
> >       err = reftable_new_reader(&rd, &source, "filename");
> > -     EXPECT_ERR(err);
> > +     check(!err);
> >
> >       hash_id = reftable_reader_hash_id(rd);
> > -     EXPECT(hash_id == GIT_SHA1_FORMAT_ID);
> > +     check_int(hash_id, ==, GIT_SHA1_FORMAT_ID);
> >
> >       reftable_table_from_reader(&tab[0], rd);
> >       err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA1_FORMAT_ID);
> > -     EXPECT_ERR(err);
> > +     check(!err);
> >
> >       reftable_reader_free(rd);
> >       reftable_merged_table_free(merged);
> > @@ -451,11 +456,12 @@ static void test_default_write_opts(void)
> >
> >  /* XXX test refs_for(oid) */
>
> Not sure what this comment is for, let's remove it.
>
> > -int merged_test_main(int argc, const char *argv[])
> > +int cmd_main(int argc, const char *argv[])
> >  {
> > -     RUN_TEST(test_merged_logs);
> > -     RUN_TEST(test_merged_between);
> > -     RUN_TEST(test_merged);
> > -     RUN_TEST(test_default_write_opts);
> > -     return 0;
> > +     TEST(t_merged_logs(), "merged table with log records");
> > +     TEST(t_merged_between(), "seek ref in a merged table");
> > +     TEST(t_merged(), "merged table with multiple updates to same ref");
> > +     TEST(t_default_write_opts(), "merged table with default write opts");
>
> Nit: Could we order these alphabetically?
>
> > +     return test_done();
> >  }
> > --
>
> The diff only shows parts of the file which were renamed, some generic
> comments though:
> * Some of the parameters passed to functions can be made 'const', this
> makes it clearer if/not it is read-only.
> * The functions names could definitely be a little more verbose and
> clearer. e.g. `t_merged_between` doesn't even signify what we're doing.
> Even the test descriptions could definitely be improved.
> * Some single lined for loops have '{}' which can be removed.
>
> Now coming to some individual tests:
> - t_merged_between: This test is to check and ensure that a ref which
> only occurds in one of the records can be retrieved. This is fine, It
> would be nice if we could however add another record 'r3' and ensure the
> ref 'a' only occurs in 'r2', this would also ensure that merged tables
> don't simply read the last record.
> - t_default_write_opts: What is this test trying to achieve?

The test passes an empty 'reftable_write_options' parameter to the
'reftable_new_writer()' function which creates a writer with default
options. The test then tests the merged table created from this writer.

[PATCH 2/5] t: harmonize t-reftable-merged.c with coding guidelines

[Chandra Pratap]

Harmonize the newly ported test unit-tests/t-reftable-merged.c
with the following guidelines:
- Single line control flow statements like 'for' and 'if'
  must omit curly braces.
- Structs must be 0-initialized with '= { 0 }' instead of '= { NULL }'.
- Array indices must be of type 'size_t', not 'int'.
- It is fine to use C99 initial declaration in 'for' loop.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 52 ++++++++++++--------------------
 1 file changed, 20 insertions(+), 32 deletions(-)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index 1718489f06..28bf6f6696 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -40,12 +40,10 @@ static void write_test_table(struct strbuf *buf,
 	struct reftable_writer *w = NULL;
 	for (i = 0; i < n; i++) {
 		uint64_t ui = refs[i].update_index;
-		if (ui > max) {
+		if (ui > max)
 			max = ui;
-		}
-		if (ui < min) {
+		if (ui < min)
 			min = ui;
-		}
 	}
 
 	w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
@@ -68,7 +66,6 @@ static void write_test_log_table(struct strbuf *buf,
 				 struct reftable_log_record logs[], int n,
 				 uint64_t update_index)
 {
-	int i = 0;
 	int err;
 
 	struct reftable_write_options opts = {
@@ -79,7 +76,7 @@ static void write_test_log_table(struct strbuf *buf,
 	w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
 	reftable_writer_set_limits(w, update_index, update_index);
 
-	for (i = 0; i < n; i++) {
+	for (int i = 0; i < n; i++) {
 		int err = reftable_writer_add_log(w, &logs[i]);
 		check(!err);
 	}
@@ -121,8 +118,7 @@ merged_table_from_records(struct reftable_ref_record **refs,
 
 static void readers_destroy(struct reftable_reader **readers, size_t n)
 {
-	int i = 0;
-	for (; i < n; i++)
+	for (size_t i = 0; i < n; i++)
 		reftable_reader_free(readers[i]);
 	reftable_free(readers);
 }
@@ -148,9 +144,8 @@ static void t_merged_between(void)
 	struct reftable_reader **readers = NULL;
 	struct reftable_merged_table *mt =
 		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 2);
-	int i;
-	struct reftable_ref_record ref = { NULL };
-	struct reftable_iterator it = { NULL };
+	struct reftable_ref_record ref = { 0 };
+	struct reftable_iterator it = { 0 };
 	int err;
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
@@ -164,9 +159,8 @@ static void t_merged_between(void)
 	reftable_iterator_destroy(&it);
 	readers_destroy(readers, 2);
 	reftable_merged_table_free(mt);
-	for (i = 0; i < ARRAY_SIZE(bufs); i++) {
+	for (size_t i = 0; i < ARRAY_SIZE(bufs); i++)
 		strbuf_release(&bufs[i]);
-	}
 	reftable_free(bs);
 }
 
@@ -226,12 +220,12 @@ static void t_merged(void)
 	struct reftable_reader **readers = NULL;
 	struct reftable_merged_table *mt =
 		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 3);
-	struct reftable_iterator it = { NULL };
+	struct reftable_iterator it = { 0 };
 	int err;
 	struct reftable_ref_record *out = NULL;
 	size_t len = 0;
 	size_t cap = 0;
-	int i = 0;
+	size_t i;
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
 	err = reftable_iterator_seek_ref(&it, "a");
@@ -240,7 +234,7 @@ static void t_merged(void)
 	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
 
 	while (len < 100) { /* cap loops/recursion. */
-		struct reftable_ref_record ref = { NULL };
+		struct reftable_ref_record ref = { 0 };
 		int err = reftable_iterator_next_ref(&it, &ref);
 		if (err > 0)
 			break;
@@ -251,18 +245,15 @@ static void t_merged(void)
 	reftable_iterator_destroy(&it);
 
 	check_int(ARRAY_SIZE(want), ==, len);
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		check(reftable_ref_record_equal(want[i], &out[i],
 						 GIT_SHA1_RAWSZ));
-	}
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		reftable_ref_record_release(&out[i]);
-	}
 	reftable_free(out);
 
-	for (i = 0; i < 3; i++) {
+	for (i = 0; i < 3; i++)
 		strbuf_release(&bufs[i]);
-	}
 	readers_destroy(readers, 3);
 	reftable_merged_table_free(mt);
 	reftable_free(bs);
@@ -358,12 +349,12 @@ static void t_merged_logs(void)
 	struct reftable_reader **readers = NULL;
 	struct reftable_merged_table *mt = merged_table_from_log_records(
 		logs, &bs, &readers, sizes, bufs, 3);
-	struct reftable_iterator it = { NULL };
+	struct reftable_iterator it = { 0 };
 	int err;
 	struct reftable_log_record *out = NULL;
 	size_t len = 0;
 	size_t cap = 0;
-	int i = 0;
+	size_t i = 0;
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
 	err = reftable_iterator_seek_log(&it, "a");
@@ -372,7 +363,7 @@ static void t_merged_logs(void)
 	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
 
 	while (len < 100) { /* cap loops/recursion. */
-		struct reftable_log_record log = { NULL };
+		struct reftable_log_record log = { 0 };
 		int err = reftable_iterator_next_log(&it, &log);
 		if (err > 0)
 			break;
@@ -383,10 +374,9 @@ static void t_merged_logs(void)
 	reftable_iterator_destroy(&it);
 
 	check_int(ARRAY_SIZE(want), ==, len);
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		check(reftable_log_record_equal(want[i], &out[i],
 						 GIT_SHA1_RAWSZ));
-	}
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
 	err = reftable_iterator_seek_log_at(&it, "a", 2);
@@ -397,14 +387,12 @@ static void t_merged_logs(void)
 	check(reftable_log_record_equal(&out[0], &r3[0], GIT_SHA1_RAWSZ));
 	reftable_iterator_destroy(&it);
 
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		reftable_log_record_release(&out[i]);
-	}
 	reftable_free(out);
 
-	for (i = 0; i < 3; i++) {
+	for (i = 0; i < 3; i++)
 		strbuf_release(&bufs[i]);
-	}
 	readers_destroy(readers, 3);
 	reftable_merged_table_free(mt);
 	reftable_free(bs);
@@ -422,7 +410,7 @@ static void t_default_write_opts(void)
 		.update_index = 1,
 	};
 	int err;
-	struct reftable_block_source source = { NULL };
+	struct reftable_block_source source = { 0 };
 	struct reftable_table *tab = reftable_calloc(1, sizeof(*tab));
 	uint32_t hash_id;
 	struct reftable_reader *rd = NULL;
-- 
2.45.2.404.g9eaef5822c

Re: [PATCH 2/5] t: harmonize t-reftable-merged.c with coding guidelines

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 547 bytes --]

Chandra Pratap <chandrapratap3519@gmail.com> writes:

> Harmonize the newly ported test unit-tests/t-reftable-merged.c
> with the following guidelines:
> - Single line control flow statements like 'for' and 'if'
>   must omit curly braces.
> - Structs must be 0-initialized with '= { 0 }' instead of '= { NULL }'.
> - Array indices must be of type 'size_t', not 'int'.
> - It is fine to use C99 initial declaration in 'for' loop.
>

Seems like I spoke too soon, some of my comments from the previous
commit have been addressed here. Nice.

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH 3/5] t-reftable-merged: add tests for reftable_merged_table_max_update_index

[Chandra Pratap]

reftable_merged_table_max_update_index() as defined by reftable/
merged.{c, h} returns the maximum update index in a merged table.
Since this function is currently unexercised, add tests for it.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index 28bf6f6696..543113f3d4 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -232,6 +232,7 @@ static void t_merged(void)
 	check(!err);
 	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
 	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
+	check_int(reftable_merged_table_max_update_index(mt), ==, 3);
 
 	while (len < 100) { /* cap loops/recursion. */
 		struct reftable_ref_record ref = { 0 };
@@ -361,6 +362,7 @@ static void t_merged_logs(void)
 	check(!err);
 	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
 	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
+	check_int(reftable_merged_table_max_update_index(mt), ==, 3);
 
 	while (len < 100) { /* cap loops/recursion. */
 		struct reftable_log_record log = { 0 };
-- 
2.45.2.404.g9eaef5822c

[PATCH 4/5] t-reftable-merged: use reftable_ref_record_equal to compare ref records

[Chandra Pratap]

In the test test_merged_between() defined in t-reftable-merged.c,
the 'input' and 'expected' ref records are checked for equality
by comparing their update indices. It is very much possible for
two different ref records to have the same update indices. Use
reftable_ref_record_equal() as well for a stronger check.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index 543113f3d4..656193550d 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -155,6 +155,7 @@ static void t_merged_between(void)
 	err = reftable_iterator_next_ref(&it, &ref);
 	check(!err);
 	check_int(ref.update_index, ==, 2);
+	check(reftable_ref_record_equal(&r2[0], &ref, GIT_SHA1_RAWSZ));
 	reftable_ref_record_release(&ref);
 	reftable_iterator_destroy(&it);
 	readers_destroy(readers, 2);
-- 
2.45.2.404.g9eaef5822c

Re: [PATCH 4/5] t-reftable-merged: use reftable_ref_record_equal to compare ref records

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1365 bytes --]

Chandra Pratap <chandrapratap3519@gmail.com> writes:

> In the test test_merged_between() defined in t-reftable-merged.c,
>

s/test_merged_between/t_merged_between

> the 'input' and 'expected' ref records are checked for equality
> by comparing their update indices. It is very much possible for
> two different ref records to have the same update indices. Use
> reftable_ref_record_equal() as well for a stronger check.
>
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
>  t/unit-tests/t-reftable-merged.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
> index 543113f3d4..656193550d 100644
> --- a/t/unit-tests/t-reftable-merged.c
> +++ b/t/unit-tests/t-reftable-merged.c
> @@ -155,6 +155,7 @@ static void t_merged_between(void)
>  	err = reftable_iterator_next_ref(&it, &ref);
>  	check(!err);
>  	check_int(ref.update_index, ==, 2);
> +	check(reftable_ref_record_equal(&r2[0], &ref, GIT_SHA1_RAWSZ));

We can actually remove `check_int(ref.update_index, ==, 2)` since the
new check also would compare the update_index.

>  	reftable_ref_record_release(&ref);
>  	reftable_iterator_destroy(&it);
>  	readers_destroy(readers, 2);
> --
> 2.45.2.404.g9eaef5822c

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH 5/5] t-reftable-merged: add test for REFTABLE_FORMAT_ERROR

[Chandra Pratap]

When calling reftable_new_merged_table(), if the hash ID of the
passsed reftable_table parameter doesn't match the passed hash_id
parameter, a REFTABLE_FORMAT_ERROR is thrown. This case is
currently left unexercised, so add a test for the same.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index 656193550d..a213867cf5 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -11,6 +11,7 @@ license that can be found in the LICENSE file or at
 #include "reftable/constants.h"
 #include "reftable/merged.h"
 #include "reftable/reader.h"
+#include "reftable/reftable-error.h"
 #include "reftable/reftable-generic.h"
 #include "reftable/reftable-merged.h"
 #include "reftable/reftable-writer.h"
@@ -437,6 +438,8 @@ static void t_default_write_opts(void)
 	check_int(hash_id, ==, GIT_SHA1_FORMAT_ID);
 
 	reftable_table_from_reader(&tab[0], rd);
+	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA256_FORMAT_ID);
+	check_int(err, ==, REFTABLE_FORMAT_ERROR);
 	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA1_FORMAT_ID);
 	check(!err);
 
-- 
2.45.2.404.g9eaef5822c

Re: [GSoC][PATCH 0/5] t: port reftable/merged_test.c to the unit testing framework

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 734 bytes --]

Chandra Pratap <chandrapratap3519@gmail.com> writes:

> In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit
> testing framework written entirely in C was introduced to the Git project
> aimed at simplifying testing and reducing test run times.
> Currently, tests for the reftable refs-backend are performed by a custom
> testing framework defined by reftable/test_framework.{c, h}. Port
> reftable/merged_test.c to the unit testing framework and improve upon
> the ported test.
>
> The first patch in the series moves the test to the unit testing framework,
> and the rest of the patches improve upon the ported test.
>

I went through the commits and left some small comments. Looks great
already.

Thanks

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[GSoC][PATCH v2 0/7] t: port reftable/merged_test.c to the unit testing framework

[Chandra Pratap]

In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit
testing framework written entirely in C was introduced to the Git project
aimed at simplifying testing and reducing test run times.
Currently, tests for the reftable refs-backend are performed by a custom
testing framework defined by reftable/test_framework.{c, h}. Port
reftable/merged_test.c to the unit testing framework and improve upon
the ported test.

The first patch in the series moves the test to the unit testing framework,
and the rest of the patches improve upon the ported test.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>

---
Changes in v2:
- Rename test functions & description to be more explanatory
- Remove a redundant comment in patch 1
- Add the 3rd patch which makes improvements to a test
- Add the 4th patch which makes apt function parameters 'const'
- Remove a redundant check in patch 6

CI/PR: https://github.com/gitgitgadget/git/pull/1755

Chandra Pratap (7):
[PATCH 1/7] t: move reftable/merged_test.c to the unit testing framework
[PATCH 2/7] t: harmonize t-reftable-merged.c with coding guidelines
[PATCH 3/7] t-reftable-merged: improve the test for t_merged_single_record()
[PATCH 4/7] t-reftable-merged: improve the const-correctness of helper functions
[PATCH 5/7] t-reftable-merged: add tests for reftable_merged_table_max_update_index
[PATCH 6/7] t-reftable-merged: use reftable_ref_record_equal to compare ref records
[PATCH 7/7] t-reftable-merged: add test for REFTABLE_FORMAT_ERROR

Makefile                                                   |   2 +-
t/helper/test-reftable.c                                   |   1 -
reftable/merged_test.c => t/unit-tests/t-reftable-merged.c | 202 +++++++++++++++----------------
3 files changed, 103 insertions(+), 102 deletions(-)

Range-diff against v2:
1:  f4d5da52f5 ! 1:  0d71deffad t: move reftable/merged_test.c to the unit testing framework
    @@ t/unit-tests/t-reftable-merged.c: static void readers_destroy(struct reftable_re
      }
      
     -static void test_merged_between(void)
    -+static void t_merged_between(void)
    ++static void t_merged_single_record(void)
      {
      	struct reftable_ref_record r1[] = { {
      		.refname = (char *) "b",
    @@ t/unit-tests/t-reftable-merged.c: static void test_merged_between(void)
      }
      
     -static void test_merged(void)
    -+static void t_merged(void)
    ++static void t_merged_refs(void)
      {
      	struct reftable_ref_record r1[] = {
      		{
    @@ t/unit-tests/t-reftable-merged.c: static void test_default_write_opts(void)
      
      	reftable_reader_free(rd);
      	reftable_merged_table_free(merged);
    -@@ t/unit-tests/t-reftable-merged.c: static void test_default_write_opts(void)
    + 	strbuf_release(&buf);
    + }
      
    - /* XXX test refs_for(oid) */
    +-/* XXX test refs_for(oid) */
      
     -int merged_test_main(int argc, const char *argv[])
     +int cmd_main(int argc, const char *argv[])
    @@ t/unit-tests/t-reftable-merged.c: static void test_default_write_opts(void)
     -	RUN_TEST(test_merged);
     -	RUN_TEST(test_default_write_opts);
     -	return 0;
    -+	TEST(t_merged_logs(), "merged table with log records");
    -+	TEST(t_merged_between(), "seek ref in a merged table");
    -+	TEST(t_merged(), "merged table with multiple updates to same ref");
     +	TEST(t_default_write_opts(), "merged table with default write opts");
    ++	TEST(t_merged_logs(), "merged table with multiple log updates for same ref");
    ++	TEST(t_merged_refs(), "merged table with multiple updates to same ref");
    ++	TEST(t_merged_single_record(), "ref ocurring in only one record can be fetched");
     +
     +	return test_done();
      }
2:  fb0f0946b4 ! 2:  a449e2edcf t: harmonize t-reftable-merged.c with coding guidelines
    @@ t/unit-tests/t-reftable-merged.c: merged_table_from_records(struct reftable_ref_
      		reftable_reader_free(readers[i]);
      	reftable_free(readers);
      }
    -@@ t/unit-tests/t-reftable-merged.c: static void t_merged_between(void)
    +@@ t/unit-tests/t-reftable-merged.c: static void t_merged_single_record(void)
      	struct reftable_reader **readers = NULL;
      	struct reftable_merged_table *mt =
      		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 2);
    @@ t/unit-tests/t-reftable-merged.c: static void t_merged_between(void)
      	int err;
      
      	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
    -@@ t/unit-tests/t-reftable-merged.c: static void t_merged_between(void)
    +@@ t/unit-tests/t-reftable-merged.c: static void t_merged_single_record(void)
      	reftable_iterator_destroy(&it);
      	readers_destroy(readers, 2);
      	reftable_merged_table_free(mt);
    @@ t/unit-tests/t-reftable-merged.c: static void t_merged_between(void)
      	reftable_free(bs);
      }
      
    -@@ t/unit-tests/t-reftable-merged.c: static void t_merged(void)
    +@@ t/unit-tests/t-reftable-merged.c: static void t_merged_refs(void)
      	struct reftable_reader **readers = NULL;
      	struct reftable_merged_table *mt =
      		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 3);
    @@ t/unit-tests/t-reftable-merged.c: static void t_merged(void)
      
      	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
      	err = reftable_iterator_seek_ref(&it, "a");
    -@@ t/unit-tests/t-reftable-merged.c: static void t_merged(void)
    +@@ t/unit-tests/t-reftable-merged.c: static void t_merged_refs(void)
      	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
      
      	while (len < 100) { /* cap loops/recursion. */
    @@ t/unit-tests/t-reftable-merged.c: static void t_merged(void)
      		int err = reftable_iterator_next_ref(&it, &ref);
      		if (err > 0)
      			break;
    -@@ t/unit-tests/t-reftable-merged.c: static void t_merged(void)
    +@@ t/unit-tests/t-reftable-merged.c: static void t_merged_refs(void)
      	reftable_iterator_destroy(&it);
      
      	check_int(ARRAY_SIZE(want), ==, len);
-:  ---------- > 3:  bba993bb26 t-reftable-merged: improve the test t_merged_single_record()
-:  ---------- > 4:  4d508eaa02 t-reftable-merged: improve the const-correctness of helper functions
3:  85731b6358 ! 5:  1c9ba26c22 t-reftable-merged: add tests for reftable_merged_table_max_update_index
    @@ Commit message
         Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
     
      ## t/unit-tests/t-reftable-merged.c ##
    -@@ t/unit-tests/t-reftable-merged.c: static void t_merged(void)
    +@@ t/unit-tests/t-reftable-merged.c: static void t_merged_refs(void)
      	check(!err);
      	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
      	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
4:  5d6fd842ac < -:  ---------- t-reftable-merged: use reftable_ref_record_equal to compare ref records
-:  ---------- > 6:  309c7f412e t-reftable-merged: use reftable_ref_record_equal to compare ref records
5:  d8cbb73533 = 7:  2cd7f5b0b4 t-reftable-merged: add test for REFTABLE_FORMAT_ERROR

[PATCH v2 1/7] t: move reftable/merged_test.c to the unit testing framework

[Chandra Pratap]

reftable/merged_test.c exercises the functions defined in
reftable/merged.{c, h}. Migrate reftable/merged_test.c to the unit
testing framework. Migration involves refactoring the tests
to use the unit testing framework instead of reftable's test
framework and renaming the tests according to unit-tests' naming
conventions.

Also, move strbuf_add_void() and noop_flush() from
reftable/test_framework.c to the ported test. This is because
both these functions are used in the merged tests and
reftable/test_framework.{c, h} is not #included in the ported test.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 Makefile                                      |   2 +-
 t/helper/test-reftable.c                      |   1 -
 .../unit-tests/t-reftable-merged.c            | 113 +++++++++---------
 3 files changed, 60 insertions(+), 56 deletions(-)
 rename reftable/merged_test.c => t/unit-tests/t-reftable-merged.c (84%)

diff --git a/Makefile b/Makefile
index 3eab701b10..e5d1b53991 100644
--- a/Makefile
+++ b/Makefile
@@ -1340,6 +1340,7 @@ UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-oidtree
 UNIT_TEST_PROGRAMS += t-prio-queue
 UNIT_TEST_PROGRAMS += t-reftable-basics
+UNIT_TEST_PROGRAMS += t-reftable-merged
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-strcmp-offset
 UNIT_TEST_PROGRAMS += t-strvec
@@ -2679,7 +2680,6 @@ REFTABLE_OBJS += reftable/writer.o
 
 REFTABLE_TEST_OBJS += reftable/block_test.o
 REFTABLE_TEST_OBJS += reftable/dump.o
-REFTABLE_TEST_OBJS += reftable/merged_test.o
 REFTABLE_TEST_OBJS += reftable/pq_test.o
 REFTABLE_TEST_OBJS += reftable/record_test.o
 REFTABLE_TEST_OBJS += reftable/readwrite_test.o
diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
index 9160bc5da6..0357718fa8 100644
--- a/t/helper/test-reftable.c
+++ b/t/helper/test-reftable.c
@@ -10,7 +10,6 @@ int cmd__reftable(int argc, const char **argv)
 	tree_test_main(argc, argv);
 	pq_test_main(argc, argv);
 	readwrite_test_main(argc, argv);
-	merged_test_main(argc, argv);
 	stack_test_main(argc, argv);
 	return 0;
 }
diff --git a/reftable/merged_test.c b/t/unit-tests/t-reftable-merged.c
similarity index 84%
rename from reftable/merged_test.c
rename to t/unit-tests/t-reftable-merged.c
index a9d6661c13..78a864a54f 100644
--- a/reftable/merged_test.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -6,20 +6,25 @@ license that can be found in the LICENSE file or at
 https://developers.google.com/open-source/licenses/bsd
 */
 
-#include "merged.h"
-
-#include "system.h"
+#include "test-lib.h"
+#include "reftable/blocksource.h"
+#include "reftable/constants.h"
+#include "reftable/merged.h"
+#include "reftable/reader.h"
+#include "reftable/reftable-generic.h"
+#include "reftable/reftable-merged.h"
+#include "reftable/reftable-writer.h"
+
+static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)
+{
+	strbuf_add(b, data, sz);
+	return sz;
+}
 
-#include "basics.h"
-#include "blocksource.h"
-#include "constants.h"
-#include "reader.h"
-#include "record.h"
-#include "test_framework.h"
-#include "reftable-merged.h"
-#include "reftable-tests.h"
-#include "reftable-generic.h"
-#include "reftable-writer.h"
+static int noop_flush(void *arg)
+{
+	return 0;
+}
 
 static void write_test_table(struct strbuf *buf,
 			     struct reftable_ref_record refs[], int n)
@@ -49,12 +54,12 @@ static void write_test_table(struct strbuf *buf,
 	for (i = 0; i < n; i++) {
 		uint64_t before = refs[i].update_index;
 		int n = reftable_writer_add_ref(w, &refs[i]);
-		EXPECT(n == 0);
-		EXPECT(before == refs[i].update_index);
+		check_int(n, ==, 0);
+		check_int(before, ==, refs[i].update_index);
 	}
 
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_writer_free(w);
 }
@@ -76,11 +81,11 @@ static void write_test_log_table(struct strbuf *buf,
 
 	for (i = 0; i < n; i++) {
 		int err = reftable_writer_add_log(w, &logs[i]);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_writer_free(w);
 }
@@ -105,12 +110,12 @@ merged_table_from_records(struct reftable_ref_record **refs,
 
 		err = reftable_new_reader(&(*readers)[i], &(*source)[i],
 					  "name");
-		EXPECT_ERR(err);
+		check(!err);
 		reftable_table_from_reader(&tabs[i], (*readers)[i]);
 	}
 
 	err = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
-	EXPECT_ERR(err);
+	check(!err);
 	return mt;
 }
 
@@ -122,7 +127,7 @@ static void readers_destroy(struct reftable_reader **readers, size_t n)
 	reftable_free(readers);
 }
 
-static void test_merged_between(void)
+static void t_merged_single_record(void)
 {
 	struct reftable_ref_record r1[] = { {
 		.refname = (char *) "b",
@@ -150,11 +155,11 @@ static void test_merged_between(void)
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
 	err = reftable_iterator_seek_ref(&it, "a");
-	EXPECT_ERR(err);
+	check(!err);
 
 	err = reftable_iterator_next_ref(&it, &ref);
-	EXPECT_ERR(err);
-	EXPECT(ref.update_index == 2);
+	check(!err);
+	check_int(ref.update_index, ==, 2);
 	reftable_ref_record_release(&ref);
 	reftable_iterator_destroy(&it);
 	readers_destroy(readers, 2);
@@ -165,7 +170,7 @@ static void test_merged_between(void)
 	reftable_free(bs);
 }
 
-static void test_merged(void)
+static void t_merged_refs(void)
 {
 	struct reftable_ref_record r1[] = {
 		{
@@ -230,9 +235,9 @@ static void test_merged(void)
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
 	err = reftable_iterator_seek_ref(&it, "a");
-	EXPECT_ERR(err);
-	EXPECT(reftable_merged_table_hash_id(mt) == GIT_SHA1_FORMAT_ID);
-	EXPECT(reftable_merged_table_min_update_index(mt) == 1);
+	check(!err);
+	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
+	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
 
 	while (len < 100) { /* cap loops/recursion. */
 		struct reftable_ref_record ref = { NULL };
@@ -245,9 +250,9 @@ static void test_merged(void)
 	}
 	reftable_iterator_destroy(&it);
 
-	EXPECT(ARRAY_SIZE(want) == len);
+	check_int(ARRAY_SIZE(want), ==, len);
 	for (i = 0; i < len; i++) {
-		EXPECT(reftable_ref_record_equal(want[i], &out[i],
+		check(reftable_ref_record_equal(want[i], &out[i],
 						 GIT_SHA1_RAWSZ));
 	}
 	for (i = 0; i < len; i++) {
@@ -283,16 +288,16 @@ merged_table_from_log_records(struct reftable_log_record **logs,
 
 		err = reftable_new_reader(&(*readers)[i], &(*source)[i],
 					  "name");
-		EXPECT_ERR(err);
+		check(!err);
 		reftable_table_from_reader(&tabs[i], (*readers)[i]);
 	}
 
 	err = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
-	EXPECT_ERR(err);
+	check(!err);
 	return mt;
 }
 
-static void test_merged_logs(void)
+static void t_merged_logs(void)
 {
 	struct reftable_log_record r1[] = {
 		{
@@ -362,9 +367,9 @@ static void test_merged_logs(void)
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
 	err = reftable_iterator_seek_log(&it, "a");
-	EXPECT_ERR(err);
-	EXPECT(reftable_merged_table_hash_id(mt) == GIT_SHA1_FORMAT_ID);
-	EXPECT(reftable_merged_table_min_update_index(mt) == 1);
+	check(!err);
+	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
+	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
 
 	while (len < 100) { /* cap loops/recursion. */
 		struct reftable_log_record log = { NULL };
@@ -377,19 +382,19 @@ static void test_merged_logs(void)
 	}
 	reftable_iterator_destroy(&it);
 
-	EXPECT(ARRAY_SIZE(want) == len);
+	check_int(ARRAY_SIZE(want), ==, len);
 	for (i = 0; i < len; i++) {
-		EXPECT(reftable_log_record_equal(want[i], &out[i],
+		check(reftable_log_record_equal(want[i], &out[i],
 						 GIT_SHA1_RAWSZ));
 	}
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
 	err = reftable_iterator_seek_log_at(&it, "a", 2);
-	EXPECT_ERR(err);
+	check(!err);
 	reftable_log_record_release(&out[0]);
 	err = reftable_iterator_next_log(&it, &out[0]);
-	EXPECT_ERR(err);
-	EXPECT(reftable_log_record_equal(&out[0], &r3[0], GIT_SHA1_RAWSZ));
+	check(!err);
+	check(reftable_log_record_equal(&out[0], &r3[0], GIT_SHA1_RAWSZ));
 	reftable_iterator_destroy(&it);
 
 	for (i = 0; i < len; i++) {
@@ -405,7 +410,7 @@ static void test_merged_logs(void)
 	reftable_free(bs);
 }
 
-static void test_default_write_opts(void)
+static void t_default_write_opts(void)
 {
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
@@ -426,36 +431,36 @@ static void test_default_write_opts(void)
 	reftable_writer_set_limits(w, 1, 1);
 
 	err = reftable_writer_add_ref(w, &rec);
-	EXPECT_ERR(err);
+	check(!err);
 
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
+	check(!err);
 	reftable_writer_free(w);
 
 	block_source_from_strbuf(&source, &buf);
 
 	err = reftable_new_reader(&rd, &source, "filename");
-	EXPECT_ERR(err);
+	check(!err);
 
 	hash_id = reftable_reader_hash_id(rd);
-	EXPECT(hash_id == GIT_SHA1_FORMAT_ID);
+	check_int(hash_id, ==, GIT_SHA1_FORMAT_ID);
 
 	reftable_table_from_reader(&tab[0], rd);
 	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA1_FORMAT_ID);
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_free(rd);
 	reftable_merged_table_free(merged);
 	strbuf_release(&buf);
 }
 
-/* XXX test refs_for(oid) */
 
-int merged_test_main(int argc, const char *argv[])
+int cmd_main(int argc, const char *argv[])
 {
-	RUN_TEST(test_merged_logs);
-	RUN_TEST(test_merged_between);
-	RUN_TEST(test_merged);
-	RUN_TEST(test_default_write_opts);
-	return 0;
+	TEST(t_default_write_opts(), "merged table with default write opts");
+	TEST(t_merged_logs(), "merged table with multiple log updates for same ref");
+	TEST(t_merged_refs(), "merged table with multiple updates to same ref");
+	TEST(t_merged_single_record(), "ref ocurring in only one record can be fetched");
+
+	return test_done();
 }
-- 
2.45.2.404.g9eaef5822c

Re: [PATCH v2 1/7] t: move reftable/merged_test.c to the unit testing framework

[Justin Tobler]

On 24/07/09 10:58AM, Chandra Pratap wrote:
> reftable/merged_test.c exercises the functions defined in
> reftable/merged.{c, h}. Migrate reftable/merged_test.c to the unit
> testing framework. Migration involves refactoring the tests
> to use the unit testing framework instead of reftable's test
> framework and renaming the tests according to unit-tests' naming
> conventions.
> 
> Also, move strbuf_add_void() and noop_flush() from
> reftable/test_framework.c to the ported test. This is because
> both these functions are used in the merged tests and
> reftable/test_framework.{c, h} is not #included in the ported test.
> 
[snip]
>  
> -int merged_test_main(int argc, const char *argv[])

Since we are removing this function definition, should we also remove
`merged_test_main` from "reftable/reftable-tests.h"?

> +int cmd_main(int argc, const char *argv[])
>  {
> -	RUN_TEST(test_merged_logs);
> -	RUN_TEST(test_merged_between);
> -	RUN_TEST(test_merged);
> -	RUN_TEST(test_default_write_opts);
> -	return 0;
> +	TEST(t_default_write_opts(), "merged table with default write opts");
> +	TEST(t_merged_logs(), "merged table with multiple log updates for same ref");
> +	TEST(t_merged_refs(), "merged table with multiple updates to same ref");
> +	TEST(t_merged_single_record(), "ref ocurring in only one record can be fetched");
> +
> +	return test_done();
>  }
> -- 
> 2.45.2.404.g9eaef5822c
> 
> 

[PATCH v2 2/7] t: harmonize t-reftable-merged.c with coding guidelines

[Chandra Pratap]

Harmonize the newly ported test unit-tests/t-reftable-merged.c
with the following guidelines:
- Single line control flow statements like 'for' and 'if'
  must omit curly braces.
- Structs must be 0-initialized with '= { 0 }' instead of '= { NULL }'.
- Array indices must be of type 'size_t', not 'int'.
- It is fine to use C99 initial declaration in 'for' loop.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 52 ++++++++++++--------------------
 1 file changed, 20 insertions(+), 32 deletions(-)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index 78a864a54f..a984116619 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -40,12 +40,10 @@ static void write_test_table(struct strbuf *buf,
 	struct reftable_writer *w = NULL;
 	for (i = 0; i < n; i++) {
 		uint64_t ui = refs[i].update_index;
-		if (ui > max) {
+		if (ui > max)
 			max = ui;
-		}
-		if (ui < min) {
+		if (ui < min)
 			min = ui;
-		}
 	}
 
 	w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
@@ -68,7 +66,6 @@ static void write_test_log_table(struct strbuf *buf,
 				 struct reftable_log_record logs[], int n,
 				 uint64_t update_index)
 {
-	int i = 0;
 	int err;
 
 	struct reftable_write_options opts = {
@@ -79,7 +76,7 @@ static void write_test_log_table(struct strbuf *buf,
 	w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
 	reftable_writer_set_limits(w, update_index, update_index);
 
-	for (i = 0; i < n; i++) {
+	for (int i = 0; i < n; i++) {
 		int err = reftable_writer_add_log(w, &logs[i]);
 		check(!err);
 	}
@@ -121,8 +118,7 @@ merged_table_from_records(struct reftable_ref_record **refs,
 
 static void readers_destroy(struct reftable_reader **readers, size_t n)
 {
-	int i = 0;
-	for (; i < n; i++)
+	for (size_t i = 0; i < n; i++)
 		reftable_reader_free(readers[i]);
 	reftable_free(readers);
 }
@@ -148,9 +144,8 @@ static void t_merged_single_record(void)
 	struct reftable_reader **readers = NULL;
 	struct reftable_merged_table *mt =
 		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 2);
-	int i;
-	struct reftable_ref_record ref = { NULL };
-	struct reftable_iterator it = { NULL };
+	struct reftable_ref_record ref = { 0 };
+	struct reftable_iterator it = { 0 };
 	int err;
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
@@ -164,9 +159,8 @@ static void t_merged_single_record(void)
 	reftable_iterator_destroy(&it);
 	readers_destroy(readers, 2);
 	reftable_merged_table_free(mt);
-	for (i = 0; i < ARRAY_SIZE(bufs); i++) {
+	for (size_t i = 0; i < ARRAY_SIZE(bufs); i++)
 		strbuf_release(&bufs[i]);
-	}
 	reftable_free(bs);
 }
 
@@ -226,12 +220,12 @@ static void t_merged_refs(void)
 	struct reftable_reader **readers = NULL;
 	struct reftable_merged_table *mt =
 		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 3);
-	struct reftable_iterator it = { NULL };
+	struct reftable_iterator it = { 0 };
 	int err;
 	struct reftable_ref_record *out = NULL;
 	size_t len = 0;
 	size_t cap = 0;
-	int i = 0;
+	size_t i;
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
 	err = reftable_iterator_seek_ref(&it, "a");
@@ -240,7 +234,7 @@ static void t_merged_refs(void)
 	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
 
 	while (len < 100) { /* cap loops/recursion. */
-		struct reftable_ref_record ref = { NULL };
+		struct reftable_ref_record ref = { 0 };
 		int err = reftable_iterator_next_ref(&it, &ref);
 		if (err > 0)
 			break;
@@ -251,18 +245,15 @@ static void t_merged_refs(void)
 	reftable_iterator_destroy(&it);
 
 	check_int(ARRAY_SIZE(want), ==, len);
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		check(reftable_ref_record_equal(want[i], &out[i],
 						 GIT_SHA1_RAWSZ));
-	}
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		reftable_ref_record_release(&out[i]);
-	}
 	reftable_free(out);
 
-	for (i = 0; i < 3; i++) {
+	for (i = 0; i < 3; i++)
 		strbuf_release(&bufs[i]);
-	}
 	readers_destroy(readers, 3);
 	reftable_merged_table_free(mt);
 	reftable_free(bs);
@@ -358,12 +349,12 @@ static void t_merged_logs(void)
 	struct reftable_reader **readers = NULL;
 	struct reftable_merged_table *mt = merged_table_from_log_records(
 		logs, &bs, &readers, sizes, bufs, 3);
-	struct reftable_iterator it = { NULL };
+	struct reftable_iterator it = { 0 };
 	int err;
 	struct reftable_log_record *out = NULL;
 	size_t len = 0;
 	size_t cap = 0;
-	int i = 0;
+	size_t i = 0;
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
 	err = reftable_iterator_seek_log(&it, "a");
@@ -372,7 +363,7 @@ static void t_merged_logs(void)
 	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
 
 	while (len < 100) { /* cap loops/recursion. */
-		struct reftable_log_record log = { NULL };
+		struct reftable_log_record log = { 0 };
 		int err = reftable_iterator_next_log(&it, &log);
 		if (err > 0)
 			break;
@@ -383,10 +374,9 @@ static void t_merged_logs(void)
 	reftable_iterator_destroy(&it);
 
 	check_int(ARRAY_SIZE(want), ==, len);
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		check(reftable_log_record_equal(want[i], &out[i],
 						 GIT_SHA1_RAWSZ));
-	}
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
 	err = reftable_iterator_seek_log_at(&it, "a", 2);
@@ -397,14 +387,12 @@ static void t_merged_logs(void)
 	check(reftable_log_record_equal(&out[0], &r3[0], GIT_SHA1_RAWSZ));
 	reftable_iterator_destroy(&it);
 
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		reftable_log_record_release(&out[i]);
-	}
 	reftable_free(out);
 
-	for (i = 0; i < 3; i++) {
+	for (i = 0; i < 3; i++)
 		strbuf_release(&bufs[i]);
-	}
 	readers_destroy(readers, 3);
 	reftable_merged_table_free(mt);
 	reftable_free(bs);
@@ -422,7 +410,7 @@ static void t_default_write_opts(void)
 		.update_index = 1,
 	};
 	int err;
-	struct reftable_block_source source = { NULL };
+	struct reftable_block_source source = { 0 };
 	struct reftable_table *tab = reftable_calloc(1, sizeof(*tab));
 	uint32_t hash_id;
 	struct reftable_reader *rd = NULL;
-- 
2.45.2.404.g9eaef5822c

[PATCH v2 3/7] t-reftable-merged: improve the test t_merged_single_record()

[Chandra Pratap]

In t-reftable-merged.c, the test t_merged_single_record() ensures
that a ref ('a') which occurs in only one of the records ('r2')
can be retrieved. Improve this test by adding another record 'r3'
to ensure that ref 'a' only occurs in 'r2' and that merged tables
don't simply read the last record.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index a984116619..85ebb96aaa 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -136,14 +136,19 @@ static void t_merged_single_record(void)
 		.update_index = 2,
 		.value_type = REFTABLE_REF_DELETION,
 	} };
+	struct reftable_ref_record r3[] = { {
+		.refname = (char *) "c",
+		.update_index = 3,
+		.value_type = REFTABLE_REF_DELETION,
+	} };
 
-	struct reftable_ref_record *refs[] = { r1, r2 };
-	int sizes[] = { 1, 1 };
-	struct strbuf bufs[2] = { STRBUF_INIT, STRBUF_INIT };
+	struct reftable_ref_record *refs[] = { r1, r2, r3 };
+	int sizes[] = { 1, 1, 1 };
+	struct strbuf bufs[3] = { STRBUF_INIT, STRBUF_INIT, STRBUF_INIT };
 	struct reftable_block_source *bs = NULL;
 	struct reftable_reader **readers = NULL;
 	struct reftable_merged_table *mt =
-		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 2);
+		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 3);
 	struct reftable_ref_record ref = { 0 };
 	struct reftable_iterator it = { 0 };
 	int err;
@@ -157,7 +162,7 @@ static void t_merged_single_record(void)
 	check_int(ref.update_index, ==, 2);
 	reftable_ref_record_release(&ref);
 	reftable_iterator_destroy(&it);
-	readers_destroy(readers, 2);
+	readers_destroy(readers, 3);
 	reftable_merged_table_free(mt);
 	for (size_t i = 0; i < ARRAY_SIZE(bufs); i++)
 		strbuf_release(&bufs[i]);
-- 
2.45.2.404.g9eaef5822c

[PATCH v2 4/7] t-reftable-merged: improve the const-correctness of helper functions

[Chandra Pratap]

In t-reftable-merged.c, a number of helper functions used by the
tests can be re-defined with parameters made 'const' which makes
it easier to understand if they're read-only or not. Re-define
these functions along these lines.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index 85ebb96aaa..d151d6557b 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -15,7 +15,7 @@ license that can be found in the LICENSE file or at
 #include "reftable/reftable-merged.h"
 #include "reftable/reftable-writer.h"
 
-static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)
+static ssize_t strbuf_add_void(void *b, const void *data, const size_t sz)
 {
 	strbuf_add(b, data, sz);
 	return sz;
@@ -27,7 +27,7 @@ static int noop_flush(void *arg)
 }
 
 static void write_test_table(struct strbuf *buf,
-			     struct reftable_ref_record refs[], int n)
+			     struct reftable_ref_record refs[], const int n)
 {
 	uint64_t min = 0xffffffff;
 	uint64_t max = 0;
@@ -62,9 +62,8 @@ static void write_test_table(struct strbuf *buf,
 	reftable_writer_free(w);
 }
 
-static void write_test_log_table(struct strbuf *buf,
-				 struct reftable_log_record logs[], int n,
-				 uint64_t update_index)
+static void write_test_log_table(struct strbuf *buf, struct reftable_log_record logs[],
+				 const int n, const uint64_t update_index)
 {
 	int err;
 
@@ -90,8 +89,8 @@ static void write_test_log_table(struct strbuf *buf,
 static struct reftable_merged_table *
 merged_table_from_records(struct reftable_ref_record **refs,
 			  struct reftable_block_source **source,
-			  struct reftable_reader ***readers, int *sizes,
-			  struct strbuf *buf, size_t n)
+			  struct reftable_reader ***readers, const int *sizes,
+			  struct strbuf *buf, const size_t n)
 {
 	struct reftable_merged_table *mt = NULL;
 	struct reftable_table *tabs;
@@ -116,7 +115,7 @@ merged_table_from_records(struct reftable_ref_record **refs,
 	return mt;
 }
 
-static void readers_destroy(struct reftable_reader **readers, size_t n)
+static void readers_destroy(struct reftable_reader **readers, const size_t n)
 {
 	for (size_t i = 0; i < n; i++)
 		reftable_reader_free(readers[i]);
@@ -267,8 +266,8 @@ static void t_merged_refs(void)
 static struct reftable_merged_table *
 merged_table_from_log_records(struct reftable_log_record **logs,
 			      struct reftable_block_source **source,
-			      struct reftable_reader ***readers, int *sizes,
-			      struct strbuf *buf, size_t n)
+			      struct reftable_reader ***readers, const int *sizes,
+			      struct strbuf *buf, const size_t n)
 {
 	struct reftable_merged_table *mt = NULL;
 	struct reftable_table *tabs;
-- 
2.45.2.404.g9eaef5822c

[PATCH v2 5/7] t-reftable-merged: add tests for reftable_merged_table_max_update_index

[Chandra Pratap]

reftable_merged_table_max_update_index() as defined by reftable/
merged.{c, h} returns the maximum update index in a merged table.
Since this function is currently unexercised, add tests for it.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index d151d6557b..ffc9bd25d2 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -236,6 +236,7 @@ static void t_merged_refs(void)
 	check(!err);
 	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
 	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
+	check_int(reftable_merged_table_max_update_index(mt), ==, 3);
 
 	while (len < 100) { /* cap loops/recursion. */
 		struct reftable_ref_record ref = { 0 };
@@ -365,6 +366,7 @@ static void t_merged_logs(void)
 	check(!err);
 	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
 	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
+	check_int(reftable_merged_table_max_update_index(mt), ==, 3);
 
 	while (len < 100) { /* cap loops/recursion. */
 		struct reftable_log_record log = { 0 };
-- 
2.45.2.404.g9eaef5822c

[PATCH v2 6/7] t-reftable-merged: use reftable_ref_record_equal to compare ref records

[Chandra Pratap]

In the test t_merged_single_record() defined in t-reftable-merged.c,
the 'input' and 'expected' ref records are checked for equality
by comparing their update indices. It is very much possible for
two different ref records to have the same update indices. Use
reftable_ref_record_equal() instead for a stronger check.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index ffc9bd25d2..e0054e379e 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -158,7 +158,7 @@ static void t_merged_single_record(void)
 
 	err = reftable_iterator_next_ref(&it, &ref);
 	check(!err);
-	check_int(ref.update_index, ==, 2);
+	check(reftable_ref_record_equal(&r2[0], &ref, GIT_SHA1_RAWSZ));
 	reftable_ref_record_release(&ref);
 	reftable_iterator_destroy(&it);
 	readers_destroy(readers, 3);
-- 
2.45.2.404.g9eaef5822c

[PATCH v2 7/7] t-reftable-merged: add test for REFTABLE_FORMAT_ERROR

[Chandra Pratap]

When calling reftable_new_merged_table(), if the hash ID of the
passsed reftable_table parameter doesn't match the passed hash_id
parameter, a REFTABLE_FORMAT_ERROR is thrown. This case is
currently left unexercised, so add a test for the same.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index e0054e379e..50047aa90b 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -11,6 +11,7 @@ license that can be found in the LICENSE file or at
 #include "reftable/constants.h"
 #include "reftable/merged.h"
 #include "reftable/reader.h"
+#include "reftable/reftable-error.h"
 #include "reftable/reftable-generic.h"
 #include "reftable/reftable-merged.h"
 #include "reftable/reftable-writer.h"
@@ -440,6 +441,8 @@ static void t_default_write_opts(void)
 	check_int(hash_id, ==, GIT_SHA1_FORMAT_ID);
 
 	reftable_table_from_reader(&tab[0], rd);
+	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA256_FORMAT_ID);
+	check_int(err, ==, REFTABLE_FORMAT_ERROR);
 	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA1_FORMAT_ID);
 	check(!err);
 
-- 
2.45.2.404.g9eaef5822c

Re: [PATCH v2 7/7] t-reftable-merged: add test for REFTABLE_FORMAT_ERROR

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1521 bytes --]

Chandra Pratap <chandrapratap3519@gmail.com> writes:

> When calling reftable_new_merged_table(), if the hash ID of the
> passsed reftable_table parameter doesn't match the passed hash_id

s/passsed/passed

> parameter, a REFTABLE_FORMAT_ERROR is thrown. This case is
> currently left unexercised, so add a test for the same.
>
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
>  t/unit-tests/t-reftable-merged.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
> index e0054e379e..50047aa90b 100644
> --- a/t/unit-tests/t-reftable-merged.c
> +++ b/t/unit-tests/t-reftable-merged.c
> @@ -11,6 +11,7 @@ license that can be found in the LICENSE file or at
>  #include "reftable/constants.h"
>  #include "reftable/merged.h"
>  #include "reftable/reader.h"
> +#include "reftable/reftable-error.h"
>  #include "reftable/reftable-generic.h"
>  #include "reftable/reftable-merged.h"
>  #include "reftable/reftable-writer.h"
> @@ -440,6 +441,8 @@ static void t_default_write_opts(void)
>  	check_int(hash_id, ==, GIT_SHA1_FORMAT_ID);
>
>  	reftable_table_from_reader(&tab[0], rd);
> +	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA256_FORMAT_ID);
> +	check_int(err, ==, REFTABLE_FORMAT_ERROR);
>  	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA1_FORMAT_ID);
>  	check(!err);
>
> --
> 2.45.2.404.g9eaef5822c

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [GSoC][PATCH v2 0/7] t: port reftable/merged_test.c to the unit testing framework

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 7481 bytes --]

Chandra Pratap <chandrapratap3519@gmail.com> writes:

> In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit
> testing framework written entirely in C was introduced to the Git project
> aimed at simplifying testing and reducing test run times.
> Currently, tests for the reftable refs-backend are performed by a custom
> testing framework defined by reftable/test_framework.{c, h}. Port
> reftable/merged_test.c to the unit testing framework and improve upon
> the ported test.
>
> The first patch in the series moves the test to the unit testing framework,
> and the rest of the patches improve upon the ported test.
>

This series looks good now, sans a typo and Justin's comment!
Thanks!

> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>
> ---
> Changes in v2:
> - Rename test functions & description to be more explanatory
> - Remove a redundant comment in patch 1
> - Add the 3rd patch which makes improvements to a test
> - Add the 4th patch which makes apt function parameters 'const'
> - Remove a redundant check in patch 6
>
> CI/PR: https://github.com/gitgitgadget/git/pull/1755
>
> Chandra Pratap (7):
> [PATCH 1/7] t: move reftable/merged_test.c to the unit testing framework
> [PATCH 2/7] t: harmonize t-reftable-merged.c with coding guidelines
> [PATCH 3/7] t-reftable-merged: improve the test for t_merged_single_record()
> [PATCH 4/7] t-reftable-merged: improve the const-correctness of helper functions
> [PATCH 5/7] t-reftable-merged: add tests for reftable_merged_table_max_update_index
> [PATCH 6/7] t-reftable-merged: use reftable_ref_record_equal to compare ref records
> [PATCH 7/7] t-reftable-merged: add test for REFTABLE_FORMAT_ERROR
>
> Makefile                                                   |   2 +-
> t/helper/test-reftable.c                                   |   1 -
> reftable/merged_test.c => t/unit-tests/t-reftable-merged.c | 202 +++++++++++++++----------------
> 3 files changed, 103 insertions(+), 102 deletions(-)
>
> Range-diff against v2:
> 1:  f4d5da52f5 ! 1:  0d71deffad t: move reftable/merged_test.c to the unit testing framework
>     @@ t/unit-tests/t-reftable-merged.c: static void readers_destroy(struct reftable_re
>       }
>
>      -static void test_merged_between(void)
>     -+static void t_merged_between(void)
>     ++static void t_merged_single_record(void)
>       {
>       	struct reftable_ref_record r1[] = { {
>       		.refname = (char *) "b",
>     @@ t/unit-tests/t-reftable-merged.c: static void test_merged_between(void)
>       }
>
>      -static void test_merged(void)
>     -+static void t_merged(void)
>     ++static void t_merged_refs(void)
>       {
>       	struct reftable_ref_record r1[] = {
>       		{
>     @@ t/unit-tests/t-reftable-merged.c: static void test_default_write_opts(void)
>
>       	reftable_reader_free(rd);
>       	reftable_merged_table_free(merged);
>     -@@ t/unit-tests/t-reftable-merged.c: static void test_default_write_opts(void)
>     + 	strbuf_release(&buf);
>     + }
>
>     - /* XXX test refs_for(oid) */
>     +-/* XXX test refs_for(oid) */
>
>      -int merged_test_main(int argc, const char *argv[])
>      +int cmd_main(int argc, const char *argv[])
>     @@ t/unit-tests/t-reftable-merged.c: static void test_default_write_opts(void)
>      -	RUN_TEST(test_merged);
>      -	RUN_TEST(test_default_write_opts);
>      -	return 0;
>     -+	TEST(t_merged_logs(), "merged table with log records");
>     -+	TEST(t_merged_between(), "seek ref in a merged table");
>     -+	TEST(t_merged(), "merged table with multiple updates to same ref");
>      +	TEST(t_default_write_opts(), "merged table with default write opts");
>     ++	TEST(t_merged_logs(), "merged table with multiple log updates for same ref");
>     ++	TEST(t_merged_refs(), "merged table with multiple updates to same ref");
>     ++	TEST(t_merged_single_record(), "ref ocurring in only one record can be fetched");
>      +
>      +	return test_done();
>       }
> 2:  fb0f0946b4 ! 2:  a449e2edcf t: harmonize t-reftable-merged.c with coding guidelines
>     @@ t/unit-tests/t-reftable-merged.c: merged_table_from_records(struct reftable_ref_
>       		reftable_reader_free(readers[i]);
>       	reftable_free(readers);
>       }
>     -@@ t/unit-tests/t-reftable-merged.c: static void t_merged_between(void)
>     +@@ t/unit-tests/t-reftable-merged.c: static void t_merged_single_record(void)
>       	struct reftable_reader **readers = NULL;
>       	struct reftable_merged_table *mt =
>       		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 2);
>     @@ t/unit-tests/t-reftable-merged.c: static void t_merged_between(void)
>       	int err;
>
>       	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
>     -@@ t/unit-tests/t-reftable-merged.c: static void t_merged_between(void)
>     +@@ t/unit-tests/t-reftable-merged.c: static void t_merged_single_record(void)
>       	reftable_iterator_destroy(&it);
>       	readers_destroy(readers, 2);
>       	reftable_merged_table_free(mt);
>     @@ t/unit-tests/t-reftable-merged.c: static void t_merged_between(void)
>       	reftable_free(bs);
>       }
>
>     -@@ t/unit-tests/t-reftable-merged.c: static void t_merged(void)
>     +@@ t/unit-tests/t-reftable-merged.c: static void t_merged_refs(void)
>       	struct reftable_reader **readers = NULL;
>       	struct reftable_merged_table *mt =
>       		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 3);
>     @@ t/unit-tests/t-reftable-merged.c: static void t_merged(void)
>
>       	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
>       	err = reftable_iterator_seek_ref(&it, "a");
>     -@@ t/unit-tests/t-reftable-merged.c: static void t_merged(void)
>     +@@ t/unit-tests/t-reftable-merged.c: static void t_merged_refs(void)
>       	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
>
>       	while (len < 100) { /* cap loops/recursion. */
>     @@ t/unit-tests/t-reftable-merged.c: static void t_merged(void)
>       		int err = reftable_iterator_next_ref(&it, &ref);
>       		if (err > 0)
>       			break;
>     -@@ t/unit-tests/t-reftable-merged.c: static void t_merged(void)
>     +@@ t/unit-tests/t-reftable-merged.c: static void t_merged_refs(void)
>       	reftable_iterator_destroy(&it);
>
>       	check_int(ARRAY_SIZE(want), ==, len);
> -:  ---------- > 3:  bba993bb26 t-reftable-merged: improve the test t_merged_single_record()
> -:  ---------- > 4:  4d508eaa02 t-reftable-merged: improve the const-correctness of helper functions
> 3:  85731b6358 ! 5:  1c9ba26c22 t-reftable-merged: add tests for reftable_merged_table_max_update_index
>     @@ Commit message
>          Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>
>       ## t/unit-tests/t-reftable-merged.c ##
>     -@@ t/unit-tests/t-reftable-merged.c: static void t_merged(void)
>     +@@ t/unit-tests/t-reftable-merged.c: static void t_merged_refs(void)
>       	check(!err);
>       	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
>       	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
> 4:  5d6fd842ac < -:  ---------- t-reftable-merged: use reftable_ref_record_equal to compare ref records
> -:  ---------- > 6:  309c7f412e t-reftable-merged: use reftable_ref_record_equal to compare ref records
> 5:  d8cbb73533 = 7:  2cd7f5b0b4 t-reftable-merged: add test for REFTABLE_FORMAT_ERROR

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [GSoC][PATCH v2 0/7] t: port reftable/merged_test.c to the unit testing framework

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> This series looks good now, sans a typo and Justin's comment!
> Thanks!

Thanks, all, for helping the series (and Chandra) along.

[GSoC][PATCH v3 0/7] t: port reftable/merged_test.c to the unit testing framework

[Chandra Pratap]

In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit
testing framework written entirely in C was introduced to the Git project
aimed at simplifying testing and reducing test run times.
Currently, tests for the reftable refs-backend are performed by a custom
testing framework defined by reftable/test_framework.{c, h}. Port
reftable/merged_test.c to the unit testing framework and improve upon
the ported test.

The first patch in the series moves the test to the unit testing framework,
and the rest of the patches improve upon the ported test.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>

---
Changes in v3:
- Remove merged_test_main() from reftable/reftable-tests.h
- Fix a typo in patch 7

CI/PR: https://github.com/gitgitgadget/git/pull/1755

Chandra Pratap (7):
[PATCH 1/7] t: move reftable/merged_test.c to the unit testing framework
[PATCH 2/7] t: harmonize t-reftable-merged.c with coding guidelines
[PATCH 3/7] t-reftable-merged: improve the test for t_merged_single_record()
[PATCH 4/7] t-reftable-merged: improve the const-correctness of helper functions
[PATCH 5/7] t-reftable-merged: add tests for reftable_merged_table_max_update_index
[PATCH 6/7] t-reftable-merged: use reftable_ref_record_equal to compare ref records
[PATCH 7/7] t-reftable-merged: add test for REFTABLE_FORMAT_ERROR

Makefile                                                   |   2 +-
t/helper/test-reftable.c                                   |   1 -
reftable/reftable-tests.h				   |   1 -
reftable/merged_test.c => t/unit-tests/t-reftable-merged.c | 202 +++++++++++++++----------------
4 files changed, 103 insertions(+), 103 deletions(-)

Range-diff against v2:
1:  0d71deffad ! 1:  9c9fbaf75c t: move reftable/merged_test.c to the unit testing framework
    @@ Makefile: REFTABLE_OBJS += reftable/writer.o
      REFTABLE_TEST_OBJS += reftable/record_test.o
      REFTABLE_TEST_OBJS += reftable/readwrite_test.o

    + ## reftable/reftable-tests.h ##
    +@@ reftable/reftable-tests.h: license that can be found in the LICENSE file or at
    +
    + int basics_test_main(int argc, const char **argv);
    + int block_test_main(int argc, const char **argv);
    +-int merged_test_main(int argc, const char **argv);
    + int pq_test_main(int argc, const char **argv);
    + int record_test_main(int argc, const char **argv);
    + int readwrite_test_main(int argc, const char **argv);
    +
      ## t/helper/test-reftable.c ##
     @@ t/helper/test-reftable.c: int cmd__reftable(int argc, const char **argv)
      	tree_test_main(argc, argv);
2:  a449e2edcf = 2:  08c993f5f6 t: harmonize t-reftable-merged.c with coding guidelines
3:  20fb20bb59 = 3:  fa3085bd9b t-reftable-merged: improve the test t_merged_single_record()
4:  617f668b08 = 4:  d491c1f383 t-reftable-merged: improve the const-correctness of helper functions
5:  bf51af687f = 5:  ee9909f7ce t-reftable-merged: add tests for reftable_merged_table_max_update_index
6:  e2b8f6b3fe = 6:  5ce16e9cfc t-reftable-merged: use reftable_ref_record_equal to compare ref records
7:  b606d67ee7 ! 7:  d090e9ca5b t-reftable-merged: add test for REFTABLE_FORMAT_ERROR
    @@ Commit message
         t-reftable-merged: add test for REFTABLE_FORMAT_ERROR

         When calling reftable_new_merged_table(), if the hash ID of the
    -    passsed reftable_table parameter doesn't match the passed hash_id
    +    passed reftable_table parameter doesn't match the passed hash_id
         parameter, a REFTABLE_FORMAT_ERROR is thrown. This case is
         currently left unexercised, so add a test for the same.

[PATCH v3 1/7] t: move reftable/merged_test.c to the unit testing framework

[Chandra Pratap]

reftable/merged_test.c exercises the functions defined in
reftable/merged.{c, h}. Migrate reftable/merged_test.c to the unit
testing framework. Migration involves refactoring the tests
to use the unit testing framework instead of reftable's test
framework and renaming the tests according to unit-tests' naming
conventions.

Also, move strbuf_add_void() and noop_flush() from
reftable/test_framework.c to the ported test. This is because
both these functions are used in the merged tests and
reftable/test_framework.{c, h} is not #included in the ported test.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 Makefile                                      |   2 +-
 reftable/reftable-tests.h                     |   1 -
 t/helper/test-reftable.c                      |   1 -
 .../unit-tests/t-reftable-merged.c            | 113 +++++++++---------
 4 files changed, 60 insertions(+), 57 deletions(-)
 rename reftable/merged_test.c => t/unit-tests/t-reftable-merged.c (84%)

diff --git a/Makefile b/Makefile
index 3eab701b10..e5d1b53991 100644
--- a/Makefile
+++ b/Makefile
@@ -1340,6 +1340,7 @@ UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-oidtree
 UNIT_TEST_PROGRAMS += t-prio-queue
 UNIT_TEST_PROGRAMS += t-reftable-basics
+UNIT_TEST_PROGRAMS += t-reftable-merged
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-strcmp-offset
 UNIT_TEST_PROGRAMS += t-strvec
@@ -2679,7 +2680,6 @@ REFTABLE_OBJS += reftable/writer.o
 
 REFTABLE_TEST_OBJS += reftable/block_test.o
 REFTABLE_TEST_OBJS += reftable/dump.o
-REFTABLE_TEST_OBJS += reftable/merged_test.o
 REFTABLE_TEST_OBJS += reftable/pq_test.o
 REFTABLE_TEST_OBJS += reftable/record_test.o
 REFTABLE_TEST_OBJS += reftable/readwrite_test.o
diff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h
index 114cc3d053..d5e03dcc1b 100644
--- a/reftable/reftable-tests.h
+++ b/reftable/reftable-tests.h
@@ -11,7 +11,6 @@ license that can be found in the LICENSE file or at
 
 int basics_test_main(int argc, const char **argv);
 int block_test_main(int argc, const char **argv);
-int merged_test_main(int argc, const char **argv);
 int pq_test_main(int argc, const char **argv);
 int record_test_main(int argc, const char **argv);
 int readwrite_test_main(int argc, const char **argv);
diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
index 9160bc5da6..0357718fa8 100644
--- a/t/helper/test-reftable.c
+++ b/t/helper/test-reftable.c
@@ -10,7 +10,6 @@ int cmd__reftable(int argc, const char **argv)
 	tree_test_main(argc, argv);
 	pq_test_main(argc, argv);
 	readwrite_test_main(argc, argv);
-	merged_test_main(argc, argv);
 	stack_test_main(argc, argv);
 	return 0;
 }
diff --git a/reftable/merged_test.c b/t/unit-tests/t-reftable-merged.c
similarity index 84%
rename from reftable/merged_test.c
rename to t/unit-tests/t-reftable-merged.c
index a9d6661c13..78a864a54f 100644
--- a/reftable/merged_test.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -6,20 +6,25 @@ license that can be found in the LICENSE file or at
 https://developers.google.com/open-source/licenses/bsd
 */
 
-#include "merged.h"
-
-#include "system.h"
+#include "test-lib.h"
+#include "reftable/blocksource.h"
+#include "reftable/constants.h"
+#include "reftable/merged.h"
+#include "reftable/reader.h"
+#include "reftable/reftable-generic.h"
+#include "reftable/reftable-merged.h"
+#include "reftable/reftable-writer.h"
+
+static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)
+{
+	strbuf_add(b, data, sz);
+	return sz;
+}
 
-#include "basics.h"
-#include "blocksource.h"
-#include "constants.h"
-#include "reader.h"
-#include "record.h"
-#include "test_framework.h"
-#include "reftable-merged.h"
-#include "reftable-tests.h"
-#include "reftable-generic.h"
-#include "reftable-writer.h"
+static int noop_flush(void *arg)
+{
+	return 0;
+}
 
 static void write_test_table(struct strbuf *buf,
 			     struct reftable_ref_record refs[], int n)
@@ -49,12 +54,12 @@ static void write_test_table(struct strbuf *buf,
 	for (i = 0; i < n; i++) {
 		uint64_t before = refs[i].update_index;
 		int n = reftable_writer_add_ref(w, &refs[i]);
-		EXPECT(n == 0);
-		EXPECT(before == refs[i].update_index);
+		check_int(n, ==, 0);
+		check_int(before, ==, refs[i].update_index);
 	}
 
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_writer_free(w);
 }
@@ -76,11 +81,11 @@ static void write_test_log_table(struct strbuf *buf,
 
 	for (i = 0; i < n; i++) {
 		int err = reftable_writer_add_log(w, &logs[i]);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_writer_free(w);
 }
@@ -105,12 +110,12 @@ merged_table_from_records(struct reftable_ref_record **refs,
 
 		err = reftable_new_reader(&(*readers)[i], &(*source)[i],
 					  "name");
-		EXPECT_ERR(err);
+		check(!err);
 		reftable_table_from_reader(&tabs[i], (*readers)[i]);
 	}
 
 	err = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
-	EXPECT_ERR(err);
+	check(!err);
 	return mt;
 }
 
@@ -122,7 +127,7 @@ static void readers_destroy(struct reftable_reader **readers, size_t n)
 	reftable_free(readers);
 }
 
-static void test_merged_between(void)
+static void t_merged_single_record(void)
 {
 	struct reftable_ref_record r1[] = { {
 		.refname = (char *) "b",
@@ -150,11 +155,11 @@ static void test_merged_between(void)
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
 	err = reftable_iterator_seek_ref(&it, "a");
-	EXPECT_ERR(err);
+	check(!err);
 
 	err = reftable_iterator_next_ref(&it, &ref);
-	EXPECT_ERR(err);
-	EXPECT(ref.update_index == 2);
+	check(!err);
+	check_int(ref.update_index, ==, 2);
 	reftable_ref_record_release(&ref);
 	reftable_iterator_destroy(&it);
 	readers_destroy(readers, 2);
@@ -165,7 +170,7 @@ static void test_merged_between(void)
 	reftable_free(bs);
 }
 
-static void test_merged(void)
+static void t_merged_refs(void)
 {
 	struct reftable_ref_record r1[] = {
 		{
@@ -230,9 +235,9 @@ static void test_merged(void)
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
 	err = reftable_iterator_seek_ref(&it, "a");
-	EXPECT_ERR(err);
-	EXPECT(reftable_merged_table_hash_id(mt) == GIT_SHA1_FORMAT_ID);
-	EXPECT(reftable_merged_table_min_update_index(mt) == 1);
+	check(!err);
+	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
+	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
 
 	while (len < 100) { /* cap loops/recursion. */
 		struct reftable_ref_record ref = { NULL };
@@ -245,9 +250,9 @@ static void test_merged(void)
 	}
 	reftable_iterator_destroy(&it);
 
-	EXPECT(ARRAY_SIZE(want) == len);
+	check_int(ARRAY_SIZE(want), ==, len);
 	for (i = 0; i < len; i++) {
-		EXPECT(reftable_ref_record_equal(want[i], &out[i],
+		check(reftable_ref_record_equal(want[i], &out[i],
 						 GIT_SHA1_RAWSZ));
 	}
 	for (i = 0; i < len; i++) {
@@ -283,16 +288,16 @@ merged_table_from_log_records(struct reftable_log_record **logs,
 
 		err = reftable_new_reader(&(*readers)[i], &(*source)[i],
 					  "name");
-		EXPECT_ERR(err);
+		check(!err);
 		reftable_table_from_reader(&tabs[i], (*readers)[i]);
 	}
 
 	err = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
-	EXPECT_ERR(err);
+	check(!err);
 	return mt;
 }
 
-static void test_merged_logs(void)
+static void t_merged_logs(void)
 {
 	struct reftable_log_record r1[] = {
 		{
@@ -362,9 +367,9 @@ static void test_merged_logs(void)
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
 	err = reftable_iterator_seek_log(&it, "a");
-	EXPECT_ERR(err);
-	EXPECT(reftable_merged_table_hash_id(mt) == GIT_SHA1_FORMAT_ID);
-	EXPECT(reftable_merged_table_min_update_index(mt) == 1);
+	check(!err);
+	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
+	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
 
 	while (len < 100) { /* cap loops/recursion. */
 		struct reftable_log_record log = { NULL };
@@ -377,19 +382,19 @@ static void test_merged_logs(void)
 	}
 	reftable_iterator_destroy(&it);
 
-	EXPECT(ARRAY_SIZE(want) == len);
+	check_int(ARRAY_SIZE(want), ==, len);
 	for (i = 0; i < len; i++) {
-		EXPECT(reftable_log_record_equal(want[i], &out[i],
+		check(reftable_log_record_equal(want[i], &out[i],
 						 GIT_SHA1_RAWSZ));
 	}
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
 	err = reftable_iterator_seek_log_at(&it, "a", 2);
-	EXPECT_ERR(err);
+	check(!err);
 	reftable_log_record_release(&out[0]);
 	err = reftable_iterator_next_log(&it, &out[0]);
-	EXPECT_ERR(err);
-	EXPECT(reftable_log_record_equal(&out[0], &r3[0], GIT_SHA1_RAWSZ));
+	check(!err);
+	check(reftable_log_record_equal(&out[0], &r3[0], GIT_SHA1_RAWSZ));
 	reftable_iterator_destroy(&it);
 
 	for (i = 0; i < len; i++) {
@@ -405,7 +410,7 @@ static void test_merged_logs(void)
 	reftable_free(bs);
 }
 
-static void test_default_write_opts(void)
+static void t_default_write_opts(void)
 {
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
@@ -426,36 +431,36 @@ static void test_default_write_opts(void)
 	reftable_writer_set_limits(w, 1, 1);
 
 	err = reftable_writer_add_ref(w, &rec);
-	EXPECT_ERR(err);
+	check(!err);
 
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
+	check(!err);
 	reftable_writer_free(w);
 
 	block_source_from_strbuf(&source, &buf);
 
 	err = reftable_new_reader(&rd, &source, "filename");
-	EXPECT_ERR(err);
+	check(!err);
 
 	hash_id = reftable_reader_hash_id(rd);
-	EXPECT(hash_id == GIT_SHA1_FORMAT_ID);
+	check_int(hash_id, ==, GIT_SHA1_FORMAT_ID);
 
 	reftable_table_from_reader(&tab[0], rd);
 	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA1_FORMAT_ID);
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_free(rd);
 	reftable_merged_table_free(merged);
 	strbuf_release(&buf);
 }
 
-/* XXX test refs_for(oid) */
 
-int merged_test_main(int argc, const char *argv[])
+int cmd_main(int argc, const char *argv[])
 {
-	RUN_TEST(test_merged_logs);
-	RUN_TEST(test_merged_between);
-	RUN_TEST(test_merged);
-	RUN_TEST(test_default_write_opts);
-	return 0;
+	TEST(t_default_write_opts(), "merged table with default write opts");
+	TEST(t_merged_logs(), "merged table with multiple log updates for same ref");
+	TEST(t_merged_refs(), "merged table with multiple updates to same ref");
+	TEST(t_merged_single_record(), "ref ocurring in only one record can be fetched");
+
+	return test_done();
 }
-- 
2.45.GIT

[PATCH v3 2/7] t: harmonize t-reftable-merged.c with coding guidelines

[Chandra Pratap]

Harmonize the newly ported test unit-tests/t-reftable-merged.c
with the following guidelines:
- Single line control flow statements like 'for' and 'if'
  must omit curly braces.
- Structs must be 0-initialized with '= { 0 }' instead of '= { NULL }'.
- Array indices must be of type 'size_t', not 'int'.
- It is fine to use C99 initial declaration in 'for' loop.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 52 ++++++++++++--------------------
 1 file changed, 20 insertions(+), 32 deletions(-)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index 78a864a54f..a984116619 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -40,12 +40,10 @@ static void write_test_table(struct strbuf *buf,
 	struct reftable_writer *w = NULL;
 	for (i = 0; i < n; i++) {
 		uint64_t ui = refs[i].update_index;
-		if (ui > max) {
+		if (ui > max)
 			max = ui;
-		}
-		if (ui < min) {
+		if (ui < min)
 			min = ui;
-		}
 	}
 
 	w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
@@ -68,7 +66,6 @@ static void write_test_log_table(struct strbuf *buf,
 				 struct reftable_log_record logs[], int n,
 				 uint64_t update_index)
 {
-	int i = 0;
 	int err;
 
 	struct reftable_write_options opts = {
@@ -79,7 +76,7 @@ static void write_test_log_table(struct strbuf *buf,
 	w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
 	reftable_writer_set_limits(w, update_index, update_index);
 
-	for (i = 0; i < n; i++) {
+	for (int i = 0; i < n; i++) {
 		int err = reftable_writer_add_log(w, &logs[i]);
 		check(!err);
 	}
@@ -121,8 +118,7 @@ merged_table_from_records(struct reftable_ref_record **refs,
 
 static void readers_destroy(struct reftable_reader **readers, size_t n)
 {
-	int i = 0;
-	for (; i < n; i++)
+	for (size_t i = 0; i < n; i++)
 		reftable_reader_free(readers[i]);
 	reftable_free(readers);
 }
@@ -148,9 +144,8 @@ static void t_merged_single_record(void)
 	struct reftable_reader **readers = NULL;
 	struct reftable_merged_table *mt =
 		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 2);
-	int i;
-	struct reftable_ref_record ref = { NULL };
-	struct reftable_iterator it = { NULL };
+	struct reftable_ref_record ref = { 0 };
+	struct reftable_iterator it = { 0 };
 	int err;
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
@@ -164,9 +159,8 @@ static void t_merged_single_record(void)
 	reftable_iterator_destroy(&it);
 	readers_destroy(readers, 2);
 	reftable_merged_table_free(mt);
-	for (i = 0; i < ARRAY_SIZE(bufs); i++) {
+	for (size_t i = 0; i < ARRAY_SIZE(bufs); i++)
 		strbuf_release(&bufs[i]);
-	}
 	reftable_free(bs);
 }
 
@@ -226,12 +220,12 @@ static void t_merged_refs(void)
 	struct reftable_reader **readers = NULL;
 	struct reftable_merged_table *mt =
 		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 3);
-	struct reftable_iterator it = { NULL };
+	struct reftable_iterator it = { 0 };
 	int err;
 	struct reftable_ref_record *out = NULL;
 	size_t len = 0;
 	size_t cap = 0;
-	int i = 0;
+	size_t i;
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
 	err = reftable_iterator_seek_ref(&it, "a");
@@ -240,7 +234,7 @@ static void t_merged_refs(void)
 	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
 
 	while (len < 100) { /* cap loops/recursion. */
-		struct reftable_ref_record ref = { NULL };
+		struct reftable_ref_record ref = { 0 };
 		int err = reftable_iterator_next_ref(&it, &ref);
 		if (err > 0)
 			break;
@@ -251,18 +245,15 @@ static void t_merged_refs(void)
 	reftable_iterator_destroy(&it);
 
 	check_int(ARRAY_SIZE(want), ==, len);
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		check(reftable_ref_record_equal(want[i], &out[i],
 						 GIT_SHA1_RAWSZ));
-	}
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		reftable_ref_record_release(&out[i]);
-	}
 	reftable_free(out);
 
-	for (i = 0; i < 3; i++) {
+	for (i = 0; i < 3; i++)
 		strbuf_release(&bufs[i]);
-	}
 	readers_destroy(readers, 3);
 	reftable_merged_table_free(mt);
 	reftable_free(bs);
@@ -358,12 +349,12 @@ static void t_merged_logs(void)
 	struct reftable_reader **readers = NULL;
 	struct reftable_merged_table *mt = merged_table_from_log_records(
 		logs, &bs, &readers, sizes, bufs, 3);
-	struct reftable_iterator it = { NULL };
+	struct reftable_iterator it = { 0 };
 	int err;
 	struct reftable_log_record *out = NULL;
 	size_t len = 0;
 	size_t cap = 0;
-	int i = 0;
+	size_t i = 0;
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
 	err = reftable_iterator_seek_log(&it, "a");
@@ -372,7 +363,7 @@ static void t_merged_logs(void)
 	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
 
 	while (len < 100) { /* cap loops/recursion. */
-		struct reftable_log_record log = { NULL };
+		struct reftable_log_record log = { 0 };
 		int err = reftable_iterator_next_log(&it, &log);
 		if (err > 0)
 			break;
@@ -383,10 +374,9 @@ static void t_merged_logs(void)
 	reftable_iterator_destroy(&it);
 
 	check_int(ARRAY_SIZE(want), ==, len);
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		check(reftable_log_record_equal(want[i], &out[i],
 						 GIT_SHA1_RAWSZ));
-	}
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
 	err = reftable_iterator_seek_log_at(&it, "a", 2);
@@ -397,14 +387,12 @@ static void t_merged_logs(void)
 	check(reftable_log_record_equal(&out[0], &r3[0], GIT_SHA1_RAWSZ));
 	reftable_iterator_destroy(&it);
 
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		reftable_log_record_release(&out[i]);
-	}
 	reftable_free(out);
 
-	for (i = 0; i < 3; i++) {
+	for (i = 0; i < 3; i++)
 		strbuf_release(&bufs[i]);
-	}
 	readers_destroy(readers, 3);
 	reftable_merged_table_free(mt);
 	reftable_free(bs);
@@ -422,7 +410,7 @@ static void t_default_write_opts(void)
 		.update_index = 1,
 	};
 	int err;
-	struct reftable_block_source source = { NULL };
+	struct reftable_block_source source = { 0 };
 	struct reftable_table *tab = reftable_calloc(1, sizeof(*tab));
 	uint32_t hash_id;
 	struct reftable_reader *rd = NULL;
-- 
2.45.GIT

Re: [PATCH v3 2/7] t: harmonize t-reftable-merged.c with coding guidelines

[Junio C Hamano]

Chandra Pratap <chandrapratap3519@gmail.com> writes:

It is very nice that the steps [1/7] and [2/7] are split this way.

> Harmonize the newly ported test unit-tests/t-reftable-merged.c
> with the following guidelines:
> - Single line control flow statements like 'for' and 'if'
>   must omit curly braces.

OK.

> - Structs must be 0-initialized with '= { 0 }' instead of '= { NULL }'.

Correct.

> - Array indices must be of type 'size_t', not 'int'.

OK, but "must be" is probably a bit too strong (see below).

> - It is fine to use C99 initial declaration in 'for' loop.

Yes, it is fine.  You do not have to, but you can.

> @@ -68,7 +66,6 @@ static void write_test_log_table(struct strbuf *buf,
>  				 struct reftable_log_record logs[], int n,
>  				 uint64_t update_index)
>  {
> -	int i = 0;
>  	int err;
>  
>  	struct reftable_write_options opts = {
> @@ -79,7 +76,7 @@ static void write_test_log_table(struct strbuf *buf,
>  	w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
>  	reftable_writer_set_limits(w, update_index, update_index);
>  
> -	for (i = 0; i < n; i++) {
> +	for (int i = 0; i < n; i++) {
>  		int err = reftable_writer_add_log(w, &logs[i]);

Did you mean size_t instead of int here?  Probably not, because the
iteration goes up to "int n" that is supplied by the caller of this
test, so iterating with "int" is perfectly fine here.

So, "must be size_t" is already violated here.  You could update the
type of the incoming parameter "n", but given that this is a test
program that deals with a known logs[] array of a small bounded
size, that may be way overkill and "int" can be justified, too.
On the other hand, if it does not require too much investigation,
you may want to check the caller and if it can be updated to use
"size_t" instead of "int".

The general rule is probably "think twice before using 'int' as an
array index; otherwise use 'size_t'", which covers what I said in
the above paragraph.

> @@ -121,8 +118,7 @@ merged_table_from_records(struct reftable_ref_record **refs,
>  
>  static void readers_destroy(struct reftable_reader **readers, size_t n)
>  {
> -	int i = 0;
> -	for (; i < n; i++)
> +	for (size_t i = 0; i < n; i++)
>  		reftable_reader_free(readers[i]);
>  	reftable_free(readers);
>  }

Much better.

> @@ -148,9 +144,8 @@ static void t_merged_single_record(void)
>  	struct reftable_reader **readers = NULL;
>  	struct reftable_merged_table *mt =
>  		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 2);
> -	int i;
> -	struct reftable_ref_record ref = { NULL };
> -	struct reftable_iterator it = { NULL };
> +	struct reftable_ref_record ref = { 0 };
> +	struct reftable_iterator it = { 0 };
>  	int err;
>  
>  	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
> @@ -164,9 +159,8 @@ static void t_merged_single_record(void)
>  	reftable_iterator_destroy(&it);
>  	readers_destroy(readers, 2);
>  	reftable_merged_table_free(mt);
> -	for (i = 0; i < ARRAY_SIZE(bufs); i++) {
> +	for (size_t i = 0; i < ARRAY_SIZE(bufs); i++)
>  		strbuf_release(&bufs[i]);
> -	}

OK.  size_t is overkill here because bufs[] is a function local
array with only two elements in it, but once the patch to use
"size_t" (i.e., this one) is written, it is not worth to go in and
make it use "int" again.

> @@ -226,12 +220,12 @@ static void t_merged_refs(void)
>  	struct reftable_reader **readers = NULL;
>  	struct reftable_merged_table *mt =
>  		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 3);
> -	struct reftable_iterator it = { NULL };
> +	struct reftable_iterator it = { 0 };
>  	int err;
>  	struct reftable_ref_record *out = NULL;
>  	size_t len = 0;
>  	size_t cap = 0;
> -	int i = 0;
> +	size_t i;

OK.  It is good that we got rid of useless initialization, as this
is used to drive more than one loops below.

> @@ -358,12 +349,12 @@ static void t_merged_logs(void)
>  	struct reftable_reader **readers = NULL;
>  	struct reftable_merged_table *mt = merged_table_from_log_records(
>  		logs, &bs, &readers, sizes, bufs, 3);
> -	struct reftable_iterator it = { NULL };
> +	struct reftable_iterator it = { 0 };
>  	int err;
>  	struct reftable_log_record *out = NULL;
>  	size_t len = 0;
>  	size_t cap = 0;
> -	int i = 0;
> +	size_t i = 0;

Lose the useless initialization here, too.

[PATCH v3 3/7] t-reftable-merged: improve the test t_merged_single_record()

[Chandra Pratap]

In t-reftable-merged.c, the test t_merged_single_record() ensures
that a ref ('a') which occurs in only one of the records ('r2')
can be retrieved. Improve this test by adding another record 'r3'
to ensure that ref 'a' only occurs in 'r2' and that merged tables
don't simply read the last record.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index a984116619..85ebb96aaa 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -136,14 +136,19 @@ static void t_merged_single_record(void)
 		.update_index = 2,
 		.value_type = REFTABLE_REF_DELETION,
 	} };
+	struct reftable_ref_record r3[] = { {
+		.refname = (char *) "c",
+		.update_index = 3,
+		.value_type = REFTABLE_REF_DELETION,
+	} };
 
-	struct reftable_ref_record *refs[] = { r1, r2 };
-	int sizes[] = { 1, 1 };
-	struct strbuf bufs[2] = { STRBUF_INIT, STRBUF_INIT };
+	struct reftable_ref_record *refs[] = { r1, r2, r3 };
+	int sizes[] = { 1, 1, 1 };
+	struct strbuf bufs[3] = { STRBUF_INIT, STRBUF_INIT, STRBUF_INIT };
 	struct reftable_block_source *bs = NULL;
 	struct reftable_reader **readers = NULL;
 	struct reftable_merged_table *mt =
-		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 2);
+		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 3);
 	struct reftable_ref_record ref = { 0 };
 	struct reftable_iterator it = { 0 };
 	int err;
@@ -157,7 +162,7 @@ static void t_merged_single_record(void)
 	check_int(ref.update_index, ==, 2);
 	reftable_ref_record_release(&ref);
 	reftable_iterator_destroy(&it);
-	readers_destroy(readers, 2);
+	readers_destroy(readers, 3);
 	reftable_merged_table_free(mt);
 	for (size_t i = 0; i < ARRAY_SIZE(bufs); i++)
 		strbuf_release(&bufs[i]);
-- 
2.45.GIT

[PATCH v3 4/7] t-reftable-merged: improve the const-correctness of helper functions

[Chandra Pratap]

In t-reftable-merged.c, a number of helper functions used by the
tests can be re-defined with parameters made 'const' which makes
it easier to understand if they're read-only or not. Re-define
these functions along these lines.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index 85ebb96aaa..d151d6557b 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -15,7 +15,7 @@ license that can be found in the LICENSE file or at
 #include "reftable/reftable-merged.h"
 #include "reftable/reftable-writer.h"
 
-static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)
+static ssize_t strbuf_add_void(void *b, const void *data, const size_t sz)
 {
 	strbuf_add(b, data, sz);
 	return sz;
@@ -27,7 +27,7 @@ static int noop_flush(void *arg)
 }
 
 static void write_test_table(struct strbuf *buf,
-			     struct reftable_ref_record refs[], int n)
+			     struct reftable_ref_record refs[], const int n)
 {
 	uint64_t min = 0xffffffff;
 	uint64_t max = 0;
@@ -62,9 +62,8 @@ static void write_test_table(struct strbuf *buf,
 	reftable_writer_free(w);
 }
 
-static void write_test_log_table(struct strbuf *buf,
-				 struct reftable_log_record logs[], int n,
-				 uint64_t update_index)
+static void write_test_log_table(struct strbuf *buf, struct reftable_log_record logs[],
+				 const int n, const uint64_t update_index)
 {
 	int err;
 
@@ -90,8 +89,8 @@ static void write_test_log_table(struct strbuf *buf,
 static struct reftable_merged_table *
 merged_table_from_records(struct reftable_ref_record **refs,
 			  struct reftable_block_source **source,
-			  struct reftable_reader ***readers, int *sizes,
-			  struct strbuf *buf, size_t n)
+			  struct reftable_reader ***readers, const int *sizes,
+			  struct strbuf *buf, const size_t n)
 {
 	struct reftable_merged_table *mt = NULL;
 	struct reftable_table *tabs;
@@ -116,7 +115,7 @@ merged_table_from_records(struct reftable_ref_record **refs,
 	return mt;
 }
 
-static void readers_destroy(struct reftable_reader **readers, size_t n)
+static void readers_destroy(struct reftable_reader **readers, const size_t n)
 {
 	for (size_t i = 0; i < n; i++)
 		reftable_reader_free(readers[i]);
@@ -267,8 +266,8 @@ static void t_merged_refs(void)
 static struct reftable_merged_table *
 merged_table_from_log_records(struct reftable_log_record **logs,
 			      struct reftable_block_source **source,
-			      struct reftable_reader ***readers, int *sizes,
-			      struct strbuf *buf, size_t n)
+			      struct reftable_reader ***readers, const int *sizes,
+			      struct strbuf *buf, const size_t n)
 {
 	struct reftable_merged_table *mt = NULL;
 	struct reftable_table *tabs;
-- 
2.45.GIT

[PATCH v3 5/7] t-reftable-merged: add tests for reftable_merged_table_max_update_index

[Chandra Pratap]

reftable_merged_table_max_update_index() as defined by reftable/
merged.{c, h} returns the maximum update index in a merged table.
Since this function is currently unexercised, add tests for it.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index d151d6557b..ffc9bd25d2 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -236,6 +236,7 @@ static void t_merged_refs(void)
 	check(!err);
 	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
 	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
+	check_int(reftable_merged_table_max_update_index(mt), ==, 3);
 
 	while (len < 100) { /* cap loops/recursion. */
 		struct reftable_ref_record ref = { 0 };
@@ -365,6 +366,7 @@ static void t_merged_logs(void)
 	check(!err);
 	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
 	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
+	check_int(reftable_merged_table_max_update_index(mt), ==, 3);
 
 	while (len < 100) { /* cap loops/recursion. */
 		struct reftable_log_record log = { 0 };
-- 
2.45.GIT

[PATCH v3 6/7] t-reftable-merged: use reftable_ref_record_equal to compare ref records

[Chandra Pratap]

In the test t_merged_single_record() defined in t-reftable-merged.c,
the 'input' and 'expected' ref records are checked for equality
by comparing their update indices. It is very much possible for
two different ref records to have the same update indices. Use
reftable_ref_record_equal() instead for a stronger check.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index ffc9bd25d2..e0054e379e 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -158,7 +158,7 @@ static void t_merged_single_record(void)
 
 	err = reftable_iterator_next_ref(&it, &ref);
 	check(!err);
-	check_int(ref.update_index, ==, 2);
+	check(reftable_ref_record_equal(&r2[0], &ref, GIT_SHA1_RAWSZ));
 	reftable_ref_record_release(&ref);
 	reftable_iterator_destroy(&it);
 	readers_destroy(readers, 3);
-- 
2.45.GIT

[PATCH v3 7/7] t-reftable-merged: add test for REFTABLE_FORMAT_ERROR

[Chandra Pratap]

When calling reftable_new_merged_table(), if the hash ID of the
passed reftable_table parameter doesn't match the passed hash_id
parameter, a REFTABLE_FORMAT_ERROR is thrown. This case is
currently left unexercised, so add a test for the same.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index e0054e379e..50047aa90b 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -11,6 +11,7 @@ license that can be found in the LICENSE file or at
 #include "reftable/constants.h"
 #include "reftable/merged.h"
 #include "reftable/reader.h"
+#include "reftable/reftable-error.h"
 #include "reftable/reftable-generic.h"
 #include "reftable/reftable-merged.h"
 #include "reftable/reftable-writer.h"
@@ -440,6 +441,8 @@ static void t_default_write_opts(void)
 	check_int(hash_id, ==, GIT_SHA1_FORMAT_ID);
 
 	reftable_table_from_reader(&tab[0], rd);
+	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA256_FORMAT_ID);
+	check_int(err, ==, REFTABLE_FORMAT_ERROR);
 	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA1_FORMAT_ID);
 	check(!err);
 
-- 
2.45.GIT

[GSoC][PATCH v4 0/7] t: port reftable/merged_test.c to the unit testing framework

[Chandra Pratap]

In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit
testing framework written entirely in C was introduced to the Git project
aimed at simplifying testing and reducing test run times.
Currently, tests for the reftable refs-backend are performed by a custom
testing framework defined by reftable/test_framework.{c, h}. Port
reftable/merged_test.c to the unit testing framework and improve upon
the ported test.

The first patch in the series moves the test to the unit testing framework,
and the rest of the patches improve upon the ported test.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>

---
Changes in v4:
- Make more variables 'size_t' and remove redundant initialization
  in patch 2 in-line with Junio's comments on v3.
- Use ARRAY_SIZE in patch 2 instead of hardcoding arrays' element counts.

CI/PR: https://github.com/gitgitgadget/git/pull/1755

Chandra Pratap (7):
[PATCH 1/7] t: move reftable/merged_test.c to the unit testing framework
[PATCH 2/7] t: harmonize t-reftable-merged.c with coding guidelines
[PATCH 3/7] t-reftable-merged: improve the test for t_merged_single_record()
[PATCH 4/7] t-reftable-merged: improve the const-correctness of helper functions
[PATCH 5/7] t-reftable-merged: add tests for reftable_merged_table_max_update_index
[PATCH 6/7] t-reftable-merged: use reftable_ref_record_equal to compare ref records
[PATCH 7/7] t-reftable-merged: add test for REFTABLE_FORMAT_ERROR

Makefile                                                   |   2 +-
t/helper/test-reftable.c                                   |   1 -
reftable/reftable-tests.h				   |   1 -
reftable/merged_test.c => t/unit-tests/t-reftable-merged.c | 208 +++++++++++++++----------------
4 files changed, 106 insertions(+), 106 deletions(-)

Range-diff against v3:
1:  08c993f5f6 ! 1:  963b9397b2 t: harmonize t-reftable-merged.c with coding guidelines
    @@ Commit message
         - Single line control flow statements like 'for' and 'if'
           must omit curly braces.
         - Structs must be 0-initialized with '= { 0 }' instead of '= { NULL }'.
    -    - Array indices must be of type 'size_t', not 'int'.
    +    - Array indices should preferably be of type 'size_t', not 'int'.
         - It is fine to use C99 initial declaration in 'for' loop.
     
    +    While at it, use 'ARRAY_SIZE(x)' to store the number of elements
    +    in an array instead of hardcoding them.
    +
         Mentored-by: Patrick Steinhardt <ps@pks.im>
         Mentored-by: Christian Couder <chriscool@tuxfamily.org>
         Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
     
      ## t/unit-tests/t-reftable-merged.c ##
    +@@ t/unit-tests/t-reftable-merged.c: static int noop_flush(void *arg)
    + }
    + 
    + static void write_test_table(struct strbuf *buf,
    +-			     struct reftable_ref_record refs[], int n)
    ++			     struct reftable_ref_record refs[], size_t n)
    + {
    + 	uint64_t min = 0xffffffff;
    + 	uint64_t max = 0;
    +-	int i = 0;
    ++	size_t i;
    + 	int err;
    + 
    + 	struct reftable_write_options opts = {
     @@ t/unit-tests/t-reftable-merged.c: static void write_test_table(struct strbuf *buf,
      	struct reftable_writer *w = NULL;
      	for (i = 0; i < n; i++) {
    @@ t/unit-tests/t-reftable-merged.c: static void write_test_table(struct strbuf *bu
      	}
      
      	w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
    -@@ t/unit-tests/t-reftable-merged.c: static void write_test_log_table(struct strbuf *buf,
    - 				 struct reftable_log_record logs[], int n,
    +@@ t/unit-tests/t-reftable-merged.c: static void write_test_table(struct strbuf *buf,
    + }
    + 
    + static void write_test_log_table(struct strbuf *buf,
    +-				 struct reftable_log_record logs[], int n,
    ++				 struct reftable_log_record logs[], size_t n,
      				 uint64_t update_index)
      {
     -	int i = 0;
    @@ t/unit-tests/t-reftable-merged.c: static void write_test_log_table(struct strbuf
      	reftable_writer_set_limits(w, update_index, update_index);
      
     -	for (i = 0; i < n; i++) {
    -+	for (int i = 0; i < n; i++) {
    ++	for (size_t i = 0; i < n; i++) {
      		int err = reftable_writer_add_log(w, &logs[i]);
      		check(!err);
      	}
    +@@ t/unit-tests/t-reftable-merged.c: static void write_test_log_table(struct strbuf *buf,
    + static struct reftable_merged_table *
    + merged_table_from_records(struct reftable_ref_record **refs,
    + 			  struct reftable_block_source **source,
    +-			  struct reftable_reader ***readers, int *sizes,
    ++			  struct reftable_reader ***readers, size_t *sizes,
    + 			  struct strbuf *buf, size_t n)
    + {
    + 	struct reftable_merged_table *mt = NULL;
     @@ t/unit-tests/t-reftable-merged.c: merged_table_from_records(struct reftable_ref_record **refs,
      
      static void readers_destroy(struct reftable_reader **readers, size_t n)
    @@ t/unit-tests/t-reftable-merged.c: merged_table_from_records(struct reftable_ref_
      	reftable_free(readers);
      }
     @@ t/unit-tests/t-reftable-merged.c: static void t_merged_single_record(void)
    + 	} };
    + 
    + 	struct reftable_ref_record *refs[] = { r1, r2 };
    +-	int sizes[] = { 1, 1 };
    ++	size_t sizes[] = { ARRAY_SIZE(r1), ARRAY_SIZE(r2) };
    + 	struct strbuf bufs[2] = { STRBUF_INIT, STRBUF_INIT };
    + 	struct reftable_block_source *bs = NULL;
      	struct reftable_reader **readers = NULL;
      	struct reftable_merged_table *mt =
      		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 2);
    @@ t/unit-tests/t-reftable-merged.c: static void t_merged_single_record(void)
      }
      
     @@ t/unit-tests/t-reftable-merged.c: static void t_merged_refs(void)
    + 	};
    + 
    + 	struct reftable_ref_record *refs[] = { r1, r2, r3 };
    +-	int sizes[3] = { 3, 1, 2 };
    ++	size_t sizes[3] = { ARRAY_SIZE(r1), ARRAY_SIZE(r2), ARRAY_SIZE(r3) };
    + 	struct strbuf bufs[3] = { STRBUF_INIT, STRBUF_INIT, STRBUF_INIT };
    + 	struct reftable_block_source *bs = NULL;
      	struct reftable_reader **readers = NULL;
      	struct reftable_merged_table *mt =
      		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 3);
    @@ t/unit-tests/t-reftable-merged.c: static void t_merged_refs(void)
      	readers_destroy(readers, 3);
      	reftable_merged_table_free(mt);
      	reftable_free(bs);
    +@@ t/unit-tests/t-reftable-merged.c: static void t_merged_refs(void)
    + static struct reftable_merged_table *
    + merged_table_from_log_records(struct reftable_log_record **logs,
    + 			      struct reftable_block_source **source,
    +-			      struct reftable_reader ***readers, int *sizes,
    ++			      struct reftable_reader ***readers, size_t *sizes,
    + 			      struct strbuf *buf, size_t n)
    + {
    + 	struct reftable_merged_table *mt = NULL;
     @@ t/unit-tests/t-reftable-merged.c: static void t_merged_logs(void)
    + 	};
    + 
    + 	struct reftable_log_record *logs[] = { r1, r2, r3 };
    +-	int sizes[3] = { 2, 1, 1 };
    ++	size_t sizes[3] = { ARRAY_SIZE(r1), ARRAY_SIZE(r2), ARRAY_SIZE(r3) };
    + 	struct strbuf bufs[3] = { STRBUF_INIT, STRBUF_INIT, STRBUF_INIT };
    + 	struct reftable_block_source *bs = NULL;
      	struct reftable_reader **readers = NULL;
      	struct reftable_merged_table *mt = merged_table_from_log_records(
      		logs, &bs, &readers, sizes, bufs, 3);
    @@ t/unit-tests/t-reftable-merged.c: static void t_merged_logs(void)
      	size_t len = 0;
      	size_t cap = 0;
     -	int i = 0;
    -+	size_t i = 0;
    ++	size_t i;
      
      	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
      	err = reftable_iterator_seek_log(&it, "a");
2:  fa3085bd9b ! 2:  b7a0bd8165 t-reftable-merged: improve the test t_merged_single_record()
    @@ t/unit-tests/t-reftable-merged.c: static void t_merged_single_record(void)
     +	} };
      
     -	struct reftable_ref_record *refs[] = { r1, r2 };
    --	int sizes[] = { 1, 1 };
    +-	size_t sizes[] = { ARRAY_SIZE(r1), ARRAY_SIZE(r2) };
     -	struct strbuf bufs[2] = { STRBUF_INIT, STRBUF_INIT };
     +	struct reftable_ref_record *refs[] = { r1, r2, r3 };
    -+	int sizes[] = { 1, 1, 1 };
    ++	size_t sizes[] = { ARRAY_SIZE(r1), ARRAY_SIZE(r2), ARRAY_SIZE(r3) };
     +	struct strbuf bufs[3] = { STRBUF_INIT, STRBUF_INIT, STRBUF_INIT };
      	struct reftable_block_source *bs = NULL;
      	struct reftable_reader **readers = NULL;
3:  d491c1f383 ! 3:  e86d19a1c8 t-reftable-merged: improve the const-correctness of helper functions
    @@ t/unit-tests/t-reftable-merged.c: static int noop_flush(void *arg)
      }
      
      static void write_test_table(struct strbuf *buf,
    --			     struct reftable_ref_record refs[], int n)
    -+			     struct reftable_ref_record refs[], const int n)
    +-			     struct reftable_ref_record refs[], size_t n)
    ++			     struct reftable_ref_record refs[], const size_t n)
      {
      	uint64_t min = 0xffffffff;
      	uint64_t max = 0;
    @@ t/unit-tests/t-reftable-merged.c: static void write_test_table(struct strbuf *bu
      }
      
     -static void write_test_log_table(struct strbuf *buf,
    --				 struct reftable_log_record logs[], int n,
    +-				 struct reftable_log_record logs[], size_t n,
     -				 uint64_t update_index)
     +static void write_test_log_table(struct strbuf *buf, struct reftable_log_record logs[],
    -+				 const int n, const uint64_t update_index)
    ++				 const size_t n, const uint64_t update_index)
      {
      	int err;
      
    @@ t/unit-tests/t-reftable-merged.c: static void write_test_log_table(struct strbuf
      static struct reftable_merged_table *
      merged_table_from_records(struct reftable_ref_record **refs,
      			  struct reftable_block_source **source,
    --			  struct reftable_reader ***readers, int *sizes,
    +-			  struct reftable_reader ***readers, size_t *sizes,
     -			  struct strbuf *buf, size_t n)
    -+			  struct reftable_reader ***readers, const int *sizes,
    ++			  struct reftable_reader ***readers, const size_t *sizes,
     +			  struct strbuf *buf, const size_t n)
      {
      	struct reftable_merged_table *mt = NULL;
    @@ t/unit-tests/t-reftable-merged.c: static void t_merged_refs(void)
      static struct reftable_merged_table *
      merged_table_from_log_records(struct reftable_log_record **logs,
      			      struct reftable_block_source **source,
    --			      struct reftable_reader ***readers, int *sizes,
    +-			      struct reftable_reader ***readers, size_t *sizes,
     -			      struct strbuf *buf, size_t n)
    -+			      struct reftable_reader ***readers, const int *sizes,
    ++			      struct reftable_reader ***readers, const size_t *sizes,
     +			      struct strbuf *buf, const size_t n)
      {
      	struct reftable_merged_table *mt = NULL;
4:  ee9909f7ce = 4:  561ea9c5e9 t-reftable-merged: add tests for reftable_merged_table_max_update_index
5:  5ce16e9cfc = 5:  7f1a329e94 t-reftable-merged: use reftable_ref_record_equal to compare ref records
6:  d090e9ca5b = 6:  feaf46b765 t-reftable-merged: add test for REFTABLE_FORMAT_ERROR

[PATCH v4 1/7] t: move reftable/merged_test.c to the unit testing framework

[Chandra Pratap]

reftable/merged_test.c exercises the functions defined in
reftable/merged.{c, h}. Migrate reftable/merged_test.c to the unit
testing framework. Migration involves refactoring the tests
to use the unit testing framework instead of reftable's test
framework and renaming the tests according to unit-tests' naming
conventions.

Also, move strbuf_add_void() and noop_flush() from
reftable/test_framework.c to the ported test. This is because
both these functions are used in the merged tests and
reftable/test_framework.{c, h} is not #included in the ported test.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 Makefile                                      |   2 +-
 reftable/reftable-tests.h                     |   1 -
 t/helper/test-reftable.c                      |   1 -
 .../unit-tests/t-reftable-merged.c            | 113 +++++++++---------
 4 files changed, 60 insertions(+), 57 deletions(-)
 rename reftable/merged_test.c => t/unit-tests/t-reftable-merged.c (84%)

diff --git a/Makefile b/Makefile
index 3eab701b10..e5d1b53991 100644
--- a/Makefile
+++ b/Makefile
@@ -1340,6 +1340,7 @@ UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-oidtree
 UNIT_TEST_PROGRAMS += t-prio-queue
 UNIT_TEST_PROGRAMS += t-reftable-basics
+UNIT_TEST_PROGRAMS += t-reftable-merged
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-strcmp-offset
 UNIT_TEST_PROGRAMS += t-strvec
@@ -2679,7 +2680,6 @@ REFTABLE_OBJS += reftable/writer.o
 
 REFTABLE_TEST_OBJS += reftable/block_test.o
 REFTABLE_TEST_OBJS += reftable/dump.o
-REFTABLE_TEST_OBJS += reftable/merged_test.o
 REFTABLE_TEST_OBJS += reftable/pq_test.o
 REFTABLE_TEST_OBJS += reftable/record_test.o
 REFTABLE_TEST_OBJS += reftable/readwrite_test.o
diff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h
index 114cc3d053..d5e03dcc1b 100644
--- a/reftable/reftable-tests.h
+++ b/reftable/reftable-tests.h
@@ -11,7 +11,6 @@ license that can be found in the LICENSE file or at
 
 int basics_test_main(int argc, const char **argv);
 int block_test_main(int argc, const char **argv);
-int merged_test_main(int argc, const char **argv);
 int pq_test_main(int argc, const char **argv);
 int record_test_main(int argc, const char **argv);
 int readwrite_test_main(int argc, const char **argv);
diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
index 9160bc5da6..0357718fa8 100644
--- a/t/helper/test-reftable.c
+++ b/t/helper/test-reftable.c
@@ -10,7 +10,6 @@ int cmd__reftable(int argc, const char **argv)
 	tree_test_main(argc, argv);
 	pq_test_main(argc, argv);
 	readwrite_test_main(argc, argv);
-	merged_test_main(argc, argv);
 	stack_test_main(argc, argv);
 	return 0;
 }
diff --git a/reftable/merged_test.c b/t/unit-tests/t-reftable-merged.c
similarity index 84%
rename from reftable/merged_test.c
rename to t/unit-tests/t-reftable-merged.c
index a9d6661c13..78a864a54f 100644
--- a/reftable/merged_test.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -6,20 +6,25 @@ license that can be found in the LICENSE file or at
 https://developers.google.com/open-source/licenses/bsd
 */
 
-#include "merged.h"
-
-#include "system.h"
+#include "test-lib.h"
+#include "reftable/blocksource.h"
+#include "reftable/constants.h"
+#include "reftable/merged.h"
+#include "reftable/reader.h"
+#include "reftable/reftable-generic.h"
+#include "reftable/reftable-merged.h"
+#include "reftable/reftable-writer.h"
+
+static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)
+{
+	strbuf_add(b, data, sz);
+	return sz;
+}
 
-#include "basics.h"
-#include "blocksource.h"
-#include "constants.h"
-#include "reader.h"
-#include "record.h"
-#include "test_framework.h"
-#include "reftable-merged.h"
-#include "reftable-tests.h"
-#include "reftable-generic.h"
-#include "reftable-writer.h"
+static int noop_flush(void *arg)
+{
+	return 0;
+}
 
 static void write_test_table(struct strbuf *buf,
 			     struct reftable_ref_record refs[], int n)
@@ -49,12 +54,12 @@ static void write_test_table(struct strbuf *buf,
 	for (i = 0; i < n; i++) {
 		uint64_t before = refs[i].update_index;
 		int n = reftable_writer_add_ref(w, &refs[i]);
-		EXPECT(n == 0);
-		EXPECT(before == refs[i].update_index);
+		check_int(n, ==, 0);
+		check_int(before, ==, refs[i].update_index);
 	}
 
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_writer_free(w);
 }
@@ -76,11 +81,11 @@ static void write_test_log_table(struct strbuf *buf,
 
 	for (i = 0; i < n; i++) {
 		int err = reftable_writer_add_log(w, &logs[i]);
-		EXPECT_ERR(err);
+		check(!err);
 	}
 
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_writer_free(w);
 }
@@ -105,12 +110,12 @@ merged_table_from_records(struct reftable_ref_record **refs,
 
 		err = reftable_new_reader(&(*readers)[i], &(*source)[i],
 					  "name");
-		EXPECT_ERR(err);
+		check(!err);
 		reftable_table_from_reader(&tabs[i], (*readers)[i]);
 	}
 
 	err = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
-	EXPECT_ERR(err);
+	check(!err);
 	return mt;
 }
 
@@ -122,7 +127,7 @@ static void readers_destroy(struct reftable_reader **readers, size_t n)
 	reftable_free(readers);
 }
 
-static void test_merged_between(void)
+static void t_merged_single_record(void)
 {
 	struct reftable_ref_record r1[] = { {
 		.refname = (char *) "b",
@@ -150,11 +155,11 @@ static void test_merged_between(void)
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
 	err = reftable_iterator_seek_ref(&it, "a");
-	EXPECT_ERR(err);
+	check(!err);
 
 	err = reftable_iterator_next_ref(&it, &ref);
-	EXPECT_ERR(err);
-	EXPECT(ref.update_index == 2);
+	check(!err);
+	check_int(ref.update_index, ==, 2);
 	reftable_ref_record_release(&ref);
 	reftable_iterator_destroy(&it);
 	readers_destroy(readers, 2);
@@ -165,7 +170,7 @@ static void test_merged_between(void)
 	reftable_free(bs);
 }
 
-static void test_merged(void)
+static void t_merged_refs(void)
 {
 	struct reftable_ref_record r1[] = {
 		{
@@ -230,9 +235,9 @@ static void test_merged(void)
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
 	err = reftable_iterator_seek_ref(&it, "a");
-	EXPECT_ERR(err);
-	EXPECT(reftable_merged_table_hash_id(mt) == GIT_SHA1_FORMAT_ID);
-	EXPECT(reftable_merged_table_min_update_index(mt) == 1);
+	check(!err);
+	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
+	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
 
 	while (len < 100) { /* cap loops/recursion. */
 		struct reftable_ref_record ref = { NULL };
@@ -245,9 +250,9 @@ static void test_merged(void)
 	}
 	reftable_iterator_destroy(&it);
 
-	EXPECT(ARRAY_SIZE(want) == len);
+	check_int(ARRAY_SIZE(want), ==, len);
 	for (i = 0; i < len; i++) {
-		EXPECT(reftable_ref_record_equal(want[i], &out[i],
+		check(reftable_ref_record_equal(want[i], &out[i],
 						 GIT_SHA1_RAWSZ));
 	}
 	for (i = 0; i < len; i++) {
@@ -283,16 +288,16 @@ merged_table_from_log_records(struct reftable_log_record **logs,
 
 		err = reftable_new_reader(&(*readers)[i], &(*source)[i],
 					  "name");
-		EXPECT_ERR(err);
+		check(!err);
 		reftable_table_from_reader(&tabs[i], (*readers)[i]);
 	}
 
 	err = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
-	EXPECT_ERR(err);
+	check(!err);
 	return mt;
 }
 
-static void test_merged_logs(void)
+static void t_merged_logs(void)
 {
 	struct reftable_log_record r1[] = {
 		{
@@ -362,9 +367,9 @@ static void test_merged_logs(void)
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
 	err = reftable_iterator_seek_log(&it, "a");
-	EXPECT_ERR(err);
-	EXPECT(reftable_merged_table_hash_id(mt) == GIT_SHA1_FORMAT_ID);
-	EXPECT(reftable_merged_table_min_update_index(mt) == 1);
+	check(!err);
+	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
+	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
 
 	while (len < 100) { /* cap loops/recursion. */
 		struct reftable_log_record log = { NULL };
@@ -377,19 +382,19 @@ static void test_merged_logs(void)
 	}
 	reftable_iterator_destroy(&it);
 
-	EXPECT(ARRAY_SIZE(want) == len);
+	check_int(ARRAY_SIZE(want), ==, len);
 	for (i = 0; i < len; i++) {
-		EXPECT(reftable_log_record_equal(want[i], &out[i],
+		check(reftable_log_record_equal(want[i], &out[i],
 						 GIT_SHA1_RAWSZ));
 	}
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
 	err = reftable_iterator_seek_log_at(&it, "a", 2);
-	EXPECT_ERR(err);
+	check(!err);
 	reftable_log_record_release(&out[0]);
 	err = reftable_iterator_next_log(&it, &out[0]);
-	EXPECT_ERR(err);
-	EXPECT(reftable_log_record_equal(&out[0], &r3[0], GIT_SHA1_RAWSZ));
+	check(!err);
+	check(reftable_log_record_equal(&out[0], &r3[0], GIT_SHA1_RAWSZ));
 	reftable_iterator_destroy(&it);
 
 	for (i = 0; i < len; i++) {
@@ -405,7 +410,7 @@ static void test_merged_logs(void)
 	reftable_free(bs);
 }
 
-static void test_default_write_opts(void)
+static void t_default_write_opts(void)
 {
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
@@ -426,36 +431,36 @@ static void test_default_write_opts(void)
 	reftable_writer_set_limits(w, 1, 1);
 
 	err = reftable_writer_add_ref(w, &rec);
-	EXPECT_ERR(err);
+	check(!err);
 
 	err = reftable_writer_close(w);
-	EXPECT_ERR(err);
+	check(!err);
 	reftable_writer_free(w);
 
 	block_source_from_strbuf(&source, &buf);
 
 	err = reftable_new_reader(&rd, &source, "filename");
-	EXPECT_ERR(err);
+	check(!err);
 
 	hash_id = reftable_reader_hash_id(rd);
-	EXPECT(hash_id == GIT_SHA1_FORMAT_ID);
+	check_int(hash_id, ==, GIT_SHA1_FORMAT_ID);
 
 	reftable_table_from_reader(&tab[0], rd);
 	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA1_FORMAT_ID);
-	EXPECT_ERR(err);
+	check(!err);
 
 	reftable_reader_free(rd);
 	reftable_merged_table_free(merged);
 	strbuf_release(&buf);
 }
 
-/* XXX test refs_for(oid) */
 
-int merged_test_main(int argc, const char *argv[])
+int cmd_main(int argc, const char *argv[])
 {
-	RUN_TEST(test_merged_logs);
-	RUN_TEST(test_merged_between);
-	RUN_TEST(test_merged);
-	RUN_TEST(test_default_write_opts);
-	return 0;
+	TEST(t_default_write_opts(), "merged table with default write opts");
+	TEST(t_merged_logs(), "merged table with multiple log updates for same ref");
+	TEST(t_merged_refs(), "merged table with multiple updates to same ref");
+	TEST(t_merged_single_record(), "ref ocurring in only one record can be fetched");
+
+	return test_done();
 }
-- 
2.45.GIT

[PATCH v4 2/7] t: harmonize t-reftable-merged.c with coding guidelines

[Chandra Pratap]

Harmonize the newly ported test unit-tests/t-reftable-merged.c
with the following guidelines:
- Single line control flow statements like 'for' and 'if'
  must omit curly braces.
- Structs must be 0-initialized with '= { 0 }' instead of '= { NULL }'.
- Array indices should preferably be of type 'size_t', not 'int'.
- It is fine to use C99 initial declaration in 'for' loop.

While at it, use 'ARRAY_SIZE(x)' to store the number of elements
in an array instead of hardcoding them.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 68 +++++++++++++-------------------
 1 file changed, 28 insertions(+), 40 deletions(-)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index 78a864a54f..9791f53418 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -27,11 +27,11 @@ static int noop_flush(void *arg)
 }
 
 static void write_test_table(struct strbuf *buf,
-			     struct reftable_ref_record refs[], int n)
+			     struct reftable_ref_record refs[], size_t n)
 {
 	uint64_t min = 0xffffffff;
 	uint64_t max = 0;
-	int i = 0;
+	size_t i;
 	int err;
 
 	struct reftable_write_options opts = {
@@ -40,12 +40,10 @@ static void write_test_table(struct strbuf *buf,
 	struct reftable_writer *w = NULL;
 	for (i = 0; i < n; i++) {
 		uint64_t ui = refs[i].update_index;
-		if (ui > max) {
+		if (ui > max)
 			max = ui;
-		}
-		if (ui < min) {
+		if (ui < min)
 			min = ui;
-		}
 	}
 
 	w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
@@ -65,10 +63,9 @@ static void write_test_table(struct strbuf *buf,
 }
 
 static void write_test_log_table(struct strbuf *buf,
-				 struct reftable_log_record logs[], int n,
+				 struct reftable_log_record logs[], size_t n,
 				 uint64_t update_index)
 {
-	int i = 0;
 	int err;
 
 	struct reftable_write_options opts = {
@@ -79,7 +76,7 @@ static void write_test_log_table(struct strbuf *buf,
 	w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
 	reftable_writer_set_limits(w, update_index, update_index);
 
-	for (i = 0; i < n; i++) {
+	for (size_t i = 0; i < n; i++) {
 		int err = reftable_writer_add_log(w, &logs[i]);
 		check(!err);
 	}
@@ -93,7 +90,7 @@ static void write_test_log_table(struct strbuf *buf,
 static struct reftable_merged_table *
 merged_table_from_records(struct reftable_ref_record **refs,
 			  struct reftable_block_source **source,
-			  struct reftable_reader ***readers, int *sizes,
+			  struct reftable_reader ***readers, size_t *sizes,
 			  struct strbuf *buf, size_t n)
 {
 	struct reftable_merged_table *mt = NULL;
@@ -121,8 +118,7 @@ merged_table_from_records(struct reftable_ref_record **refs,
 
 static void readers_destroy(struct reftable_reader **readers, size_t n)
 {
-	int i = 0;
-	for (; i < n; i++)
+	for (size_t i = 0; i < n; i++)
 		reftable_reader_free(readers[i]);
 	reftable_free(readers);
 }
@@ -142,15 +138,14 @@ static void t_merged_single_record(void)
 	} };
 
 	struct reftable_ref_record *refs[] = { r1, r2 };
-	int sizes[] = { 1, 1 };
+	size_t sizes[] = { ARRAY_SIZE(r1), ARRAY_SIZE(r2) };
 	struct strbuf bufs[2] = { STRBUF_INIT, STRBUF_INIT };
 	struct reftable_block_source *bs = NULL;
 	struct reftable_reader **readers = NULL;
 	struct reftable_merged_table *mt =
 		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 2);
-	int i;
-	struct reftable_ref_record ref = { NULL };
-	struct reftable_iterator it = { NULL };
+	struct reftable_ref_record ref = { 0 };
+	struct reftable_iterator it = { 0 };
 	int err;
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
@@ -164,9 +159,8 @@ static void t_merged_single_record(void)
 	reftable_iterator_destroy(&it);
 	readers_destroy(readers, 2);
 	reftable_merged_table_free(mt);
-	for (i = 0; i < ARRAY_SIZE(bufs); i++) {
+	for (size_t i = 0; i < ARRAY_SIZE(bufs); i++)
 		strbuf_release(&bufs[i]);
-	}
 	reftable_free(bs);
 }
 
@@ -220,18 +214,18 @@ static void t_merged_refs(void)
 	};
 
 	struct reftable_ref_record *refs[] = { r1, r2, r3 };
-	int sizes[3] = { 3, 1, 2 };
+	size_t sizes[3] = { ARRAY_SIZE(r1), ARRAY_SIZE(r2), ARRAY_SIZE(r3) };
 	struct strbuf bufs[3] = { STRBUF_INIT, STRBUF_INIT, STRBUF_INIT };
 	struct reftable_block_source *bs = NULL;
 	struct reftable_reader **readers = NULL;
 	struct reftable_merged_table *mt =
 		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 3);
-	struct reftable_iterator it = { NULL };
+	struct reftable_iterator it = { 0 };
 	int err;
 	struct reftable_ref_record *out = NULL;
 	size_t len = 0;
 	size_t cap = 0;
-	int i = 0;
+	size_t i;
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
 	err = reftable_iterator_seek_ref(&it, "a");
@@ -240,7 +234,7 @@ static void t_merged_refs(void)
 	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
 
 	while (len < 100) { /* cap loops/recursion. */
-		struct reftable_ref_record ref = { NULL };
+		struct reftable_ref_record ref = { 0 };
 		int err = reftable_iterator_next_ref(&it, &ref);
 		if (err > 0)
 			break;
@@ -251,18 +245,15 @@ static void t_merged_refs(void)
 	reftable_iterator_destroy(&it);
 
 	check_int(ARRAY_SIZE(want), ==, len);
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		check(reftable_ref_record_equal(want[i], &out[i],
 						 GIT_SHA1_RAWSZ));
-	}
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		reftable_ref_record_release(&out[i]);
-	}
 	reftable_free(out);
 
-	for (i = 0; i < 3; i++) {
+	for (i = 0; i < 3; i++)
 		strbuf_release(&bufs[i]);
-	}
 	readers_destroy(readers, 3);
 	reftable_merged_table_free(mt);
 	reftable_free(bs);
@@ -271,7 +262,7 @@ static void t_merged_refs(void)
 static struct reftable_merged_table *
 merged_table_from_log_records(struct reftable_log_record **logs,
 			      struct reftable_block_source **source,
-			      struct reftable_reader ***readers, int *sizes,
+			      struct reftable_reader ***readers, size_t *sizes,
 			      struct strbuf *buf, size_t n)
 {
 	struct reftable_merged_table *mt = NULL;
@@ -352,18 +343,18 @@ static void t_merged_logs(void)
 	};
 
 	struct reftable_log_record *logs[] = { r1, r2, r3 };
-	int sizes[3] = { 2, 1, 1 };
+	size_t sizes[3] = { ARRAY_SIZE(r1), ARRAY_SIZE(r2), ARRAY_SIZE(r3) };
 	struct strbuf bufs[3] = { STRBUF_INIT, STRBUF_INIT, STRBUF_INIT };
 	struct reftable_block_source *bs = NULL;
 	struct reftable_reader **readers = NULL;
 	struct reftable_merged_table *mt = merged_table_from_log_records(
 		logs, &bs, &readers, sizes, bufs, 3);
-	struct reftable_iterator it = { NULL };
+	struct reftable_iterator it = { 0 };
 	int err;
 	struct reftable_log_record *out = NULL;
 	size_t len = 0;
 	size_t cap = 0;
-	int i = 0;
+	size_t i;
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
 	err = reftable_iterator_seek_log(&it, "a");
@@ -372,7 +363,7 @@ static void t_merged_logs(void)
 	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
 
 	while (len < 100) { /* cap loops/recursion. */
-		struct reftable_log_record log = { NULL };
+		struct reftable_log_record log = { 0 };
 		int err = reftable_iterator_next_log(&it, &log);
 		if (err > 0)
 			break;
@@ -383,10 +374,9 @@ static void t_merged_logs(void)
 	reftable_iterator_destroy(&it);
 
 	check_int(ARRAY_SIZE(want), ==, len);
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		check(reftable_log_record_equal(want[i], &out[i],
 						 GIT_SHA1_RAWSZ));
-	}
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
 	err = reftable_iterator_seek_log_at(&it, "a", 2);
@@ -397,14 +387,12 @@ static void t_merged_logs(void)
 	check(reftable_log_record_equal(&out[0], &r3[0], GIT_SHA1_RAWSZ));
 	reftable_iterator_destroy(&it);
 
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		reftable_log_record_release(&out[i]);
-	}
 	reftable_free(out);
 
-	for (i = 0; i < 3; i++) {
+	for (i = 0; i < 3; i++)
 		strbuf_release(&bufs[i]);
-	}
 	readers_destroy(readers, 3);
 	reftable_merged_table_free(mt);
 	reftable_free(bs);
@@ -422,7 +410,7 @@ static void t_default_write_opts(void)
 		.update_index = 1,
 	};
 	int err;
-	struct reftable_block_source source = { NULL };
+	struct reftable_block_source source = { 0 };
 	struct reftable_table *tab = reftable_calloc(1, sizeof(*tab));
 	uint32_t hash_id;
 	struct reftable_reader *rd = NULL;
-- 
2.45.GIT

[PATCH v4 3/7] t-reftable-merged: improve the test t_merged_single_record()

[Chandra Pratap]

In t-reftable-merged.c, the test t_merged_single_record() ensures
that a ref ('a') which occurs in only one of the records ('r2')
can be retrieved. Improve this test by adding another record 'r3'
to ensure that ref 'a' only occurs in 'r2' and that merged tables
don't simply read the last record.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index 9791f53418..f4c14c5d47 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -136,14 +136,19 @@ static void t_merged_single_record(void)
 		.update_index = 2,
 		.value_type = REFTABLE_REF_DELETION,
 	} };
+	struct reftable_ref_record r3[] = { {
+		.refname = (char *) "c",
+		.update_index = 3,
+		.value_type = REFTABLE_REF_DELETION,
+	} };
 
-	struct reftable_ref_record *refs[] = { r1, r2 };
-	size_t sizes[] = { ARRAY_SIZE(r1), ARRAY_SIZE(r2) };
-	struct strbuf bufs[2] = { STRBUF_INIT, STRBUF_INIT };
+	struct reftable_ref_record *refs[] = { r1, r2, r3 };
+	size_t sizes[] = { ARRAY_SIZE(r1), ARRAY_SIZE(r2), ARRAY_SIZE(r3) };
+	struct strbuf bufs[3] = { STRBUF_INIT, STRBUF_INIT, STRBUF_INIT };
 	struct reftable_block_source *bs = NULL;
 	struct reftable_reader **readers = NULL;
 	struct reftable_merged_table *mt =
-		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 2);
+		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 3);
 	struct reftable_ref_record ref = { 0 };
 	struct reftable_iterator it = { 0 };
 	int err;
@@ -157,7 +162,7 @@ static void t_merged_single_record(void)
 	check_int(ref.update_index, ==, 2);
 	reftable_ref_record_release(&ref);
 	reftable_iterator_destroy(&it);
-	readers_destroy(readers, 2);
+	readers_destroy(readers, 3);
 	reftable_merged_table_free(mt);
 	for (size_t i = 0; i < ARRAY_SIZE(bufs); i++)
 		strbuf_release(&bufs[i]);
-- 
2.45.GIT

[PATCH v4 4/7] t-reftable-merged: improve the const-correctness of helper functions

[Chandra Pratap]

In t-reftable-merged.c, a number of helper functions used by the
tests can be re-defined with parameters made 'const' which makes
it easier to understand if they're read-only or not. Re-define
these functions along these lines.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index f4c14c5d47..ff2f448bb6 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -15,7 +15,7 @@ license that can be found in the LICENSE file or at
 #include "reftable/reftable-merged.h"
 #include "reftable/reftable-writer.h"
 
-static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)
+static ssize_t strbuf_add_void(void *b, const void *data, const size_t sz)
 {
 	strbuf_add(b, data, sz);
 	return sz;
@@ -27,7 +27,7 @@ static int noop_flush(void *arg)
 }
 
 static void write_test_table(struct strbuf *buf,
-			     struct reftable_ref_record refs[], size_t n)
+			     struct reftable_ref_record refs[], const size_t n)
 {
 	uint64_t min = 0xffffffff;
 	uint64_t max = 0;
@@ -62,9 +62,8 @@ static void write_test_table(struct strbuf *buf,
 	reftable_writer_free(w);
 }
 
-static void write_test_log_table(struct strbuf *buf,
-				 struct reftable_log_record logs[], size_t n,
-				 uint64_t update_index)
+static void write_test_log_table(struct strbuf *buf, struct reftable_log_record logs[],
+				 const size_t n, const uint64_t update_index)
 {
 	int err;
 
@@ -90,8 +89,8 @@ static void write_test_log_table(struct strbuf *buf,
 static struct reftable_merged_table *
 merged_table_from_records(struct reftable_ref_record **refs,
 			  struct reftable_block_source **source,
-			  struct reftable_reader ***readers, size_t *sizes,
-			  struct strbuf *buf, size_t n)
+			  struct reftable_reader ***readers, const size_t *sizes,
+			  struct strbuf *buf, const size_t n)
 {
 	struct reftable_merged_table *mt = NULL;
 	struct reftable_table *tabs;
@@ -116,7 +115,7 @@ merged_table_from_records(struct reftable_ref_record **refs,
 	return mt;
 }
 
-static void readers_destroy(struct reftable_reader **readers, size_t n)
+static void readers_destroy(struct reftable_reader **readers, const size_t n)
 {
 	for (size_t i = 0; i < n; i++)
 		reftable_reader_free(readers[i]);
@@ -267,8 +266,8 @@ static void t_merged_refs(void)
 static struct reftable_merged_table *
 merged_table_from_log_records(struct reftable_log_record **logs,
 			      struct reftable_block_source **source,
-			      struct reftable_reader ***readers, size_t *sizes,
-			      struct strbuf *buf, size_t n)
+			      struct reftable_reader ***readers, const size_t *sizes,
+			      struct strbuf *buf, const size_t n)
 {
 	struct reftable_merged_table *mt = NULL;
 	struct reftable_table *tabs;
-- 
2.45.GIT

Re: [PATCH v4 4/7] t-reftable-merged: improve the const-correctness of helper functions

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1521 bytes --]

On Fri, Jul 12, 2024 at 11:09:00AM +0530, Chandra Pratap wrote:
> In t-reftable-merged.c, a number of helper functions used by the
> tests can be re-defined with parameters made 'const' which makes
> it easier to understand if they're read-only or not. Re-define
> these functions along these lines.
> 
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
>  t/unit-tests/t-reftable-merged.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
> index f4c14c5d47..ff2f448bb6 100644
> --- a/t/unit-tests/t-reftable-merged.c
> +++ b/t/unit-tests/t-reftable-merged.c
> @@ -15,7 +15,7 @@ license that can be found in the LICENSE file or at
>  #include "reftable/reftable-merged.h"
>  #include "reftable/reftable-writer.h"
>  
> -static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)
> +static ssize_t strbuf_add_void(void *b, const void *data, const size_t sz)

It is quite uncustomary for the Git codebase to mark such plain values
as `const`. While there is value in marking pointers as constant such
that the caller knows that the data it points to won't get modified,
there isn't really any value in marking pass-by-value parameters.

As far as I can see all changes relate to pass-by-value parameters, so
I'd rather drop this patch.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4 5/7] t-reftable-merged: add tests for reftable_merged_table_max_update_index

[Chandra Pratap]

reftable_merged_table_max_update_index() as defined by reftable/
merged.{c, h} returns the maximum update index in a merged table.
Since this function is currently unexercised, add tests for it.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index ff2f448bb6..065b359200 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -236,6 +236,7 @@ static void t_merged_refs(void)
 	check(!err);
 	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
 	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
+	check_int(reftable_merged_table_max_update_index(mt), ==, 3);
 
 	while (len < 100) { /* cap loops/recursion. */
 		struct reftable_ref_record ref = { 0 };
@@ -365,6 +366,7 @@ static void t_merged_logs(void)
 	check(!err);
 	check_int(reftable_merged_table_hash_id(mt), ==, GIT_SHA1_FORMAT_ID);
 	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
+	check_int(reftable_merged_table_max_update_index(mt), ==, 3);
 
 	while (len < 100) { /* cap loops/recursion. */
 		struct reftable_log_record log = { 0 };
-- 
2.45.GIT

[PATCH v4 6/7] t-reftable-merged: use reftable_ref_record_equal to compare ref records

[Chandra Pratap]

In the test t_merged_single_record() defined in t-reftable-merged.c,
the 'input' and 'expected' ref records are checked for equality
by comparing their update indices. It is very much possible for
two different ref records to have the same update indices. Use
reftable_ref_record_equal() instead for a stronger check.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index 065b359200..9f9275f871 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -158,7 +158,7 @@ static void t_merged_single_record(void)
 
 	err = reftable_iterator_next_ref(&it, &ref);
 	check(!err);
-	check_int(ref.update_index, ==, 2);
+	check(reftable_ref_record_equal(&r2[0], &ref, GIT_SHA1_RAWSZ));
 	reftable_ref_record_release(&ref);
 	reftable_iterator_destroy(&it);
 	readers_destroy(readers, 3);
-- 
2.45.GIT

[PATCH v4 7/7] t-reftable-merged: add test for REFTABLE_FORMAT_ERROR

[Chandra Pratap]

When calling reftable_new_merged_table(), if the hash ID of the
passed reftable_table parameter doesn't match the passed hash_id
parameter, a REFTABLE_FORMAT_ERROR is thrown. This case is
currently left unexercised, so add a test for the same.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index 9f9275f871..b6263ee8b5 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -11,6 +11,7 @@ license that can be found in the LICENSE file or at
 #include "reftable/constants.h"
 #include "reftable/merged.h"
 #include "reftable/reader.h"
+#include "reftable/reftable-error.h"
 #include "reftable/reftable-generic.h"
 #include "reftable/reftable-merged.h"
 #include "reftable/reftable-writer.h"
@@ -440,6 +441,8 @@ static void t_default_write_opts(void)
 	check_int(hash_id, ==, GIT_SHA1_FORMAT_ID);
 
 	reftable_table_from_reader(&tab[0], rd);
+	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA256_FORMAT_ID);
+	check_int(err, ==, REFTABLE_FORMAT_ERROR);
 	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA1_FORMAT_ID);
 	check(!err);
 
-- 
2.45.GIT