[GSoC][PATCH 00/10] t: port reftable/block_test.c to the unit testing framework

[GSoC][PATCH 00/10] t: port reftable/block_test.c to the unit testing framework

[Chandra Pratap]

The reftable library comes with self tests, which are exercised
as part of the usual end-to-end tests and are designed to
observe the end-user visible effects of Git commands. What it
exercises, however, is a better match for the unit-testing
framework, merged at 8bf6fbd0 (Merge branch 'js/doc-unit-tests',
2023-12-09), which is designed to observe how low level
implementation details, at the level of sequences of individual
function calls, behave.

Hence, port reftable/block_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/1749

Chandra Pratap(10):
t: move reftable/block_test.c to the unit testing framework
t-reftable-block: release used block reader
t-reftable-block: use reftable_record_equal() instead of check_str()
t-reftable-block: use reftable_record_key() instead of strbuf_addstr()
t-reftable-block: use block_iter_reset() instead of block_iter_close()
t-reftable-block: use xstrfmt() instead of xstrdup()
t-reftable-block: remove unnecessary variable 'j'
t-reftable-block: add tests for log blocks
t-reftable-block: add tests for obj blocks
t-reftable-block: add tests for index blocks

Makefile                        |   2 +-
reftable/block_test.c           | 123 --------------
reftable/reftable-tests.h       |   1 -
t/helper/test-reftable.c        |   1 -
t/unit-tests/t-reftable-block.c | 359 ++++++++++++++++++++++++++++++++++++++++
5 files changed, 360 insertions(+), 126 deletions(-)

[PATCH 01/10] t: move reftable/block_test.c to the unit testing framework

[Chandra Pratap]

reftable/block_test.c exercises the functions defined in
reftable/block.{c, h}. Migrate reftable/block_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 to follow the unit-tests'
naming conventions.

While at it, ensure structs are 0-initialized with '= { 0 }'
instead of '= { NULL }' and array indices have type 'size_t'
instead of 'int'.

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-block.c             | 63 +++++++++----------
 4 files changed, 30 insertions(+), 37 deletions(-)
 rename reftable/block_test.c => t/unit-tests/t-reftable-block.c (67%)

diff --git a/Makefile b/Makefile
index 3863e60b66..f030283447 100644
--- a/Makefile
+++ b/Makefile
@@ -1340,6 +1340,7 @@ UNIT_TEST_PROGRAMS += t-oidmap
 UNIT_TEST_PROGRAMS += t-oidtree
 UNIT_TEST_PROGRAMS += t-prio-queue
 UNIT_TEST_PROGRAMS += t-reftable-basics
+UNIT_TEST_PROGRAMS += t-reftable-block
 UNIT_TEST_PROGRAMS += t-reftable-merged
 UNIT_TEST_PROGRAMS += t-reftable-record
 UNIT_TEST_PROGRAMS += t-strbuf
@@ -2679,7 +2680,6 @@ REFTABLE_OBJS += reftable/stack.o
 REFTABLE_OBJS += reftable/tree.o
 REFTABLE_OBJS += reftable/writer.o
 
-REFTABLE_TEST_OBJS += reftable/block_test.o
 REFTABLE_TEST_OBJS += reftable/dump.o
 REFTABLE_TEST_OBJS += reftable/pq_test.o
 REFTABLE_TEST_OBJS += reftable/readwrite_test.o
diff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h
index d5e03dcc1b..e25ca86ede 100644
--- a/reftable/reftable-tests.h
+++ b/reftable/reftable-tests.h
@@ -10,7 +10,6 @@ license that can be found in the LICENSE file or at
 #define REFTABLE_TESTS_H
 
 int basics_test_main(int argc, const char **argv);
-int block_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 9d378427da..4eda545fad 100644
--- a/t/helper/test-reftable.c
+++ b/t/helper/test-reftable.c
@@ -5,7 +5,6 @@
 int cmd__reftable(int argc, const char **argv)
 {
 	/* test from simple to complex. */
-	block_test_main(argc, argv);
 	tree_test_main(argc, argv);
 	pq_test_main(argc, argv);
 	readwrite_test_main(argc, argv);
diff --git a/reftable/block_test.c b/t/unit-tests/t-reftable-block.c
similarity index 67%
rename from reftable/block_test.c
rename to t/unit-tests/t-reftable-block.c
index 90aecd5a7c..8ab3ff9ebe 100644
--- a/reftable/block_test.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -6,34 +6,30 @@ license that can be found in the LICENSE file or at
 https://developers.google.com/open-source/licenses/bsd
 */
 
-#include "block.h"
+#include "test-lib.h"
+#include "reftable/block.h"
+#include "reftable/blocksource.h"
+#include "reftable/constants.h"
+#include "reftable/reftable-error.h"
 
-#include "system.h"
-#include "blocksource.h"
-#include "basics.h"
-#include "constants.h"
-#include "record.h"
-#include "test_framework.h"
-#include "reftable-tests.h"
-
-static void test_block_read_write(void)
+static void t_block_read_write(void)
 {
 	const int header_off = 21; /* random */
 	char *names[30];
-	const int N = ARRAY_SIZE(names);
-	const int block_size = 1024;
-	struct reftable_block block = { NULL };
+	const size_t N = ARRAY_SIZE(names);
+	const size_t block_size = 1024;
+	struct reftable_block block = { 0 };
 	struct block_writer bw = {
 		.last_key = STRBUF_INIT,
 	};
 	struct reftable_record rec = {
 		.type = BLOCK_TYPE_REF,
 	};
-	int i = 0;
+	size_t i = 0;
 	int n;
 	struct block_reader br = { 0 };
 	struct block_iter it = BLOCK_ITER_INIT;
-	int j = 0;
+	size_t j = 0;
 	struct strbuf want = STRBUF_INIT;
 
 	REFTABLE_CALLOC_ARRAY(block.data, block_size);
@@ -45,11 +41,11 @@ static void test_block_read_write(void)
 	rec.u.ref.refname = (char *) "";
 	rec.u.ref.value_type = REFTABLE_REF_DELETION;
 	n = block_writer_add(&bw, &rec);
-	EXPECT(n == REFTABLE_API_ERROR);
+	check_int(n, ==, REFTABLE_API_ERROR);
 
 	for (i = 0; i < N; i++) {
 		char name[100];
-		snprintf(name, sizeof(name), "branch%02d", i);
+		snprintf(name, sizeof(name), "branch%02"PRIuMAX , (uintmax_t)i);
 
 		rec.u.ref.refname = name;
 		rec.u.ref.value_type = REFTABLE_REF_VAL1;
@@ -59,11 +55,11 @@ static void test_block_read_write(void)
 		n = block_writer_add(&bw, &rec);
 		rec.u.ref.refname = NULL;
 		rec.u.ref.value_type = REFTABLE_REF_DELETION;
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 	}
 
 	n = block_writer_finish(&bw);
-	EXPECT(n > 0);
+	check_int(n, >, 0);
 
 	block_writer_release(&bw);
 
@@ -73,11 +69,10 @@ static void test_block_read_write(void)
 
 	while (1) {
 		int r = block_iter_next(&it, &rec);
-		EXPECT(r >= 0);
-		if (r > 0) {
+		check_int(r, >=, 0);
+		if (r > 0)
 			break;
-		}
-		EXPECT_STREQ(names[j], rec.u.ref.refname);
+		check_str(names[j], rec.u.ref.refname);
 		j++;
 	}
 
@@ -90,20 +85,20 @@ static void test_block_read_write(void)
 		strbuf_addstr(&want, names[i]);
 
 		n = block_iter_seek_key(&it, &br, &want);
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 
 		n = block_iter_next(&it, &rec);
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 
-		EXPECT_STREQ(names[i], rec.u.ref.refname);
+		check_str(names[i], rec.u.ref.refname);
 
 		want.len--;
 		n = block_iter_seek_key(&it, &br, &want);
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 
 		n = block_iter_next(&it, &rec);
-		EXPECT(n == 0);
-		EXPECT_STREQ(names[10 * (i / 10)], rec.u.ref.refname);
+		check_int(n, ==, 0);
+		check_str(names[10 * (i / 10)], rec.u.ref.refname);
 
 		block_iter_close(&it);
 	}
@@ -111,13 +106,13 @@ static void test_block_read_write(void)
 	reftable_record_release(&rec);
 	reftable_block_done(&br.block);
 	strbuf_release(&want);
-	for (i = 0; i < N; i++) {
+	for (i = 0; i < N; i++)
 		reftable_free(names[i]);
-	}
 }
 
-int block_test_main(int argc, const char *argv[])
+int cmd_main(int argc, const char *argv[])
 {
-	RUN_TEST(test_block_read_write);
-	return 0;
+	TEST(t_block_read_write(), "read-write operations on blocks work");
+
+	return test_done();
 }
-- 
2.45.GIT

[PATCH 02/10] t-reftable-block: release used block reader

[Chandra Pratap]

Used block readers must be released using block_reader_release() to
prevent the occurence of a memory leak. Make test_block_read_write()
conform to this statement.

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-block.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index 8ab3ff9ebe..31d179a50a 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -103,6 +103,7 @@ static void t_block_read_write(void)
 		block_iter_close(&it);
 	}
 
+	block_reader_release(&br);
 	reftable_record_release(&rec);
 	reftable_block_done(&br.block);
 	strbuf_release(&want);
-- 
2.45.GIT

Re: [PATCH 02/10] t-reftable-block: release used block reader

[Patrick Steinhardt]

On Wed, Aug 14, 2024 at 05:33:10PM +0530, Chandra Pratap wrote:
> Used block readers must be released using block_reader_release() to
> prevent the occurence of a memory leak. Make test_block_read_write()
> conform to this statement.

Interesting. Didn't the old tests run with the leak checker enabled?

Patrick

Re: [PATCH 02/10] t-reftable-block: release used block reader

[Chandra Pratap]

On Thu, 15 Aug 2024 at 15:10, Patrick Steinhardt <ps@pks.im> wrote:
>
> On Wed, Aug 14, 2024 at 05:33:10PM +0530, Chandra Pratap wrote:
> > Used block readers must be released using block_reader_release() to
> > prevent the occurence of a memory leak. Make test_block_read_write()
> > conform to this statement.
>
> Interesting. Didn't the old tests run with the leak checker enabled?

I think not, I was able to find this error due to the GitHub CI.

[PATCH 03/10] t-reftable-block: use reftable_record_equal() instead of check_str()

[Chandra Pratap]

In the current testing setup, operations like read and write for
reftable blocks as defined by reftable/block.{c, h} are verified by
comparing only the keys of input and output reftable records. This is
not ideal because there can exist inequal reftable records with the
same key. Use the dedicated function for record comparison,
reftable_record_equal() instead of key-based comparison.

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-block.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index 31d179a50a..baeb9c8b07 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -15,8 +15,8 @@ license that can be found in the LICENSE file or at
 static void t_block_read_write(void)
 {
 	const int header_off = 21; /* random */
-	char *names[30];
-	const size_t N = ARRAY_SIZE(names);
+	struct reftable_record recs[30];
+	const size_t N = ARRAY_SIZE(recs);
 	const size_t block_size = 1024;
 	struct reftable_block block = { 0 };
 	struct block_writer bw = {
@@ -47,11 +47,11 @@ static void t_block_read_write(void)
 		char name[100];
 		snprintf(name, sizeof(name), "branch%02"PRIuMAX , (uintmax_t)i);
 
-		rec.u.ref.refname = name;
+		rec.u.ref.refname = xstrdup(name);
 		rec.u.ref.value_type = REFTABLE_REF_VAL1;
 		memset(rec.u.ref.value.val1, i, GIT_SHA1_RAWSZ);
 
-		names[i] = xstrdup(name);
+		recs[i] = rec;
 		n = block_writer_add(&bw, &rec);
 		rec.u.ref.refname = NULL;
 		rec.u.ref.value_type = REFTABLE_REF_DELETION;
@@ -72,7 +72,7 @@ static void t_block_read_write(void)
 		check_int(r, >=, 0);
 		if (r > 0)
 			break;
-		check_str(names[j], rec.u.ref.refname);
+		check(reftable_record_equal(&recs[j], &rec, GIT_SHA1_RAWSZ));
 		j++;
 	}
 
@@ -82,7 +82,7 @@ static void t_block_read_write(void)
 	for (i = 0; i < N; i++) {
 		struct block_iter it = BLOCK_ITER_INIT;
 		strbuf_reset(&want);
-		strbuf_addstr(&want, names[i]);
+		strbuf_addstr(&want, recs[i].u.ref.refname);
 
 		n = block_iter_seek_key(&it, &br, &want);
 		check_int(n, ==, 0);
@@ -90,7 +90,7 @@ static void t_block_read_write(void)
 		n = block_iter_next(&it, &rec);
 		check_int(n, ==, 0);
 
-		check_str(names[i], rec.u.ref.refname);
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
 
 		want.len--;
 		n = block_iter_seek_key(&it, &br, &want);
@@ -98,7 +98,7 @@ static void t_block_read_write(void)
 
 		n = block_iter_next(&it, &rec);
 		check_int(n, ==, 0);
-		check_str(names[10 * (i / 10)], rec.u.ref.refname);
+		check(reftable_record_equal(&recs[10 * (i / 10)], &rec, GIT_SHA1_RAWSZ));
 
 		block_iter_close(&it);
 	}
@@ -108,7 +108,7 @@ static void t_block_read_write(void)
 	reftable_block_done(&br.block);
 	strbuf_release(&want);
 	for (i = 0; i < N; i++)
-		reftable_free(names[i]);
+		reftable_record_release(&recs[i]);
 }
 
 int cmd_main(int argc, const char *argv[])
-- 
2.45.GIT

Re: [PATCH 03/10] t-reftable-block: use reftable_record_equal() instead of check_str()

[Patrick Steinhardt]

On Wed, Aug 14, 2024 at 05:33:11PM +0530, Chandra Pratap wrote:
> In the current testing setup, operations like read and write for
> reftable blocks as defined by reftable/block.{c, h} are verified by
> comparing only the keys of input and output reftable records. This is
> not ideal because there can exist inequal reftable records with the
> same key. Use the dedicated function for record comparison,
> reftable_record_equal() instead of key-based comparison.

Nit: there should probably be a comma after the closing brace.

> diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
> index 31d179a50a..baeb9c8b07 100644
> --- a/t/unit-tests/t-reftable-block.c
> +++ b/t/unit-tests/t-reftable-block.c
> @@ -15,8 +15,8 @@ license that can be found in the LICENSE file or at
>  static void t_block_read_write(void)
>  {
>  	const int header_off = 21; /* random */
> -	char *names[30];
> -	const size_t N = ARRAY_SIZE(names);
> +	struct reftable_record recs[30];
> +	const size_t N = ARRAY_SIZE(recs);
>  	const size_t block_size = 1024;
>  	struct reftable_block block = { 0 };
>  	struct block_writer bw = {
> @@ -47,11 +47,11 @@ static void t_block_read_write(void)
>  		char name[100];
>  		snprintf(name, sizeof(name), "branch%02"PRIuMAX , (uintmax_t)i);
>  
> -		rec.u.ref.refname = name;
> +		rec.u.ref.refname = xstrdup(name);
>  		rec.u.ref.value_type = REFTABLE_REF_VAL1;
>  		memset(rec.u.ref.value.val1, i, GIT_SHA1_RAWSZ);
>  
> -		names[i] = xstrdup(name);
> +		recs[i] = rec;
>  		n = block_writer_add(&bw, &rec);
>  		rec.u.ref.refname = NULL;
>  		rec.u.ref.value_type = REFTABLE_REF_DELETION;
> @@ -72,7 +72,7 @@ static void t_block_read_write(void)
>  		check_int(r, >=, 0);
>  		if (r > 0)
>  			break;
> -		check_str(names[j], rec.u.ref.refname);
> +		check(reftable_record_equal(&recs[j], &rec, GIT_SHA1_RAWSZ));
>  		j++;
>  	}

Okay. Because we're not only checking for the refname anymore, we now
need to store the expected records as full records, which also requires
us to allocate the refname. Makes sense.

> @@ -90,7 +90,7 @@ static void t_block_read_write(void)
>  		n = block_iter_next(&it, &rec);
>  		check_int(n, ==, 0);
>  
> -		check_str(names[i], rec.u.ref.refname);
> +		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
>  
>  		want.len--;
>  		n = block_iter_seek_key(&it, &br, &want);

It would of course be great if we didn't only verify that SHA1 works as
expected, but that we can also read and write SHA256 records. But that
would be a new addition to the test suite that doesn't have to be part
of this patch series.

Patrick

[PATCH 04/10] t-reftable-block: use reftable_record_key() instead of strbuf_addstr()

[Chandra Pratap]

In the current testing setup, the record key required for many block
iterator functions is manually stored in a strbuf struct and then
passed to these functions. This is not ideal when there exists a
dedicated function to encode a record's key into a strbuf, namely
reftable_record_key(). Use this function instead of manual encoding.

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-block.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index baeb9c8b07..0d73fb98d6 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -81,8 +81,7 @@ static void t_block_read_write(void)
 
 	for (i = 0; i < N; i++) {
 		struct block_iter it = BLOCK_ITER_INIT;
-		strbuf_reset(&want);
-		strbuf_addstr(&want, recs[i].u.ref.refname);
+		reftable_record_key(&recs[i], &want);
 
 		n = block_iter_seek_key(&it, &br, &want);
 		check_int(n, ==, 0);
-- 
2.45.GIT

Re: [PATCH 04/10] t-reftable-block: use reftable_record_key() instead of strbuf_addstr()

[Patrick Steinhardt]

On Wed, Aug 14, 2024 at 05:33:12PM +0530, Chandra Pratap wrote:
> In the current testing setup, the record key required for many block
> iterator functions is manually stored in a strbuf struct and then
> passed to these functions. This is not ideal when there exists a
> dedicated function to encode a record's key into a strbuf, namely
> reftable_record_key(). Use this function instead of manual encoding.
> 
> 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-block.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
> index baeb9c8b07..0d73fb98d6 100644
> --- a/t/unit-tests/t-reftable-block.c
> +++ b/t/unit-tests/t-reftable-block.c
> @@ -81,8 +81,7 @@ static void t_block_read_write(void)
>  
>  	for (i = 0; i < N; i++) {
>  		struct block_iter it = BLOCK_ITER_INIT;
> -		strbuf_reset(&want);
> -		strbuf_addstr(&want, recs[i].u.ref.refname);
> +		reftable_record_key(&recs[i], &want);
>  
>  		n = block_iter_seek_key(&it, &br, &want);
>  		check_int(n, ==, 0);

Yup, for ref records this is equivalent indeed, as their key only
consists of of the refname. It would be different for log records, which
also include the log index.

Patrick

[PATCH 05/10] t-reftable-block: use block_iter_reset() instead of block_iter_close()

[Chandra Pratap]

block_iter_reset() restores a block iterator to its state at the time
of initialization without freeing any memory while block_iter_close()
deallocates the memory for the iterator.

In the current testing setup, a block iterator is allocated and
deallocated for every iteration of a loop, which hurts performance.
Improve upon this by using block_iter_reset() at the start of each
iteration instead. This has the added benifit of testing
block_iter_reset(), which currently remains untested.

Similarly, remove reftable_record_release() for a reftable record
that is still in use.

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-block.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index 0d73fb98d6..dfb7262a65 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -76,11 +76,8 @@ static void t_block_read_write(void)
 		j++;
 	}
 
-	reftable_record_release(&rec);
-	block_iter_close(&it);
-
 	for (i = 0; i < N; i++) {
-		struct block_iter it = BLOCK_ITER_INIT;
+		block_iter_reset(&it);
 		reftable_record_key(&recs[i], &want);
 
 		n = block_iter_seek_key(&it, &br, &want);
@@ -98,11 +95,10 @@ static void t_block_read_write(void)
 		n = block_iter_next(&it, &rec);
 		check_int(n, ==, 0);
 		check(reftable_record_equal(&recs[10 * (i / 10)], &rec, GIT_SHA1_RAWSZ));
-
-		block_iter_close(&it);
 	}
 
 	block_reader_release(&br);
+	block_iter_close(&it);
 	reftable_record_release(&rec);
 	reftable_block_done(&br.block);
 	strbuf_release(&want);
-- 
2.45.GIT

Re: [PATCH 05/10] t-reftable-block: use block_iter_reset() instead of block_iter_close()

[Patrick Steinhardt]

On Wed, Aug 14, 2024 at 05:33:13PM +0530, Chandra Pratap wrote:
> block_iter_reset() restores a block iterator to its state at the time
> of initialization without freeing any memory while block_iter_close()
> deallocates the memory for the iterator.
> 
> In the current testing setup, a block iterator is allocated and
> deallocated for every iteration of a loop, which hurts performance.
> Improve upon this by using block_iter_reset() at the start of each
> iteration instead. This has the added benifit of testing
> block_iter_reset(), which currently remains untested.

I don't think that performance is a good argument, but exercising the
reset function certainly is.

> Similarly, remove reftable_record_release() for a reftable record
> that is still in use.

This is a welcome change, too, to verify that reading into the same
record multiple times does not leak memory and otherwise works as
expected.

Patrick

[PATCH 06/10] t-reftable-block: use xstrfmt() instead of xstrdup()

[Chandra Pratap]

Use xstrfmt() to assign a formatted string to a ref record's
refname instead of xstrdup(). This helps save the overhead of
a local 'char' buffer as well as makes the test more compact.

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-block.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index dfb7262a65..d762980589 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -44,10 +44,7 @@ static void t_block_read_write(void)
 	check_int(n, ==, REFTABLE_API_ERROR);
 
 	for (i = 0; i < N; i++) {
-		char name[100];
-		snprintf(name, sizeof(name), "branch%02"PRIuMAX , (uintmax_t)i);
-
-		rec.u.ref.refname = xstrdup(name);
+		rec.u.ref.refname = xstrfmt("branch%02"PRIuMAX , (uintmax_t)i);
 		rec.u.ref.value_type = REFTABLE_REF_VAL1;
 		memset(rec.u.ref.value.val1, i, GIT_SHA1_RAWSZ);
 
-- 
2.45.GIT

[PATCH 07/10] t-reftable-block: remove unnecessary variable 'j'

[Chandra Pratap]

Currently, there are two variables for array indices, 'i' and 'j'.
The variable 'j' is used only once and can be easily replaced with
'i'. Get rid of 'j' and replace its occurence with 'i'.

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-block.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index d762980589..fa289e10f2 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -29,7 +29,6 @@ static void t_block_read_write(void)
 	int n;
 	struct block_reader br = { 0 };
 	struct block_iter it = BLOCK_ITER_INIT;
-	size_t j = 0;
 	struct strbuf want = STRBUF_INIT;
 
 	REFTABLE_CALLOC_ARRAY(block.data, block_size);
@@ -64,13 +63,12 @@ static void t_block_read_write(void)
 
 	block_iter_seek_start(&it, &br);
 
-	while (1) {
+	for (i = 0; ; i++) {
 		int r = block_iter_next(&it, &rec);
 		check_int(r, >=, 0);
 		if (r > 0)
 			break;
-		check(reftable_record_equal(&recs[j], &rec, GIT_SHA1_RAWSZ));
-		j++;
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
 	}
 
 	for (i = 0; i < N; i++) {
-- 
2.45.GIT

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

[Chandra Pratap]

In the current testing setup, block operations are only exercised
for ref blocks. Add another test that exercises these operations
for log blocks as well.

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-block.c | 90 ++++++++++++++++++++++++++++++++-
 1 file changed, 88 insertions(+), 2 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index fa289e10f2..01ef10e7a6 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -12,7 +12,7 @@ license that can be found in the LICENSE file or at
 #include "reftable/constants.h"
 #include "reftable/reftable-error.h"
 
-static void t_block_read_write(void)
+static void t_ref_block_read_write(void)
 {
 	const int header_off = 21; /* random */
 	struct reftable_record recs[30];
@@ -101,9 +101,95 @@ static void t_block_read_write(void)
 		reftable_record_release(&recs[i]);
 }
 
+static void t_log_block_read_write(void)
+{
+	const int header_off = 21;
+	struct reftable_record recs[30];
+	const size_t N = ARRAY_SIZE(recs);
+	const size_t block_size = 2048;
+	struct reftable_block block = { 0 };
+	struct block_writer bw = {
+		.last_key = STRBUF_INIT,
+	};
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_LOG,
+	};
+	size_t i = 0;
+	int n;
+	struct block_reader br = { 0 };
+	struct block_iter it = BLOCK_ITER_INIT;
+	struct strbuf want = STRBUF_INIT;
+
+	REFTABLE_CALLOC_ARRAY(block.data, block_size);
+	block.len = block_size;
+	block.source = malloc_block_source();
+	block_writer_init(&bw, BLOCK_TYPE_LOG, block.data, block_size,
+			  header_off, hash_size(GIT_SHA1_FORMAT_ID));
+
+	for (i = 0; i < N; i++) {
+		rec.u.log.refname = xstrfmt("branch%02"PRIuMAX , (uintmax_t)i);
+		rec.u.log.update_index = i;
+		rec.u.log.value_type = REFTABLE_LOG_UPDATE;
+
+		recs[i] = rec;
+		n = block_writer_add(&bw, &rec);
+		rec.u.log.refname = NULL;
+		rec.u.log.value_type = REFTABLE_LOG_DELETION;
+		check_int(n, ==, 0);
+	}
+
+	n = block_writer_finish(&bw);
+	check_int(n, >, 0);
+
+	block_writer_release(&bw);
+
+	block_reader_init(&br, &block, header_off, block_size, GIT_SHA1_RAWSZ);
+
+	block_iter_seek_start(&it, &br);
+
+	for (i = 0; ; i++) {
+		int r = block_iter_next(&it, &rec);
+		check_int(r, >=, 0);
+		if (r > 0)
+			break;
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+	}
+
+	for (i = 0; i < N; i++) {
+		block_iter_reset(&it);
+		strbuf_reset(&want);
+		strbuf_addstr(&want, recs[i].u.log.refname);
+
+		n = block_iter_seek_key(&it, &br, &want);
+		check_int(n, ==, 0);
+
+		n = block_iter_next(&it, &rec);
+		check_int(n, ==, 0);
+
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+
+		want.len--;
+		n = block_iter_seek_key(&it, &br, &want);
+		check_int(n, ==, 0);
+
+		n = block_iter_next(&it, &rec);
+		check_int(n, ==, 0);
+		check(reftable_record_equal(&recs[10 * (i / 10)], &rec, GIT_SHA1_RAWSZ));
+	}
+
+	block_reader_release(&br);
+	block_iter_close(&it);
+	reftable_record_release(&rec);
+	reftable_block_done(&br.block);
+	strbuf_release(&want);
+	for (i = 0; i < N; i++)
+		reftable_record_release(&recs[i]);
+}
+
 int cmd_main(int argc, const char *argv[])
 {
-	TEST(t_block_read_write(), "read-write operations on blocks work");
+	TEST(t_log_block_read_write(), "read-write operations on log blocks work");
+	TEST(t_ref_block_read_write(), "read-write operations on ref blocks work");
 
 	return test_done();
 }
-- 
2.45.GIT

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

[Patrick Steinhardt]

On Wed, Aug 14, 2024 at 05:33:16PM +0530, Chandra Pratap wrote:
> @@ -101,9 +101,95 @@ static void t_block_read_write(void)
>  		reftable_record_release(&recs[i]);
>  }
>  
> +static void t_log_block_read_write(void)
> +{
> +	const int header_off = 21;
> +	struct reftable_record recs[30];
> +	const size_t N = ARRAY_SIZE(recs);
> +	const size_t block_size = 2048;
> +	struct reftable_block block = { 0 };
> +	struct block_writer bw = {
> +		.last_key = STRBUF_INIT,
> +	};
> +	struct reftable_record rec = {
> +		.type = BLOCK_TYPE_LOG,
> +	};
> +	size_t i = 0;
> +	int n;
> +	struct block_reader br = { 0 };
> +	struct block_iter it = BLOCK_ITER_INIT;
> +	struct strbuf want = STRBUF_INIT;
> +
> +	REFTABLE_CALLOC_ARRAY(block.data, block_size);
> +	block.len = block_size;
> +	block.source = malloc_block_source();
> +	block_writer_init(&bw, BLOCK_TYPE_LOG, block.data, block_size,
> +			  header_off, hash_size(GIT_SHA1_FORMAT_ID));
> +
> +	for (i = 0; i < N; i++) {
> +		rec.u.log.refname = xstrfmt("branch%02"PRIuMAX , (uintmax_t)i);
> +		rec.u.log.update_index = i;
> +		rec.u.log.value_type = REFTABLE_LOG_UPDATE;
> +
> +		recs[i] = rec;
> +		n = block_writer_add(&bw, &rec);
> +		rec.u.log.refname = NULL;
> +		rec.u.log.value_type = REFTABLE_LOG_DELETION;
> +		check_int(n, ==, 0);
> +	}
> +
> +	n = block_writer_finish(&bw);
> +	check_int(n, >, 0);

Do we maybe want to rename `n` to `ret`? That's way more customary in
our codebase.

> +	block_writer_release(&bw);
> +
> +	block_reader_init(&br, &block, header_off, block_size, GIT_SHA1_RAWSZ);
> +
> +	block_iter_seek_start(&it, &br);
> +
> +	for (i = 0; ; i++) {
> +		int r = block_iter_next(&it, &rec);
> +		check_int(r, >=, 0);
> +		if (r > 0)
> +			break;

We can also reuse `n` (or `ret`) here, right?

> +		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
> +	}

One thing that this loop doesn't verify is whether we actually got the
expected number of log records. It could be that the first iteration
already returns `r > 0`, which is not our expectation. So we should
likely add a check for `i == N` after the loop.

Patrick

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

[Chandra Pratap]

On Thu, 15 Aug 2024 at 15:11, Patrick Steinhardt <ps@pks.im> wrote:
>
> On Wed, Aug 14, 2024 at 05:33:16PM +0530, Chandra Pratap wrote:
> > @@ -101,9 +101,95 @@ static void t_block_read_write(void)
> >               reftable_record_release(&recs[i]);
> >  }
> >
> > +static void t_log_block_read_write(void)
> > +{
> > +     const int header_off = 21;
> > +     struct reftable_record recs[30];
> > +     const size_t N = ARRAY_SIZE(recs);
> > +     const size_t block_size = 2048;
> > +     struct reftable_block block = { 0 };
> > +     struct block_writer bw = {
> > +             .last_key = STRBUF_INIT,
> > +     };
> > +     struct reftable_record rec = {
> > +             .type = BLOCK_TYPE_LOG,
> > +     };
> > +     size_t i = 0;
> > +     int n;
> > +     struct block_reader br = { 0 };
> > +     struct block_iter it = BLOCK_ITER_INIT;
> > +     struct strbuf want = STRBUF_INIT;
> > +
> > +     REFTABLE_CALLOC_ARRAY(block.data, block_size);
> > +     block.len = block_size;
> > +     block.source = malloc_block_source();
> > +     block_writer_init(&bw, BLOCK_TYPE_LOG, block.data, block_size,
> > +                       header_off, hash_size(GIT_SHA1_FORMAT_ID));
> > +
> > +     for (i = 0; i < N; i++) {
> > +             rec.u.log.refname = xstrfmt("branch%02"PRIuMAX , (uintmax_t)i);
> > +             rec.u.log.update_index = i;
> > +             rec.u.log.value_type = REFTABLE_LOG_UPDATE;
> > +
> > +             recs[i] = rec;
> > +             n = block_writer_add(&bw, &rec);
> > +             rec.u.log.refname = NULL;
> > +             rec.u.log.value_type = REFTABLE_LOG_DELETION;
> > +             check_int(n, ==, 0);
> > +     }
> > +
> > +     n = block_writer_finish(&bw);
> > +     check_int(n, >, 0);
>
> Do we maybe want to rename `n` to `ret`? That's way more customary in
> our codebase.

Sure thing, but then I would want to change the existing test (which gets
renamed as t_ref_block_read_write) and I'm unsure of which patch would
be the most suitable for that change. Would it be fine to include that
change as a part of this patch?

> > +     block_writer_release(&bw);
> > +
> > +     block_reader_init(&br, &block, header_off, block_size, GIT_SHA1_RAWSZ);
> > +
> > +     block_iter_seek_start(&it, &br);
> > +
> > +     for (i = 0; ; i++) {
> > +             int r = block_iter_next(&it, &rec);
> > +             check_int(r, >=, 0);
> > +             if (r > 0)
> > +                     break;
>
> We can also reuse `n` (or `ret`) here, right?
>
> > +             check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
> > +     }
>
> One thing that this loop doesn't verify is whether we actually got the
> expected number of log records. It could be that the first iteration
> already returns `r > 0`, which is not our expectation. So we should
> likely add a check for `i == N` after the loop.

What about something like
if (r > 0) {
    check_int(i, ==, N);
    break;
}
That should achieve the same results if I'm not wrong.

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

[Patrick Steinhardt]

On Fri, Aug 16, 2024 at 12:06:47AM +0530, Chandra Pratap wrote:
> On Thu, 15 Aug 2024 at 15:11, Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Wed, Aug 14, 2024 at 05:33:16PM +0530, Chandra Pratap wrote:
> > > @@ -101,9 +101,95 @@ static void t_block_read_write(void)
> > >               reftable_record_release(&recs[i]);
> > >  }
> > >
> > > +static void t_log_block_read_write(void)
> > > +{
> > > +     const int header_off = 21;
> > > +     struct reftable_record recs[30];
> > > +     const size_t N = ARRAY_SIZE(recs);
> > > +     const size_t block_size = 2048;
> > > +     struct reftable_block block = { 0 };
> > > +     struct block_writer bw = {
> > > +             .last_key = STRBUF_INIT,
> > > +     };
> > > +     struct reftable_record rec = {
> > > +             .type = BLOCK_TYPE_LOG,
> > > +     };
> > > +     size_t i = 0;
> > > +     int n;
> > > +     struct block_reader br = { 0 };
> > > +     struct block_iter it = BLOCK_ITER_INIT;
> > > +     struct strbuf want = STRBUF_INIT;
> > > +
> > > +     REFTABLE_CALLOC_ARRAY(block.data, block_size);
> > > +     block.len = block_size;
> > > +     block.source = malloc_block_source();
> > > +     block_writer_init(&bw, BLOCK_TYPE_LOG, block.data, block_size,
> > > +                       header_off, hash_size(GIT_SHA1_FORMAT_ID));
> > > +
> > > +     for (i = 0; i < N; i++) {
> > > +             rec.u.log.refname = xstrfmt("branch%02"PRIuMAX , (uintmax_t)i);
> > > +             rec.u.log.update_index = i;
> > > +             rec.u.log.value_type = REFTABLE_LOG_UPDATE;
> > > +
> > > +             recs[i] = rec;
> > > +             n = block_writer_add(&bw, &rec);
> > > +             rec.u.log.refname = NULL;
> > > +             rec.u.log.value_type = REFTABLE_LOG_DELETION;
> > > +             check_int(n, ==, 0);
> > > +     }
> > > +
> > > +     n = block_writer_finish(&bw);
> > > +     check_int(n, >, 0);
> >
> > Do we maybe want to rename `n` to `ret`? That's way more customary in
> > our codebase.
> 
> Sure thing, but then I would want to change the existing test (which gets
> renamed as t_ref_block_read_write) and I'm unsure of which patch would
> be the most suitable for that change. Would it be fine to include that
> change as a part of this patch?

The way I'd do it is to first do the minimum required changes to port
the old test to the new testing framework, without any of the cleanups
to align code style. Then I'd put another commit on top that does all
the changes like removing braces, converting types and also adapting
names.

> > > +     block_writer_release(&bw);
> > > +
> > > +     block_reader_init(&br, &block, header_off, block_size, GIT_SHA1_RAWSZ);
> > > +
> > > +     block_iter_seek_start(&it, &br);
> > > +
> > > +     for (i = 0; ; i++) {
> > > +             int r = block_iter_next(&it, &rec);
> > > +             check_int(r, >=, 0);
> > > +             if (r > 0)
> > > +                     break;
> >
> > We can also reuse `n` (or `ret`) here, right?
> >
> > > +             check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
> > > +     }
> >
> > One thing that this loop doesn't verify is whether we actually got the
> > expected number of log records. It could be that the first iteration
> > already returns `r > 0`, which is not our expectation. So we should
> > likely add a check for `i == N` after the loop.
> 
> What about something like
> if (r > 0) {
>     check_int(i, ==, N);
>     break;
> }
> That should achieve the same results if I'm not wrong.

Yup, looks good to me.

Patrick

[PATCH 09/10] t-reftable-block: add tests for obj blocks

[Chandra Pratap]

In the current testing setup, block operations are left unexercised
for obj blocks. Add a test that exercises these operations for obj
blocks.

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-block.c | 79 +++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index 01ef10e7a6..34d37fe1a7 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -186,9 +186,88 @@ static void t_log_block_read_write(void)
 		reftable_record_release(&recs[i]);
 }
 
+static void t_obj_block_read_write(void)
+{
+	const int header_off = 21;
+	struct reftable_record recs[30];
+	const size_t N = ARRAY_SIZE(recs);
+	const size_t block_size = 1024;
+	struct reftable_block block = { 0 };
+	struct block_writer bw = {
+		.last_key = STRBUF_INIT,
+	};
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_OBJ,
+	};
+	size_t i = 0;
+	int n;
+	struct block_reader br = { 0 };
+	struct block_iter it = BLOCK_ITER_INIT;
+	struct strbuf want = STRBUF_INIT;
+
+	REFTABLE_CALLOC_ARRAY(block.data, block_size);
+	block.len = block_size;
+	block.source = malloc_block_source();
+	block_writer_init(&bw, BLOCK_TYPE_OBJ, block.data, block_size,
+			  header_off, hash_size(GIT_SHA1_FORMAT_ID));
+
+	for (i = 0; i < N; i++) {
+		uint8_t *bytes = reftable_malloc(sizeof(uint8_t[5]));
+		memcpy(bytes, (uint8_t[]){i, i+1, i+2, i+3, i+5}, sizeof(uint8_t[5]));
+
+		rec.u.obj.hash_prefix = bytes;
+		rec.u.obj.hash_prefix_len = 5;
+
+		recs[i] = rec;
+		n = block_writer_add(&bw, &rec);
+		rec.u.obj.hash_prefix = NULL;
+		rec.u.obj.hash_prefix_len = 0;
+		check_int(n, ==, 0);
+	}
+
+	n = block_writer_finish(&bw);
+	check_int(n, >, 0);
+
+	block_writer_release(&bw);
+
+	block_reader_init(&br, &block, header_off, block_size, GIT_SHA1_RAWSZ);
+
+	block_iter_seek_start(&it, &br);
+
+	for (i = 0; ; i++) {
+		int r = block_iter_next(&it, &rec);
+		check_int(r, >=, 0);
+		if (r > 0)
+			break;
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+	}
+
+	for (i = 0; i < N; i++) {
+		block_iter_reset(&it);
+		reftable_record_key(&recs[i], &want);
+
+		n = block_iter_seek_key(&it, &br, &want);
+		check_int(n, ==, 0);
+
+		n = block_iter_next(&it, &rec);
+		check_int(n, ==, 0);
+
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+	}
+
+	block_reader_release(&br);
+	block_iter_close(&it);
+	reftable_record_release(&rec);
+	reftable_block_done(&br.block);
+	strbuf_release(&want);
+	for (i = 0; i < N; i++)
+		reftable_record_release(&recs[i]);
+}
+
 int cmd_main(int argc, const char *argv[])
 {
 	TEST(t_log_block_read_write(), "read-write operations on log blocks work");
+	TEST(t_obj_block_read_write(), "read-write operations on obj blocks work");
 	TEST(t_ref_block_read_write(), "read-write operations on ref blocks work");
 
 	return test_done();
-- 
2.45.GIT

Re: [PATCH 09/10] t-reftable-block: add tests for obj blocks

[Patrick Steinhardt]

On Wed, Aug 14, 2024 at 05:33:17PM +0530, Chandra Pratap wrote:
> In the current testing setup, block operations are left unexercised
> for obj blocks. Add a test that exercises these operations for obj
> blocks.

Same remarks here as for the preceding commit.

> @@ -186,9 +186,88 @@ static void t_log_block_read_write(void)
>  		reftable_record_release(&recs[i]);
>  }
>  
> +static void t_obj_block_read_write(void)
> +{
> +	const int header_off = 21;
> +	struct reftable_record recs[30];
> +	const size_t N = ARRAY_SIZE(recs);
> +	const size_t block_size = 1024;
> +	struct reftable_block block = { 0 };
> +	struct block_writer bw = {
> +		.last_key = STRBUF_INIT,
> +	};
> +	struct reftable_record rec = {
> +		.type = BLOCK_TYPE_OBJ,
> +	};
> +	size_t i = 0;
> +	int n;
> +	struct block_reader br = { 0 };
> +	struct block_iter it = BLOCK_ITER_INIT;
> +	struct strbuf want = STRBUF_INIT;
> +
> +	REFTABLE_CALLOC_ARRAY(block.data, block_size);
> +	block.len = block_size;
> +	block.source = malloc_block_source();
> +	block_writer_init(&bw, BLOCK_TYPE_OBJ, block.data, block_size,
> +			  header_off, hash_size(GIT_SHA1_FORMAT_ID));
> +
> +	for (i = 0; i < N; i++) {
> +		uint8_t *bytes = reftable_malloc(sizeof(uint8_t[5]));
> +		memcpy(bytes, (uint8_t[]){i, i+1, i+2, i+3, i+5}, sizeof(uint8_t[5]));

From the top of my head I'm not sure whether we use inline-array
declarations like this anywhere. I'd rather just make it a separate
variable, which also allows us to get rid of the magic 5 via
`ARRAY_SIZE()`.

Patrick

Re: [PATCH 09/10] t-reftable-block: add tests for obj blocks

[Chandra Pratap]

On Thu, 15 Aug 2024 at 15:11, Patrick Steinhardt <ps@pks.im> wrote:
>
> On Wed, Aug 14, 2024 at 05:33:17PM +0530, Chandra Pratap wrote:
> > In the current testing setup, block operations are left unexercised
> > for obj blocks. Add a test that exercises these operations for obj
> > blocks.
>
> Same remarks here as for the preceding commit.
>
> > @@ -186,9 +186,88 @@ static void t_log_block_read_write(void)
> >               reftable_record_release(&recs[i]);
> >  }
> >
> > +static void t_obj_block_read_write(void)
> > +{
> > +     const int header_off = 21;
> > +     struct reftable_record recs[30];
> > +     const size_t N = ARRAY_SIZE(recs);
> > +     const size_t block_size = 1024;
> > +     struct reftable_block block = { 0 };
> > +     struct block_writer bw = {
> > +             .last_key = STRBUF_INIT,
> > +     };
> > +     struct reftable_record rec = {
> > +             .type = BLOCK_TYPE_OBJ,
> > +     };
> > +     size_t i = 0;
> > +     int n;
> > +     struct block_reader br = { 0 };
> > +     struct block_iter it = BLOCK_ITER_INIT;
> > +     struct strbuf want = STRBUF_INIT;
> > +
> > +     REFTABLE_CALLOC_ARRAY(block.data, block_size);
> > +     block.len = block_size;
> > +     block.source = malloc_block_source();
> > +     block_writer_init(&bw, BLOCK_TYPE_OBJ, block.data, block_size,
> > +                       header_off, hash_size(GIT_SHA1_FORMAT_ID));
> > +
> > +     for (i = 0; i < N; i++) {
> > +             uint8_t *bytes = reftable_malloc(sizeof(uint8_t[5]));
> > +             memcpy(bytes, (uint8_t[]){i, i+1, i+2, i+3, i+5}, sizeof(uint8_t[5]));
>
> From the top of my head I'm not sure whether we use inline-array
> declarations like this anywhere. I'd rather just make it a separate
> variable, which also allows us to get rid of the magic 5 via
> `ARRAY_SIZE()`.

We _do_ use inline array declarations like this, here's an example from
t/unit-tests/t-prio-queue.c:
TEST(TEST_INPUT(((int []){ STACK, 1, 2, 3, 4, 5, 6, REVERSE, DUMP }),
          ((int []){ 1, 2, 3, 4, 5, 6 })), "prio-queue works when LIFO
stack is reversed");

I did implement bytes[] as a local variable array when I first worked
on this patch but that turned out to be tricky due to variable scoping
and pointer semantics, so I ultimately settled on this approach.

Re: [PATCH 09/10] t-reftable-block: add tests for obj blocks

[Patrick Steinhardt]

On Fri, Aug 16, 2024 at 12:41:34AM +0530, Chandra Pratap wrote:
> On Thu, 15 Aug 2024 at 15:11, Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Wed, Aug 14, 2024 at 05:33:17PM +0530, Chandra Pratap wrote:
> > > In the current testing setup, block operations are left unexercised
> > > for obj blocks. Add a test that exercises these operations for obj
> > > blocks.
> >
> > Same remarks here as for the preceding commit.
> >
> > > @@ -186,9 +186,88 @@ static void t_log_block_read_write(void)
> > >               reftable_record_release(&recs[i]);
> > >  }
> > >
> > > +static void t_obj_block_read_write(void)
> > > +{
> > > +     const int header_off = 21;
> > > +     struct reftable_record recs[30];
> > > +     const size_t N = ARRAY_SIZE(recs);
> > > +     const size_t block_size = 1024;
> > > +     struct reftable_block block = { 0 };
> > > +     struct block_writer bw = {
> > > +             .last_key = STRBUF_INIT,
> > > +     };
> > > +     struct reftable_record rec = {
> > > +             .type = BLOCK_TYPE_OBJ,
> > > +     };
> > > +     size_t i = 0;
> > > +     int n;
> > > +     struct block_reader br = { 0 };
> > > +     struct block_iter it = BLOCK_ITER_INIT;
> > > +     struct strbuf want = STRBUF_INIT;
> > > +
> > > +     REFTABLE_CALLOC_ARRAY(block.data, block_size);
> > > +     block.len = block_size;
> > > +     block.source = malloc_block_source();
> > > +     block_writer_init(&bw, BLOCK_TYPE_OBJ, block.data, block_size,
> > > +                       header_off, hash_size(GIT_SHA1_FORMAT_ID));
> > > +
> > > +     for (i = 0; i < N; i++) {
> > > +             uint8_t *bytes = reftable_malloc(sizeof(uint8_t[5]));
> > > +             memcpy(bytes, (uint8_t[]){i, i+1, i+2, i+3, i+5}, sizeof(uint8_t[5]));
> >
> > From the top of my head I'm not sure whether we use inline-array
> > declarations like this anywhere. I'd rather just make it a separate
> > variable, which also allows us to get rid of the magic 5 via
> > `ARRAY_SIZE()`.
> 
> We _do_ use inline array declarations like this, here's an example from
> t/unit-tests/t-prio-queue.c:
> TEST(TEST_INPUT(((int []){ STACK, 1, 2, 3, 4, 5, 6, REVERSE, DUMP }),
>           ((int []){ 1, 2, 3, 4, 5, 6 })), "prio-queue works when LIFO
> stack is reversed");
> 
> I did implement bytes[] as a local variable array when I first worked
> on this patch but that turned out to be tricky due to variable scoping
> and pointer semantics, so I ultimately settled on this approach.

Oh, I didn't mean to say that you should _only_ use the local array.
Rather something like this:

        uint8_t[] bytes = { i, i + 1, i + 2, i + 3, i + 5 }, *allocated;
        DUP_ARRAY(allocated, bytes, ARRAY_SIZE(bytes));

Patrick

[PATCH 10/10] t-reftable-block: add tests for index blocks

[Chandra Pratap]

In the current testing setup, block operations are left unexercised
for index blocks. Add a test that exercises these operations for
index blocks.

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-block.c | 85 +++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index 34d37fe1a7..bbf6a5371e 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -264,8 +264,93 @@ static void t_obj_block_read_write(void)
 		reftable_record_release(&recs[i]);
 }
 
+static void t_index_block_read_write(void)
+{
+	const int header_off = 21;
+	struct reftable_record recs[30];
+	const size_t N = ARRAY_SIZE(recs);
+	const size_t block_size = 1024;
+	struct reftable_block block = { 0 };
+	struct block_writer bw = {
+		.last_key = STRBUF_INIT,
+	};
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_INDEX,
+		.u.idx.last_key = STRBUF_INIT,
+	};
+	size_t i = 0;
+	int n;
+	struct block_reader br = { 0 };
+	struct block_iter it = BLOCK_ITER_INIT;
+	struct strbuf want = STRBUF_INIT;
+
+	REFTABLE_CALLOC_ARRAY(block.data, block_size);
+	block.len = block_size;
+	block.source = malloc_block_source();
+	block_writer_init(&bw, BLOCK_TYPE_INDEX, block.data, block_size,
+			  header_off, hash_size(GIT_SHA1_FORMAT_ID));
+
+	for (i = 0; i < N; i++) {
+		strbuf_init(&recs[i].u.idx.last_key, 9);
+
+		recs[i].type = BLOCK_TYPE_INDEX;
+		strbuf_addf(&recs[i].u.idx.last_key, "branch%02"PRIuMAX, (uintmax_t)i);
+		recs[i].u.idx.offset = i;
+
+		n = block_writer_add(&bw, &recs[i]);
+		check_int(n, ==, 0);
+	}
+
+	n = block_writer_finish(&bw);
+	check_int(n, >, 0);
+
+	block_writer_release(&bw);
+
+	block_reader_init(&br, &block, header_off, block_size, GIT_SHA1_RAWSZ);
+
+	block_iter_seek_start(&it, &br);
+
+	for (i = 0; ; i++) {
+		int r = block_iter_next(&it, &rec);
+		check_int(r, >=, 0);
+		if (r > 0)
+			break;
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+	}
+
+	for (i = 0; i < N; i++) {
+		block_iter_reset(&it);
+		reftable_record_key(&recs[i], &want);
+
+		n = block_iter_seek_key(&it, &br, &want);
+		check_int(n, ==, 0);
+
+		n = block_iter_next(&it, &rec);
+		check_int(n, ==, 0);
+
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+
+		want.len--;
+		n = block_iter_seek_key(&it, &br, &want);
+		check_int(n, ==, 0);
+
+		n = block_iter_next(&it, &rec);
+		check_int(n, ==, 0);
+		check(reftable_record_equal(&recs[10 * (i / 10)], &rec, GIT_SHA1_RAWSZ));
+	}
+
+	block_reader_release(&br);
+	block_iter_close(&it);
+	reftable_record_release(&rec);
+	reftable_block_done(&br.block);
+	strbuf_release(&want);
+	for (i = 0; i < N; i++)
+		reftable_record_release(&recs[i]);
+}
+
 int cmd_main(int argc, const char *argv[])
 {
+	TEST(t_index_block_read_write(), "read-write operations on index blocks work");
 	TEST(t_log_block_read_write(), "read-write operations on log blocks work");
 	TEST(t_obj_block_read_write(), "read-write operations on obj blocks work");
 	TEST(t_ref_block_read_write(), "read-write operations on ref blocks work");
-- 
2.45.GIT

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

[Patrick Steinhardt]

On Wed, Aug 14, 2024 at 05:33:18PM +0530, Chandra Pratap wrote:
> In the current testing setup, block operations are left unexercised
> for index blocks. Add a test that exercises these operations for
> index blocks.

Again, the same remarks as for the preceding two patches apply here,
too.

Patrick

[GSoC][PATCH v2 00/11] t: port reftable/block_test.c to the unit testing framework

[Chandra Pratap]

The reftable library comes with self tests, which are exercised
as part of the usual end-to-end tests and are designed to
observe the end-user visible effects of Git commands. What it
exercises, however, is a better match for the unit-testing
framework, merged at 8bf6fbd0 (Merge branch 'js/doc-unit-tests',
2023-12-09), which is designed to observe how low level
implementation details, at the level of sequences of individual
function calls, behave.

Hence, port reftable/block_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:
- Split the first patch into two, one that moves the test to the
  unit testing framework and another that adds improvements.
- Fix a comma-error in the commit message of  patch 4
  (previously patch 3).
- Use 'ret' as the name for a variable that stores return codes
  rather than 'n' in all the tests.
- Add a check for 'i == N' in the block iterator loop in all
  the tests.
- Modify the obj test (patch 10) to no longer use magic numbers.
- Rebase the branch on top of the latest 'master' branch.

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

Chandra Pratap(11):
t: move reftable/block_test.c to the unit testing framework
t: harmonize t-reftable-block.c with coding guidelines
t-reftable-block: release used block reader
t-reftable-block: use reftable_record_equal() instead of check_str()
t-reftable-block: use reftable_record_key() instead of strbuf_addstr()
t-reftable-block: use block_iter_reset() instead of block_iter_close()
t-reftable-block: use xstrfmt() instead of xstrdup()
t-reftable-block: remove unnecessary variable 'j'
t-reftable-block: add tests for log blocks
t-reftable-block: add tests for obj blocks
t-reftable-block: add tests for index blocks

Makefile                        |   2 +-
reftable/block_test.c           | 123 ------------------------------
reftable/reftable-tests.h       |   1 -
t/helper/test-reftable.c        |   1 -
t/unit-tests/t-reftable-block.c | 368 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 369 insertions(+), 126 deletions(-)

Range-diff against v2:
<rebase commits>
 1:  0712c3df22 !  1:  c0d8c4149f t: move reftable/block_test.c to the unit testing framework
    @@ Commit message
         framework and renaming the tests to follow the unit-tests'
         naming conventions.

    -    While at it, ensure structs are 0-initialized with '= { 0 }'
    -    instead of '= { NULL }' and array indices have type 'size_t'
    -    instead of 'int'.
    -
         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-block.c: license that can be found in the LICENSE file o
      {
      	const int header_off = 21; /* random */
      	char *names[30];
    --	const int N = ARRAY_SIZE(names);
    --	const int block_size = 1024;
    --	struct reftable_block block = { NULL };
    -+	const size_t N = ARRAY_SIZE(names);
    -+	const size_t block_size = 1024;
    -+	struct reftable_block block = { 0 };
    - 	struct block_writer bw = {
    - 		.last_key = STRBUF_INIT,
    - 	};
    - 	struct reftable_record rec = {
    - 		.type = BLOCK_TYPE_REF,
    - 	};
    --	int i = 0;
    -+	size_t i = 0;
    - 	int n;
    - 	struct block_reader br = { 0 };
    - 	struct block_iter it = BLOCK_ITER_INIT;
    --	int j = 0;
    -+	size_t j = 0;
    - 	struct strbuf want = STRBUF_INIT;
    -
    - 	REFTABLE_CALLOC_ARRAY(block.data, block_size);
     @@ t/unit-tests/t-reftable-block.c: static void test_block_read_write(void)
      	rec.u.ref.refname = (char *) "";
      	rec.u.ref.value_type = REFTABLE_REF_DELETION;
    @@ t/unit-tests/t-reftable-block.c: static void test_block_read_write(void)

      	for (i = 0; i < N; i++) {
      		char name[100];
    --		snprintf(name, sizeof(name), "branch%02d", i);
    -+		snprintf(name, sizeof(name), "branch%02"PRIuMAX , (uintmax_t)i);
    -
    - 		rec.u.ref.refname = name;
    - 		rec.u.ref.value_type = REFTABLE_REF_VAL1;
     @@ t/unit-tests/t-reftable-block.c: static void test_block_read_write(void)
      		n = block_writer_add(&bw, &rec);
      		rec.u.ref.refname = NULL;
    @@ t/unit-tests/t-reftable-block.c: static void test_block_read_write(void)
      	while (1) {
      		int r = block_iter_next(&it, &rec);
     -		EXPECT(r >= 0);
    --		if (r > 0) {
     +		check_int(r, >=, 0);
    -+		if (r > 0)
    + 		if (r > 0) {
      			break;
    --		}
    + 		}
     -		EXPECT_STREQ(names[j], rec.u.ref.refname);
     +		check_str(names[j], rec.u.ref.refname);
      		j++;
    @@ t/unit-tests/t-reftable-block.c: static void test_block_read_write(void)
      		block_iter_close(&it);
      	}
     @@ t/unit-tests/t-reftable-block.c: static void test_block_read_write(void)
    - 	reftable_record_release(&rec);
    - 	reftable_block_done(&br.block);
    - 	strbuf_release(&want);
    --	for (i = 0; i < N; i++) {
    -+	for (i = 0; i < N; i++)
    - 		reftable_free(names[i]);
    --	}
    + 	}
      }

     -int block_test_main(int argc, const char *argv[])
 -:  ---------- >  2:  017658ddb3 t: harmonize t-reftable-block.c with coding guidelines
 2:  3282dbeba1 =  3:  2042e2fde0 t-reftable-block: release used block reader
 3:  5a3fea003a !  4:  853efb0c0b t-reftable-block: use reftable_record_equal() instead of check_str()
    @@ Commit message
         comparing only the keys of input and output reftable records. This is
         not ideal because there can exist inequal reftable records with the
         same key. Use the dedicated function for record comparison,
    -    reftable_record_equal() instead of key-based comparison.
    +    reftable_record_equal(), instead of key-based comparison.

         Mentored-by: Patrick Steinhardt <ps@pks.im>
         Mentored-by: Christian Couder <chriscool@tuxfamily.org>
    @@ t/unit-tests/t-reftable-block.c: license that can be found in the LICENSE file o
      	struct block_writer bw = {
     @@ t/unit-tests/t-reftable-block.c: static void t_block_read_write(void)
      		char name[100];
    - 		snprintf(name, sizeof(name), "branch%02"PRIuMAX , (uintmax_t)i);
    + 		snprintf(name, sizeof(name), "branch%02"PRIuMAX, (uintmax_t)i);

     -		rec.u.ref.refname = name;
     +		rec.u.ref.refname = xstrdup(name);
    @@ t/unit-tests/t-reftable-block.c: static void t_block_read_write(void)

     -		names[i] = xstrdup(name);
     +		recs[i] = rec;
    - 		n = block_writer_add(&bw, &rec);
    + 		ret = block_writer_add(&bw, &rec);
      		rec.u.ref.refname = NULL;
      		rec.u.ref.value_type = REFTABLE_REF_DELETION;
     @@ t/unit-tests/t-reftable-block.c: static void t_block_read_write(void)
    - 		check_int(r, >=, 0);
    - 		if (r > 0)
    + 			check_int(i, ==, N);
      			break;
    + 		}
     -		check_str(names[j], rec.u.ref.refname);
     +		check(reftable_record_equal(&recs[j], &rec, GIT_SHA1_RAWSZ));
      		j++;
    @@ t/unit-tests/t-reftable-block.c: static void t_block_read_write(void)
     -		strbuf_addstr(&want, names[i]);
     +		strbuf_addstr(&want, recs[i].u.ref.refname);

    - 		n = block_iter_seek_key(&it, &br, &want);
    - 		check_int(n, ==, 0);
    + 		ret = block_iter_seek_key(&it, &br, &want);
    + 		check_int(ret, ==, 0);
     @@ t/unit-tests/t-reftable-block.c: static void t_block_read_write(void)
    - 		n = block_iter_next(&it, &rec);
    - 		check_int(n, ==, 0);
    + 		ret = block_iter_next(&it, &rec);
    + 		check_int(ret, ==, 0);

     -		check_str(names[i], rec.u.ref.refname);
     +		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));

      		want.len--;
    - 		n = block_iter_seek_key(&it, &br, &want);
    + 		ret = block_iter_seek_key(&it, &br, &want);
     @@ t/unit-tests/t-reftable-block.c: static void t_block_read_write(void)

    - 		n = block_iter_next(&it, &rec);
    - 		check_int(n, ==, 0);
    + 		ret = block_iter_next(&it, &rec);
    + 		check_int(ret, ==, 0);
     -		check_str(names[10 * (i / 10)], rec.u.ref.refname);
     +		check(reftable_record_equal(&recs[10 * (i / 10)], &rec, GIT_SHA1_RAWSZ));

 4:  7ca77be75c !  5:  0de1e0cbe2 t-reftable-block: use reftable_record_key() instead of strbuf_addstr()
    @@ t/unit-tests/t-reftable-block.c: static void t_block_read_write(void)
     -		strbuf_addstr(&want, recs[i].u.ref.refname);
     +		reftable_record_key(&recs[i], &want);

    - 		n = block_iter_seek_key(&it, &br, &want);
    - 		check_int(n, ==, 0);
    + 		ret = block_iter_seek_key(&it, &br, &want);
    + 		check_int(ret, ==, 0);
 5:  0016d8828a !  6:  74187586c9 t-reftable-block: use block_iter_reset() instead of block_iter_close()
    @@ t/unit-tests/t-reftable-block.c: static void t_block_read_write(void)
     +		block_iter_reset(&it);
      		reftable_record_key(&recs[i], &want);

    - 		n = block_iter_seek_key(&it, &br, &want);
    + 		ret = block_iter_seek_key(&it, &br, &want);
     @@ t/unit-tests/t-reftable-block.c: static void t_block_read_write(void)
    - 		n = block_iter_next(&it, &rec);
    - 		check_int(n, ==, 0);
    + 		ret = block_iter_next(&it, &rec);
    + 		check_int(ret, ==, 0);
      		check(reftable_record_equal(&recs[10 * (i / 10)], &rec, GIT_SHA1_RAWSZ));
     -
     -		block_iter_close(&it);
 6:  49fe881f0e <  -:  ---------- t-reftable-block: use xstrfmt() instead of xstrdup()
 -:  ---------- >  7:  5e35cae987 t-reftable-block: use xstrfmt() instead of xstrdup()
 7:  5dbc756799 !  8:  bacd52a236 t-reftable-block: remove unnecessary variable 'j'
    @@ Commit message

      ## t/unit-tests/t-reftable-block.c ##
     @@ t/unit-tests/t-reftable-block.c: static void t_block_read_write(void)
    - 	int n;
    + 	int ret;
      	struct block_reader br = { 0 };
      	struct block_iter it = BLOCK_ITER_INIT;
     -	size_t j = 0;
    @@ t/unit-tests/t-reftable-block.c: static void t_block_read_write(void)

     -	while (1) {
     +	for (i = 0; ; i++) {
    - 		int r = block_iter_next(&it, &rec);
    - 		check_int(r, >=, 0);
    - 		if (r > 0)
    + 		ret = block_iter_next(&it, &rec);
    + 		check_int(ret, >=, 0);
    + 		if (ret > 0) {
    + 			check_int(i, ==, N);
      			break;
    + 		}
     -		check(reftable_record_equal(&recs[j], &rec, GIT_SHA1_RAWSZ));
     -		j++;
     +		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
 8:  0916b8b2ca !  9:  6e8632d3e6 t-reftable-block: add tests for log blocks
    @@ t/unit-tests/t-reftable-block.c: static void t_block_read_write(void)
     +		.type = BLOCK_TYPE_LOG,
     +	};
     +	size_t i = 0;
    -+	int n;
    ++	int ret;
     +	struct block_reader br = { 0 };
     +	struct block_iter it = BLOCK_ITER_INIT;
     +	struct strbuf want = STRBUF_INIT;
    @@ t/unit-tests/t-reftable-block.c: static void t_block_read_write(void)
     +		rec.u.log.value_type = REFTABLE_LOG_UPDATE;
     +
     +		recs[i] = rec;
    -+		n = block_writer_add(&bw, &rec);
    ++		ret = block_writer_add(&bw, &rec);
     +		rec.u.log.refname = NULL;
     +		rec.u.log.value_type = REFTABLE_LOG_DELETION;
    -+		check_int(n, ==, 0);
    ++		check_int(ret, ==, 0);
     +	}
     +
    -+	n = block_writer_finish(&bw);
    -+	check_int(n, >, 0);
    ++	ret = block_writer_finish(&bw);
    ++	check_int(ret, >, 0);
     +
     +	block_writer_release(&bw);
     +
    @@ t/unit-tests/t-reftable-block.c: static void t_block_read_write(void)
     +	block_iter_seek_start(&it, &br);
     +
     +	for (i = 0; ; i++) {
    -+		int r = block_iter_next(&it, &rec);
    -+		check_int(r, >=, 0);
    -+		if (r > 0)
    ++		ret = block_iter_next(&it, &rec);
    ++		check_int(ret, >=, 0);
    ++		if (ret > 0) {
    ++			check_int(i, ==, N);
     +			break;
    ++		}
     +		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
     +	}
     +
    @@ t/unit-tests/t-reftable-block.c: static void t_block_read_write(void)
     +		strbuf_reset(&want);
     +		strbuf_addstr(&want, recs[i].u.log.refname);
     +
    -+		n = block_iter_seek_key(&it, &br, &want);
    -+		check_int(n, ==, 0);
    ++		ret = block_iter_seek_key(&it, &br, &want);
    ++		check_int(ret, ==, 0);
     +
    -+		n = block_iter_next(&it, &rec);
    -+		check_int(n, ==, 0);
    ++		ret = block_iter_next(&it, &rec);
    ++		check_int(ret, ==, 0);
     +
     +		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
     +
     +		want.len--;
    -+		n = block_iter_seek_key(&it, &br, &want);
    -+		check_int(n, ==, 0);
    ++		ret = block_iter_seek_key(&it, &br, &want);
    ++		check_int(ret, ==, 0);
     +
    -+		n = block_iter_next(&it, &rec);
    -+		check_int(n, ==, 0);
    ++		ret = block_iter_next(&it, &rec);
    ++		check_int(ret, ==, 0);
     +		check(reftable_record_equal(&recs[10 * (i / 10)], &rec, GIT_SHA1_RAWSZ));
     +	}
     +
 9:  4794f2d610 ! 10:  d147d5e9a9 t-reftable-block: add tests for obj blocks
    @@ t/unit-tests/t-reftable-block.c: static void t_log_block_read_write(void)
     +		.type = BLOCK_TYPE_OBJ,
     +	};
     +	size_t i = 0;
    -+	int n;
    ++	int ret;
     +	struct block_reader br = { 0 };
     +	struct block_iter it = BLOCK_ITER_INIT;
     +	struct strbuf want = STRBUF_INIT;
    @@ t/unit-tests/t-reftable-block.c: static void t_log_block_read_write(void)
     +			  header_off, hash_size(GIT_SHA1_FORMAT_ID));
     +
     +	for (i = 0; i < N; i++) {
    -+		uint8_t *bytes = reftable_malloc(sizeof(uint8_t[5]));
    -+		memcpy(bytes, (uint8_t[]){i, i+1, i+2, i+3, i+5}, sizeof(uint8_t[5]));
    ++		uint8_t bytes[] = { i, i + 1, i + 2, i + 3, i + 5 }, *allocated;
    ++		allocated = reftable_malloc(ARRAY_SIZE(bytes));
    ++		DUP_ARRAY(allocated, bytes, ARRAY_SIZE(bytes));
     +
    -+		rec.u.obj.hash_prefix = bytes;
    ++		rec.u.obj.hash_prefix = allocated;
     +		rec.u.obj.hash_prefix_len = 5;
     +
     +		recs[i] = rec;
    -+		n = block_writer_add(&bw, &rec);
    ++		ret = block_writer_add(&bw, &rec);
     +		rec.u.obj.hash_prefix = NULL;
     +		rec.u.obj.hash_prefix_len = 0;
    -+		check_int(n, ==, 0);
    ++		check_int(ret, ==, 0);
     +	}
     +
    -+	n = block_writer_finish(&bw);
    -+	check_int(n, >, 0);
    ++	ret = block_writer_finish(&bw);
    ++	check_int(ret, >, 0);
     +
     +	block_writer_release(&bw);
     +
    @@ t/unit-tests/t-reftable-block.c: static void t_log_block_read_write(void)
     +	block_iter_seek_start(&it, &br);
     +
     +	for (i = 0; ; i++) {
    -+		int r = block_iter_next(&it, &rec);
    -+		check_int(r, >=, 0);
    -+		if (r > 0)
    ++		ret = block_iter_next(&it, &rec);
    ++		check_int(ret, >=, 0);
    ++		if (ret > 0) {
    ++			check_int(i, ==, N);
     +			break;
    ++		}
     +		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
     +	}
     +
    @@ t/unit-tests/t-reftable-block.c: static void t_log_block_read_write(void)
     +		block_iter_reset(&it);
     +		reftable_record_key(&recs[i], &want);
     +
    -+		n = block_iter_seek_key(&it, &br, &want);
    -+		check_int(n, ==, 0);
    ++		ret = block_iter_seek_key(&it, &br, &want);
    ++		check_int(ret, ==, 0);
     +
    -+		n = block_iter_next(&it, &rec);
    -+		check_int(n, ==, 0);
    ++		ret = block_iter_next(&it, &rec);
    ++		check_int(ret, ==, 0);
     +
     +		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
     +	}
10:  40fcc98173 ! 11:  2b43b1ac9a t-reftable-block: add tests for index blocks
    @@ t/unit-tests/t-reftable-block.c: static void t_obj_block_read_write(void)
     +		.u.idx.last_key = STRBUF_INIT,
     +	};
     +	size_t i = 0;
    -+	int n;
    ++	int ret;
     +	struct block_reader br = { 0 };
     +	struct block_iter it = BLOCK_ITER_INIT;
     +	struct strbuf want = STRBUF_INIT;
    @@ t/unit-tests/t-reftable-block.c: static void t_obj_block_read_write(void)
     +		strbuf_addf(&recs[i].u.idx.last_key, "branch%02"PRIuMAX, (uintmax_t)i);
     +		recs[i].u.idx.offset = i;
     +
    -+		n = block_writer_add(&bw, &recs[i]);
    -+		check_int(n, ==, 0);
    ++		ret = block_writer_add(&bw, &recs[i]);
    ++		check_int(ret, ==, 0);
     +	}
     +
    -+	n = block_writer_finish(&bw);
    -+	check_int(n, >, 0);
    ++	ret = block_writer_finish(&bw);
    ++	check_int(ret, >, 0);
     +
     +	block_writer_release(&bw);
     +
    @@ t/unit-tests/t-reftable-block.c: static void t_obj_block_read_write(void)
     +	block_iter_seek_start(&it, &br);
     +
     +	for (i = 0; ; i++) {
    -+		int r = block_iter_next(&it, &rec);
    -+		check_int(r, >=, 0);
    -+		if (r > 0)
    ++		ret = block_iter_next(&it, &rec);
    ++		check_int(ret, >=, 0);
    ++		if (ret > 0) {
    ++			check_int(i, ==, N);
     +			break;
    ++		}
     +		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
     +	}
     +
    @@ t/unit-tests/t-reftable-block.c: static void t_obj_block_read_write(void)
     +		block_iter_reset(&it);
     +		reftable_record_key(&recs[i], &want);
     +
    -+		n = block_iter_seek_key(&it, &br, &want);
    -+		check_int(n, ==, 0);
    ++		ret = block_iter_seek_key(&it, &br, &want);
    ++		check_int(ret, ==, 0);
     +
    -+		n = block_iter_next(&it, &rec);
    -+		check_int(n, ==, 0);
    ++		ret = block_iter_next(&it, &rec);
    ++		check_int(ret, ==, 0);
     +
     +		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
     +
     +		want.len--;
    -+		n = block_iter_seek_key(&it, &br, &want);
    -+		check_int(n, ==, 0);
    ++		ret = block_iter_seek_key(&it, &br, &want);
    ++		check_int(ret, ==, 0);
     +
    -+		n = block_iter_next(&it, &rec);
    -+		check_int(n, ==, 0);
    ++		ret = block_iter_next(&it, &rec);
    ++		check_int(ret, ==, 0);
     +		check(reftable_record_equal(&recs[10 * (i / 10)], &rec, GIT_SHA1_RAWSZ));
     +	}
     +

[PATCH v2 01/11] t: move reftable/block_test.c to the unit testing framework

[Chandra Pratap]

reftable/block_test.c exercises the functions defined in
reftable/block.{c, h}. Migrate reftable/block_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 to follow the unit-tests'
naming conventions.

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-block.c             | 45 +++++++++----------
 4 files changed, 22 insertions(+), 27 deletions(-)
 rename reftable/block_test.c => t/unit-tests/t-reftable-block.c (76%)

diff --git a/Makefile b/Makefile
index 13890710f8..a30cd636f8 100644
--- a/Makefile
+++ b/Makefile
@@ -1340,6 +1340,7 @@ UNIT_TEST_PROGRAMS += t-oidmap
 UNIT_TEST_PROGRAMS += t-oidtree
 UNIT_TEST_PROGRAMS += t-prio-queue
 UNIT_TEST_PROGRAMS += t-reftable-basics
+UNIT_TEST_PROGRAMS += t-reftable-block
 UNIT_TEST_PROGRAMS += t-reftable-merged
 UNIT_TEST_PROGRAMS += t-reftable-pq
 UNIT_TEST_PROGRAMS += t-reftable-record
@@ -2681,7 +2682,6 @@ REFTABLE_OBJS += reftable/stack.o
 REFTABLE_OBJS += reftable/tree.o
 REFTABLE_OBJS += reftable/writer.o
 
-REFTABLE_TEST_OBJS += reftable/block_test.o
 REFTABLE_TEST_OBJS += reftable/dump.o
 REFTABLE_TEST_OBJS += reftable/readwrite_test.o
 REFTABLE_TEST_OBJS += reftable/stack_test.o
diff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h
index 4b666810af..3d9118b91b 100644
--- a/reftable/reftable-tests.h
+++ b/reftable/reftable-tests.h
@@ -10,7 +10,6 @@ license that can be found in the LICENSE file or at
 #define REFTABLE_TESTS_H
 
 int basics_test_main(int argc, const char **argv);
-int block_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);
 int stack_test_main(int argc, const char **argv);
diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
index 623cf3f0f5..7bdd18430b 100644
--- a/t/helper/test-reftable.c
+++ b/t/helper/test-reftable.c
@@ -5,7 +5,6 @@
 int cmd__reftable(int argc, const char **argv)
 {
 	/* test from simple to complex. */
-	block_test_main(argc, argv);
 	readwrite_test_main(argc, argv);
 	stack_test_main(argc, argv);
 	return 0;
diff --git a/reftable/block_test.c b/t/unit-tests/t-reftable-block.c
similarity index 76%
rename from reftable/block_test.c
rename to t/unit-tests/t-reftable-block.c
index 90aecd5a7c..f2b9a8a6f4 100644
--- a/reftable/block_test.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -6,17 +6,13 @@ license that can be found in the LICENSE file or at
 https://developers.google.com/open-source/licenses/bsd
 */
 
-#include "block.h"
+#include "test-lib.h"
+#include "reftable/block.h"
+#include "reftable/blocksource.h"
+#include "reftable/constants.h"
+#include "reftable/reftable-error.h"
 
-#include "system.h"
-#include "blocksource.h"
-#include "basics.h"
-#include "constants.h"
-#include "record.h"
-#include "test_framework.h"
-#include "reftable-tests.h"
-
-static void test_block_read_write(void)
+static void t_block_read_write(void)
 {
 	const int header_off = 21; /* random */
 	char *names[30];
@@ -45,7 +41,7 @@ static void test_block_read_write(void)
 	rec.u.ref.refname = (char *) "";
 	rec.u.ref.value_type = REFTABLE_REF_DELETION;
 	n = block_writer_add(&bw, &rec);
-	EXPECT(n == REFTABLE_API_ERROR);
+	check_int(n, ==, REFTABLE_API_ERROR);
 
 	for (i = 0; i < N; i++) {
 		char name[100];
@@ -59,11 +55,11 @@ static void test_block_read_write(void)
 		n = block_writer_add(&bw, &rec);
 		rec.u.ref.refname = NULL;
 		rec.u.ref.value_type = REFTABLE_REF_DELETION;
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 	}
 
 	n = block_writer_finish(&bw);
-	EXPECT(n > 0);
+	check_int(n, >, 0);
 
 	block_writer_release(&bw);
 
@@ -73,11 +69,11 @@ static void test_block_read_write(void)
 
 	while (1) {
 		int r = block_iter_next(&it, &rec);
-		EXPECT(r >= 0);
+		check_int(r, >=, 0);
 		if (r > 0) {
 			break;
 		}
-		EXPECT_STREQ(names[j], rec.u.ref.refname);
+		check_str(names[j], rec.u.ref.refname);
 		j++;
 	}
 
@@ -90,20 +86,20 @@ static void test_block_read_write(void)
 		strbuf_addstr(&want, names[i]);
 
 		n = block_iter_seek_key(&it, &br, &want);
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 
 		n = block_iter_next(&it, &rec);
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 
-		EXPECT_STREQ(names[i], rec.u.ref.refname);
+		check_str(names[i], rec.u.ref.refname);
 
 		want.len--;
 		n = block_iter_seek_key(&it, &br, &want);
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 
 		n = block_iter_next(&it, &rec);
-		EXPECT(n == 0);
-		EXPECT_STREQ(names[10 * (i / 10)], rec.u.ref.refname);
+		check_int(n, ==, 0);
+		check_str(names[10 * (i / 10)], rec.u.ref.refname);
 
 		block_iter_close(&it);
 	}
@@ -116,8 +112,9 @@ static void test_block_read_write(void)
 	}
 }
 
-int block_test_main(int argc, const char *argv[])
+int cmd_main(int argc, const char *argv[])
 {
-	RUN_TEST(test_block_read_write);
-	return 0;
+	TEST(t_block_read_write(), "read-write operations on blocks work");
+
+	return test_done();
 }
-- 
2.45.GIT

[PATCH v2 02/11] t: harmonize t-reftable-block.c with coding guidelines

[Chandra Pratap]

Harmonize the newly ported test unit-tests/t-reftable-block.c
with the following guidelines:
- Single line 'for' statements must omit curly braces.
- Structs must be 0-initialized with '= { 0 }' instead of '= { NULL }'.
- Array sizes and indices should preferably be of type 'size_t'and
  not 'int'.
- Return code variable should preferably be named 'ret', not 'n'.

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-block.c | 52 ++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index f2b9a8a6f4..b1b238ac2a 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -16,20 +16,20 @@ static void t_block_read_write(void)
 {
 	const int header_off = 21; /* random */
 	char *names[30];
-	const int N = ARRAY_SIZE(names);
-	const int block_size = 1024;
-	struct reftable_block block = { NULL };
+	const size_t N = ARRAY_SIZE(names);
+	const size_t block_size = 1024;
+	struct reftable_block block = { 0 };
 	struct block_writer bw = {
 		.last_key = STRBUF_INIT,
 	};
 	struct reftable_record rec = {
 		.type = BLOCK_TYPE_REF,
 	};
-	int i = 0;
-	int n;
+	size_t i = 0;
+	int ret;
 	struct block_reader br = { 0 };
 	struct block_iter it = BLOCK_ITER_INIT;
-	int j = 0;
+	size_t j = 0;
 	struct strbuf want = STRBUF_INIT;
 
 	REFTABLE_CALLOC_ARRAY(block.data, block_size);
@@ -40,26 +40,26 @@ static void t_block_read_write(void)
 
 	rec.u.ref.refname = (char *) "";
 	rec.u.ref.value_type = REFTABLE_REF_DELETION;
-	n = block_writer_add(&bw, &rec);
-	check_int(n, ==, REFTABLE_API_ERROR);
+	ret = block_writer_add(&bw, &rec);
+	check_int(ret, ==, REFTABLE_API_ERROR);
 
 	for (i = 0; i < N; i++) {
 		char name[100];
-		snprintf(name, sizeof(name), "branch%02d", i);
+		snprintf(name, sizeof(name), "branch%02"PRIuMAX, (uintmax_t)i);
 
 		rec.u.ref.refname = name;
 		rec.u.ref.value_type = REFTABLE_REF_VAL1;
 		memset(rec.u.ref.value.val1, i, GIT_SHA1_RAWSZ);
 
 		names[i] = xstrdup(name);
-		n = block_writer_add(&bw, &rec);
+		ret = block_writer_add(&bw, &rec);
 		rec.u.ref.refname = NULL;
 		rec.u.ref.value_type = REFTABLE_REF_DELETION;
-		check_int(n, ==, 0);
+		check_int(ret, ==, 0);
 	}
 
-	n = block_writer_finish(&bw);
-	check_int(n, >, 0);
+	ret = block_writer_finish(&bw);
+	check_int(ret, >, 0);
 
 	block_writer_release(&bw);
 
@@ -68,9 +68,10 @@ static void t_block_read_write(void)
 	block_iter_seek_start(&it, &br);
 
 	while (1) {
-		int r = block_iter_next(&it, &rec);
-		check_int(r, >=, 0);
-		if (r > 0) {
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, >=, 0);
+		if (ret > 0) {
+			check_int(i, ==, N);
 			break;
 		}
 		check_str(names[j], rec.u.ref.refname);
@@ -85,20 +86,20 @@ static void t_block_read_write(void)
 		strbuf_reset(&want);
 		strbuf_addstr(&want, names[i]);
 
-		n = block_iter_seek_key(&it, &br, &want);
-		check_int(n, ==, 0);
+		ret = block_iter_seek_key(&it, &br, &want);
+		check_int(ret, ==, 0);
 
-		n = block_iter_next(&it, &rec);
-		check_int(n, ==, 0);
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, ==, 0);
 
 		check_str(names[i], rec.u.ref.refname);
 
 		want.len--;
-		n = block_iter_seek_key(&it, &br, &want);
-		check_int(n, ==, 0);
+		ret = block_iter_seek_key(&it, &br, &want);
+		check_int(ret, ==, 0);
 
-		n = block_iter_next(&it, &rec);
-		check_int(n, ==, 0);
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, ==, 0);
 		check_str(names[10 * (i / 10)], rec.u.ref.refname);
 
 		block_iter_close(&it);
@@ -107,9 +108,8 @@ static void t_block_read_write(void)
 	reftable_record_release(&rec);
 	reftable_block_done(&br.block);
 	strbuf_release(&want);
-	for (i = 0; i < N; i++) {
+	for (i = 0; i < N; i++)
 		reftable_free(names[i]);
-	}
 }
 
 int cmd_main(int argc, const char *argv[])
-- 
2.45.GIT

[PATCH v2 03/11] t-reftable-block: release used block reader

[Chandra Pratap]

Used block readers must be released using block_reader_release() to
prevent the occurence of a memory leak. Make test_block_read_write()
conform to this statement.

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-block.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index b1b238ac2a..eafe1fdee9 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -105,6 +105,7 @@ static void t_block_read_write(void)
 		block_iter_close(&it);
 	}
 
+	block_reader_release(&br);
 	reftable_record_release(&rec);
 	reftable_block_done(&br.block);
 	strbuf_release(&want);
-- 
2.45.GIT

[PATCH v2 04/11] t-reftable-block: use reftable_record_equal() instead of check_str()

[Chandra Pratap]

In the current testing setup, operations like read and write for
reftable blocks as defined by reftable/block.{c, h} are verified by
comparing only the keys of input and output reftable records. This is
not ideal because there can exist inequal reftable records with the
same key. Use the dedicated function for record comparison,
reftable_record_equal(), instead of key-based comparison.

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-block.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index eafe1fdee9..b106d3c1e4 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -15,8 +15,8 @@ license that can be found in the LICENSE file or at
 static void t_block_read_write(void)
 {
 	const int header_off = 21; /* random */
-	char *names[30];
-	const size_t N = ARRAY_SIZE(names);
+	struct reftable_record recs[30];
+	const size_t N = ARRAY_SIZE(recs);
 	const size_t block_size = 1024;
 	struct reftable_block block = { 0 };
 	struct block_writer bw = {
@@ -47,11 +47,11 @@ static void t_block_read_write(void)
 		char name[100];
 		snprintf(name, sizeof(name), "branch%02"PRIuMAX, (uintmax_t)i);
 
-		rec.u.ref.refname = name;
+		rec.u.ref.refname = xstrdup(name);
 		rec.u.ref.value_type = REFTABLE_REF_VAL1;
 		memset(rec.u.ref.value.val1, i, GIT_SHA1_RAWSZ);
 
-		names[i] = xstrdup(name);
+		recs[i] = rec;
 		ret = block_writer_add(&bw, &rec);
 		rec.u.ref.refname = NULL;
 		rec.u.ref.value_type = REFTABLE_REF_DELETION;
@@ -74,7 +74,7 @@ static void t_block_read_write(void)
 			check_int(i, ==, N);
 			break;
 		}
-		check_str(names[j], rec.u.ref.refname);
+		check(reftable_record_equal(&recs[j], &rec, GIT_SHA1_RAWSZ));
 		j++;
 	}
 
@@ -84,7 +84,7 @@ static void t_block_read_write(void)
 	for (i = 0; i < N; i++) {
 		struct block_iter it = BLOCK_ITER_INIT;
 		strbuf_reset(&want);
-		strbuf_addstr(&want, names[i]);
+		strbuf_addstr(&want, recs[i].u.ref.refname);
 
 		ret = block_iter_seek_key(&it, &br, &want);
 		check_int(ret, ==, 0);
@@ -92,7 +92,7 @@ static void t_block_read_write(void)
 		ret = block_iter_next(&it, &rec);
 		check_int(ret, ==, 0);
 
-		check_str(names[i], rec.u.ref.refname);
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
 
 		want.len--;
 		ret = block_iter_seek_key(&it, &br, &want);
@@ -100,7 +100,7 @@ static void t_block_read_write(void)
 
 		ret = block_iter_next(&it, &rec);
 		check_int(ret, ==, 0);
-		check_str(names[10 * (i / 10)], rec.u.ref.refname);
+		check(reftable_record_equal(&recs[10 * (i / 10)], &rec, GIT_SHA1_RAWSZ));
 
 		block_iter_close(&it);
 	}
@@ -110,7 +110,7 @@ static void t_block_read_write(void)
 	reftable_block_done(&br.block);
 	strbuf_release(&want);
 	for (i = 0; i < N; i++)
-		reftable_free(names[i]);
+		reftable_record_release(&recs[i]);
 }
 
 int cmd_main(int argc, const char *argv[])
-- 
2.45.GIT

[PATCH v2 05/11] t-reftable-block: use reftable_record_key() instead of strbuf_addstr()

[Chandra Pratap]

In the current testing setup, the record key required for many block
iterator functions is manually stored in a strbuf struct and then
passed to these functions. This is not ideal when there exists a
dedicated function to encode a record's key into a strbuf, namely
reftable_record_key(). Use this function instead of manual encoding.

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-block.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index b106d3c1e4..5887e9205d 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -83,8 +83,7 @@ static void t_block_read_write(void)
 
 	for (i = 0; i < N; i++) {
 		struct block_iter it = BLOCK_ITER_INIT;
-		strbuf_reset(&want);
-		strbuf_addstr(&want, recs[i].u.ref.refname);
+		reftable_record_key(&recs[i], &want);
 
 		ret = block_iter_seek_key(&it, &br, &want);
 		check_int(ret, ==, 0);
-- 
2.45.GIT

[PATCH v2 06/11] t-reftable-block: use block_iter_reset() instead of block_iter_close()

[Chandra Pratap]

block_iter_reset() restores a block iterator to its state at the time
of initialization without freeing any memory while block_iter_close()
deallocates the memory for the iterator.

In the current testing setup, a block iterator is allocated and
deallocated for every iteration of a loop, which hurts performance.
Improve upon this by using block_iter_reset() at the start of each
iteration instead. This has the added benifit of testing
block_iter_reset(), which currently remains untested.

Similarly, remove reftable_record_release() for a reftable record
that is still in use.

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-block.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index 5887e9205d..ad3d128ea7 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -78,11 +78,8 @@ static void t_block_read_write(void)
 		j++;
 	}
 
-	reftable_record_release(&rec);
-	block_iter_close(&it);
-
 	for (i = 0; i < N; i++) {
-		struct block_iter it = BLOCK_ITER_INIT;
+		block_iter_reset(&it);
 		reftable_record_key(&recs[i], &want);
 
 		ret = block_iter_seek_key(&it, &br, &want);
@@ -100,11 +97,10 @@ static void t_block_read_write(void)
 		ret = block_iter_next(&it, &rec);
 		check_int(ret, ==, 0);
 		check(reftable_record_equal(&recs[10 * (i / 10)], &rec, GIT_SHA1_RAWSZ));
-
-		block_iter_close(&it);
 	}
 
 	block_reader_release(&br);
+	block_iter_close(&it);
 	reftable_record_release(&rec);
 	reftable_block_done(&br.block);
 	strbuf_release(&want);
-- 
2.45.GIT

[PATCH v2 07/11] t-reftable-block: use xstrfmt() instead of xstrdup()

[Chandra Pratap]

Use xstrfmt() to assign a formatted string to a ref record's
refname instead of xstrdup(). This helps save the overhead of
a local 'char' buffer as well as makes the test more compact.

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-block.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index ad3d128ea7..81484bc646 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -44,10 +44,7 @@ static void t_block_read_write(void)
 	check_int(ret, ==, REFTABLE_API_ERROR);
 
 	for (i = 0; i < N; i++) {
-		char name[100];
-		snprintf(name, sizeof(name), "branch%02"PRIuMAX, (uintmax_t)i);
-
-		rec.u.ref.refname = xstrdup(name);
+		rec.u.ref.refname = xstrfmt("branch%02"PRIuMAX, (uintmax_t)i);
 		rec.u.ref.value_type = REFTABLE_REF_VAL1;
 		memset(rec.u.ref.value.val1, i, GIT_SHA1_RAWSZ);
 
-- 
2.45.GIT

[PATCH v2 08/11] t-reftable-block: remove unnecessary variable 'j'

[Chandra Pratap]

Currently, there are two variables for array indices, 'i' and 'j'.
The variable 'j' is used only once and can be easily replaced with
'i'. Get rid of 'j' and replace its occurence with 'i'.

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-block.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index 81484bc646..6aa86a3edf 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -29,7 +29,6 @@ static void t_block_read_write(void)
 	int ret;
 	struct block_reader br = { 0 };
 	struct block_iter it = BLOCK_ITER_INIT;
-	size_t j = 0;
 	struct strbuf want = STRBUF_INIT;
 
 	REFTABLE_CALLOC_ARRAY(block.data, block_size);
@@ -64,15 +63,14 @@ static void t_block_read_write(void)
 
 	block_iter_seek_start(&it, &br);
 
-	while (1) {
+	for (i = 0; ; i++) {
 		ret = block_iter_next(&it, &rec);
 		check_int(ret, >=, 0);
 		if (ret > 0) {
 			check_int(i, ==, N);
 			break;
 		}
-		check(reftable_record_equal(&recs[j], &rec, GIT_SHA1_RAWSZ));
-		j++;
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
 	}
 
 	for (i = 0; i < N; i++) {
-- 
2.45.GIT

[PATCH v2 09/11] t-reftable-block: add tests for log blocks

[Chandra Pratap]

In the current testing setup, block operations are only exercised
for ref blocks. Add another test that exercises these operations
for log blocks as well.

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-block.c | 92 ++++++++++++++++++++++++++++++++-
 1 file changed, 90 insertions(+), 2 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index 6aa86a3edf..1256c7df6a 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -12,7 +12,7 @@ license that can be found in the LICENSE file or at
 #include "reftable/constants.h"
 #include "reftable/reftable-error.h"
 
-static void t_block_read_write(void)
+static void t_ref_block_read_write(void)
 {
 	const int header_off = 21; /* random */
 	struct reftable_record recs[30];
@@ -103,9 +103,97 @@ static void t_block_read_write(void)
 		reftable_record_release(&recs[i]);
 }
 
+static void t_log_block_read_write(void)
+{
+	const int header_off = 21;
+	struct reftable_record recs[30];
+	const size_t N = ARRAY_SIZE(recs);
+	const size_t block_size = 2048;
+	struct reftable_block block = { 0 };
+	struct block_writer bw = {
+		.last_key = STRBUF_INIT,
+	};
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_LOG,
+	};
+	size_t i = 0;
+	int ret;
+	struct block_reader br = { 0 };
+	struct block_iter it = BLOCK_ITER_INIT;
+	struct strbuf want = STRBUF_INIT;
+
+	REFTABLE_CALLOC_ARRAY(block.data, block_size);
+	block.len = block_size;
+	block.source = malloc_block_source();
+	block_writer_init(&bw, BLOCK_TYPE_LOG, block.data, block_size,
+			  header_off, hash_size(GIT_SHA1_FORMAT_ID));
+
+	for (i = 0; i < N; i++) {
+		rec.u.log.refname = xstrfmt("branch%02"PRIuMAX , (uintmax_t)i);
+		rec.u.log.update_index = i;
+		rec.u.log.value_type = REFTABLE_LOG_UPDATE;
+
+		recs[i] = rec;
+		ret = block_writer_add(&bw, &rec);
+		rec.u.log.refname = NULL;
+		rec.u.log.value_type = REFTABLE_LOG_DELETION;
+		check_int(ret, ==, 0);
+	}
+
+	ret = block_writer_finish(&bw);
+	check_int(ret, >, 0);
+
+	block_writer_release(&bw);
+
+	block_reader_init(&br, &block, header_off, block_size, GIT_SHA1_RAWSZ);
+
+	block_iter_seek_start(&it, &br);
+
+	for (i = 0; ; i++) {
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, >=, 0);
+		if (ret > 0) {
+			check_int(i, ==, N);
+			break;
+		}
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+	}
+
+	for (i = 0; i < N; i++) {
+		block_iter_reset(&it);
+		strbuf_reset(&want);
+		strbuf_addstr(&want, recs[i].u.log.refname);
+
+		ret = block_iter_seek_key(&it, &br, &want);
+		check_int(ret, ==, 0);
+
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, ==, 0);
+
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+
+		want.len--;
+		ret = block_iter_seek_key(&it, &br, &want);
+		check_int(ret, ==, 0);
+
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, ==, 0);
+		check(reftable_record_equal(&recs[10 * (i / 10)], &rec, GIT_SHA1_RAWSZ));
+	}
+
+	block_reader_release(&br);
+	block_iter_close(&it);
+	reftable_record_release(&rec);
+	reftable_block_done(&br.block);
+	strbuf_release(&want);
+	for (i = 0; i < N; i++)
+		reftable_record_release(&recs[i]);
+}
+
 int cmd_main(int argc, const char *argv[])
 {
-	TEST(t_block_read_write(), "read-write operations on blocks work");
+	TEST(t_log_block_read_write(), "read-write operations on log blocks work");
+	TEST(t_ref_block_read_write(), "read-write operations on ref blocks work");
 
 	return test_done();
 }
-- 
2.45.GIT

Re: [PATCH v2 09/11] t-reftable-block: add tests for log blocks

[Patrick Steinhardt]

On Fri, Aug 16, 2024 at 10:55:32PM +0530, Chandra Pratap wrote:
> @@ -103,9 +103,97 @@ static void t_block_read_write(void)
>  		reftable_record_release(&recs[i]);
>  }
>  
> +static void t_log_block_read_write(void)
> +{
> +	const int header_off = 21;
> +	struct reftable_record recs[30];
> +	const size_t N = ARRAY_SIZE(recs);
> +	const size_t block_size = 2048;
> +	struct reftable_block block = { 0 };
> +	struct block_writer bw = {
> +		.last_key = STRBUF_INIT,
> +	};
> +	struct reftable_record rec = {
> +		.type = BLOCK_TYPE_LOG,
> +	};
> +	size_t i = 0;
> +	int ret;
> +	struct block_reader br = { 0 };
> +	struct block_iter it = BLOCK_ITER_INIT;
> +	struct strbuf want = STRBUF_INIT;
> +
> +	REFTABLE_CALLOC_ARRAY(block.data, block_size);
> +	block.len = block_size;
> +	block.source = malloc_block_source();
> +	block_writer_init(&bw, BLOCK_TYPE_LOG, block.data, block_size,
> +			  header_off, hash_size(GIT_SHA1_FORMAT_ID));

Nit: instead of a `malloc_block_source()`, you may use
`block_source_from_strbuf()`. The former will go away with the patch
series at [1].

I'm also happy to rebase my patch series once yours lands and do this
myself. Guess yours will land faster anyway, and there are conflicts
regardless of whether you do or don't update the test here. The same
applies to the subsequent patches which use a `malloc_block_source()`.

So this isn't really worth a reroll by itself, and other than that this
patch looks good to me.

Patrick

[1]: <cover.1724080006.git.ps@pks.im>

Re: [PATCH v2 09/11] t-reftable-block: add tests for log blocks

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> +	REFTABLE_CALLOC_ARRAY(block.data, block_size);
>> +	block.len = block_size;
>> +	block.source = malloc_block_source();
>> +	block_writer_init(&bw, BLOCK_TYPE_LOG, block.data, block_size,
>> +			  header_off, hash_size(GIT_SHA1_FORMAT_ID));
>
> Nit: instead of a `malloc_block_source()`, you may use
> `block_source_from_strbuf()`. The former will go away with the patch
> series at [1].

I noticed that need for rewriting while merging the topic.  If the
patch uses the surviving alternative, that's one fewer thing I have
to worry about ;-).

Thanks.

[PATCH v2 10/11] t-reftable-block: add tests for obj blocks

[Chandra Pratap]

In the current testing setup, block operations are left unexercised
for obj blocks. Add a test that exercises these operations for obj
blocks.

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-block.c | 82 +++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index 1256c7df6a..7671a40969 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -190,9 +190,91 @@ static void t_log_block_read_write(void)
 		reftable_record_release(&recs[i]);
 }
 
+static void t_obj_block_read_write(void)
+{
+	const int header_off = 21;
+	struct reftable_record recs[30];
+	const size_t N = ARRAY_SIZE(recs);
+	const size_t block_size = 1024;
+	struct reftable_block block = { 0 };
+	struct block_writer bw = {
+		.last_key = STRBUF_INIT,
+	};
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_OBJ,
+	};
+	size_t i = 0;
+	int ret;
+	struct block_reader br = { 0 };
+	struct block_iter it = BLOCK_ITER_INIT;
+	struct strbuf want = STRBUF_INIT;
+
+	REFTABLE_CALLOC_ARRAY(block.data, block_size);
+	block.len = block_size;
+	block.source = malloc_block_source();
+	block_writer_init(&bw, BLOCK_TYPE_OBJ, block.data, block_size,
+			  header_off, hash_size(GIT_SHA1_FORMAT_ID));
+
+	for (i = 0; i < N; i++) {
+		uint8_t bytes[] = { i, i + 1, i + 2, i + 3, i + 5 }, *allocated;
+		allocated = reftable_malloc(ARRAY_SIZE(bytes));
+		DUP_ARRAY(allocated, bytes, ARRAY_SIZE(bytes));
+
+		rec.u.obj.hash_prefix = allocated;
+		rec.u.obj.hash_prefix_len = 5;
+
+		recs[i] = rec;
+		ret = block_writer_add(&bw, &rec);
+		rec.u.obj.hash_prefix = NULL;
+		rec.u.obj.hash_prefix_len = 0;
+		check_int(ret, ==, 0);
+	}
+
+	ret = block_writer_finish(&bw);
+	check_int(ret, >, 0);
+
+	block_writer_release(&bw);
+
+	block_reader_init(&br, &block, header_off, block_size, GIT_SHA1_RAWSZ);
+
+	block_iter_seek_start(&it, &br);
+
+	for (i = 0; ; i++) {
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, >=, 0);
+		if (ret > 0) {
+			check_int(i, ==, N);
+			break;
+		}
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+	}
+
+	for (i = 0; i < N; i++) {
+		block_iter_reset(&it);
+		reftable_record_key(&recs[i], &want);
+
+		ret = block_iter_seek_key(&it, &br, &want);
+		check_int(ret, ==, 0);
+
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, ==, 0);
+
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+	}
+
+	block_reader_release(&br);
+	block_iter_close(&it);
+	reftable_record_release(&rec);
+	reftable_block_done(&br.block);
+	strbuf_release(&want);
+	for (i = 0; i < N; i++)
+		reftable_record_release(&recs[i]);
+}
+
 int cmd_main(int argc, const char *argv[])
 {
 	TEST(t_log_block_read_write(), "read-write operations on log blocks work");
+	TEST(t_obj_block_read_write(), "read-write operations on obj blocks work");
 	TEST(t_ref_block_read_write(), "read-write operations on ref blocks work");
 
 	return test_done();
-- 
2.45.GIT

Re: [PATCH v2 10/11] t-reftable-block: add tests for obj blocks

[Chandra Pratap]

On Fri, 16 Aug 2024 at 23:25, Chandra Pratap
<chandrapratap3519@gmail.com> wrote:
>
> In the current testing setup, block operations are left unexercised
> for obj blocks. Add a test that exercises these operations for obj
> blocks.
>
> 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-block.c | 82 +++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
>
> diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
> index 1256c7df6a..7671a40969 100644
> --- a/t/unit-tests/t-reftable-block.c
> +++ b/t/unit-tests/t-reftable-block.c
> @@ -190,9 +190,91 @@ static void t_log_block_read_write(void)
>                 reftable_record_release(&recs[i]);
>  }
>
> +static void t_obj_block_read_write(void)
> +{
> +       const int header_off = 21;
> +       struct reftable_record recs[30];
> +       const size_t N = ARRAY_SIZE(recs);
> +       const size_t block_size = 1024;
> +       struct reftable_block block = { 0 };
> +       struct block_writer bw = {
> +               .last_key = STRBUF_INIT,
> +       };
> +       struct reftable_record rec = {
> +               .type = BLOCK_TYPE_OBJ,
> +       };
> +       size_t i = 0;
> +       int ret;
> +       struct block_reader br = { 0 };
> +       struct block_iter it = BLOCK_ITER_INIT;
> +       struct strbuf want = STRBUF_INIT;
> +
> +       REFTABLE_CALLOC_ARRAY(block.data, block_size);
> +       block.len = block_size;
> +       block.source = malloc_block_source();
> +       block_writer_init(&bw, BLOCK_TYPE_OBJ, block.data, block_size,
> +                         header_off, hash_size(GIT_SHA1_FORMAT_ID));
> +
> +       for (i = 0; i < N; i++) {
> +               uint8_t bytes[] = { i, i + 1, i + 2, i + 3, i + 5 }, *allocated;
> +               allocated = reftable_malloc(ARRAY_SIZE(bytes));
> +               DUP_ARRAY(allocated, bytes, ARRAY_SIZE(bytes));

The second line of this loop is redundant and causes a memory leak
in the GitHub CI. I'll fix this in the next iteration.

---snip---

[PATCH v2 11/11] t-reftable-block: add tests for index blocks

[Chandra Pratap]

In the current testing setup, block operations are left unexercised
for index blocks. Add a test that exercises these operations for
index blocks.

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-block.c | 87 +++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index 7671a40969..c2b67a11bc 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -271,8 +271,95 @@ static void t_obj_block_read_write(void)
 		reftable_record_release(&recs[i]);
 }
 
+static void t_index_block_read_write(void)
+{
+	const int header_off = 21;
+	struct reftable_record recs[30];
+	const size_t N = ARRAY_SIZE(recs);
+	const size_t block_size = 1024;
+	struct reftable_block block = { 0 };
+	struct block_writer bw = {
+		.last_key = STRBUF_INIT,
+	};
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_INDEX,
+		.u.idx.last_key = STRBUF_INIT,
+	};
+	size_t i = 0;
+	int ret;
+	struct block_reader br = { 0 };
+	struct block_iter it = BLOCK_ITER_INIT;
+	struct strbuf want = STRBUF_INIT;
+
+	REFTABLE_CALLOC_ARRAY(block.data, block_size);
+	block.len = block_size;
+	block.source = malloc_block_source();
+	block_writer_init(&bw, BLOCK_TYPE_INDEX, block.data, block_size,
+			  header_off, hash_size(GIT_SHA1_FORMAT_ID));
+
+	for (i = 0; i < N; i++) {
+		strbuf_init(&recs[i].u.idx.last_key, 9);
+
+		recs[i].type = BLOCK_TYPE_INDEX;
+		strbuf_addf(&recs[i].u.idx.last_key, "branch%02"PRIuMAX, (uintmax_t)i);
+		recs[i].u.idx.offset = i;
+
+		ret = block_writer_add(&bw, &recs[i]);
+		check_int(ret, ==, 0);
+	}
+
+	ret = block_writer_finish(&bw);
+	check_int(ret, >, 0);
+
+	block_writer_release(&bw);
+
+	block_reader_init(&br, &block, header_off, block_size, GIT_SHA1_RAWSZ);
+
+	block_iter_seek_start(&it, &br);
+
+	for (i = 0; ; i++) {
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, >=, 0);
+		if (ret > 0) {
+			check_int(i, ==, N);
+			break;
+		}
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+	}
+
+	for (i = 0; i < N; i++) {
+		block_iter_reset(&it);
+		reftable_record_key(&recs[i], &want);
+
+		ret = block_iter_seek_key(&it, &br, &want);
+		check_int(ret, ==, 0);
+
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, ==, 0);
+
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+
+		want.len--;
+		ret = block_iter_seek_key(&it, &br, &want);
+		check_int(ret, ==, 0);
+
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, ==, 0);
+		check(reftable_record_equal(&recs[10 * (i / 10)], &rec, GIT_SHA1_RAWSZ));
+	}
+
+	block_reader_release(&br);
+	block_iter_close(&it);
+	reftable_record_release(&rec);
+	reftable_block_done(&br.block);
+	strbuf_release(&want);
+	for (i = 0; i < N; i++)
+		reftable_record_release(&recs[i]);
+}
+
 int cmd_main(int argc, const char *argv[])
 {
+	TEST(t_index_block_read_write(), "read-write operations on index blocks work");
 	TEST(t_log_block_read_write(), "read-write operations on log blocks work");
 	TEST(t_obj_block_read_write(), "read-write operations on obj blocks work");
 	TEST(t_ref_block_read_write(), "read-write operations on ref blocks work");
-- 
2.45.GIT

Re: [GSoC][PATCH v2 00/11] t: port reftable/block_test.c to the unit testing framework

[Chandra Pratap]

Reminder for reviews/acks.

[GSoC][PATCH v3 00/11] t: port reftable/block_test.c to the unit testing framework

[Chandra Pratap]

The reftable library comes with self tests, which are exercised
as part of the usual end-to-end tests and are designed to
observe the end-user visible effects of Git commands. What it
exercises, however, is a better match for the unit-testing
framework, merged at 8bf6fbd0 (Merge branch 'js/doc-unit-tests',
2023-12-09), which is designed to observe how low level
implementation details, at the level of sequences of individual
function calls, behave.

Hence, port reftable/block_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:
- Use block_source_from_strbuf() instead of malloc_block_source()
  in log, obj, and index block tests (patches 9, 10, 11).
- Remove a line that causes a memory leak in obj test (patch 10).

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

Chandra Pratap(11):
t: move reftable/block_test.c to the unit testing framework
t: harmonize t-reftable-block.c with coding guidelines
t-reftable-block: release used block reader
t-reftable-block: use reftable_record_equal() instead of check_str()
t-reftable-block: use reftable_record_key() instead of strbuf_addstr()
t-reftable-block: use block_iter_reset() instead of block_iter_close()
t-reftable-block: use xstrfmt() instead of xstrdup()
t-reftable-block: remove unnecessary variable 'j'
t-reftable-block: add tests for log blocks
t-reftable-block: add tests for obj blocks
t-reftable-block: add tests for index blocks

Makefile                        |   2 +-
reftable/block_test.c           | 123 ------------------------------
reftable/reftable-tests.h       |   1 -
t/helper/test-reftable.c        |   1 -
t/unit-tests/t-reftable-block.c | 370 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 371 insertions(+), 126 deletions(-)

Range-diff against v2:
1:  502fcc1374 ! 1:  4cd1981016 t-reftable-block: add tests for log blocks
    @@ t/unit-tests/t-reftable-block.c: static void t_block_read_write(void)
     +	int ret;
     +	struct block_reader br = { 0 };
     +	struct block_iter it = BLOCK_ITER_INIT;
    -+	struct strbuf want = STRBUF_INIT;
    ++	struct strbuf want = STRBUF_INIT, buf = STRBUF_INIT;
     +
     +	REFTABLE_CALLOC_ARRAY(block.data, block_size);
     +	block.len = block_size;
    -+	block.source = malloc_block_source();
    ++	block_source_from_strbuf(&block.source ,&buf);
     +	block_writer_init(&bw, BLOCK_TYPE_LOG, block.data, block_size,
     +			  header_off, hash_size(GIT_SHA1_FORMAT_ID));
     +
    @@ t/unit-tests/t-reftable-block.c: static void t_block_read_write(void)
     +	reftable_record_release(&rec);
     +	reftable_block_done(&br.block);
     +	strbuf_release(&want);
    ++	strbuf_release(&buf);
     +	for (i = 0; i < N; i++)
     +		reftable_record_release(&recs[i]);
     +}
2:  e3cefa7e3d ! 2:  4e9752e72a t-reftable-block: add tests for obj blocks
    @@ t/unit-tests/t-reftable-block.c: static void t_log_block_read_write(void)
     +	int ret;
     +	struct block_reader br = { 0 };
     +	struct block_iter it = BLOCK_ITER_INIT;
    -+	struct strbuf want = STRBUF_INIT;
    ++	struct strbuf want = STRBUF_INIT, buf = STRBUF_INIT;
     +
     +	REFTABLE_CALLOC_ARRAY(block.data, block_size);
     +	block.len = block_size;
    -+	block.source = malloc_block_source();
    ++	block_source_from_strbuf(&block.source, &buf);
     +	block_writer_init(&bw, BLOCK_TYPE_OBJ, block.data, block_size,
     +			  header_off, hash_size(GIT_SHA1_FORMAT_ID));
     +
     +	for (i = 0; i < N; i++) {
     +		uint8_t bytes[] = { i, i + 1, i + 2, i + 3, i + 5 }, *allocated;
    -+		allocated = reftable_malloc(ARRAY_SIZE(bytes));
     +		DUP_ARRAY(allocated, bytes, ARRAY_SIZE(bytes));
     +
     +		rec.u.obj.hash_prefix = allocated;
    @@ t/unit-tests/t-reftable-block.c: static void t_log_block_read_write(void)
     +	reftable_record_release(&rec);
     +	reftable_block_done(&br.block);
     +	strbuf_release(&want);
    ++	strbuf_release(&buf);
     +	for (i = 0; i < N; i++)
     +		reftable_record_release(&recs[i]);
     +}
3:  4be7749c4b ! 3:  db62f23594 t-reftable-block: add tests for index blocks
    @@ t/unit-tests/t-reftable-block.c: static void t_obj_block_read_write(void)
     +	int ret;
     +	struct block_reader br = { 0 };
     +	struct block_iter it = BLOCK_ITER_INIT;
    -+	struct strbuf want = STRBUF_INIT;
    ++	struct strbuf want = STRBUF_INIT, buf = STRBUF_INIT;
     +
     +	REFTABLE_CALLOC_ARRAY(block.data, block_size);
     +	block.len = block_size;
    -+	block.source = malloc_block_source();
    ++	block_source_from_strbuf(&block.source, &buf);
     +	block_writer_init(&bw, BLOCK_TYPE_INDEX, block.data, block_size,
     +			  header_off, hash_size(GIT_SHA1_FORMAT_ID));
     +
    @@ t/unit-tests/t-reftable-block.c: static void t_obj_block_read_write(void)
     +	reftable_record_release(&rec);
     +	reftable_block_done(&br.block);
     +	strbuf_release(&want);
    ++	strbuf_release(&buf);
     +	for (i = 0; i < N; i++)
     +		reftable_record_release(&recs[i]);
     +}

[PATCH v3 01/11] t: move reftable/block_test.c to the unit testing framework

[Chandra Pratap]

reftable/block_test.c exercises the functions defined in
reftable/block.{c, h}. Migrate reftable/block_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 to follow the unit-tests'
naming conventions.

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-block.c             | 45 +++++++++----------
 4 files changed, 22 insertions(+), 27 deletions(-)
 rename reftable/block_test.c => t/unit-tests/t-reftable-block.c (76%)

diff --git a/Makefile b/Makefile
index 13890710f8..a30cd636f8 100644
--- a/Makefile
+++ b/Makefile
@@ -1340,6 +1340,7 @@ UNIT_TEST_PROGRAMS += t-oidmap
 UNIT_TEST_PROGRAMS += t-oidtree
 UNIT_TEST_PROGRAMS += t-prio-queue
 UNIT_TEST_PROGRAMS += t-reftable-basics
+UNIT_TEST_PROGRAMS += t-reftable-block
 UNIT_TEST_PROGRAMS += t-reftable-merged
 UNIT_TEST_PROGRAMS += t-reftable-pq
 UNIT_TEST_PROGRAMS += t-reftable-record
@@ -2681,7 +2682,6 @@ REFTABLE_OBJS += reftable/stack.o
 REFTABLE_OBJS += reftable/tree.o
 REFTABLE_OBJS += reftable/writer.o
 
-REFTABLE_TEST_OBJS += reftable/block_test.o
 REFTABLE_TEST_OBJS += reftable/dump.o
 REFTABLE_TEST_OBJS += reftable/readwrite_test.o
 REFTABLE_TEST_OBJS += reftable/stack_test.o
diff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h
index 4b666810af..3d9118b91b 100644
--- a/reftable/reftable-tests.h
+++ b/reftable/reftable-tests.h
@@ -10,7 +10,6 @@ license that can be found in the LICENSE file or at
 #define REFTABLE_TESTS_H
 
 int basics_test_main(int argc, const char **argv);
-int block_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);
 int stack_test_main(int argc, const char **argv);
diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
index 623cf3f0f5..7bdd18430b 100644
--- a/t/helper/test-reftable.c
+++ b/t/helper/test-reftable.c
@@ -5,7 +5,6 @@
 int cmd__reftable(int argc, const char **argv)
 {
 	/* test from simple to complex. */
-	block_test_main(argc, argv);
 	readwrite_test_main(argc, argv);
 	stack_test_main(argc, argv);
 	return 0;
diff --git a/reftable/block_test.c b/t/unit-tests/t-reftable-block.c
similarity index 76%
rename from reftable/block_test.c
rename to t/unit-tests/t-reftable-block.c
index 90aecd5a7c..f2b9a8a6f4 100644
--- a/reftable/block_test.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -6,17 +6,13 @@ license that can be found in the LICENSE file or at
 https://developers.google.com/open-source/licenses/bsd
 */
 
-#include "block.h"
+#include "test-lib.h"
+#include "reftable/block.h"
+#include "reftable/blocksource.h"
+#include "reftable/constants.h"
+#include "reftable/reftable-error.h"
 
-#include "system.h"
-#include "blocksource.h"
-#include "basics.h"
-#include "constants.h"
-#include "record.h"
-#include "test_framework.h"
-#include "reftable-tests.h"
-
-static void test_block_read_write(void)
+static void t_block_read_write(void)
 {
 	const int header_off = 21; /* random */
 	char *names[30];
@@ -45,7 +41,7 @@ static void test_block_read_write(void)
 	rec.u.ref.refname = (char *) "";
 	rec.u.ref.value_type = REFTABLE_REF_DELETION;
 	n = block_writer_add(&bw, &rec);
-	EXPECT(n == REFTABLE_API_ERROR);
+	check_int(n, ==, REFTABLE_API_ERROR);
 
 	for (i = 0; i < N; i++) {
 		char name[100];
@@ -59,11 +55,11 @@ static void test_block_read_write(void)
 		n = block_writer_add(&bw, &rec);
 		rec.u.ref.refname = NULL;
 		rec.u.ref.value_type = REFTABLE_REF_DELETION;
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 	}
 
 	n = block_writer_finish(&bw);
-	EXPECT(n > 0);
+	check_int(n, >, 0);
 
 	block_writer_release(&bw);
 
@@ -73,11 +69,11 @@ static void test_block_read_write(void)
 
 	while (1) {
 		int r = block_iter_next(&it, &rec);
-		EXPECT(r >= 0);
+		check_int(r, >=, 0);
 		if (r > 0) {
 			break;
 		}
-		EXPECT_STREQ(names[j], rec.u.ref.refname);
+		check_str(names[j], rec.u.ref.refname);
 		j++;
 	}
 
@@ -90,20 +86,20 @@ static void test_block_read_write(void)
 		strbuf_addstr(&want, names[i]);
 
 		n = block_iter_seek_key(&it, &br, &want);
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 
 		n = block_iter_next(&it, &rec);
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 
-		EXPECT_STREQ(names[i], rec.u.ref.refname);
+		check_str(names[i], rec.u.ref.refname);
 
 		want.len--;
 		n = block_iter_seek_key(&it, &br, &want);
-		EXPECT(n == 0);
+		check_int(n, ==, 0);
 
 		n = block_iter_next(&it, &rec);
-		EXPECT(n == 0);
-		EXPECT_STREQ(names[10 * (i / 10)], rec.u.ref.refname);
+		check_int(n, ==, 0);
+		check_str(names[10 * (i / 10)], rec.u.ref.refname);
 
 		block_iter_close(&it);
 	}
@@ -116,8 +112,9 @@ static void test_block_read_write(void)
 	}
 }
 
-int block_test_main(int argc, const char *argv[])
+int cmd_main(int argc, const char *argv[])
 {
-	RUN_TEST(test_block_read_write);
-	return 0;
+	TEST(t_block_read_write(), "read-write operations on blocks work");
+
+	return test_done();
 }
-- 
2.45.GIT

[PATCH v3 02/11] t: harmonize t-reftable-block.c with coding guidelines

[Chandra Pratap]

Harmonize the newly ported test unit-tests/t-reftable-block.c
with the following guidelines:
- Single line 'for' statements must omit curly braces.
- Structs must be 0-initialized with '= { 0 }' instead of '= { NULL }'.
- Array sizes and indices should preferably be of type 'size_t'and
  not 'int'.
- Return code variable should preferably be named 'ret', not 'n'.

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-block.c | 52 ++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index f2b9a8a6f4..b1b238ac2a 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -16,20 +16,20 @@ static void t_block_read_write(void)
 {
 	const int header_off = 21; /* random */
 	char *names[30];
-	const int N = ARRAY_SIZE(names);
-	const int block_size = 1024;
-	struct reftable_block block = { NULL };
+	const size_t N = ARRAY_SIZE(names);
+	const size_t block_size = 1024;
+	struct reftable_block block = { 0 };
 	struct block_writer bw = {
 		.last_key = STRBUF_INIT,
 	};
 	struct reftable_record rec = {
 		.type = BLOCK_TYPE_REF,
 	};
-	int i = 0;
-	int n;
+	size_t i = 0;
+	int ret;
 	struct block_reader br = { 0 };
 	struct block_iter it = BLOCK_ITER_INIT;
-	int j = 0;
+	size_t j = 0;
 	struct strbuf want = STRBUF_INIT;
 
 	REFTABLE_CALLOC_ARRAY(block.data, block_size);
@@ -40,26 +40,26 @@ static void t_block_read_write(void)
 
 	rec.u.ref.refname = (char *) "";
 	rec.u.ref.value_type = REFTABLE_REF_DELETION;
-	n = block_writer_add(&bw, &rec);
-	check_int(n, ==, REFTABLE_API_ERROR);
+	ret = block_writer_add(&bw, &rec);
+	check_int(ret, ==, REFTABLE_API_ERROR);
 
 	for (i = 0; i < N; i++) {
 		char name[100];
-		snprintf(name, sizeof(name), "branch%02d", i);
+		snprintf(name, sizeof(name), "branch%02"PRIuMAX, (uintmax_t)i);
 
 		rec.u.ref.refname = name;
 		rec.u.ref.value_type = REFTABLE_REF_VAL1;
 		memset(rec.u.ref.value.val1, i, GIT_SHA1_RAWSZ);
 
 		names[i] = xstrdup(name);
-		n = block_writer_add(&bw, &rec);
+		ret = block_writer_add(&bw, &rec);
 		rec.u.ref.refname = NULL;
 		rec.u.ref.value_type = REFTABLE_REF_DELETION;
-		check_int(n, ==, 0);
+		check_int(ret, ==, 0);
 	}
 
-	n = block_writer_finish(&bw);
-	check_int(n, >, 0);
+	ret = block_writer_finish(&bw);
+	check_int(ret, >, 0);
 
 	block_writer_release(&bw);
 
@@ -68,9 +68,10 @@ static void t_block_read_write(void)
 	block_iter_seek_start(&it, &br);
 
 	while (1) {
-		int r = block_iter_next(&it, &rec);
-		check_int(r, >=, 0);
-		if (r > 0) {
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, >=, 0);
+		if (ret > 0) {
+			check_int(i, ==, N);
 			break;
 		}
 		check_str(names[j], rec.u.ref.refname);
@@ -85,20 +86,20 @@ static void t_block_read_write(void)
 		strbuf_reset(&want);
 		strbuf_addstr(&want, names[i]);
 
-		n = block_iter_seek_key(&it, &br, &want);
-		check_int(n, ==, 0);
+		ret = block_iter_seek_key(&it, &br, &want);
+		check_int(ret, ==, 0);
 
-		n = block_iter_next(&it, &rec);
-		check_int(n, ==, 0);
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, ==, 0);
 
 		check_str(names[i], rec.u.ref.refname);
 
 		want.len--;
-		n = block_iter_seek_key(&it, &br, &want);
-		check_int(n, ==, 0);
+		ret = block_iter_seek_key(&it, &br, &want);
+		check_int(ret, ==, 0);
 
-		n = block_iter_next(&it, &rec);
-		check_int(n, ==, 0);
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, ==, 0);
 		check_str(names[10 * (i / 10)], rec.u.ref.refname);
 
 		block_iter_close(&it);
@@ -107,9 +108,8 @@ static void t_block_read_write(void)
 	reftable_record_release(&rec);
 	reftable_block_done(&br.block);
 	strbuf_release(&want);
-	for (i = 0; i < N; i++) {
+	for (i = 0; i < N; i++)
 		reftable_free(names[i]);
-	}
 }
 
 int cmd_main(int argc, const char *argv[])
-- 
2.45.GIT

[PATCH v3 03/11] t-reftable-block: release used block reader

[Chandra Pratap]

Used block readers must be released using block_reader_release() to
prevent the occurence of a memory leak. Make test_block_read_write()
conform to this statement.

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-block.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index b1b238ac2a..eafe1fdee9 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -105,6 +105,7 @@ static void t_block_read_write(void)
 		block_iter_close(&it);
 	}
 
+	block_reader_release(&br);
 	reftable_record_release(&rec);
 	reftable_block_done(&br.block);
 	strbuf_release(&want);
-- 
2.45.GIT

[PATCH v3 04/11] t-reftable-block: use reftable_record_equal() instead of check_str()

[Chandra Pratap]

In the current testing setup, operations like read and write for
reftable blocks as defined by reftable/block.{c, h} are verified by
comparing only the keys of input and output reftable records. This is
not ideal because there can exist inequal reftable records with the
same key. Use the dedicated function for record comparison,
reftable_record_equal(), instead of key-based comparison.

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-block.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index eafe1fdee9..b106d3c1e4 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -15,8 +15,8 @@ license that can be found in the LICENSE file or at
 static void t_block_read_write(void)
 {
 	const int header_off = 21; /* random */
-	char *names[30];
-	const size_t N = ARRAY_SIZE(names);
+	struct reftable_record recs[30];
+	const size_t N = ARRAY_SIZE(recs);
 	const size_t block_size = 1024;
 	struct reftable_block block = { 0 };
 	struct block_writer bw = {
@@ -47,11 +47,11 @@ static void t_block_read_write(void)
 		char name[100];
 		snprintf(name, sizeof(name), "branch%02"PRIuMAX, (uintmax_t)i);
 
-		rec.u.ref.refname = name;
+		rec.u.ref.refname = xstrdup(name);
 		rec.u.ref.value_type = REFTABLE_REF_VAL1;
 		memset(rec.u.ref.value.val1, i, GIT_SHA1_RAWSZ);
 
-		names[i] = xstrdup(name);
+		recs[i] = rec;
 		ret = block_writer_add(&bw, &rec);
 		rec.u.ref.refname = NULL;
 		rec.u.ref.value_type = REFTABLE_REF_DELETION;
@@ -74,7 +74,7 @@ static void t_block_read_write(void)
 			check_int(i, ==, N);
 			break;
 		}
-		check_str(names[j], rec.u.ref.refname);
+		check(reftable_record_equal(&recs[j], &rec, GIT_SHA1_RAWSZ));
 		j++;
 	}
 
@@ -84,7 +84,7 @@ static void t_block_read_write(void)
 	for (i = 0; i < N; i++) {
 		struct block_iter it = BLOCK_ITER_INIT;
 		strbuf_reset(&want);
-		strbuf_addstr(&want, names[i]);
+		strbuf_addstr(&want, recs[i].u.ref.refname);
 
 		ret = block_iter_seek_key(&it, &br, &want);
 		check_int(ret, ==, 0);
@@ -92,7 +92,7 @@ static void t_block_read_write(void)
 		ret = block_iter_next(&it, &rec);
 		check_int(ret, ==, 0);
 
-		check_str(names[i], rec.u.ref.refname);
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
 
 		want.len--;
 		ret = block_iter_seek_key(&it, &br, &want);
@@ -100,7 +100,7 @@ static void t_block_read_write(void)
 
 		ret = block_iter_next(&it, &rec);
 		check_int(ret, ==, 0);
-		check_str(names[10 * (i / 10)], rec.u.ref.refname);
+		check(reftable_record_equal(&recs[10 * (i / 10)], &rec, GIT_SHA1_RAWSZ));
 
 		block_iter_close(&it);
 	}
@@ -110,7 +110,7 @@ static void t_block_read_write(void)
 	reftable_block_done(&br.block);
 	strbuf_release(&want);
 	for (i = 0; i < N; i++)
-		reftable_free(names[i]);
+		reftable_record_release(&recs[i]);
 }
 
 int cmd_main(int argc, const char *argv[])
-- 
2.45.GIT

[PATCH v3 05/11] t-reftable-block: use reftable_record_key() instead of strbuf_addstr()

[Chandra Pratap]

In the current testing setup, the record key required for many block
iterator functions is manually stored in a strbuf struct and then
passed to these functions. This is not ideal when there exists a
dedicated function to encode a record's key into a strbuf, namely
reftable_record_key(). Use this function instead of manual encoding.

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-block.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index b106d3c1e4..5887e9205d 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -83,8 +83,7 @@ static void t_block_read_write(void)
 
 	for (i = 0; i < N; i++) {
 		struct block_iter it = BLOCK_ITER_INIT;
-		strbuf_reset(&want);
-		strbuf_addstr(&want, recs[i].u.ref.refname);
+		reftable_record_key(&recs[i], &want);
 
 		ret = block_iter_seek_key(&it, &br, &want);
 		check_int(ret, ==, 0);
-- 
2.45.GIT

[PATCH v3 06/11] t-reftable-block: use block_iter_reset() instead of block_iter_close()

[Chandra Pratap]

block_iter_reset() restores a block iterator to its state at the time
of initialization without freeing any memory while block_iter_close()
deallocates the memory for the iterator.

In the current testing setup, a block iterator is allocated and
deallocated for every iteration of a loop, which hurts performance.
Improve upon this by using block_iter_reset() at the start of each
iteration instead. This has the added benifit of testing
block_iter_reset(), which currently remains untested.

Similarly, remove reftable_record_release() for a reftable record
that is still in use.

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-block.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index 5887e9205d..ad3d128ea7 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -78,11 +78,8 @@ static void t_block_read_write(void)
 		j++;
 	}
 
-	reftable_record_release(&rec);
-	block_iter_close(&it);
-
 	for (i = 0; i < N; i++) {
-		struct block_iter it = BLOCK_ITER_INIT;
+		block_iter_reset(&it);
 		reftable_record_key(&recs[i], &want);
 
 		ret = block_iter_seek_key(&it, &br, &want);
@@ -100,11 +97,10 @@ static void t_block_read_write(void)
 		ret = block_iter_next(&it, &rec);
 		check_int(ret, ==, 0);
 		check(reftable_record_equal(&recs[10 * (i / 10)], &rec, GIT_SHA1_RAWSZ));
-
-		block_iter_close(&it);
 	}
 
 	block_reader_release(&br);
+	block_iter_close(&it);
 	reftable_record_release(&rec);
 	reftable_block_done(&br.block);
 	strbuf_release(&want);
-- 
2.45.GIT

[PATCH v3 07/11] t-reftable-block: use xstrfmt() instead of xstrdup()

[Chandra Pratap]

Use xstrfmt() to assign a formatted string to a ref record's
refname instead of xstrdup(). This helps save the overhead of
a local 'char' buffer as well as makes the test more compact.

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-block.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index ad3d128ea7..81484bc646 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -44,10 +44,7 @@ static void t_block_read_write(void)
 	check_int(ret, ==, REFTABLE_API_ERROR);
 
 	for (i = 0; i < N; i++) {
-		char name[100];
-		snprintf(name, sizeof(name), "branch%02"PRIuMAX, (uintmax_t)i);
-
-		rec.u.ref.refname = xstrdup(name);
+		rec.u.ref.refname = xstrfmt("branch%02"PRIuMAX, (uintmax_t)i);
 		rec.u.ref.value_type = REFTABLE_REF_VAL1;
 		memset(rec.u.ref.value.val1, i, GIT_SHA1_RAWSZ);
 
-- 
2.45.GIT

[PATCH v3 08/11] t-reftable-block: remove unnecessary variable 'j'

[Chandra Pratap]

Currently, there are two variables for array indices, 'i' and 'j'.
The variable 'j' is used only once and can be easily replaced with
'i'. Get rid of 'j' and replace its occurence with 'i'.

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-block.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index 81484bc646..6aa86a3edf 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -29,7 +29,6 @@ static void t_block_read_write(void)
 	int ret;
 	struct block_reader br = { 0 };
 	struct block_iter it = BLOCK_ITER_INIT;
-	size_t j = 0;
 	struct strbuf want = STRBUF_INIT;
 
 	REFTABLE_CALLOC_ARRAY(block.data, block_size);
@@ -64,15 +63,14 @@ static void t_block_read_write(void)
 
 	block_iter_seek_start(&it, &br);
 
-	while (1) {
+	for (i = 0; ; i++) {
 		ret = block_iter_next(&it, &rec);
 		check_int(ret, >=, 0);
 		if (ret > 0) {
 			check_int(i, ==, N);
 			break;
 		}
-		check(reftable_record_equal(&recs[j], &rec, GIT_SHA1_RAWSZ));
-		j++;
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
 	}
 
 	for (i = 0; i < N; i++) {
-- 
2.45.GIT

[PATCH v3 09/11] t-reftable-block: add tests for log blocks

[Chandra Pratap]

In the current testing setup, block operations are only exercised
for ref blocks. Add another test that exercises these operations
for log blocks as well.

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-block.c | 93 ++++++++++++++++++++++++++++++++-
 1 file changed, 91 insertions(+), 2 deletions(-)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index 6aa86a3edf..4c4fb39ab4 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -12,7 +12,7 @@ license that can be found in the LICENSE file or at
 #include "reftable/constants.h"
 #include "reftable/reftable-error.h"
 
-static void t_block_read_write(void)
+static void t_ref_block_read_write(void)
 {
 	const int header_off = 21; /* random */
 	struct reftable_record recs[30];
@@ -103,9 +103,98 @@ static void t_block_read_write(void)
 		reftable_record_release(&recs[i]);
 }
 
+static void t_log_block_read_write(void)
+{
+	const int header_off = 21;
+	struct reftable_record recs[30];
+	const size_t N = ARRAY_SIZE(recs);
+	const size_t block_size = 2048;
+	struct reftable_block block = { 0 };
+	struct block_writer bw = {
+		.last_key = STRBUF_INIT,
+	};
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_LOG,
+	};
+	size_t i = 0;
+	int ret;
+	struct block_reader br = { 0 };
+	struct block_iter it = BLOCK_ITER_INIT;
+	struct strbuf want = STRBUF_INIT, buf = STRBUF_INIT;
+
+	REFTABLE_CALLOC_ARRAY(block.data, block_size);
+	block.len = block_size;
+	block_source_from_strbuf(&block.source ,&buf);
+	block_writer_init(&bw, BLOCK_TYPE_LOG, block.data, block_size,
+			  header_off, hash_size(GIT_SHA1_FORMAT_ID));
+
+	for (i = 0; i < N; i++) {
+		rec.u.log.refname = xstrfmt("branch%02"PRIuMAX , (uintmax_t)i);
+		rec.u.log.update_index = i;
+		rec.u.log.value_type = REFTABLE_LOG_UPDATE;
+
+		recs[i] = rec;
+		ret = block_writer_add(&bw, &rec);
+		rec.u.log.refname = NULL;
+		rec.u.log.value_type = REFTABLE_LOG_DELETION;
+		check_int(ret, ==, 0);
+	}
+
+	ret = block_writer_finish(&bw);
+	check_int(ret, >, 0);
+
+	block_writer_release(&bw);
+
+	block_reader_init(&br, &block, header_off, block_size, GIT_SHA1_RAWSZ);
+
+	block_iter_seek_start(&it, &br);
+
+	for (i = 0; ; i++) {
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, >=, 0);
+		if (ret > 0) {
+			check_int(i, ==, N);
+			break;
+		}
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+	}
+
+	for (i = 0; i < N; i++) {
+		block_iter_reset(&it);
+		strbuf_reset(&want);
+		strbuf_addstr(&want, recs[i].u.log.refname);
+
+		ret = block_iter_seek_key(&it, &br, &want);
+		check_int(ret, ==, 0);
+
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, ==, 0);
+
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+
+		want.len--;
+		ret = block_iter_seek_key(&it, &br, &want);
+		check_int(ret, ==, 0);
+
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, ==, 0);
+		check(reftable_record_equal(&recs[10 * (i / 10)], &rec, GIT_SHA1_RAWSZ));
+	}
+
+	block_reader_release(&br);
+	block_iter_close(&it);
+	reftable_record_release(&rec);
+	reftable_block_done(&br.block);
+	strbuf_release(&want);
+	strbuf_release(&buf);
+	for (i = 0; i < N; i++)
+		reftable_record_release(&recs[i]);
+}
+
 int cmd_main(int argc, const char *argv[])
 {
-	TEST(t_block_read_write(), "read-write operations on blocks work");
+	TEST(t_log_block_read_write(), "read-write operations on log blocks work");
+	TEST(t_ref_block_read_write(), "read-write operations on ref blocks work");
 
 	return test_done();
 }
-- 
2.45.GIT

[PATCH v3 10/11] t-reftable-block: add tests for obj blocks

[Chandra Pratap]

In the current testing setup, block operations are left unexercised
for obj blocks. Add a test that exercises these operations for obj
blocks.

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-block.c | 82 +++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index 4c4fb39ab4..beb2d6f81b 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -191,9 +191,91 @@ static void t_log_block_read_write(void)
 		reftable_record_release(&recs[i]);
 }
 
+static void t_obj_block_read_write(void)
+{
+	const int header_off = 21;
+	struct reftable_record recs[30];
+	const size_t N = ARRAY_SIZE(recs);
+	const size_t block_size = 1024;
+	struct reftable_block block = { 0 };
+	struct block_writer bw = {
+		.last_key = STRBUF_INIT,
+	};
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_OBJ,
+	};
+	size_t i = 0;
+	int ret;
+	struct block_reader br = { 0 };
+	struct block_iter it = BLOCK_ITER_INIT;
+	struct strbuf want = STRBUF_INIT, buf = STRBUF_INIT;
+
+	REFTABLE_CALLOC_ARRAY(block.data, block_size);
+	block.len = block_size;
+	block_source_from_strbuf(&block.source, &buf);
+	block_writer_init(&bw, BLOCK_TYPE_OBJ, block.data, block_size,
+			  header_off, hash_size(GIT_SHA1_FORMAT_ID));
+
+	for (i = 0; i < N; i++) {
+		uint8_t bytes[] = { i, i + 1, i + 2, i + 3, i + 5 }, *allocated;
+		DUP_ARRAY(allocated, bytes, ARRAY_SIZE(bytes));
+
+		rec.u.obj.hash_prefix = allocated;
+		rec.u.obj.hash_prefix_len = 5;
+
+		recs[i] = rec;
+		ret = block_writer_add(&bw, &rec);
+		rec.u.obj.hash_prefix = NULL;
+		rec.u.obj.hash_prefix_len = 0;
+		check_int(ret, ==, 0);
+	}
+
+	ret = block_writer_finish(&bw);
+	check_int(ret, >, 0);
+
+	block_writer_release(&bw);
+
+	block_reader_init(&br, &block, header_off, block_size, GIT_SHA1_RAWSZ);
+
+	block_iter_seek_start(&it, &br);
+
+	for (i = 0; ; i++) {
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, >=, 0);
+		if (ret > 0) {
+			check_int(i, ==, N);
+			break;
+		}
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+	}
+
+	for (i = 0; i < N; i++) {
+		block_iter_reset(&it);
+		reftable_record_key(&recs[i], &want);
+
+		ret = block_iter_seek_key(&it, &br, &want);
+		check_int(ret, ==, 0);
+
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, ==, 0);
+
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+	}
+
+	block_reader_release(&br);
+	block_iter_close(&it);
+	reftable_record_release(&rec);
+	reftable_block_done(&br.block);
+	strbuf_release(&want);
+	strbuf_release(&buf);
+	for (i = 0; i < N; i++)
+		reftable_record_release(&recs[i]);
+}
+
 int cmd_main(int argc, const char *argv[])
 {
 	TEST(t_log_block_read_write(), "read-write operations on log blocks work");
+	TEST(t_obj_block_read_write(), "read-write operations on obj blocks work");
 	TEST(t_ref_block_read_write(), "read-write operations on ref blocks work");
 
 	return test_done();
-- 
2.45.GIT

[PATCH v3 11/11] t-reftable-block: add tests for index blocks

[Chandra Pratap]

In the current testing setup, block operations are left unexercised
for index blocks. Add a test that exercises these operations for
index blocks.

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-block.c | 88 +++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index beb2d6f81b..582a8e6036 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -272,8 +272,96 @@ static void t_obj_block_read_write(void)
 		reftable_record_release(&recs[i]);
 }
 
+static void t_index_block_read_write(void)
+{
+	const int header_off = 21;
+	struct reftable_record recs[30];
+	const size_t N = ARRAY_SIZE(recs);
+	const size_t block_size = 1024;
+	struct reftable_block block = { 0 };
+	struct block_writer bw = {
+		.last_key = STRBUF_INIT,
+	};
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_INDEX,
+		.u.idx.last_key = STRBUF_INIT,
+	};
+	size_t i = 0;
+	int ret;
+	struct block_reader br = { 0 };
+	struct block_iter it = BLOCK_ITER_INIT;
+	struct strbuf want = STRBUF_INIT, buf = STRBUF_INIT;
+
+	REFTABLE_CALLOC_ARRAY(block.data, block_size);
+	block.len = block_size;
+	block_source_from_strbuf(&block.source, &buf);
+	block_writer_init(&bw, BLOCK_TYPE_INDEX, block.data, block_size,
+			  header_off, hash_size(GIT_SHA1_FORMAT_ID));
+
+	for (i = 0; i < N; i++) {
+		strbuf_init(&recs[i].u.idx.last_key, 9);
+
+		recs[i].type = BLOCK_TYPE_INDEX;
+		strbuf_addf(&recs[i].u.idx.last_key, "branch%02"PRIuMAX, (uintmax_t)i);
+		recs[i].u.idx.offset = i;
+
+		ret = block_writer_add(&bw, &recs[i]);
+		check_int(ret, ==, 0);
+	}
+
+	ret = block_writer_finish(&bw);
+	check_int(ret, >, 0);
+
+	block_writer_release(&bw);
+
+	block_reader_init(&br, &block, header_off, block_size, GIT_SHA1_RAWSZ);
+
+	block_iter_seek_start(&it, &br);
+
+	for (i = 0; ; i++) {
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, >=, 0);
+		if (ret > 0) {
+			check_int(i, ==, N);
+			break;
+		}
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+	}
+
+	for (i = 0; i < N; i++) {
+		block_iter_reset(&it);
+		reftable_record_key(&recs[i], &want);
+
+		ret = block_iter_seek_key(&it, &br, &want);
+		check_int(ret, ==, 0);
+
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, ==, 0);
+
+		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
+
+		want.len--;
+		ret = block_iter_seek_key(&it, &br, &want);
+		check_int(ret, ==, 0);
+
+		ret = block_iter_next(&it, &rec);
+		check_int(ret, ==, 0);
+		check(reftable_record_equal(&recs[10 * (i / 10)], &rec, GIT_SHA1_RAWSZ));
+	}
+
+	block_reader_release(&br);
+	block_iter_close(&it);
+	reftable_record_release(&rec);
+	reftable_block_done(&br.block);
+	strbuf_release(&want);
+	strbuf_release(&buf);
+	for (i = 0; i < N; i++)
+		reftable_record_release(&recs[i]);
+}
+
 int cmd_main(int argc, const char *argv[])
 {
+	TEST(t_index_block_read_write(), "read-write operations on index blocks work");
 	TEST(t_log_block_read_write(), "read-write operations on log blocks work");
 	TEST(t_obj_block_read_write(), "read-write operations on obj blocks work");
 	TEST(t_ref_block_read_write(), "read-write operations on ref blocks work");
-- 
2.45.GIT

Re: [GSoC][PATCH v3 00/11] t: port reftable/block_test.c to the unit testing framework

[Patrick Steinhardt]

On Wed, Aug 21, 2024 at 06:00:50PM +0530, Chandra Pratap wrote:
> The reftable library comes with self tests, which are exercised
> as part of the usual end-to-end tests and are designed to
> observe the end-user visible effects of Git commands. What it
> exercises, however, is a better match for the unit-testing
> framework, merged at 8bf6fbd0 (Merge branch 'js/doc-unit-tests',
> 2023-12-09), which is designed to observe how low level
> implementation details, at the level of sequences of individual
> function calls, behave.
> 
> Hence, port reftable/block_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:
> - Use block_source_from_strbuf() instead of malloc_block_source()
>   in log, obj, and index block tests (patches 9, 10, 11).
> - Remove a line that causes a memory leak in obj test (patch 10).

This addresses all comments I had on the preceding version and
plugs the memory leak. So this looks good to me, thanks!

Patrick

Re: [GSoC][PATCH v3 00/11] t: port reftable/block_test.c to the unit testing framework

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> - Use block_source_from_strbuf() instead of malloc_block_source()
>>   in log, obj, and index block tests (patches 9, 10, 11).
>> - Remove a line that causes a memory leak in obj test (patch 10).
>
> This addresses all comments I had on the preceding version and
> plugs the memory leak. So this looks good to me, thanks!

Thanks, both.  Let me mark it for 'next', then.