{"thread":{"id":"61916","subject":"[GSoC][PATCH 0/5] t: port reftable/readwrite_test.c to the unit testing framework","startedAt":"2024-08-07T14:16:52Z","lastAt":"2024-08-14T13:08:43Z","messageCount":30,"participants":["Chandra Pratap","Patrick Steinhardt","Junio C Hamano","Josh Steadmon"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"500323","messageId":"20240807141608.4524-1-chandrapratap3519@gmail.com","threadId":"61916","inReplyTo":null,"subject":"[GSoC][PATCH 0/5] t: port reftable/readwrite_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-07T14:11:56Z","receivedAt":"2024-08-07T14:16:52Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"The reftable library comes with self tests, which are exercised\nas part of the usual end-to-end tests and are designed to\nobserve the end-user visible effects of Git commands. What it\nexercises, however, is a better match for the unit-testing\nframework, merged at 8bf6fbd0 (Merge branch 'js/doc-unit-tests',\n2023-12-09), which is designed to observe how low level\nimplementation details, at the level of sequences of individual\nfunction calls, behave.\n\nHence, port reftable/readwrite_test.c to the unit testing framework\nand improve upon the ported test. The first patch in the series moves\nthe test to the unit testing framework, and the rest of the patches\nimprove upon the ported test.\n\nMentored-by: Patrick Steinhardt <ps@pks.im>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n\n---\nCI/PR: https://github.com/gitgitgadget/git/pull/1770\n\nChandra Pratap(5):\nt: move reftable/readwrite_test.c to the unit testing framework\nt-reftable-readwrite: use free_names() instead of a for loop\nt-reftable-readwrite: use 'for' in place of infinite 'while' loops\nt-reftable-readwrite: add test for known error\nt-reftable-readwrite: add tests for print functions\n\nMakefile                                                         |   2 +-\nreftable/reftable-tests.h                                        |   1 -\nt/helper/test-reftable.c                                         |   1 -\nreftable/readwrite_test.c => t/unit-tests/t-reftable-readwrite.c | 517 +++++++++++++++++++++++-----------------\n4 files changed, 294 insertions(+), 227 deletions(-)\n"},{"id":"500324","messageId":"20240807141608.4524-2-chandrapratap3519@gmail.com","threadId":"61916","inReplyTo":"20240807141608.4524-1-chandrapratap3519@gmail.com","subject":"[PATCH 1/5] t: move reftable/readwrite_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-07T14:11:57Z","receivedAt":"2024-08-07T14:16:56Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"reftable/readwrite_test.c exercises the functions defined in\nreftable/reader.{c,h} and reftable/writer.{c,h}. Migrate\nreftable/readwrite_test.c to the unit testing framework. Migration\ninvolves refactoring the tests to use the unit testing framework\ninstead of reftable's test framework and renaming the tests to\nalign with unit-tests' naming conventions.\n\nSince some tests in reftable/readwrite_test.c use the functions\nset_test_hash(), noop_flush() and strbuf_add_void() defined in\nreftable/test_framework.{c,h} but these files are not #included\nin the ported unit test, copy these functions in the new test file.\n\nWhile at it, ensure structs are 0-initialized with '= { 0 }'\ninstead of '= { NULL }'.\n\nMentored-by: Patrick Steinhardt <ps@pks.im>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n---\n Makefile                                      |   2 +-\n reftable/reftable-tests.h                     |   1 -\n t/helper/test-reftable.c                      |   1 -\n .../unit-tests/t-reftable-readwrite.c         | 418 +++++++++---------\n 4 files changed, 210 insertions(+), 212 deletions(-)\n rename reftable/readwrite_test.c => t/unit-tests/t-reftable-readwrite.c (70%)\n\ndiff --git a/Makefile b/Makefile\nindex 3863e60b66..76e4d1c1ec 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1341,6 +1341,7 @@ UNIT_TEST_PROGRAMS += t-oidtree\n UNIT_TEST_PROGRAMS += t-prio-queue\n UNIT_TEST_PROGRAMS += t-reftable-basics\n UNIT_TEST_PROGRAMS += t-reftable-merged\n+UNIT_TEST_PROGRAMS += t-reftable-readwrite\n UNIT_TEST_PROGRAMS += t-reftable-record\n UNIT_TEST_PROGRAMS += t-strbuf\n UNIT_TEST_PROGRAMS += t-strcmp-offset\n@@ -2682,7 +2683,6 @@ REFTABLE_OBJS += reftable/writer.o\n REFTABLE_TEST_OBJS += reftable/block_test.o\n REFTABLE_TEST_OBJS += reftable/dump.o\n REFTABLE_TEST_OBJS += reftable/pq_test.o\n-REFTABLE_TEST_OBJS += reftable/readwrite_test.o\n REFTABLE_TEST_OBJS += reftable/stack_test.o\n REFTABLE_TEST_OBJS += reftable/test_framework.o\n REFTABLE_TEST_OBJS += reftable/tree_test.o\ndiff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h\nindex d5e03dcc1b..7d955393d2 100644\n--- a/reftable/reftable-tests.h\n+++ b/reftable/reftable-tests.h\n@@ -13,7 +13,6 @@ int basics_test_main(int argc, const char **argv);\n int block_test_main(int argc, const char **argv);\n int pq_test_main(int argc, const char **argv);\n int record_test_main(int argc, const char **argv);\n-int readwrite_test_main(int argc, const char **argv);\n int stack_test_main(int argc, const char **argv);\n int tree_test_main(int argc, const char **argv);\n int reftable_dump_main(int argc, char *const *argv);\ndiff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c\nindex 9d378427da..d371e9f9dd 100644\n--- a/t/helper/test-reftable.c\n+++ b/t/helper/test-reftable.c\n@@ -8,7 +8,6 @@ int cmd__reftable(int argc, const char **argv)\n \tblock_test_main(argc, argv);\n \ttree_test_main(argc, argv);\n \tpq_test_main(argc, argv);\n-\treadwrite_test_main(argc, argv);\n \tstack_test_main(argc, argv);\n \treturn 0;\n }\ndiff --git a/reftable/readwrite_test.c b/t/unit-tests/t-reftable-readwrite.c\nsimilarity index 70%\nrename from reftable/readwrite_test.c\nrename to t/unit-tests/t-reftable-readwrite.c\nindex f411abfe9c..235e3d94c7 100644\n--- a/reftable/readwrite_test.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -6,37 +6,48 @@ license that can be found in the LICENSE file or at\n https://developers.google.com/open-source/licenses/bsd\n */\n \n-#include \"system.h\"\n-\n-#include \"basics.h\"\n-#include \"block.h\"\n-#include \"blocksource.h\"\n-#include \"reader.h\"\n-#include \"record.h\"\n-#include \"test_framework.h\"\n-#include \"reftable-tests.h\"\n-#include \"reftable-writer.h\"\n+#include \"test-lib.h\"\n+#include \"reftable/reader.h\"\n+#include \"reftable/blocksource.h\"\n+#include \"reftable/reftable-error.h\"\n+#include \"reftable/reftable-writer.h\"\n \n static const int update_index = 5;\n \n-static void test_buffer(void)\n+static void set_test_hash(uint8_t *p, int i)\n+{\n+\tmemset(p, (uint8_t)i, hash_size(GIT_SHA1_FORMAT_ID));\n+}\n+\n+static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)\n+{\n+\tstrbuf_add(b, data, sz);\n+\treturn sz;\n+}\n+\n+static int noop_flush(void *arg)\n+{\n+\treturn 0;\n+}\n+\n+static void t_buffer(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_block out = { NULL };\n+\tstruct reftable_block_source source = { 0 };\n+\tstruct reftable_block out = { 0 };\n \tint n;\n \tuint8_t in[] = \"hello\";\n \tstrbuf_add(&buf, in, sizeof(in));\n \tblock_source_from_strbuf(&source, &buf);\n-\tEXPECT(block_source_size(&source) == 6);\n+\tcheck_int(block_source_size(&source), ==, 6);\n \tn = block_source_read_block(&source, &out, 0, sizeof(in));\n-\tEXPECT(n == sizeof(in));\n-\tEXPECT(!memcmp(in, out.data, n));\n+\tcheck_int(n, ==, sizeof(in));\n+\tcheck(!memcmp(in, out.data, n));\n \treftable_block_done(&out);\n \n \tn = block_source_read_block(&source, &out, 1, 2);\n-\tEXPECT(n == 2);\n-\tEXPECT(!memcmp(out.data, \"el\", 2));\n+\tcheck_int(n, ==, 2);\n+\tcheck(!memcmp(out.data, \"el\", 2));\n \n \treftable_block_done(&out);\n \tblock_source_close(&source);\n@@ -52,9 +63,9 @@ static void write_table(char ***names, struct strbuf *buf, int N,\n \t};\n \tstruct reftable_writer *w =\n \t\treftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);\n-\tstruct reftable_ref_record ref = { NULL };\n+\tstruct reftable_ref_record ref = { 0 };\n \tint i = 0, n;\n-\tstruct reftable_log_record log = { NULL };\n+\tstruct reftable_log_record log = { 0 };\n \tconst struct reftable_stats *stats = NULL;\n \n \tREFTABLE_CALLOC_ARRAY(*names, N + 1);\n@@ -73,7 +84,7 @@ static void write_table(char ***names, struct strbuf *buf, int N,\n \t\t(*names)[i] = xstrdup(name);\n \n \t\tn = reftable_writer_add_ref(w, &ref);\n-\t\tEXPECT(n == 0);\n+\t\tcheck_int(n, ==, 0);\n \t}\n \n \tfor (i = 0; i < N; i++) {\n@@ -89,27 +100,25 @@ static void write_table(char ***names, struct strbuf *buf, int N,\n \t\tlog.value.update.message = (char *) \"message\";\n \n \t\tn = reftable_writer_add_log(w, &log);\n-\t\tEXPECT(n == 0);\n+\t\tcheck_int(n, ==, 0);\n \t}\n \n \tn = reftable_writer_close(w);\n-\tEXPECT(n == 0);\n+\tcheck_int(n, ==, 0);\n \n \tstats = reftable_writer_stats(w);\n \tfor (i = 0; i < stats->ref_stats.blocks; i++) {\n \t\tint off = i * opts.block_size;\n-\t\tif (off == 0) {\n-\t\t\toff = header_size(\n-\t\t\t\t(hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);\n-\t\t}\n-\t\tEXPECT(buf->buf[off] == 'r');\n+\t\tif (!off)\n+\t\t\toff = header_size((hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);\n+\t\tcheck(buf->buf[off] == 'r');\n \t}\n \n-\tEXPECT(stats->log_stats.blocks > 0);\n+\tcheck(stats->log_stats.blocks > 0);\n \treftable_writer_free(w);\n }\n \n-static void test_log_buffer_size(void)\n+static void t_log_buffer_size(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_write_options opts = {\n@@ -140,14 +149,14 @@ static void test_log_buffer_size(void)\n \t}\n \treftable_writer_set_limits(w, update_index, update_index);\n \terr = reftable_writer_add_log(w, &log);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \terr = reftable_writer_close(w);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n }\n \n-static void test_log_overflow(void)\n+static void t_log_overflow(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tchar msg[256] = { 0 };\n@@ -177,12 +186,12 @@ static void test_log_overflow(void)\n \tmemset(msg, 'x', sizeof(msg) - 1);\n \treftable_writer_set_limits(w, update_index, update_index);\n \terr = reftable_writer_add_log(w, &log);\n-\tEXPECT(err == REFTABLE_ENTRY_TOO_BIG_ERROR);\n+\tcheck_int(err, ==, REFTABLE_ENTRY_TOO_BIG_ERROR);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n }\n \n-static void test_log_write_read(void)\n+static void t_log_write_read(void)\n {\n \tint N = 2;\n \tchar **names = reftable_calloc(N + 1, sizeof(*names));\n@@ -190,13 +199,13 @@ static void test_log_write_read(void)\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 256,\n \t};\n-\tstruct reftable_ref_record ref = { NULL };\n+\tstruct reftable_ref_record ref = { 0 };\n \tint i = 0;\n-\tstruct reftable_log_record log = { NULL };\n+\tstruct reftable_log_record log = { 0 };\n \tint n;\n-\tstruct reftable_iterator it = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n-\tstruct reftable_block_source source = { NULL };\n+\tstruct reftable_iterator it = { 0 };\n+\tstruct reftable_reader rd = { 0 };\n+\tstruct reftable_block_source source = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n \t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n@@ -204,17 +213,17 @@ static void test_log_write_read(void)\n \treftable_writer_set_limits(w, 0, N);\n \tfor (i = 0; i < N; i++) {\n \t\tchar name[256];\n-\t\tstruct reftable_ref_record ref = { NULL };\n+\t\tstruct reftable_ref_record ref = { 0 };\n \t\tsnprintf(name, sizeof(name), \"b%02d%0*d\", i, 130, 7);\n \t\tnames[i] = xstrdup(name);\n \t\tref.refname = name;\n \t\tref.update_index = i;\n \n \t\terr = reftable_writer_add_ref(w, &ref);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \tfor (i = 0; i < N; i++) {\n-\t\tstruct reftable_log_record log = { NULL };\n+\t\tstruct reftable_log_record log = { 0 };\n \n \t\tlog.refname = names[i];\n \t\tlog.update_index = i;\n@@ -223,33 +232,33 @@ static void test_log_write_read(void)\n \t\tset_test_hash(log.value.update.new_hash, i + 1);\n \n \t\terr = reftable_writer_add_log(w, &log);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \n \tn = reftable_writer_close(w);\n-\tEXPECT(n == 0);\n+\tcheck_int(n, ==, 0);\n \n \tstats = reftable_writer_stats(w);\n-\tEXPECT(stats->log_stats.blocks > 0);\n+\tcheck_int(stats->log_stats.blocks, >, 0);\n \treftable_writer_free(w);\n \tw = NULL;\n \n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = init_reader(&rd, &source, \"file.log\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_reader_init_ref_iterator(&rd, &it);\n \n \terr = reftable_iterator_seek_ref(&it, names[N - 1]);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \terr = reftable_iterator_next_ref(&it, &ref);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \t/* end of iteration. */\n \terr = reftable_iterator_next_ref(&it, &ref);\n-\tEXPECT(0 < err);\n+\tcheck_int(err, >, 0);\n \n \treftable_iterator_destroy(&it);\n \treftable_ref_record_release(&ref);\n@@ -257,23 +266,21 @@ static void test_log_write_read(void)\n \treftable_reader_init_log_iterator(&rd, &it);\n \n \terr = reftable_iterator_seek_log(&it, \"\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \ti = 0;\n \twhile (1) {\n \t\tint err = reftable_iterator_next_log(&it, &log);\n-\t\tif (err > 0) {\n+\t\tif (err > 0)\n \t\t\tbreak;\n-\t\t}\n-\n-\t\tEXPECT_ERR(err);\n-\t\tEXPECT_STREQ(names[i], log.refname);\n-\t\tEXPECT(i == log.update_index);\n+\t\tcheck(!err);\n+\t\tcheck_str(names[i], log.refname);\n+\t\tcheck_int(i, ==, log.update_index);\n \t\ti++;\n \t\treftable_log_record_release(&log);\n \t}\n \n-\tEXPECT(i == N);\n+\tcheck_int(i, ==, N);\n \treftable_iterator_destroy(&it);\n \n \t/* cleanup. */\n@@ -282,7 +289,7 @@ static void test_log_write_read(void)\n \treader_close(&rd);\n }\n \n-static void test_log_zlib_corruption(void)\n+static void t_log_zlib_corruption(void)\n {\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 256,\n@@ -316,13 +323,13 @@ static void test_log_zlib_corruption(void)\n \treftable_writer_set_limits(w, 1, 1);\n \n \terr = reftable_writer_add_log(w, &log);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \tn = reftable_writer_close(w);\n-\tEXPECT(n == 0);\n+\tcheck_int(n, ==, 0);\n \n \tstats = reftable_writer_stats(w);\n-\tEXPECT(stats->log_stats.blocks > 0);\n+\tcheck_int(stats->log_stats.blocks, >, 0);\n \treftable_writer_free(w);\n \tw = NULL;\n \n@@ -332,11 +339,11 @@ static void test_log_zlib_corruption(void)\n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = init_reader(&rd, &source, \"file.log\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_reader_init_log_iterator(&rd, &it);\n \terr = reftable_iterator_seek_log(&it, \"refname\");\n-\tEXPECT(err == REFTABLE_ZLIB_ERROR);\n+\tcheck_int(err, ==, REFTABLE_ZLIB_ERROR);\n \n \treftable_iterator_destroy(&it);\n \n@@ -345,14 +352,14 @@ static void test_log_zlib_corruption(void)\n \treader_close(&rd);\n }\n \n-static void test_table_read_write_sequential(void)\n+static void t_table_read_write_sequential(void)\n {\n \tchar **names;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint N = 50;\n-\tstruct reftable_iterator it = { NULL };\n-\tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_iterator it = { 0 };\n+\tstruct reftable_block_source source = { 0 };\n+\tstruct reftable_reader rd = { 0 };\n \tint err = 0;\n \tint j = 0;\n \n@@ -361,26 +368,25 @@ static void test_table_read_write_sequential(void)\n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = init_reader(&rd, &source, \"file.ref\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_reader_init_ref_iterator(&rd, &it);\n \terr = reftable_iterator_seek_ref(&it, \"\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \twhile (1) {\n-\t\tstruct reftable_ref_record ref = { NULL };\n+\t\tstruct reftable_ref_record ref = { 0 };\n \t\tint r = reftable_iterator_next_ref(&it, &ref);\n-\t\tEXPECT(r >= 0);\n-\t\tif (r > 0) {\n+\t\tcheck_int(r, >=, 0);\n+\t\tif (r > 0)\n \t\t\tbreak;\n-\t\t}\n-\t\tEXPECT(0 == strcmp(names[j], ref.refname));\n-\t\tEXPECT(update_index == ref.update_index);\n+\t\tcheck_str(names[j], ref.refname);\n+\t\tcheck_int(update_index, ==, ref.update_index);\n \n \t\tj++;\n \t\treftable_ref_record_release(&ref);\n \t}\n-\tEXPECT(j == N);\n+\tcheck_int(j, ==, N);\n \treftable_iterator_destroy(&it);\n \tstrbuf_release(&buf);\n \tfree_names(names);\n@@ -388,90 +394,88 @@ static void test_table_read_write_sequential(void)\n \treader_close(&rd);\n }\n \n-static void test_table_write_small_table(void)\n+static void t_table_write_small_table(void)\n {\n \tchar **names;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint N = 1;\n \twrite_table(&names, &buf, N, 4096, GIT_SHA1_FORMAT_ID);\n-\tEXPECT(buf.len < 200);\n+\tcheck_int(buf.len, <, 200);\n \tstrbuf_release(&buf);\n \tfree_names(names);\n }\n \n-static void test_table_read_api(void)\n+static void t_table_read_api(void)\n {\n \tchar **names;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint N = 50;\n-\tstruct reftable_reader rd = { NULL };\n-\tstruct reftable_block_source source = { NULL };\n+\tstruct reftable_reader rd = { 0 };\n+\tstruct reftable_block_source source = { 0 };\n \tint err;\n \tint i;\n-\tstruct reftable_log_record log = { NULL };\n-\tstruct reftable_iterator it = { NULL };\n+\tstruct reftable_log_record log = { 0 };\n+\tstruct reftable_iterator it = { 0 };\n \n \twrite_table(&names, &buf, N, 256, GIT_SHA1_FORMAT_ID);\n \n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = init_reader(&rd, &source, \"file.ref\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_reader_init_ref_iterator(&rd, &it);\n \terr = reftable_iterator_seek_ref(&it, names[0]);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \terr = reftable_iterator_next_log(&it, &log);\n-\tEXPECT(err == REFTABLE_API_ERROR);\n+\tcheck_int(err, ==, REFTABLE_API_ERROR);\n \n \tstrbuf_release(&buf);\n-\tfor (i = 0; i < N; i++) {\n+\tfor (i = 0; i < N; i++)\n \t\treftable_free(names[i]);\n-\t}\n \treftable_iterator_destroy(&it);\n \treftable_free(names);\n \treader_close(&rd);\n \tstrbuf_release(&buf);\n }\n \n-static void test_table_read_write_seek(int index, int hash_id)\n+static void t_table_read_write_seek(int index, int hash_id)\n {\n \tchar **names;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint N = 50;\n-\tstruct reftable_reader rd = { NULL };\n-\tstruct reftable_block_source source = { NULL };\n+\tstruct reftable_reader rd = { 0 };\n+\tstruct reftable_block_source source = { 0 };\n \tint err;\n \tint i = 0;\n \n-\tstruct reftable_iterator it = { NULL };\n+\tstruct reftable_iterator it = { 0 };\n \tstruct strbuf pastLast = STRBUF_INIT;\n-\tstruct reftable_ref_record ref = { NULL };\n+\tstruct reftable_ref_record ref = { 0 };\n \n \twrite_table(&names, &buf, N, 256, hash_id);\n \n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = init_reader(&rd, &source, \"file.ref\");\n-\tEXPECT_ERR(err);\n-\tEXPECT(hash_id == reftable_reader_hash_id(&rd));\n+\tcheck(!err);\n+\tcheck_int(hash_id, ==, reftable_reader_hash_id(&rd));\n \n-\tif (!index) {\n+\tif (!index)\n \t\trd.ref_offsets.index_offset = 0;\n-\t} else {\n-\t\tEXPECT(rd.ref_offsets.index_offset > 0);\n-\t}\n+\telse\n+\t\tcheck_int(rd.ref_offsets.index_offset, >, 0);\n \n \tfor (i = 1; i < N; i++) {\n \t\treftable_reader_init_ref_iterator(&rd, &it);\n \t\terr = reftable_iterator_seek_ref(&it, names[i]);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t\terr = reftable_iterator_next_ref(&it, &ref);\n-\t\tEXPECT_ERR(err);\n-\t\tEXPECT(0 == strcmp(names[i], ref.refname));\n-\t\tEXPECT(REFTABLE_REF_VAL1 == ref.value_type);\n-\t\tEXPECT(i == ref.value.val1[0]);\n+\t\tcheck(!err);\n+\t\tcheck_str(names[i], ref.refname);\n+\t\tcheck_int(REFTABLE_REF_VAL1, ==, ref.value_type);\n+\t\tcheck_int(i, ==, ref.value.val1[0]);\n \n \t\treftable_ref_record_release(&ref);\n \t\treftable_iterator_destroy(&it);\n@@ -483,40 +487,39 @@ static void test_table_read_write_seek(int index, int hash_id)\n \treftable_reader_init_ref_iterator(&rd, &it);\n \terr = reftable_iterator_seek_ref(&it, pastLast.buf);\n \tif (err == 0) {\n-\t\tstruct reftable_ref_record ref = { NULL };\n+\t\tstruct reftable_ref_record ref = { 0 };\n \t\tint err = reftable_iterator_next_ref(&it, &ref);\n-\t\tEXPECT(err > 0);\n+\t\tcheck_int(err, >, 0);\n \t} else {\n-\t\tEXPECT(err > 0);\n+\t\tcheck_int(err, >, 0);\n \t}\n \n \tstrbuf_release(&pastLast);\n \treftable_iterator_destroy(&it);\n \n \tstrbuf_release(&buf);\n-\tfor (i = 0; i < N; i++) {\n+\tfor (i = 0; i < N; i++)\n \t\treftable_free(names[i]);\n-\t}\n \treftable_free(names);\n \treader_close(&rd);\n }\n \n-static void test_table_read_write_seek_linear(void)\n+static void t_table_read_write_seek_linear(void)\n {\n-\ttest_table_read_write_seek(0, GIT_SHA1_FORMAT_ID);\n+\tt_table_read_write_seek(0, GIT_SHA1_FORMAT_ID);\n }\n \n-static void test_table_read_write_seek_linear_sha256(void)\n+static void t_table_read_write_seek_linear_sha256(void)\n {\n-\ttest_table_read_write_seek(0, GIT_SHA256_FORMAT_ID);\n+\tt_table_read_write_seek(0, GIT_SHA256_FORMAT_ID);\n }\n \n-static void test_table_read_write_seek_index(void)\n+static void t_table_read_write_seek_index(void)\n {\n-\ttest_table_read_write_seek(1, GIT_SHA1_FORMAT_ID);\n+\tt_table_read_write_seek(1, GIT_SHA1_FORMAT_ID);\n }\n \n-static void test_table_refs_for(int indexed)\n+static void t_table_refs_for(int indexed)\n {\n \tint N = 50;\n \tchar **want_names = reftable_calloc(N + 1, sizeof(*want_names));\n@@ -526,18 +529,18 @@ static void test_table_refs_for(int indexed)\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 256,\n \t};\n-\tstruct reftable_ref_record ref = { NULL };\n+\tstruct reftable_ref_record ref = { 0 };\n \tint i = 0;\n \tint n;\n \tint err;\n \tstruct reftable_reader rd;\n-\tstruct reftable_block_source source = { NULL };\n+\tstruct reftable_block_source source = { 0 };\n \n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n \t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n \n-\tstruct reftable_iterator it = { NULL };\n+\tstruct reftable_iterator it = { 0 };\n \tint j;\n \n \tset_test_hash(want_hash, 4);\n@@ -546,7 +549,7 @@ static void test_table_refs_for(int indexed)\n \t\tuint8_t hash[GIT_SHA1_RAWSZ];\n \t\tchar fill[51] = { 0 };\n \t\tchar name[100];\n-\t\tstruct reftable_ref_record ref = { NULL };\n+\t\tstruct reftable_ref_record ref = { 0 };\n \n \t\tmemset(hash, i, sizeof(hash));\n \t\tmemset(fill, 'x', 50);\n@@ -563,16 +566,15 @@ static void test_table_refs_for(int indexed)\n \t\t */\n \t\t/* blocks. */\n \t\tn = reftable_writer_add_ref(w, &ref);\n-\t\tEXPECT(n == 0);\n+\t\tcheck_int(n, ==, 0);\n \n \t\tif (!memcmp(ref.value.val2.value, want_hash, GIT_SHA1_RAWSZ) ||\n-\t\t    !memcmp(ref.value.val2.target_value, want_hash, GIT_SHA1_RAWSZ)) {\n+\t\t    !memcmp(ref.value.val2.target_value, want_hash, GIT_SHA1_RAWSZ))\n \t\t\twant_names[want_names_len++] = xstrdup(name);\n-\t\t}\n \t}\n \n \tn = reftable_writer_close(w);\n-\tEXPECT(n == 0);\n+\tcheck_int(n, ==, 0);\n \n \treftable_writer_free(w);\n \tw = NULL;\n@@ -580,33 +582,30 @@ static void test_table_refs_for(int indexed)\n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = init_reader(&rd, &source, \"file.ref\");\n-\tEXPECT_ERR(err);\n-\tif (!indexed) {\n+\tcheck(!err);\n+\tif (!indexed)\n \t\trd.obj_offsets.is_present = 0;\n-\t}\n \n \treftable_reader_init_ref_iterator(&rd, &it);\n \terr = reftable_iterator_seek_ref(&it, \"\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \treftable_iterator_destroy(&it);\n \n \terr = reftable_reader_refs_for(&rd, &it, want_hash);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \tj = 0;\n \twhile (1) {\n \t\tint err = reftable_iterator_next_ref(&it, &ref);\n-\t\tEXPECT(err >= 0);\n-\t\tif (err > 0) {\n+\t\tcheck_int(err, >=, 0);\n+\t\tif (err > 0)\n \t\t\tbreak;\n-\t\t}\n-\n-\t\tEXPECT(j < want_names_len);\n-\t\tEXPECT(0 == strcmp(ref.refname, want_names[j]));\n+\t\tcheck_int(j, <, want_names_len);\n+\t\tcheck_str(ref.refname, want_names[j]);\n \t\tj++;\n \t\treftable_ref_record_release(&ref);\n \t}\n-\tEXPECT(j == want_names_len);\n+\tcheck_int(j, ==, want_names_len);\n \n \tstrbuf_release(&buf);\n \tfree_names(want_names);\n@@ -614,54 +613,54 @@ static void test_table_refs_for(int indexed)\n \treader_close(&rd);\n }\n \n-static void test_table_refs_for_no_index(void)\n+static void t_table_refs_for_no_index(void)\n {\n-\ttest_table_refs_for(0);\n+\tt_table_refs_for(0);\n }\n \n-static void test_table_refs_for_obj_index(void)\n+static void t_table_refs_for_obj_index(void)\n {\n-\ttest_table_refs_for(1);\n+\tt_table_refs_for(1);\n }\n \n-static void test_write_empty_table(void)\n+static void t_write_empty_table(void)\n {\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n \t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n-\tstruct reftable_block_source source = { NULL };\n+\tstruct reftable_block_source source = { 0 };\n \tstruct reftable_reader *rd = NULL;\n-\tstruct reftable_ref_record rec = { NULL };\n-\tstruct reftable_iterator it = { NULL };\n+\tstruct reftable_ref_record rec = { 0 };\n+\tstruct reftable_iterator it = { 0 };\n \tint err;\n \n \treftable_writer_set_limits(w, 1, 1);\n \n \terr = reftable_writer_close(w);\n-\tEXPECT(err == REFTABLE_EMPTY_TABLE_ERROR);\n+\tcheck_int(err, ==, REFTABLE_EMPTY_TABLE_ERROR);\n \treftable_writer_free(w);\n \n-\tEXPECT(buf.len == header_size(1) + footer_size(1));\n+\tcheck_int(buf.len, ==, header_size(1) + footer_size(1));\n \n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = reftable_new_reader(&rd, &source, \"filename\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_reader_init_ref_iterator(rd, &it);\n \terr = reftable_iterator_seek_ref(&it, \"\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \terr = reftable_iterator_next_ref(&it, &rec);\n-\tEXPECT(err > 0);\n+\tcheck_int(err, >, 0);\n \n \treftable_iterator_destroy(&it);\n \treftable_reader_free(rd);\n \tstrbuf_release(&buf);\n }\n \n-static void test_write_object_id_min_length(void)\n+static void t_write_object_id_min_length(void)\n {\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 75,\n@@ -686,17 +685,17 @@ static void test_write_object_id_min_length(void)\n \t\tsnprintf(name, sizeof(name), \"ref%05d\", i);\n \t\tref.refname = name;\n \t\terr = reftable_writer_add_ref(w, &ref);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \n \terr = reftable_writer_close(w);\n-\tEXPECT_ERR(err);\n-\tEXPECT(reftable_writer_stats(w)->object_id_len == 2);\n+\tcheck(!err);\n+\tcheck_int(reftable_writer_stats(w)->object_id_len, ==, 2);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n }\n \n-static void test_write_object_id_length(void)\n+static void t_write_object_id_length(void)\n {\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 75,\n@@ -722,17 +721,17 @@ static void test_write_object_id_length(void)\n \t\tref.refname = name;\n \t\tref.value.val1[15] = i;\n \t\terr = reftable_writer_add_ref(w, &ref);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \n \terr = reftable_writer_close(w);\n-\tEXPECT_ERR(err);\n-\tEXPECT(reftable_writer_stats(w)->object_id_len == 16);\n+\tcheck(!err);\n+\tcheck_int(reftable_writer_stats(w)->object_id_len, ==, 16);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n }\n \n-static void test_write_empty_key(void)\n+static void t_write_empty_key(void)\n {\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -747,15 +746,15 @@ static void test_write_empty_key(void)\n \n \treftable_writer_set_limits(w, 1, 1);\n \terr = reftable_writer_add_ref(w, &ref);\n-\tEXPECT(err == REFTABLE_API_ERROR);\n+\tcheck_int(err, ==, REFTABLE_API_ERROR);\n \n \terr = reftable_writer_close(w);\n-\tEXPECT(err == REFTABLE_EMPTY_TABLE_ERROR);\n+\tcheck_int(err, ==, REFTABLE_EMPTY_TABLE_ERROR);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n }\n \n-static void test_write_key_order(void)\n+static void t_write_key_order(void)\n {\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -782,15 +781,15 @@ static void test_write_key_order(void)\n \n \treftable_writer_set_limits(w, 1, 1);\n \terr = reftable_writer_add_ref(w, &refs[0]);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \terr = reftable_writer_add_ref(w, &refs[1]);\n-\tEXPECT(err == REFTABLE_API_ERROR);\n+\tcheck_int(err, ==, REFTABLE_API_ERROR);\n \treftable_writer_close(w);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n }\n \n-static void test_write_multiple_indices(void)\n+static void t_write_multiple_indices(void)\n {\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 100,\n@@ -817,7 +816,7 @@ static void test_write_multiple_indices(void)\n \t\tref.refname = buf.buf,\n \n \t\terr = reftable_writer_add_ref(writer, &ref);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \n \tfor (i = 0; i < 100; i++) {\n@@ -835,7 +834,7 @@ static void test_write_multiple_indices(void)\n \t\tlog.refname = buf.buf,\n \n \t\terr = reftable_writer_add_log(writer, &log);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \n \treftable_writer_close(writer);\n@@ -845,13 +844,13 @@ static void test_write_multiple_indices(void)\n \t * for each of the block types.\n \t */\n \tstats = reftable_writer_stats(writer);\n-\tEXPECT(stats->ref_stats.index_offset > 0);\n-\tEXPECT(stats->obj_stats.index_offset > 0);\n-\tEXPECT(stats->log_stats.index_offset > 0);\n+\tcheck_int(stats->ref_stats.index_offset, >, 0);\n+\tcheck_int(stats->obj_stats.index_offset, >, 0);\n+\tcheck_int(stats->log_stats.index_offset, >, 0);\n \n \tblock_source_from_strbuf(&source, &writer_buf);\n \terr = reftable_new_reader(&reader, &source, \"filename\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \t/*\n \t * Seeking the log uses the log index now. In case there is any\n@@ -859,7 +858,7 @@ static void test_write_multiple_indices(void)\n \t */\n \treftable_reader_init_log_iterator(reader, &it);\n \terr = reftable_iterator_seek_log(&it, \"\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_iterator_destroy(&it);\n \treftable_writer_free(writer);\n@@ -868,7 +867,7 @@ static void test_write_multiple_indices(void)\n \tstrbuf_release(&buf);\n }\n \n-static void test_write_multi_level_index(void)\n+static void t_write_multi_level_index(void)\n {\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 100,\n@@ -895,7 +894,7 @@ static void test_write_multi_level_index(void)\n \t\tref.refname = buf.buf,\n \n \t\terr = reftable_writer_add_ref(writer, &ref);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \treftable_writer_close(writer);\n \n@@ -904,18 +903,18 @@ static void test_write_multi_level_index(void)\n \t * multi-level index.\n \t */\n \tstats = reftable_writer_stats(writer);\n-\tEXPECT(stats->ref_stats.max_index_level == 2);\n+\tcheck_int(stats->ref_stats.max_index_level, ==, 2);\n \n \tblock_source_from_strbuf(&source, &writer_buf);\n \terr = reftable_new_reader(&reader, &source, \"filename\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \t/*\n \t * Seeking the last ref should work as expected.\n \t */\n \treftable_reader_init_ref_iterator(reader, &it);\n \terr = reftable_iterator_seek_ref(&it, \"refs/heads/199\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_iterator_destroy(&it);\n \treftable_writer_free(writer);\n@@ -924,56 +923,57 @@ static void test_write_multi_level_index(void)\n \tstrbuf_release(&buf);\n }\n \n-static void test_corrupt_table_empty(void)\n+static void t_corrupt_table_empty(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_block_source source = { 0 };\n+\tstruct reftable_reader rd = { 0 };\n \tint err;\n \n \tblock_source_from_strbuf(&source, &buf);\n \terr = init_reader(&rd, &source, \"file.log\");\n-\tEXPECT(err == REFTABLE_FORMAT_ERROR);\n+\tcheck_int(err, ==, REFTABLE_FORMAT_ERROR);\n }\n \n-static void test_corrupt_table(void)\n+static void t_corrupt_table(void)\n {\n \tuint8_t zeros[1024] = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_block_source source = { 0 };\n+\tstruct reftable_reader rd = { 0 };\n \tint err;\n \tstrbuf_add(&buf, zeros, sizeof(zeros));\n \n \tblock_source_from_strbuf(&source, &buf);\n \terr = init_reader(&rd, &source, \"file.log\");\n-\tEXPECT(err == REFTABLE_FORMAT_ERROR);\n+\tcheck_int(err, ==, REFTABLE_FORMAT_ERROR);\n \tstrbuf_release(&buf);\n }\n \n-int readwrite_test_main(int argc, const char *argv[])\n+int cmd_main(int argc, const char *argv[])\n {\n-\tRUN_TEST(test_log_zlib_corruption);\n-\tRUN_TEST(test_corrupt_table);\n-\tRUN_TEST(test_corrupt_table_empty);\n-\tRUN_TEST(test_log_write_read);\n-\tRUN_TEST(test_write_key_order);\n-\tRUN_TEST(test_table_read_write_seek_linear_sha256);\n-\tRUN_TEST(test_log_buffer_size);\n-\tRUN_TEST(test_table_write_small_table);\n-\tRUN_TEST(test_buffer);\n-\tRUN_TEST(test_table_read_api);\n-\tRUN_TEST(test_table_read_write_sequential);\n-\tRUN_TEST(test_table_read_write_seek_linear);\n-\tRUN_TEST(test_table_read_write_seek_index);\n-\tRUN_TEST(test_table_refs_for_no_index);\n-\tRUN_TEST(test_table_refs_for_obj_index);\n-\tRUN_TEST(test_write_empty_key);\n-\tRUN_TEST(test_write_empty_table);\n-\tRUN_TEST(test_log_overflow);\n-\tRUN_TEST(test_write_object_id_length);\n-\tRUN_TEST(test_write_object_id_min_length);\n-\tRUN_TEST(test_write_multiple_indices);\n-\tRUN_TEST(test_write_multi_level_index);\n-\treturn 0;\n+\tTEST(t_buffer(), \"strbuf works as blocksource\");\n+\tTEST(t_corrupt_table(), \"read-write on corrupted table\");\n+\tTEST(t_corrupt_table_empty(), \"read-write on an empty table\");\n+\tTEST(t_log_buffer_size(), \"buffer extension for log compression\");\n+\tTEST(t_log_overflow(), \"log overflow returns expected error\");\n+\tTEST(t_log_write_read(), \"read-write on log records\");\n+\tTEST(t_log_zlib_corruption(), \"reading corrupted log record returns expected error\");\n+\tTEST(t_table_read_api(), \"read on a table\");\n+\tTEST(t_table_read_write_seek_index(), \"read-write on a table with index\");\n+\tTEST(t_table_read_write_seek_linear(), \"read-write on a table without index (SHA1)\");\n+\tTEST(t_table_read_write_seek_linear_sha256(), \"read-write on a table without index (SHA256)\");\n+\tTEST(t_table_read_write_sequential(), \"sequential read-write on a table\");\n+\tTEST(t_table_refs_for_no_index(), \"refs-only table with no index\");\n+\tTEST(t_table_refs_for_obj_index(), \"refs-only table with index\");\n+\tTEST(t_table_write_small_table(), \"write_table works\");\n+\tTEST(t_write_empty_key(), \"write on refs with empty keys\");\n+\tTEST(t_write_empty_table(), \"read-write on empty tables\");\n+\tTEST(t_write_key_order(), \"refs must be written in increasing order\");\n+\tTEST(t_write_multi_level_index(), \"table with multi-level index\");\n+\tTEST(t_write_multiple_indices(), \"table with indices for multiple block types\");\n+\tTEST(t_write_object_id_length(), \"prefix compression on writing refs\");\n+\tTEST(t_write_object_id_min_length(), \"prefix compression on writing refs\");\n+\n+\treturn test_done();\n }\n-- \n2.45.GIT\n\n"},{"id":"500325","messageId":"20240807141608.4524-3-chandrapratap3519@gmail.com","threadId":"61916","inReplyTo":"20240807141608.4524-1-chandrapratap3519@gmail.com","subject":"[PATCH 2/5] t-reftable-readwrite: use free_names() instead of a for loop","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-07T14:11:58Z","receivedAt":"2024-08-07T14:16:58Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"free_names() as defined by reftable/basics.{c,h} frees a NULL\nterminated array of malloced strings along with the array itself.\nUse this function instead of a for loop to free such an array.\n\nMentored-by: Patrick Steinhardt <ps@pks.im>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n---\n t/unit-tests/t-reftable-readwrite.c | 9 ++-------\n 1 file changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\nindex 235e3d94c7..e90f2bf9de 100644\n--- a/t/unit-tests/t-reftable-readwrite.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -413,7 +413,6 @@ static void t_table_read_api(void)\n \tstruct reftable_reader rd = { 0 };\n \tstruct reftable_block_source source = { 0 };\n \tint err;\n-\tint i;\n \tstruct reftable_log_record log = { 0 };\n \tstruct reftable_iterator it = { 0 };\n \n@@ -432,10 +431,8 @@ static void t_table_read_api(void)\n \tcheck_int(err, ==, REFTABLE_API_ERROR);\n \n \tstrbuf_release(&buf);\n-\tfor (i = 0; i < N; i++)\n-\t\treftable_free(names[i]);\n+\tfree_names(names);\n \treftable_iterator_destroy(&it);\n-\treftable_free(names);\n \treader_close(&rd);\n \tstrbuf_release(&buf);\n }\n@@ -498,9 +495,7 @@ static void t_table_read_write_seek(int index, int hash_id)\n \treftable_iterator_destroy(&it);\n \n \tstrbuf_release(&buf);\n-\tfor (i = 0; i < N; i++)\n-\t\treftable_free(names[i]);\n-\treftable_free(names);\n+\tfree_names(names);\n \treader_close(&rd);\n }\n \n-- \n2.45.GIT\n\n"},{"id":"500326","messageId":"20240807141608.4524-4-chandrapratap3519@gmail.com","threadId":"61916","inReplyTo":"20240807141608.4524-1-chandrapratap3519@gmail.com","subject":"[PATCH 3/5] t-reftable-readwrite: use 'for' in place of infinite 'while' loops","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-07T14:11:59Z","receivedAt":"2024-08-07T14:17:01Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"Using a for loop with an empty conditional statement is more concise\nand easier to read than an infinite 'while' loop in instances\nwhere we need a loop variable. Hence, replace such instances of a\n'while' loop with the equivalent 'for' loop.\n\nMentored-by: Patrick Steinhardt <ps@pks.im>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n---\n t/unit-tests/t-reftable-readwrite.c | 12 +++---------\n 1 file changed, 3 insertions(+), 9 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\nindex e90f2bf9de..7daf28ec6d 100644\n--- a/t/unit-tests/t-reftable-readwrite.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -268,15 +268,13 @@ static void t_log_write_read(void)\n \terr = reftable_iterator_seek_log(&it, \"\");\n \tcheck(!err);\n \n-\ti = 0;\n-\twhile (1) {\n+\tfor (i = 0; ; i++) {\n \t\tint err = reftable_iterator_next_log(&it, &log);\n \t\tif (err > 0)\n \t\t\tbreak;\n \t\tcheck(!err);\n \t\tcheck_str(names[i], log.refname);\n \t\tcheck_int(i, ==, log.update_index);\n-\t\ti++;\n \t\treftable_log_record_release(&log);\n \t}\n \n@@ -374,7 +372,7 @@ static void t_table_read_write_sequential(void)\n \terr = reftable_iterator_seek_ref(&it, \"\");\n \tcheck(!err);\n \n-\twhile (1) {\n+\tfor (j = 0; ; j++) {\n \t\tstruct reftable_ref_record ref = { 0 };\n \t\tint r = reftable_iterator_next_ref(&it, &ref);\n \t\tcheck_int(r, >=, 0);\n@@ -382,8 +380,6 @@ static void t_table_read_write_sequential(void)\n \t\t\tbreak;\n \t\tcheck_str(names[j], ref.refname);\n \t\tcheck_int(update_index, ==, ref.update_index);\n-\n-\t\tj++;\n \t\treftable_ref_record_release(&ref);\n \t}\n \tcheck_int(j, ==, N);\n@@ -589,15 +585,13 @@ static void t_table_refs_for(int indexed)\n \terr = reftable_reader_refs_for(&rd, &it, want_hash);\n \tcheck(!err);\n \n-\tj = 0;\n-\twhile (1) {\n+\tfor (j = 0; ; j++) {\n \t\tint err = reftable_iterator_next_ref(&it, &ref);\n \t\tcheck_int(err, >=, 0);\n \t\tif (err > 0)\n \t\t\tbreak;\n \t\tcheck_int(j, <, want_names_len);\n \t\tcheck_str(ref.refname, want_names[j]);\n-\t\tj++;\n \t\treftable_ref_record_release(&ref);\n \t}\n \tcheck_int(j, ==, want_names_len);\n-- \n2.45.GIT\n\n"},{"id":"500327","messageId":"20240807141608.4524-5-chandrapratap3519@gmail.com","threadId":"61916","inReplyTo":"20240807141608.4524-1-chandrapratap3519@gmail.com","subject":"[PATCH 4/5] t-reftable-readwrite: add test for known error","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-07T14:12:00Z","receivedAt":"2024-08-07T14:17:04Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"When using reftable_writer_add_ref() to add a ref record to a\nreftable writer, The update_index of the ref record must be within\nthe limits set by reftable_writer_set_limits(), or REFTABLE_API_ERROR\nis returned. This scenario is currently left untested. Add a test\ncase for the same.\n\nMentored-by: Patrick Steinhardt <ps@pks.im>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n---\n t/unit-tests/t-reftable-readwrite.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\nindex 7daf28ec6d..a5462441d3 100644\n--- a/t/unit-tests/t-reftable-readwrite.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -773,6 +773,11 @@ static void t_write_key_order(void)\n \tcheck(!err);\n \terr = reftable_writer_add_ref(w, &refs[1]);\n \tcheck_int(err, ==, REFTABLE_API_ERROR);\n+\n+\trefs[0].update_index = 2;\n+\terr = reftable_writer_add_ref(w, &refs[0]);\n+\tcheck_int(err, ==, REFTABLE_API_ERROR);\n+\n \treftable_writer_close(w);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n-- \n2.45.GIT\n\n"},{"id":"500328","messageId":"20240807141608.4524-6-chandrapratap3519@gmail.com","threadId":"61916","inReplyTo":"20240807141608.4524-1-chandrapratap3519@gmail.com","subject":"[PATCH 5/5] t-reftable-readwrite: add tests for print functions","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-07T14:12:01Z","receivedAt":"2024-08-07T14:17:06Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"reftable/reftable-reader.h lists two print functions useful in\ndebugging, reftable_reader_print_file() and\nreftable_reader_print_blocks(). As of now, both these functions\nare left unexercised by all of the reftable tests. Add a test\nfunction to exercise both these functions. This has the added\nbenefit of testing reftable_block_source_from_file(), which\ncurrently remains untested as well.\n\nMentored-by: Patrick Steinhardt <ps@pks.im>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n---\n t/unit-tests/t-reftable-readwrite.c | 75 +++++++++++++++++++++++++++++\n 1 file changed, 75 insertions(+)\n\ndiff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\nindex a5462441d3..8c6f2f1f5d 100644\n--- a/t/unit-tests/t-reftable-readwrite.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -11,6 +11,8 @@ license that can be found in the LICENSE file or at\n #include \"reftable/blocksource.h\"\n #include \"reftable/reftable-error.h\"\n #include \"reftable/reftable-writer.h\"\n+#include \"tempfile.h\"\n+#include \"write-or-die.h\"\n \n static const int update_index = 5;\n \n@@ -25,11 +27,23 @@ static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)\n \treturn sz;\n }\n \n+static ssize_t fd_write(void *b, const void *data, size_t sz)\n+{\n+\tint *fdp = (int *)b;\n+\treturn write_in_full(*fdp, data, sz);\n+}\n+\n static int noop_flush(void *arg)\n {\n \treturn 0;\n }\n \n+static int fd_flush(void *arg)\n+{\n+\tint *fdp = (int *)arg;\n+\treturn fsync_component(FSYNC_COMPONENT_REFERENCE, *fdp);\n+}\n+\n static void t_buffer(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -944,6 +958,66 @@ static void t_corrupt_table(void)\n \tstrbuf_release(&buf);\n }\n \n+static void t_table_print(void)\n+{\n+\tchar name[100];\n+\tstruct reftable_write_options opts = {\n+\t\t.block_size = 512,\n+\t\t.hash_id = GIT_SHA1_FORMAT_ID,\n+\t};\n+\tstruct reftable_ref_record ref = { 0 };\n+\tstruct reftable_log_record log = { 0 };\n+\tstruct reftable_writer *w = NULL;\n+\tstruct tempfile *tmp = NULL;\n+\tsize_t i, N = 3;\n+\tint n, fd;\n+\n+\txsnprintf(name, sizeof(name), \"t-reftable-readwrite-%d-XXXXXX\", __LINE__);\n+\ttmp = mks_tempfile_t(name);\n+\tfd = get_tempfile_fd(tmp);\n+\tw = reftable_new_writer(&fd_write, &fd_flush, &fd, &opts);\n+\treftable_writer_set_limits(w, 0, update_index);\n+\n+\tfor (i = 0; i < N; i++) {\n+\t\txsnprintf(name, sizeof(name), \"refs/heads/branch%02\"PRIuMAX, (uintmax_t)i);\n+\t\tref.refname = name;\n+\t\tref.update_index = i;\n+\t\tref.value_type = REFTABLE_REF_VAL1;\n+\t\tset_test_hash(ref.value.val1, i);\n+\n+\t\tn = reftable_writer_add_ref(w, &ref);\n+\t\tcheck_int(n, ==, 0);\n+\t}\n+\n+\tfor (i = 0; i < N; i++) {\n+\t\txsnprintf(name, sizeof(name), \"refs/heads/branch%02\"PRIuMAX, (uintmax_t)i);\n+\t\tlog.refname = name;\n+\t\tlog.update_index = i;\n+\t\tlog.value_type = REFTABLE_LOG_UPDATE;\n+\t\tset_test_hash(log.value.update.new_hash, i);\n+\t\tlog.value.update.name = (char *) \"John Doe\";\n+\t\tlog.value.update.email = (char *) \"johndoe@anon.org\";\n+\t\tlog.value.update.time = 0x6673e5b9;\n+\t\tlog.value.update.message = (char *) \"message\";\n+\n+\t\tn = reftable_writer_add_log(w, &log);\n+\t\tcheck_int(n, ==, 0);\n+\t}\n+\n+\tn = reftable_writer_close(w);\n+\tcheck_int(n, ==, 0);\n+\n+\ttest_msg(\"testing printing functionality:\");\n+\tn = reftable_reader_print_file(tmp->filename.buf);\n+\tcheck_int(n, ==, 0);\n+\tn = reftable_reader_print_blocks(tmp->filename.buf);\n+\t/* end of blocks is denoted by a return value of 1 */\n+\tcheck_int(n, ==, 1);\n+\n+\tdelete_tempfile(&tmp);\n+\treftable_writer_free(w);\n+}\n+\n int cmd_main(int argc, const char *argv[])\n {\n \tTEST(t_buffer(), \"strbuf works as blocksource\");\n@@ -953,6 +1027,7 @@ int cmd_main(int argc, const char *argv[])\n \tTEST(t_log_overflow(), \"log overflow returns expected error\");\n \tTEST(t_log_write_read(), \"read-write on log records\");\n \tTEST(t_log_zlib_corruption(), \"reading corrupted log record returns expected error\");\n+\tTEST(t_table_print(), \"print tables and blocks\");\n \tTEST(t_table_read_api(), \"read on a table\");\n \tTEST(t_table_read_write_seek_index(), \"read-write on a table with index\");\n \tTEST(t_table_read_write_seek_linear(), \"read-write on a table without index (SHA1)\");\n-- \n2.45.GIT\n\n"},{"id":"500417","messageId":"ZrR91dR3G06L9dy7@tanuki","threadId":"61916","inReplyTo":"20240807141608.4524-6-chandrapratap3519@gmail.com","subject":"Re: [PATCH 5/5] t-reftable-readwrite: add tests for print functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-08T08:12:05Z","receivedAt":"2024-08-08T08:12:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Aug 07, 2024 at 07:42:01PM +0530, Chandra Pratap wrote:\n> +static void t_table_print(void)\n> +{\n> +\tchar name[100];\n> +\tstruct reftable_write_options opts = {\n> +\t\t.block_size = 512,\n> +\t\t.hash_id = GIT_SHA1_FORMAT_ID,\n> +\t};\n> +\tstruct reftable_ref_record ref = { 0 };\n> +\tstruct reftable_log_record log = { 0 };\n> +\tstruct reftable_writer *w = NULL;\n> +\tstruct tempfile *tmp = NULL;\n> +\tsize_t i, N = 3;\n> +\tint n, fd;\n> +\n> +\txsnprintf(name, sizeof(name), \"t-reftable-readwrite-%d-XXXXXX\", __LINE__);\n\nIs it really required to include the line number in this file? This\nfeels unnecessarily defensive to me as `mks_tempfile_t()` should already\nmake sure that we get a unique filename. So if we drop that, we could\nskip this call to `xsnprintf()`.\n\n> +\ttmp = mks_tempfile_t(name);\n> +\tfd = get_tempfile_fd(tmp);\n> +\tw = reftable_new_writer(&fd_write, &fd_flush, &fd, &opts);\n> +\treftable_writer_set_limits(w, 0, update_index);\n> +\n> +\tfor (i = 0; i < N; i++) {\n> +\t\txsnprintf(name, sizeof(name), \"refs/heads/branch%02\"PRIuMAX, (uintmax_t)i);\n> +\t\tref.refname = name;\n> +\t\tref.update_index = i;\n> +\t\tref.value_type = REFTABLE_REF_VAL1;\n> +\t\tset_test_hash(ref.value.val1, i);\n> +\n> +\t\tn = reftable_writer_add_ref(w, &ref);\n> +\t\tcheck_int(n, ==, 0);\n> +\t}\n> +\n> +\tfor (i = 0; i < N; i++) {\n> +\t\txsnprintf(name, sizeof(name), \"refs/heads/branch%02\"PRIuMAX, (uintmax_t)i);\n> +\t\tlog.refname = name;\n> +\t\tlog.update_index = i;\n> +\t\tlog.value_type = REFTABLE_LOG_UPDATE;\n> +\t\tset_test_hash(log.value.update.new_hash, i);\n> +\t\tlog.value.update.name = (char *) \"John Doe\";\n> +\t\tlog.value.update.email = (char *) \"johndoe@anon.org\";\n> +\t\tlog.value.update.time = 0x6673e5b9;\n> +\t\tlog.value.update.message = (char *) \"message\";\n> +\n> +\t\tn = reftable_writer_add_log(w, &log);\n> +\t\tcheck_int(n, ==, 0);\n> +\t}\n> +\n> +\tn = reftable_writer_close(w);\n> +\tcheck_int(n, ==, 0);\n> +\n> +\ttest_msg(\"testing printing functionality:\");\n\nIs it intentionally that this line still exists? If so, I think it\nreally only causes unnecessary noise and should rather be dropped.\n\n> +\tn = reftable_reader_print_file(tmp->filename.buf);\n> +\tcheck_int(n, ==, 0);\n\nWait, doesn't this print to stdout? I don't think it is a good idea to\nexercise the function as-is. For one, it would pollute stdout with data\nthat we shouldn't care about. Second, it doesn't verify that the result\nis actually what we expect.\n\nI can see two options:\n\n  1. Refactor these interfaces such that they take a file descriptor as\n     input that they are writing to. This would allow us to exercise\n     that the output is correct.\n\n  2. Rip out this function. I don't think this functionality should be\n     part of the library in the first place, and it really only exists\n     because of \"reftable/dump.c\".\n\nI think the latter is the better option. The functionality exists to\ndrive `cmd__dump_reftable()` in our reftable test helper. We should\nlikely make the whole implementation of this an internal implementation\ndetail and not expose it.\n\nPatrick\n"},{"id":"500435","messageId":"ZrSzbdNkCS2LOXaL@tanuki","threadId":"61916","inReplyTo":"ZrR91dR3G06L9dy7@tanuki","subject":"Re: [PATCH 5/5] t-reftable-readwrite: add tests for print functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-08T12:00:45Z","receivedAt":"2024-08-08T12:00:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Aug 08, 2024 at 10:12:07AM +0200, Patrick Steinhardt wrote:\n> On Wed, Aug 07, 2024 at 07:42:01PM +0530, Chandra Pratap wrote:\n> > +static void t_table_print(void)\n> > +{\n> > +\tchar name[100];\n> > +\tstruct reftable_write_options opts = {\n> > +\t\t.block_size = 512,\n> > +\t\t.hash_id = GIT_SHA1_FORMAT_ID,\n> > +\t};\n> > +\tstruct reftable_ref_record ref = { 0 };\n> > +\tstruct reftable_log_record log = { 0 };\n> > +\tstruct reftable_writer *w = NULL;\n> > +\tstruct tempfile *tmp = NULL;\n> > +\tsize_t i, N = 3;\n> > +\tint n, fd;\n> > +\n> > +\txsnprintf(name, sizeof(name), \"t-reftable-readwrite-%d-XXXXXX\", __LINE__);\n> \n> Is it really required to include the line number in this file? This\n> feels unnecessarily defensive to me as `mks_tempfile_t()` should already\n> make sure that we get a unique filename. So if we drop that, we could\n> skip this call to `xsnprintf()`.\n> \n> > +\ttmp = mks_tempfile_t(name);\n> > +\tfd = get_tempfile_fd(tmp);\n> > +\tw = reftable_new_writer(&fd_write, &fd_flush, &fd, &opts);\n> > +\treftable_writer_set_limits(w, 0, update_index);\n> > +\n> > +\tfor (i = 0; i < N; i++) {\n> > +\t\txsnprintf(name, sizeof(name), \"refs/heads/branch%02\"PRIuMAX, (uintmax_t)i);\n> > +\t\tref.refname = name;\n> > +\t\tref.update_index = i;\n> > +\t\tref.value_type = REFTABLE_REF_VAL1;\n> > +\t\tset_test_hash(ref.value.val1, i);\n> > +\n> > +\t\tn = reftable_writer_add_ref(w, &ref);\n> > +\t\tcheck_int(n, ==, 0);\n> > +\t}\n> > +\n> > +\tfor (i = 0; i < N; i++) {\n> > +\t\txsnprintf(name, sizeof(name), \"refs/heads/branch%02\"PRIuMAX, (uintmax_t)i);\n> > +\t\tlog.refname = name;\n> > +\t\tlog.update_index = i;\n> > +\t\tlog.value_type = REFTABLE_LOG_UPDATE;\n> > +\t\tset_test_hash(log.value.update.new_hash, i);\n> > +\t\tlog.value.update.name = (char *) \"John Doe\";\n> > +\t\tlog.value.update.email = (char *) \"johndoe@anon.org\";\n> > +\t\tlog.value.update.time = 0x6673e5b9;\n> > +\t\tlog.value.update.message = (char *) \"message\";\n> > +\n> > +\t\tn = reftable_writer_add_log(w, &log);\n> > +\t\tcheck_int(n, ==, 0);\n> > +\t}\n> > +\n> > +\tn = reftable_writer_close(w);\n> > +\tcheck_int(n, ==, 0);\n> > +\n> > +\ttest_msg(\"testing printing functionality:\");\n> \n> Is it intentionally that this line still exists? If so, I think it\n> really only causes unnecessary noise and should rather be dropped.\n> \n> > +\tn = reftable_reader_print_file(tmp->filename.buf);\n> > +\tcheck_int(n, ==, 0);\n> \n> Wait, doesn't this print to stdout? I don't think it is a good idea to\n> exercise the function as-is. For one, it would pollute stdout with data\n> that we shouldn't care about. Second, it doesn't verify that the result\n> is actually what we expect.\n> \n> I can see two options:\n> \n>   1. Refactor these interfaces such that they take a file descriptor as\n>      input that they are writing to. This would allow us to exercise\n>      that the output is correct.\n> \n>   2. Rip out this function. I don't think this functionality should be\n>      part of the library in the first place, and it really only exists\n>      because of \"reftable/dump.c\".\n> \n> I think the latter is the better option. The functionality exists to\n> drive `cmd__dump_reftable()` in our reftable test helper. We should\n> likely make the whole implementation of this an internal implementation\n> detail and not expose it.\n\nFor the record: I've got a bigger patch series in development that drops\nthe generic reftable interfaces. As part of this, I'll also rip out the\nfunctionality provided by \"reftabel/dump.c\".\n\nPatrick\n"},{"id":"500477","messageId":"CA+J6zkRszqfPP=_gbzzyDCOoUsLQUd0ke-XbMbFYu30Vhtocng@mail.gmail.com","threadId":"61916","inReplyTo":"ZrSzbdNkCS2LOXaL@tanuki","subject":"Re: [PATCH 5/5] t-reftable-readwrite: add tests for print functions","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-08T14:25:35Z","receivedAt":"2024-08-08T14:26:03Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"On Thu, 8 Aug 2024 at 17:36, Patrick Steinhardt <ps@pks.im> wrote:\n>\n> On Thu, Aug 08, 2024 at 10:12:07AM +0200, Patrick Steinhardt wrote:\n> > On Wed, Aug 07, 2024 at 07:42:01PM +0530, Chandra Pratap wrote:\n> > > +static void t_table_print(void)\n> > > +{\n> > > +   char name[100];\n> > > +   struct reftable_write_options opts = {\n> > > +           .block_size = 512,\n> > > +           .hash_id = GIT_SHA1_FORMAT_ID,\n> > > +   };\n> > > +   struct reftable_ref_record ref = { 0 };\n> > > +   struct reftable_log_record log = { 0 };\n> > > +   struct reftable_writer *w = NULL;\n> > > +   struct tempfile *tmp = NULL;\n> > > +   size_t i, N = 3;\n> > > +   int n, fd;\n> > > +\n> > > +   xsnprintf(name, sizeof(name), \"t-reftable-readwrite-%d-XXXXXX\", __LINE__);\n> >\n> > Is it really required to include the line number in this file? This\n> > feels unnecessarily defensive to me as `mks_tempfile_t()` should already\n> > make sure that we get a unique filename. So if we drop that, we could\n> > skip this call to `xsnprintf()`.\n> >\n> > > +   tmp = mks_tempfile_t(name);\n> > > +   fd = get_tempfile_fd(tmp);\n> > > +   w = reftable_new_writer(&fd_write, &fd_flush, &fd, &opts);\n> > > +   reftable_writer_set_limits(w, 0, update_index);\n> > > +\n> > > +   for (i = 0; i < N; i++) {\n> > > +           xsnprintf(name, sizeof(name), \"refs/heads/branch%02\"PRIuMAX, (uintmax_t)i);\n> > > +           ref.refname = name;\n> > > +           ref.update_index = i;\n> > > +           ref.value_type = REFTABLE_REF_VAL1;\n> > > +           set_test_hash(ref.value.val1, i);\n> > > +\n> > > +           n = reftable_writer_add_ref(w, &ref);\n> > > +           check_int(n, ==, 0);\n> > > +   }\n> > > +\n> > > +   for (i = 0; i < N; i++) {\n> > > +           xsnprintf(name, sizeof(name), \"refs/heads/branch%02\"PRIuMAX, (uintmax_t)i);\n> > > +           log.refname = name;\n> > > +           log.update_index = i;\n> > > +           log.value_type = REFTABLE_LOG_UPDATE;\n> > > +           set_test_hash(log.value.update.new_hash, i);\n> > > +           log.value.update.name = (char *) \"John Doe\";\n> > > +           log.value.update.email = (char *) \"johndoe@anon.org\";\n> > > +           log.value.update.time = 0x6673e5b9;\n> > > +           log.value.update.message = (char *) \"message\";\n> > > +\n> > > +           n = reftable_writer_add_log(w, &log);\n> > > +           check_int(n, ==, 0);\n> > > +   }\n> > > +\n> > > +   n = reftable_writer_close(w);\n> > > +   check_int(n, ==, 0);\n> > > +\n> > > +   test_msg(\"testing printing functionality:\");\n> >\n> > Is it intentionally that this line still exists? If so, I think it\n> > really only causes unnecessary noise and should rather be dropped.\n> >\n> > > +   n = reftable_reader_print_file(tmp->filename.buf);\n> > > +   check_int(n, ==, 0);\n> >\n> > Wait, doesn't this print to stdout? I don't think it is a good idea to\n> > exercise the function as-is. For one, it would pollute stdout with data\n> > that we shouldn't care about. Second, it doesn't verify that the result\n> > is actually what we expect.\n> >\n> > I can see two options:\n> >\n> >   1. Refactor these interfaces such that they take a file descriptor as\n> >      input that they are writing to. This would allow us to exercise\n> >      that the output is correct.\n> >\n> >   2. Rip out this function. I don't think this functionality should be\n> >      part of the library in the first place, and it really only exists\n> >      because of \"reftable/dump.c\".\n> >\n> > I think the latter is the better option. The functionality exists to\n> > drive `cmd__dump_reftable()` in our reftable test helper. We should\n> > likely make the whole implementation of this an internal implementation\n> > detail and not expose it.\n>\n> For the record: I've got a bigger patch series in development that drops\n> the generic reftable interfaces. As part of this, I'll also rip out the\n> functionality provided by \"reftabel/dump.c\".\n\nCool, I'll just drop this patch from the series then.\n"},{"id":"500534","messageId":"20240809111312.4401-1-chandrapratap3519@gmail.com","threadId":"61916","inReplyTo":"20240807141608.4524-1-chandrapratap3519@gmail.com","subject":"[GSoC][PATCH v2 0/4] t: port reftable/readwrite_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-09T11:05:40Z","receivedAt":"2024-08-09T11:13:40Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"The reftable library comes with self tests, which are exercised\nas part of the usual end-to-end tests and are designed to\nobserve the end-user visible effects of Git commands. What it\nexercises, however, is a better match for the unit-testing\nframework, merged at 8bf6fbd0 (Merge branch 'js/doc-unit-tests',\n2023-12-09), which is designed to observe how low level\nimplementation details, at the level of sequences of individual\nfunction calls, behave.\n\nHence, port reftable/readwrite_test.c to the unit testing framework\nand improve upon the ported test. The first patch in the series moves\nthe test to the unit testing framework, and the rest of the patches\nimprove upon the ported test.\n\nMentored-by: Patrick Steinhardt <ps@pks.im>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n\n---\nChanges in v2:\n- Drop the fifth patch (t-reftable-readwrite: add tests for print\n  functions) of the previous series\n\nCI/PR: https://github.com/gitgitgadget/git/pull/1770\n\nChandra Pratap(4):\nt: move reftable/readwrite_test.c to the unit testing framework\nt-reftable-readwrite: use free_names() instead of a for loop\nt-reftable-readwrite: use 'for' in place of infinite 'while' loops\nt-reftable-readwrite: add test for known error\n\nMakefile                                                         |   2 +-\nreftable/reftable-tests.h                                        |   1 -\nt/helper/test-reftable.c                                         |   1 -\nreftable/readwrite_test.c => t/unit-tests/t-reftable-readwrite.c | 440 +++++++++++++++++++++++-----------------\n4 files changed, 218 insertions(+), 226 deletions(-)\n\nRange-diff against v1:\n\n1:  e4b01cd856 < -:  ---------- t-reftable-readwrite: add tests for print functions\n"},{"id":"500535","messageId":"20240809111312.4401-2-chandrapratap3519@gmail.com","threadId":"61916","inReplyTo":"20240809111312.4401-1-chandrapratap3519@gmail.com","subject":"[PATCH v2 1/4] t: move reftable/readwrite_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-09T11:05:41Z","receivedAt":"2024-08-09T11:13:44Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"reftable/readwrite_test.c exercises the functions defined in\nreftable/reader.{c,h} and reftable/writer.{c,h}. Migrate\nreftable/readwrite_test.c to the unit testing framework. Migration\ninvolves refactoring the tests to use the unit testing framework\ninstead of reftable's test framework and renaming the tests to\nalign with unit-tests' naming conventions.\n\nSince some tests in reftable/readwrite_test.c use the functions\nset_test_hash(), noop_flush() and strbuf_add_void() defined in\nreftable/test_framework.{c,h} but these files are not #included\nin the ported unit test, copy these functions in the new test file.\n\nWhile at it, ensure structs are 0-initialized with '= { 0 }'\ninstead of '= { NULL }'.\n\nMentored-by: Patrick Steinhardt <ps@pks.im>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n---\n Makefile                                      |   2 +-\n reftable/reftable-tests.h                     |   1 -\n t/helper/test-reftable.c                      |   1 -\n .../unit-tests/t-reftable-readwrite.c         | 418 +++++++++---------\n 4 files changed, 210 insertions(+), 212 deletions(-)\n rename reftable/readwrite_test.c => t/unit-tests/t-reftable-readwrite.c (70%)\n\ndiff --git a/Makefile b/Makefile\nindex 3863e60b66..76e4d1c1ec 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1341,6 +1341,7 @@ UNIT_TEST_PROGRAMS += t-oidtree\n UNIT_TEST_PROGRAMS += t-prio-queue\n UNIT_TEST_PROGRAMS += t-reftable-basics\n UNIT_TEST_PROGRAMS += t-reftable-merged\n+UNIT_TEST_PROGRAMS += t-reftable-readwrite\n UNIT_TEST_PROGRAMS += t-reftable-record\n UNIT_TEST_PROGRAMS += t-strbuf\n UNIT_TEST_PROGRAMS += t-strcmp-offset\n@@ -2682,7 +2683,6 @@ REFTABLE_OBJS += reftable/writer.o\n REFTABLE_TEST_OBJS += reftable/block_test.o\n REFTABLE_TEST_OBJS += reftable/dump.o\n REFTABLE_TEST_OBJS += reftable/pq_test.o\n-REFTABLE_TEST_OBJS += reftable/readwrite_test.o\n REFTABLE_TEST_OBJS += reftable/stack_test.o\n REFTABLE_TEST_OBJS += reftable/test_framework.o\n REFTABLE_TEST_OBJS += reftable/tree_test.o\ndiff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h\nindex d5e03dcc1b..7d955393d2 100644\n--- a/reftable/reftable-tests.h\n+++ b/reftable/reftable-tests.h\n@@ -13,7 +13,6 @@ int basics_test_main(int argc, const char **argv);\n int block_test_main(int argc, const char **argv);\n int pq_test_main(int argc, const char **argv);\n int record_test_main(int argc, const char **argv);\n-int readwrite_test_main(int argc, const char **argv);\n int stack_test_main(int argc, const char **argv);\n int tree_test_main(int argc, const char **argv);\n int reftable_dump_main(int argc, char *const *argv);\ndiff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c\nindex 9d378427da..d371e9f9dd 100644\n--- a/t/helper/test-reftable.c\n+++ b/t/helper/test-reftable.c\n@@ -8,7 +8,6 @@ int cmd__reftable(int argc, const char **argv)\n \tblock_test_main(argc, argv);\n \ttree_test_main(argc, argv);\n \tpq_test_main(argc, argv);\n-\treadwrite_test_main(argc, argv);\n \tstack_test_main(argc, argv);\n \treturn 0;\n }\ndiff --git a/reftable/readwrite_test.c b/t/unit-tests/t-reftable-readwrite.c\nsimilarity index 70%\nrename from reftable/readwrite_test.c\nrename to t/unit-tests/t-reftable-readwrite.c\nindex f411abfe9c..235e3d94c7 100644\n--- a/reftable/readwrite_test.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -6,37 +6,48 @@ license that can be found in the LICENSE file or at\n https://developers.google.com/open-source/licenses/bsd\n */\n \n-#include \"system.h\"\n-\n-#include \"basics.h\"\n-#include \"block.h\"\n-#include \"blocksource.h\"\n-#include \"reader.h\"\n-#include \"record.h\"\n-#include \"test_framework.h\"\n-#include \"reftable-tests.h\"\n-#include \"reftable-writer.h\"\n+#include \"test-lib.h\"\n+#include \"reftable/reader.h\"\n+#include \"reftable/blocksource.h\"\n+#include \"reftable/reftable-error.h\"\n+#include \"reftable/reftable-writer.h\"\n \n static const int update_index = 5;\n \n-static void test_buffer(void)\n+static void set_test_hash(uint8_t *p, int i)\n+{\n+\tmemset(p, (uint8_t)i, hash_size(GIT_SHA1_FORMAT_ID));\n+}\n+\n+static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)\n+{\n+\tstrbuf_add(b, data, sz);\n+\treturn sz;\n+}\n+\n+static int noop_flush(void *arg)\n+{\n+\treturn 0;\n+}\n+\n+static void t_buffer(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_block out = { NULL };\n+\tstruct reftable_block_source source = { 0 };\n+\tstruct reftable_block out = { 0 };\n \tint n;\n \tuint8_t in[] = \"hello\";\n \tstrbuf_add(&buf, in, sizeof(in));\n \tblock_source_from_strbuf(&source, &buf);\n-\tEXPECT(block_source_size(&source) == 6);\n+\tcheck_int(block_source_size(&source), ==, 6);\n \tn = block_source_read_block(&source, &out, 0, sizeof(in));\n-\tEXPECT(n == sizeof(in));\n-\tEXPECT(!memcmp(in, out.data, n));\n+\tcheck_int(n, ==, sizeof(in));\n+\tcheck(!memcmp(in, out.data, n));\n \treftable_block_done(&out);\n \n \tn = block_source_read_block(&source, &out, 1, 2);\n-\tEXPECT(n == 2);\n-\tEXPECT(!memcmp(out.data, \"el\", 2));\n+\tcheck_int(n, ==, 2);\n+\tcheck(!memcmp(out.data, \"el\", 2));\n \n \treftable_block_done(&out);\n \tblock_source_close(&source);\n@@ -52,9 +63,9 @@ static void write_table(char ***names, struct strbuf *buf, int N,\n \t};\n \tstruct reftable_writer *w =\n \t\treftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);\n-\tstruct reftable_ref_record ref = { NULL };\n+\tstruct reftable_ref_record ref = { 0 };\n \tint i = 0, n;\n-\tstruct reftable_log_record log = { NULL };\n+\tstruct reftable_log_record log = { 0 };\n \tconst struct reftable_stats *stats = NULL;\n \n \tREFTABLE_CALLOC_ARRAY(*names, N + 1);\n@@ -73,7 +84,7 @@ static void write_table(char ***names, struct strbuf *buf, int N,\n \t\t(*names)[i] = xstrdup(name);\n \n \t\tn = reftable_writer_add_ref(w, &ref);\n-\t\tEXPECT(n == 0);\n+\t\tcheck_int(n, ==, 0);\n \t}\n \n \tfor (i = 0; i < N; i++) {\n@@ -89,27 +100,25 @@ static void write_table(char ***names, struct strbuf *buf, int N,\n \t\tlog.value.update.message = (char *) \"message\";\n \n \t\tn = reftable_writer_add_log(w, &log);\n-\t\tEXPECT(n == 0);\n+\t\tcheck_int(n, ==, 0);\n \t}\n \n \tn = reftable_writer_close(w);\n-\tEXPECT(n == 0);\n+\tcheck_int(n, ==, 0);\n \n \tstats = reftable_writer_stats(w);\n \tfor (i = 0; i < stats->ref_stats.blocks; i++) {\n \t\tint off = i * opts.block_size;\n-\t\tif (off == 0) {\n-\t\t\toff = header_size(\n-\t\t\t\t(hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);\n-\t\t}\n-\t\tEXPECT(buf->buf[off] == 'r');\n+\t\tif (!off)\n+\t\t\toff = header_size((hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);\n+\t\tcheck(buf->buf[off] == 'r');\n \t}\n \n-\tEXPECT(stats->log_stats.blocks > 0);\n+\tcheck(stats->log_stats.blocks > 0);\n \treftable_writer_free(w);\n }\n \n-static void test_log_buffer_size(void)\n+static void t_log_buffer_size(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_write_options opts = {\n@@ -140,14 +149,14 @@ static void test_log_buffer_size(void)\n \t}\n \treftable_writer_set_limits(w, update_index, update_index);\n \terr = reftable_writer_add_log(w, &log);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \terr = reftable_writer_close(w);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n }\n \n-static void test_log_overflow(void)\n+static void t_log_overflow(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tchar msg[256] = { 0 };\n@@ -177,12 +186,12 @@ static void test_log_overflow(void)\n \tmemset(msg, 'x', sizeof(msg) - 1);\n \treftable_writer_set_limits(w, update_index, update_index);\n \terr = reftable_writer_add_log(w, &log);\n-\tEXPECT(err == REFTABLE_ENTRY_TOO_BIG_ERROR);\n+\tcheck_int(err, ==, REFTABLE_ENTRY_TOO_BIG_ERROR);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n }\n \n-static void test_log_write_read(void)\n+static void t_log_write_read(void)\n {\n \tint N = 2;\n \tchar **names = reftable_calloc(N + 1, sizeof(*names));\n@@ -190,13 +199,13 @@ static void test_log_write_read(void)\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 256,\n \t};\n-\tstruct reftable_ref_record ref = { NULL };\n+\tstruct reftable_ref_record ref = { 0 };\n \tint i = 0;\n-\tstruct reftable_log_record log = { NULL };\n+\tstruct reftable_log_record log = { 0 };\n \tint n;\n-\tstruct reftable_iterator it = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n-\tstruct reftable_block_source source = { NULL };\n+\tstruct reftable_iterator it = { 0 };\n+\tstruct reftable_reader rd = { 0 };\n+\tstruct reftable_block_source source = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n \t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n@@ -204,17 +213,17 @@ static void test_log_write_read(void)\n \treftable_writer_set_limits(w, 0, N);\n \tfor (i = 0; i < N; i++) {\n \t\tchar name[256];\n-\t\tstruct reftable_ref_record ref = { NULL };\n+\t\tstruct reftable_ref_record ref = { 0 };\n \t\tsnprintf(name, sizeof(name), \"b%02d%0*d\", i, 130, 7);\n \t\tnames[i] = xstrdup(name);\n \t\tref.refname = name;\n \t\tref.update_index = i;\n \n \t\terr = reftable_writer_add_ref(w, &ref);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \tfor (i = 0; i < N; i++) {\n-\t\tstruct reftable_log_record log = { NULL };\n+\t\tstruct reftable_log_record log = { 0 };\n \n \t\tlog.refname = names[i];\n \t\tlog.update_index = i;\n@@ -223,33 +232,33 @@ static void test_log_write_read(void)\n \t\tset_test_hash(log.value.update.new_hash, i + 1);\n \n \t\terr = reftable_writer_add_log(w, &log);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \n \tn = reftable_writer_close(w);\n-\tEXPECT(n == 0);\n+\tcheck_int(n, ==, 0);\n \n \tstats = reftable_writer_stats(w);\n-\tEXPECT(stats->log_stats.blocks > 0);\n+\tcheck_int(stats->log_stats.blocks, >, 0);\n \treftable_writer_free(w);\n \tw = NULL;\n \n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = init_reader(&rd, &source, \"file.log\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_reader_init_ref_iterator(&rd, &it);\n \n \terr = reftable_iterator_seek_ref(&it, names[N - 1]);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \terr = reftable_iterator_next_ref(&it, &ref);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \t/* end of iteration. */\n \terr = reftable_iterator_next_ref(&it, &ref);\n-\tEXPECT(0 < err);\n+\tcheck_int(err, >, 0);\n \n \treftable_iterator_destroy(&it);\n \treftable_ref_record_release(&ref);\n@@ -257,23 +266,21 @@ static void test_log_write_read(void)\n \treftable_reader_init_log_iterator(&rd, &it);\n \n \terr = reftable_iterator_seek_log(&it, \"\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \ti = 0;\n \twhile (1) {\n \t\tint err = reftable_iterator_next_log(&it, &log);\n-\t\tif (err > 0) {\n+\t\tif (err > 0)\n \t\t\tbreak;\n-\t\t}\n-\n-\t\tEXPECT_ERR(err);\n-\t\tEXPECT_STREQ(names[i], log.refname);\n-\t\tEXPECT(i == log.update_index);\n+\t\tcheck(!err);\n+\t\tcheck_str(names[i], log.refname);\n+\t\tcheck_int(i, ==, log.update_index);\n \t\ti++;\n \t\treftable_log_record_release(&log);\n \t}\n \n-\tEXPECT(i == N);\n+\tcheck_int(i, ==, N);\n \treftable_iterator_destroy(&it);\n \n \t/* cleanup. */\n@@ -282,7 +289,7 @@ static void test_log_write_read(void)\n \treader_close(&rd);\n }\n \n-static void test_log_zlib_corruption(void)\n+static void t_log_zlib_corruption(void)\n {\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 256,\n@@ -316,13 +323,13 @@ static void test_log_zlib_corruption(void)\n \treftable_writer_set_limits(w, 1, 1);\n \n \terr = reftable_writer_add_log(w, &log);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \tn = reftable_writer_close(w);\n-\tEXPECT(n == 0);\n+\tcheck_int(n, ==, 0);\n \n \tstats = reftable_writer_stats(w);\n-\tEXPECT(stats->log_stats.blocks > 0);\n+\tcheck_int(stats->log_stats.blocks, >, 0);\n \treftable_writer_free(w);\n \tw = NULL;\n \n@@ -332,11 +339,11 @@ static void test_log_zlib_corruption(void)\n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = init_reader(&rd, &source, \"file.log\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_reader_init_log_iterator(&rd, &it);\n \terr = reftable_iterator_seek_log(&it, \"refname\");\n-\tEXPECT(err == REFTABLE_ZLIB_ERROR);\n+\tcheck_int(err, ==, REFTABLE_ZLIB_ERROR);\n \n \treftable_iterator_destroy(&it);\n \n@@ -345,14 +352,14 @@ static void test_log_zlib_corruption(void)\n \treader_close(&rd);\n }\n \n-static void test_table_read_write_sequential(void)\n+static void t_table_read_write_sequential(void)\n {\n \tchar **names;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint N = 50;\n-\tstruct reftable_iterator it = { NULL };\n-\tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_iterator it = { 0 };\n+\tstruct reftable_block_source source = { 0 };\n+\tstruct reftable_reader rd = { 0 };\n \tint err = 0;\n \tint j = 0;\n \n@@ -361,26 +368,25 @@ static void test_table_read_write_sequential(void)\n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = init_reader(&rd, &source, \"file.ref\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_reader_init_ref_iterator(&rd, &it);\n \terr = reftable_iterator_seek_ref(&it, \"\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \twhile (1) {\n-\t\tstruct reftable_ref_record ref = { NULL };\n+\t\tstruct reftable_ref_record ref = { 0 };\n \t\tint r = reftable_iterator_next_ref(&it, &ref);\n-\t\tEXPECT(r >= 0);\n-\t\tif (r > 0) {\n+\t\tcheck_int(r, >=, 0);\n+\t\tif (r > 0)\n \t\t\tbreak;\n-\t\t}\n-\t\tEXPECT(0 == strcmp(names[j], ref.refname));\n-\t\tEXPECT(update_index == ref.update_index);\n+\t\tcheck_str(names[j], ref.refname);\n+\t\tcheck_int(update_index, ==, ref.update_index);\n \n \t\tj++;\n \t\treftable_ref_record_release(&ref);\n \t}\n-\tEXPECT(j == N);\n+\tcheck_int(j, ==, N);\n \treftable_iterator_destroy(&it);\n \tstrbuf_release(&buf);\n \tfree_names(names);\n@@ -388,90 +394,88 @@ static void test_table_read_write_sequential(void)\n \treader_close(&rd);\n }\n \n-static void test_table_write_small_table(void)\n+static void t_table_write_small_table(void)\n {\n \tchar **names;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint N = 1;\n \twrite_table(&names, &buf, N, 4096, GIT_SHA1_FORMAT_ID);\n-\tEXPECT(buf.len < 200);\n+\tcheck_int(buf.len, <, 200);\n \tstrbuf_release(&buf);\n \tfree_names(names);\n }\n \n-static void test_table_read_api(void)\n+static void t_table_read_api(void)\n {\n \tchar **names;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint N = 50;\n-\tstruct reftable_reader rd = { NULL };\n-\tstruct reftable_block_source source = { NULL };\n+\tstruct reftable_reader rd = { 0 };\n+\tstruct reftable_block_source source = { 0 };\n \tint err;\n \tint i;\n-\tstruct reftable_log_record log = { NULL };\n-\tstruct reftable_iterator it = { NULL };\n+\tstruct reftable_log_record log = { 0 };\n+\tstruct reftable_iterator it = { 0 };\n \n \twrite_table(&names, &buf, N, 256, GIT_SHA1_FORMAT_ID);\n \n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = init_reader(&rd, &source, \"file.ref\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_reader_init_ref_iterator(&rd, &it);\n \terr = reftable_iterator_seek_ref(&it, names[0]);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \terr = reftable_iterator_next_log(&it, &log);\n-\tEXPECT(err == REFTABLE_API_ERROR);\n+\tcheck_int(err, ==, REFTABLE_API_ERROR);\n \n \tstrbuf_release(&buf);\n-\tfor (i = 0; i < N; i++) {\n+\tfor (i = 0; i < N; i++)\n \t\treftable_free(names[i]);\n-\t}\n \treftable_iterator_destroy(&it);\n \treftable_free(names);\n \treader_close(&rd);\n \tstrbuf_release(&buf);\n }\n \n-static void test_table_read_write_seek(int index, int hash_id)\n+static void t_table_read_write_seek(int index, int hash_id)\n {\n \tchar **names;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint N = 50;\n-\tstruct reftable_reader rd = { NULL };\n-\tstruct reftable_block_source source = { NULL };\n+\tstruct reftable_reader rd = { 0 };\n+\tstruct reftable_block_source source = { 0 };\n \tint err;\n \tint i = 0;\n \n-\tstruct reftable_iterator it = { NULL };\n+\tstruct reftable_iterator it = { 0 };\n \tstruct strbuf pastLast = STRBUF_INIT;\n-\tstruct reftable_ref_record ref = { NULL };\n+\tstruct reftable_ref_record ref = { 0 };\n \n \twrite_table(&names, &buf, N, 256, hash_id);\n \n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = init_reader(&rd, &source, \"file.ref\");\n-\tEXPECT_ERR(err);\n-\tEXPECT(hash_id == reftable_reader_hash_id(&rd));\n+\tcheck(!err);\n+\tcheck_int(hash_id, ==, reftable_reader_hash_id(&rd));\n \n-\tif (!index) {\n+\tif (!index)\n \t\trd.ref_offsets.index_offset = 0;\n-\t} else {\n-\t\tEXPECT(rd.ref_offsets.index_offset > 0);\n-\t}\n+\telse\n+\t\tcheck_int(rd.ref_offsets.index_offset, >, 0);\n \n \tfor (i = 1; i < N; i++) {\n \t\treftable_reader_init_ref_iterator(&rd, &it);\n \t\terr = reftable_iterator_seek_ref(&it, names[i]);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t\terr = reftable_iterator_next_ref(&it, &ref);\n-\t\tEXPECT_ERR(err);\n-\t\tEXPECT(0 == strcmp(names[i], ref.refname));\n-\t\tEXPECT(REFTABLE_REF_VAL1 == ref.value_type);\n-\t\tEXPECT(i == ref.value.val1[0]);\n+\t\tcheck(!err);\n+\t\tcheck_str(names[i], ref.refname);\n+\t\tcheck_int(REFTABLE_REF_VAL1, ==, ref.value_type);\n+\t\tcheck_int(i, ==, ref.value.val1[0]);\n \n \t\treftable_ref_record_release(&ref);\n \t\treftable_iterator_destroy(&it);\n@@ -483,40 +487,39 @@ static void test_table_read_write_seek(int index, int hash_id)\n \treftable_reader_init_ref_iterator(&rd, &it);\n \terr = reftable_iterator_seek_ref(&it, pastLast.buf);\n \tif (err == 0) {\n-\t\tstruct reftable_ref_record ref = { NULL };\n+\t\tstruct reftable_ref_record ref = { 0 };\n \t\tint err = reftable_iterator_next_ref(&it, &ref);\n-\t\tEXPECT(err > 0);\n+\t\tcheck_int(err, >, 0);\n \t} else {\n-\t\tEXPECT(err > 0);\n+\t\tcheck_int(err, >, 0);\n \t}\n \n \tstrbuf_release(&pastLast);\n \treftable_iterator_destroy(&it);\n \n \tstrbuf_release(&buf);\n-\tfor (i = 0; i < N; i++) {\n+\tfor (i = 0; i < N; i++)\n \t\treftable_free(names[i]);\n-\t}\n \treftable_free(names);\n \treader_close(&rd);\n }\n \n-static void test_table_read_write_seek_linear(void)\n+static void t_table_read_write_seek_linear(void)\n {\n-\ttest_table_read_write_seek(0, GIT_SHA1_FORMAT_ID);\n+\tt_table_read_write_seek(0, GIT_SHA1_FORMAT_ID);\n }\n \n-static void test_table_read_write_seek_linear_sha256(void)\n+static void t_table_read_write_seek_linear_sha256(void)\n {\n-\ttest_table_read_write_seek(0, GIT_SHA256_FORMAT_ID);\n+\tt_table_read_write_seek(0, GIT_SHA256_FORMAT_ID);\n }\n \n-static void test_table_read_write_seek_index(void)\n+static void t_table_read_write_seek_index(void)\n {\n-\ttest_table_read_write_seek(1, GIT_SHA1_FORMAT_ID);\n+\tt_table_read_write_seek(1, GIT_SHA1_FORMAT_ID);\n }\n \n-static void test_table_refs_for(int indexed)\n+static void t_table_refs_for(int indexed)\n {\n \tint N = 50;\n \tchar **want_names = reftable_calloc(N + 1, sizeof(*want_names));\n@@ -526,18 +529,18 @@ static void test_table_refs_for(int indexed)\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 256,\n \t};\n-\tstruct reftable_ref_record ref = { NULL };\n+\tstruct reftable_ref_record ref = { 0 };\n \tint i = 0;\n \tint n;\n \tint err;\n \tstruct reftable_reader rd;\n-\tstruct reftable_block_source source = { NULL };\n+\tstruct reftable_block_source source = { 0 };\n \n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n \t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n \n-\tstruct reftable_iterator it = { NULL };\n+\tstruct reftable_iterator it = { 0 };\n \tint j;\n \n \tset_test_hash(want_hash, 4);\n@@ -546,7 +549,7 @@ static void test_table_refs_for(int indexed)\n \t\tuint8_t hash[GIT_SHA1_RAWSZ];\n \t\tchar fill[51] = { 0 };\n \t\tchar name[100];\n-\t\tstruct reftable_ref_record ref = { NULL };\n+\t\tstruct reftable_ref_record ref = { 0 };\n \n \t\tmemset(hash, i, sizeof(hash));\n \t\tmemset(fill, 'x', 50);\n@@ -563,16 +566,15 @@ static void test_table_refs_for(int indexed)\n \t\t */\n \t\t/* blocks. */\n \t\tn = reftable_writer_add_ref(w, &ref);\n-\t\tEXPECT(n == 0);\n+\t\tcheck_int(n, ==, 0);\n \n \t\tif (!memcmp(ref.value.val2.value, want_hash, GIT_SHA1_RAWSZ) ||\n-\t\t    !memcmp(ref.value.val2.target_value, want_hash, GIT_SHA1_RAWSZ)) {\n+\t\t    !memcmp(ref.value.val2.target_value, want_hash, GIT_SHA1_RAWSZ))\n \t\t\twant_names[want_names_len++] = xstrdup(name);\n-\t\t}\n \t}\n \n \tn = reftable_writer_close(w);\n-\tEXPECT(n == 0);\n+\tcheck_int(n, ==, 0);\n \n \treftable_writer_free(w);\n \tw = NULL;\n@@ -580,33 +582,30 @@ static void test_table_refs_for(int indexed)\n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = init_reader(&rd, &source, \"file.ref\");\n-\tEXPECT_ERR(err);\n-\tif (!indexed) {\n+\tcheck(!err);\n+\tif (!indexed)\n \t\trd.obj_offsets.is_present = 0;\n-\t}\n \n \treftable_reader_init_ref_iterator(&rd, &it);\n \terr = reftable_iterator_seek_ref(&it, \"\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \treftable_iterator_destroy(&it);\n \n \terr = reftable_reader_refs_for(&rd, &it, want_hash);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \tj = 0;\n \twhile (1) {\n \t\tint err = reftable_iterator_next_ref(&it, &ref);\n-\t\tEXPECT(err >= 0);\n-\t\tif (err > 0) {\n+\t\tcheck_int(err, >=, 0);\n+\t\tif (err > 0)\n \t\t\tbreak;\n-\t\t}\n-\n-\t\tEXPECT(j < want_names_len);\n-\t\tEXPECT(0 == strcmp(ref.refname, want_names[j]));\n+\t\tcheck_int(j, <, want_names_len);\n+\t\tcheck_str(ref.refname, want_names[j]);\n \t\tj++;\n \t\treftable_ref_record_release(&ref);\n \t}\n-\tEXPECT(j == want_names_len);\n+\tcheck_int(j, ==, want_names_len);\n \n \tstrbuf_release(&buf);\n \tfree_names(want_names);\n@@ -614,54 +613,54 @@ static void test_table_refs_for(int indexed)\n \treader_close(&rd);\n }\n \n-static void test_table_refs_for_no_index(void)\n+static void t_table_refs_for_no_index(void)\n {\n-\ttest_table_refs_for(0);\n+\tt_table_refs_for(0);\n }\n \n-static void test_table_refs_for_obj_index(void)\n+static void t_table_refs_for_obj_index(void)\n {\n-\ttest_table_refs_for(1);\n+\tt_table_refs_for(1);\n }\n \n-static void test_write_empty_table(void)\n+static void t_write_empty_table(void)\n {\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n \t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n-\tstruct reftable_block_source source = { NULL };\n+\tstruct reftable_block_source source = { 0 };\n \tstruct reftable_reader *rd = NULL;\n-\tstruct reftable_ref_record rec = { NULL };\n-\tstruct reftable_iterator it = { NULL };\n+\tstruct reftable_ref_record rec = { 0 };\n+\tstruct reftable_iterator it = { 0 };\n \tint err;\n \n \treftable_writer_set_limits(w, 1, 1);\n \n \terr = reftable_writer_close(w);\n-\tEXPECT(err == REFTABLE_EMPTY_TABLE_ERROR);\n+\tcheck_int(err, ==, REFTABLE_EMPTY_TABLE_ERROR);\n \treftable_writer_free(w);\n \n-\tEXPECT(buf.len == header_size(1) + footer_size(1));\n+\tcheck_int(buf.len, ==, header_size(1) + footer_size(1));\n \n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = reftable_new_reader(&rd, &source, \"filename\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_reader_init_ref_iterator(rd, &it);\n \terr = reftable_iterator_seek_ref(&it, \"\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \terr = reftable_iterator_next_ref(&it, &rec);\n-\tEXPECT(err > 0);\n+\tcheck_int(err, >, 0);\n \n \treftable_iterator_destroy(&it);\n \treftable_reader_free(rd);\n \tstrbuf_release(&buf);\n }\n \n-static void test_write_object_id_min_length(void)\n+static void t_write_object_id_min_length(void)\n {\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 75,\n@@ -686,17 +685,17 @@ static void test_write_object_id_min_length(void)\n \t\tsnprintf(name, sizeof(name), \"ref%05d\", i);\n \t\tref.refname = name;\n \t\terr = reftable_writer_add_ref(w, &ref);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \n \terr = reftable_writer_close(w);\n-\tEXPECT_ERR(err);\n-\tEXPECT(reftable_writer_stats(w)->object_id_len == 2);\n+\tcheck(!err);\n+\tcheck_int(reftable_writer_stats(w)->object_id_len, ==, 2);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n }\n \n-static void test_write_object_id_length(void)\n+static void t_write_object_id_length(void)\n {\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 75,\n@@ -722,17 +721,17 @@ static void test_write_object_id_length(void)\n \t\tref.refname = name;\n \t\tref.value.val1[15] = i;\n \t\terr = reftable_writer_add_ref(w, &ref);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \n \terr = reftable_writer_close(w);\n-\tEXPECT_ERR(err);\n-\tEXPECT(reftable_writer_stats(w)->object_id_len == 16);\n+\tcheck(!err);\n+\tcheck_int(reftable_writer_stats(w)->object_id_len, ==, 16);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n }\n \n-static void test_write_empty_key(void)\n+static void t_write_empty_key(void)\n {\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -747,15 +746,15 @@ static void test_write_empty_key(void)\n \n \treftable_writer_set_limits(w, 1, 1);\n \terr = reftable_writer_add_ref(w, &ref);\n-\tEXPECT(err == REFTABLE_API_ERROR);\n+\tcheck_int(err, ==, REFTABLE_API_ERROR);\n \n \terr = reftable_writer_close(w);\n-\tEXPECT(err == REFTABLE_EMPTY_TABLE_ERROR);\n+\tcheck_int(err, ==, REFTABLE_EMPTY_TABLE_ERROR);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n }\n \n-static void test_write_key_order(void)\n+static void t_write_key_order(void)\n {\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -782,15 +781,15 @@ static void test_write_key_order(void)\n \n \treftable_writer_set_limits(w, 1, 1);\n \terr = reftable_writer_add_ref(w, &refs[0]);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \terr = reftable_writer_add_ref(w, &refs[1]);\n-\tEXPECT(err == REFTABLE_API_ERROR);\n+\tcheck_int(err, ==, REFTABLE_API_ERROR);\n \treftable_writer_close(w);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n }\n \n-static void test_write_multiple_indices(void)\n+static void t_write_multiple_indices(void)\n {\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 100,\n@@ -817,7 +816,7 @@ static void test_write_multiple_indices(void)\n \t\tref.refname = buf.buf,\n \n \t\terr = reftable_writer_add_ref(writer, &ref);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \n \tfor (i = 0; i < 100; i++) {\n@@ -835,7 +834,7 @@ static void test_write_multiple_indices(void)\n \t\tlog.refname = buf.buf,\n \n \t\terr = reftable_writer_add_log(writer, &log);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \n \treftable_writer_close(writer);\n@@ -845,13 +844,13 @@ static void test_write_multiple_indices(void)\n \t * for each of the block types.\n \t */\n \tstats = reftable_writer_stats(writer);\n-\tEXPECT(stats->ref_stats.index_offset > 0);\n-\tEXPECT(stats->obj_stats.index_offset > 0);\n-\tEXPECT(stats->log_stats.index_offset > 0);\n+\tcheck_int(stats->ref_stats.index_offset, >, 0);\n+\tcheck_int(stats->obj_stats.index_offset, >, 0);\n+\tcheck_int(stats->log_stats.index_offset, >, 0);\n \n \tblock_source_from_strbuf(&source, &writer_buf);\n \terr = reftable_new_reader(&reader, &source, \"filename\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \t/*\n \t * Seeking the log uses the log index now. In case there is any\n@@ -859,7 +858,7 @@ static void test_write_multiple_indices(void)\n \t */\n \treftable_reader_init_log_iterator(reader, &it);\n \terr = reftable_iterator_seek_log(&it, \"\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_iterator_destroy(&it);\n \treftable_writer_free(writer);\n@@ -868,7 +867,7 @@ static void test_write_multiple_indices(void)\n \tstrbuf_release(&buf);\n }\n \n-static void test_write_multi_level_index(void)\n+static void t_write_multi_level_index(void)\n {\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 100,\n@@ -895,7 +894,7 @@ static void test_write_multi_level_index(void)\n \t\tref.refname = buf.buf,\n \n \t\terr = reftable_writer_add_ref(writer, &ref);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \treftable_writer_close(writer);\n \n@@ -904,18 +903,18 @@ static void test_write_multi_level_index(void)\n \t * multi-level index.\n \t */\n \tstats = reftable_writer_stats(writer);\n-\tEXPECT(stats->ref_stats.max_index_level == 2);\n+\tcheck_int(stats->ref_stats.max_index_level, ==, 2);\n \n \tblock_source_from_strbuf(&source, &writer_buf);\n \terr = reftable_new_reader(&reader, &source, \"filename\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \t/*\n \t * Seeking the last ref should work as expected.\n \t */\n \treftable_reader_init_ref_iterator(reader, &it);\n \terr = reftable_iterator_seek_ref(&it, \"refs/heads/199\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_iterator_destroy(&it);\n \treftable_writer_free(writer);\n@@ -924,56 +923,57 @@ static void test_write_multi_level_index(void)\n \tstrbuf_release(&buf);\n }\n \n-static void test_corrupt_table_empty(void)\n+static void t_corrupt_table_empty(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_block_source source = { 0 };\n+\tstruct reftable_reader rd = { 0 };\n \tint err;\n \n \tblock_source_from_strbuf(&source, &buf);\n \terr = init_reader(&rd, &source, \"file.log\");\n-\tEXPECT(err == REFTABLE_FORMAT_ERROR);\n+\tcheck_int(err, ==, REFTABLE_FORMAT_ERROR);\n }\n \n-static void test_corrupt_table(void)\n+static void t_corrupt_table(void)\n {\n \tuint8_t zeros[1024] = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_block_source source = { 0 };\n+\tstruct reftable_reader rd = { 0 };\n \tint err;\n \tstrbuf_add(&buf, zeros, sizeof(zeros));\n \n \tblock_source_from_strbuf(&source, &buf);\n \terr = init_reader(&rd, &source, \"file.log\");\n-\tEXPECT(err == REFTABLE_FORMAT_ERROR);\n+\tcheck_int(err, ==, REFTABLE_FORMAT_ERROR);\n \tstrbuf_release(&buf);\n }\n \n-int readwrite_test_main(int argc, const char *argv[])\n+int cmd_main(int argc, const char *argv[])\n {\n-\tRUN_TEST(test_log_zlib_corruption);\n-\tRUN_TEST(test_corrupt_table);\n-\tRUN_TEST(test_corrupt_table_empty);\n-\tRUN_TEST(test_log_write_read);\n-\tRUN_TEST(test_write_key_order);\n-\tRUN_TEST(test_table_read_write_seek_linear_sha256);\n-\tRUN_TEST(test_log_buffer_size);\n-\tRUN_TEST(test_table_write_small_table);\n-\tRUN_TEST(test_buffer);\n-\tRUN_TEST(test_table_read_api);\n-\tRUN_TEST(test_table_read_write_sequential);\n-\tRUN_TEST(test_table_read_write_seek_linear);\n-\tRUN_TEST(test_table_read_write_seek_index);\n-\tRUN_TEST(test_table_refs_for_no_index);\n-\tRUN_TEST(test_table_refs_for_obj_index);\n-\tRUN_TEST(test_write_empty_key);\n-\tRUN_TEST(test_write_empty_table);\n-\tRUN_TEST(test_log_overflow);\n-\tRUN_TEST(test_write_object_id_length);\n-\tRUN_TEST(test_write_object_id_min_length);\n-\tRUN_TEST(test_write_multiple_indices);\n-\tRUN_TEST(test_write_multi_level_index);\n-\treturn 0;\n+\tTEST(t_buffer(), \"strbuf works as blocksource\");\n+\tTEST(t_corrupt_table(), \"read-write on corrupted table\");\n+\tTEST(t_corrupt_table_empty(), \"read-write on an empty table\");\n+\tTEST(t_log_buffer_size(), \"buffer extension for log compression\");\n+\tTEST(t_log_overflow(), \"log overflow returns expected error\");\n+\tTEST(t_log_write_read(), \"read-write on log records\");\n+\tTEST(t_log_zlib_corruption(), \"reading corrupted log record returns expected error\");\n+\tTEST(t_table_read_api(), \"read on a table\");\n+\tTEST(t_table_read_write_seek_index(), \"read-write on a table with index\");\n+\tTEST(t_table_read_write_seek_linear(), \"read-write on a table without index (SHA1)\");\n+\tTEST(t_table_read_write_seek_linear_sha256(), \"read-write on a table without index (SHA256)\");\n+\tTEST(t_table_read_write_sequential(), \"sequential read-write on a table\");\n+\tTEST(t_table_refs_for_no_index(), \"refs-only table with no index\");\n+\tTEST(t_table_refs_for_obj_index(), \"refs-only table with index\");\n+\tTEST(t_table_write_small_table(), \"write_table works\");\n+\tTEST(t_write_empty_key(), \"write on refs with empty keys\");\n+\tTEST(t_write_empty_table(), \"read-write on empty tables\");\n+\tTEST(t_write_key_order(), \"refs must be written in increasing order\");\n+\tTEST(t_write_multi_level_index(), \"table with multi-level index\");\n+\tTEST(t_write_multiple_indices(), \"table with indices for multiple block types\");\n+\tTEST(t_write_object_id_length(), \"prefix compression on writing refs\");\n+\tTEST(t_write_object_id_min_length(), \"prefix compression on writing refs\");\n+\n+\treturn test_done();\n }\n-- \n2.45.GIT\n\n"},{"id":"500536","messageId":"20240809111312.4401-3-chandrapratap3519@gmail.com","threadId":"61916","inReplyTo":"20240809111312.4401-1-chandrapratap3519@gmail.com","subject":"[PATCH v2 2/4] t-reftable-readwrite: use free_names() instead of a for loop","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-09T11:05:42Z","receivedAt":"2024-08-09T11:13:46Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"free_names() as defined by reftable/basics.{c,h} frees a NULL\nterminated array of malloced strings along with the array itself.\nUse this function instead of a for loop to free such an array.\n\nMentored-by: Patrick Steinhardt <ps@pks.im>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n---\n t/unit-tests/t-reftable-readwrite.c | 9 ++-------\n 1 file changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\nindex 235e3d94c7..e90f2bf9de 100644\n--- a/t/unit-tests/t-reftable-readwrite.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -413,7 +413,6 @@ static void t_table_read_api(void)\n \tstruct reftable_reader rd = { 0 };\n \tstruct reftable_block_source source = { 0 };\n \tint err;\n-\tint i;\n \tstruct reftable_log_record log = { 0 };\n \tstruct reftable_iterator it = { 0 };\n \n@@ -432,10 +431,8 @@ static void t_table_read_api(void)\n \tcheck_int(err, ==, REFTABLE_API_ERROR);\n \n \tstrbuf_release(&buf);\n-\tfor (i = 0; i < N; i++)\n-\t\treftable_free(names[i]);\n+\tfree_names(names);\n \treftable_iterator_destroy(&it);\n-\treftable_free(names);\n \treader_close(&rd);\n \tstrbuf_release(&buf);\n }\n@@ -498,9 +495,7 @@ static void t_table_read_write_seek(int index, int hash_id)\n \treftable_iterator_destroy(&it);\n \n \tstrbuf_release(&buf);\n-\tfor (i = 0; i < N; i++)\n-\t\treftable_free(names[i]);\n-\treftable_free(names);\n+\tfree_names(names);\n \treader_close(&rd);\n }\n \n-- \n2.45.GIT\n\n"},{"id":"500537","messageId":"20240809111312.4401-4-chandrapratap3519@gmail.com","threadId":"61916","inReplyTo":"20240809111312.4401-1-chandrapratap3519@gmail.com","subject":"[PATCH v2 3/4] t-reftable-readwrite: use 'for' in place of infinite 'while' loops","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-09T11:05:43Z","receivedAt":"2024-08-09T11:13:49Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"Using a for loop with an empty conditional statement is more concise\nand easier to read than an infinite 'while' loop in instances\nwhere we need a loop variable. Hence, replace such instances of a\n'while' loop with the equivalent 'for' loop.\n\nMentored-by: Patrick Steinhardt <ps@pks.im>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n---\n t/unit-tests/t-reftable-readwrite.c | 12 +++---------\n 1 file changed, 3 insertions(+), 9 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\nindex e90f2bf9de..7daf28ec6d 100644\n--- a/t/unit-tests/t-reftable-readwrite.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -268,15 +268,13 @@ static void t_log_write_read(void)\n \terr = reftable_iterator_seek_log(&it, \"\");\n \tcheck(!err);\n \n-\ti = 0;\n-\twhile (1) {\n+\tfor (i = 0; ; i++) {\n \t\tint err = reftable_iterator_next_log(&it, &log);\n \t\tif (err > 0)\n \t\t\tbreak;\n \t\tcheck(!err);\n \t\tcheck_str(names[i], log.refname);\n \t\tcheck_int(i, ==, log.update_index);\n-\t\ti++;\n \t\treftable_log_record_release(&log);\n \t}\n \n@@ -374,7 +372,7 @@ static void t_table_read_write_sequential(void)\n \terr = reftable_iterator_seek_ref(&it, \"\");\n \tcheck(!err);\n \n-\twhile (1) {\n+\tfor (j = 0; ; j++) {\n \t\tstruct reftable_ref_record ref = { 0 };\n \t\tint r = reftable_iterator_next_ref(&it, &ref);\n \t\tcheck_int(r, >=, 0);\n@@ -382,8 +380,6 @@ static void t_table_read_write_sequential(void)\n \t\t\tbreak;\n \t\tcheck_str(names[j], ref.refname);\n \t\tcheck_int(update_index, ==, ref.update_index);\n-\n-\t\tj++;\n \t\treftable_ref_record_release(&ref);\n \t}\n \tcheck_int(j, ==, N);\n@@ -589,15 +585,13 @@ static void t_table_refs_for(int indexed)\n \terr = reftable_reader_refs_for(&rd, &it, want_hash);\n \tcheck(!err);\n \n-\tj = 0;\n-\twhile (1) {\n+\tfor (j = 0; ; j++) {\n \t\tint err = reftable_iterator_next_ref(&it, &ref);\n \t\tcheck_int(err, >=, 0);\n \t\tif (err > 0)\n \t\t\tbreak;\n \t\tcheck_int(j, <, want_names_len);\n \t\tcheck_str(ref.refname, want_names[j]);\n-\t\tj++;\n \t\treftable_ref_record_release(&ref);\n \t}\n \tcheck_int(j, ==, want_names_len);\n-- \n2.45.GIT\n\n"},{"id":"500538","messageId":"20240809111312.4401-5-chandrapratap3519@gmail.com","threadId":"61916","inReplyTo":"20240809111312.4401-1-chandrapratap3519@gmail.com","subject":"[PATCH v2 4/4] t-reftable-readwrite: add test for known error","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-09T11:05:44Z","receivedAt":"2024-08-09T11:13:52Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"When using reftable_writer_add_ref() to add a ref record to a\nreftable writer, The update_index of the ref record must be within\nthe limits set by reftable_writer_set_limits(), or REFTABLE_API_ERROR\nis returned. This scenario is currently left untested. Add a test\ncase for the same.\n\nMentored-by: Patrick Steinhardt <ps@pks.im>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n---\n t/unit-tests/t-reftable-readwrite.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\nindex 7daf28ec6d..a5462441d3 100644\n--- a/t/unit-tests/t-reftable-readwrite.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -773,6 +773,11 @@ static void t_write_key_order(void)\n \tcheck(!err);\n \terr = reftable_writer_add_ref(w, &refs[1]);\n \tcheck_int(err, ==, REFTABLE_API_ERROR);\n+\n+\trefs[0].update_index = 2;\n+\terr = reftable_writer_add_ref(w, &refs[0]);\n+\tcheck_int(err, ==, REFTABLE_API_ERROR);\n+\n \treftable_writer_close(w);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n-- \n2.45.GIT\n\n"},{"id":"500548","messageId":"xmqqplqheit4.fsf@gitster.g","threadId":"61916","inReplyTo":"ZrR91dR3G06L9dy7@tanuki","subject":"Re: [PATCH 5/5] t-reftable-readwrite: add tests for print functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-09T16:56:39Z","receivedAt":"2024-08-09T16:56:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> I can see two options:\n>\n>   1. Refactor these interfaces such that they take a file descriptor as\n>      input that they are writing to. This would allow us to exercise\n>      that the output is correct.\n>\n>   2. Rip out this function. I don't think this functionality should be\n>      part of the library in the first place, and it really only exists\n>      because of \"reftable/dump.c\".\n>\n> I think the latter is the better option. The functionality exists to\n> drive `cmd__dump_reftable()` in our reftable test helper. We should\n> likely make the whole implementation of this an internal implementation\n> detail and not expose it.\n\nThanks for a review.  Are there anything other than removing this\nstep that this series needs?\n\n"},{"id":"500555","messageId":"xmqqwmkpd0qs.fsf@gitster.g","threadId":"61916","inReplyTo":"20240809111312.4401-2-chandrapratap3519@gmail.com","subject":"Re: [PATCH v2 1/4] t: move reftable/readwrite_test.c to the unit testing framework","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-09T18:12:11Z","receivedAt":"2024-08-09T18:12:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chandra Pratap <chandrapratap3519@gmail.com> writes:\n\n> reftable/readwrite_test.c exercises the functions defined in\n> reftable/reader.{c,h} and reftable/writer.{c,h}. Migrate\n> reftable/readwrite_test.c to the unit testing framework. Migration\n> involves refactoring the tests to use the unit testing framework\n> instead of reftable's test framework and renaming the tests to\n> align with unit-tests' naming conventions.\n>\n> Since some tests in reftable/readwrite_test.c use the functions\n> set_test_hash(), noop_flush() and strbuf_add_void() defined in\n> reftable/test_framework.{c,h} but these files are not #included\n> in the ported unit test, copy these functions in the new test file.\n>\n> While at it, ensure structs are 0-initialized with '= { 0 }'\n> instead of '= { NULL }'.\n\nOK.\n\n> -\t\tEXPECT(buf->buf[off] == 'r');\n> +\t\tif (!off)\n> +\t\t\toff = header_size((hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);\n> +\t\tcheck(buf->buf[off] == 'r');\n\nWhy not \"check_char(buf->buf[off], ==, 'r')\"?\n\n>  \t}\n>  \n> -\tEXPECT(stats->log_stats.blocks > 0);\n> +\tcheck(stats->log_stats.blocks > 0);\n\nWhy not \"check_int(stats->log_stats.blocks, >, 0)\", which you used\nin the t_log_write_read() function?\n\nWhile reading this step, I looked for use of check() that is not\nrewriting EXPECT_ERR(x) to check(!x) as suspicious.  The above two\n(and a !memcmp() that is OK) were the only three such uses of\ncheck(), I think.\n\nThanks.\n\n"},{"id":"500556","messageId":"xmqqjzgpcymq.fsf@gitster.g","threadId":"61916","inReplyTo":"20240809111312.4401-3-chandrapratap3519@gmail.com","subject":"Re: [PATCH v2 2/4] t-reftable-readwrite: use free_names() instead of a for loop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-09T18:57:49Z","receivedAt":"2024-08-09T18:57:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chandra Pratap <chandrapratap3519@gmail.com> writes:\n\n> free_names() as defined by reftable/basics.{c,h} frees a NULL\n> terminated array of malloced strings along with the array itself.\n> Use this function instead of a for loop to free such an array.\n\nGoing back to [1/4], the headers included in this test looked like this:\n\n    -#include \"system.h\"\n    -\n    -#include \"basics.h\"\n    -#include \"block.h\"\n    -#include \"blocksource.h\"\n    -#include \"reader.h\"\n    -#include \"record.h\"\n    -#include \"test_framework.h\"\n    -#include \"reftable-tests.h\"\n    -#include \"reftable-writer.h\"\n    +#include \"test-lib.h\"\n    +#include \"reftable/reader.h\"\n    +#include \"reftable/blocksource.h\"\n    +#include \"reftable/reftable-error.h\"\n    +#include \"reftable/reftable-writer.h\"\n\nI found this part a bit curious, perhaps because I was not involved\nin either reftable/ or unit-tests/ development.  So I may be asking\na stupid question, but is it intended that some headers like\n\"block.h\" and \"record.h\" are no longer included?\n\nIt is understandable that inclusion of \"test-lib.h\" is new (and\nneeds to be there to work as part of t/unit-tests/), and the leading\ndirectory name \"reftable/\" added to header files are also justified,\nof course.  But if you depend on \"basics.h\" and do not include it,\nthat does not sound like the most hygenic thing to do, at least to\nme.\n\nThe code changes themselves look good; I can see that the\nimplementation of free_names() in reftable/basics.c safely replaces\nthese loops.  There is a slight behaviour difference that names[]\nthat was fed to reftable_iterator_seek_ref() earlier goes away\nbefore the iterator is destroyed, but _seek_ref() does not retain\nthe names[0] argument in the iterator object, so that is OK.\n\nThanks.\n"},{"id":"500558","messageId":"xmqqfrrdcy84.fsf@gitster.g","threadId":"61916","inReplyTo":"20240809111312.4401-4-chandrapratap3519@gmail.com","subject":"Re: [PATCH v2 3/4] t-reftable-readwrite: use 'for' in place of infinite 'while' loops","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-09T19:06:35Z","receivedAt":"2024-08-09T19:06:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chandra Pratap <chandrapratap3519@gmail.com> writes:\n\n> Using a for loop with an empty conditional statement is more concise\n> and easier to read than an infinite 'while' loop in instances\n> where we need a loop variable. Hence, replace such instances of a\n> 'while' loop with the equivalent 'for' loop.\n\nQuite honestly, the above is counter-productive if pushed as a\ngeneral guideline, because the goodness of it depends what happens\nin the third part of the for () control (i.e., what should happen at\nthe end of each iteration and if it wants to be bypassed in the\nconditional inside the loop).\n\nIn this particular case, it probably is OK, but still is a\nsubjective, borderline Meh, to me.\n\nI see no violation of correctness in the rewrite, though ;-)\n\n"},{"id":"500593","messageId":"CA+J6zkSHX892NoNOyTDc-38_giBR=Q-Hf7+7nymU9GnPu1V-5Q@mail.gmail.com","threadId":"61916","inReplyTo":"xmqqjzgpcymq.fsf@gitster.g","subject":"Re: [PATCH v2 2/4] t-reftable-readwrite: use free_names() instead of a for loop","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-10T05:50:07Z","receivedAt":"2024-08-10T05:50:34Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"On Sat, 10 Aug 2024 at 00:27, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Chandra Pratap <chandrapratap3519@gmail.com> writes:\n>\n> > free_names() as defined by reftable/basics.{c,h} frees a NULL\n> > terminated array of malloced strings along with the array itself.\n> > Use this function instead of a for loop to free such an array.\n>\n> Going back to [1/4], the headers included in this test looked like this:\n>\n>     -#include \"system.h\"\n>     -\n>     -#include \"basics.h\"\n>     -#include \"block.h\"\n>     -#include \"blocksource.h\"\n>     -#include \"reader.h\"\n>     -#include \"record.h\"\n>     -#include \"test_framework.h\"\n>     -#include \"reftable-tests.h\"\n>     -#include \"reftable-writer.h\"\n>     +#include \"test-lib.h\"\n>     +#include \"reftable/reader.h\"\n>     +#include \"reftable/blocksource.h\"\n>     +#include \"reftable/reftable-error.h\"\n>     +#include \"reftable/reftable-writer.h\"\n>\n> I found this part a bit curious, perhaps because I was not involved\n> in either reftable/ or unit-tests/ development.  So I may be asking\n> a stupid question, but is it intended that some headers like\n> \"block.h\" and \"record.h\" are no longer included?\n>\n> It is understandable that inclusion of \"test-lib.h\" is new (and\n> needs to be there to work as part of t/unit-tests/), and the leading\n> directory name \"reftable/\" added to header files are also justified,\n> of course.  But if you depend on \"basics.h\" and do not include it,\n> that does not sound like the most hygenic thing to do, at least to\n> me.\n\nI think 'basics.{c,h}' in reftable/ is equivalent to 'stdio.h' in a generic\nC program, it holds fundamental functionalities to be used by other\nreftable structures and hence, is always (implicitly or explicitly)\n#included in almost all of the files in reftable/.\n\nThis test is supposed to focus on reftable's read-write functionalities\nso it makes sense to explicitly #include only those headers that\nare directly responsible for those functionalities, namely 'reader.h',\n'blocksource.h' and 'reftable-writer.h'. 'reftable-error.h' is thrown in\nthere as well because some tests need to explicitly mention the\nvarious error codes and it doesn't make sense to rely on it being\n#included by others.\n\n> The code changes themselves look good; I can see that the\n> implementation of free_names() in reftable/basics.c safely replaces\n> these loops.  There is a slight behaviour difference that names[]\n> that was fed to reftable_iterator_seek_ref() earlier goes away\n> before the iterator is destroyed, but _seek_ref() does not retain\n> the names[0] argument in the iterator object, so that is OK.\n>\n> Thanks.\n"},{"id":"500594","messageId":"xmqqbk20aowc.fsf@gitster.g","threadId":"61916","inReplyTo":"CA+J6zkSHX892NoNOyTDc-38_giBR=Q-Hf7+7nymU9GnPu1V-5Q@mail.gmail.com","subject":"Re: [PATCH v2 2/4] t-reftable-readwrite: use free_names() instead of a for loop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-10T06:10:59Z","receivedAt":"2024-08-10T06:11:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chandra Pratap <chandrapratap3519@gmail.com> writes:\n\n> On Sat, 10 Aug 2024 at 00:27, Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Chandra Pratap <chandrapratap3519@gmail.com> writes:\n>>\n>> > free_names() as defined by reftable/basics.{c,h} frees a NULL\n>> > terminated array of malloced strings along with the array itself.\n>> > Use this function instead of a for loop to free such an array.\n>> ...\n> This test is supposed to focus on reftable's read-write functionalities\n> so it makes sense to explicitly #include only those headers that\n> are directly responsible for those functionalities, namely 'reader.h',\n> 'blocksource.h' and 'reftable-writer.h'. 'reftable-error.h' is thrown in\n> there as well because some tests need to explicitly mention the\n> various error codes and it doesn't make sense to rely on it being\n> #included by others.\n\nI think we are on the same page.  The code explicitly exercises\nfree_names() after this step, and that is exactly why I found it odd\nto rely on basics.h happen to be included by some other header\nfile(s) we explicitly include.\n\nThanks.\n"},{"id":"500638","messageId":"CA+J6zkRTRQ9o=CDgsFbJx5csjDxLfQC_E+dw+Csz3hp=_c8Ueg@mail.gmail.com","threadId":"61916","inReplyTo":"xmqqwmkpd0qs.fsf@gitster.g","subject":"Re: [PATCH v2 1/4] t: move reftable/readwrite_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-12T14:50:26Z","receivedAt":"2024-08-12T14:50:53Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"On Fri, 9 Aug 2024 at 23:42, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Chandra Pratap <chandrapratap3519@gmail.com> writes:\n>\n> > reftable/readwrite_test.c exercises the functions defined in\n> > reftable/reader.{c,h} and reftable/writer.{c,h}. Migrate\n> > reftable/readwrite_test.c to the unit testing framework. Migration\n> > involves refactoring the tests to use the unit testing framework\n> > instead of reftable's test framework and renaming the tests to\n> > align with unit-tests' naming conventions.\n> >\n> > Since some tests in reftable/readwrite_test.c use the functions\n> > set_test_hash(), noop_flush() and strbuf_add_void() defined in\n> > reftable/test_framework.{c,h} but these files are not #included\n> > in the ported unit test, copy these functions in the new test file.\n> >\n> > While at it, ensure structs are 0-initialized with '= { 0 }'\n> > instead of '= { NULL }'.\n>\n> OK.\n>\n> > -             EXPECT(buf->buf[off] == 'r');\n> > +             if (!off)\n> > +                     off = header_size((hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);\n> > +             check(buf->buf[off] == 'r');\n>\n> Why not \"check_char(buf->buf[off], ==, 'r')\"?\n\nI wrote this series quite some time ago when this functionality\nwas not yet introduced to the unit testing framework. I'll commit\nthis change in the next reroll.\n\n> >       }\n> >\n> > -     EXPECT(stats->log_stats.blocks > 0);\n> > +     check(stats->log_stats.blocks > 0);\n>\n> Why not \"check_int(stats->log_stats.blocks, >, 0)\", which you used\n> in the t_log_write_read() function?\n\nLooks like a case of too-mechanical-a-translation to me. I'll fix this\nin the next version.\n\n> While reading this step, I looked for use of check() that is not\n> rewriting EXPECT_ERR(x) to check(!x) as suspicious.  The above two\n> (and a !memcmp() that is OK) were the only three such uses of\n> check(), I think.\n\nI went through this series again and I agree on not encountering\nany other subpar translations of EXPECT() to its counterparts in\nthe unit testing framework. I'll reroll the series with only these\nchanges until someone else finds any other corrections.\n"},{"id":"500786","messageId":"20240813144440.4602-1-chandrapratap3519@gmail.com","threadId":"61916","inReplyTo":"20240809111312.4401-1-chandrapratap3519@gmail.com","subject":"[GSoC][PATCH v3 0/4] t: port reftable/readwrite_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-13T14:34:46Z","receivedAt":"2024-08-13T14:45:01Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"The reftable library comes with self tests, which are exercised\nas part of the usual end-to-end tests and are designed to\nobserve the end-user visible effects of Git commands. What it\nexercises, however, is a better match for the unit-testing\nframework, merged at 8bf6fbd0 (Merge branch 'js/doc-unit-tests',\n2023-12-09), which is designed to observe how low level\nimplementation details, at the level of sequences of individual\nfunction calls, behave.\n\nHence, port reftable/readwrite_test.c to the unit testing framework\nand improve upon the ported test. The first patch in the series moves\nthe test to the unit testing framework, and the rest of the patches\nimprove upon the ported test.\n\nMentored-by: Patrick Steinhardt <ps@pks.im>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n\n---\nChanges in v3:\n- Order the header files alphabetically in\n  t/unit-tests/t-reftable-readwrite.c in patch 1.\n- use check_char() and check_int() instead of check() at two\n  instances in patch 1.\n- #include 'reftable/basics.h' in patch 2 when introducing\n  free_names() in the test file.\n\nCI/PR: https://github.com/gitgitgadget/git/pull/1770\n\nChandra Pratap(4):\nt: move reftable/readwrite_test.c to the unit testing framework\nt-reftable-readwrite: use free_names() instead of a for loop\nt-reftable-readwrite: use 'for' in place of infinite 'while' loops\nt-reftable-readwrite: add test for known error\n\nMakefile                                                         |   2 +-\nreftable/reftable-tests.h                                        |   1 -\nt/helper/test-reftable.c                                         |   1 -\nreftable/readwrite_test.c => t/unit-tests/t-reftable-readwrite.c | 441 +++++++++++++++++++++++-----------------\n4 files changed, 219 insertions(+), 226 deletions(-)\n\nRange-diff against v2:\n1:  0ebe76c331 ! 1:  03b946434e t: move reftable/readwrite_test.c to the unit testing framework\n    @@ t/unit-tests/t-reftable-readwrite.c: license that can be found in the LICENSE fi\n     -#include \"reftable-tests.h\"\n     -#include \"reftable-writer.h\"\n     +#include \"test-lib.h\"\n    -+#include \"reftable/reader.h\"\n     +#include \"reftable/blocksource.h\"\n    ++#include \"reftable/reader.h\"\n     +#include \"reftable/reftable-error.h\"\n     +#include \"reftable/reftable-writer.h\"\n\n    @@ t/unit-tests/t-reftable-readwrite.c: static void write_table(char ***names, stru\n     -\t\tEXPECT(buf->buf[off] == 'r');\n     +\t\tif (!off)\n     +\t\t\toff = header_size((hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);\n    -+\t\tcheck(buf->buf[off] == 'r');\n    ++\t\tcheck_char(buf->buf[off], ==, 'r');\n      \t}\n\n     -\tEXPECT(stats->log_stats.blocks > 0);\n    -+\tcheck(stats->log_stats.blocks > 0);\n    ++\tcheck_int(stats->log_stats.blocks, >, 0);\n      \treftable_writer_free(w);\n      }\n\n2:  a148702451 ! 2:  e23a515736 t-reftable-readwrite: use free_names() instead of a for loop\n    @@ Commit message\n         Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n\n      ## t/unit-tests/t-reftable-readwrite.c ##\n    +@@ t/unit-tests/t-reftable-readwrite.c: license that can be found in the LICENSE file or at\n    + */\n    +\n    + #include \"test-lib.h\"\n    ++#include \"reftable/basics.h\"\n    + #include \"reftable/blocksource.h\"\n    + #include \"reftable/reader.h\"\n    + #include \"reftable/reftable-error.h\"\n     @@ t/unit-tests/t-reftable-readwrite.c: static void t_table_read_api(void)\n      \tstruct reftable_reader rd = { 0 };\n      \tstruct reftable_block_source source = { 0 };\n3:  ee15af6631 = 3:  9194a4055a t-reftable-readwrite: use 'for' in place of infinite 'while' loops\n4:  3f571c09e2 = 4:  d34b01fad8 t-reftable-readwrite: add test for known error\n\n"},{"id":"500787","messageId":"20240813144440.4602-2-chandrapratap3519@gmail.com","threadId":"61916","inReplyTo":"20240813144440.4602-1-chandrapratap3519@gmail.com","subject":"[PATCH v3 1/4] t: move reftable/readwrite_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-13T14:34:47Z","receivedAt":"2024-08-13T14:45:05Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"reftable/readwrite_test.c exercises the functions defined in\nreftable/reader.{c,h} and reftable/writer.{c,h}. Migrate\nreftable/readwrite_test.c to the unit testing framework. Migration\ninvolves refactoring the tests to use the unit testing framework\ninstead of reftable's test framework and renaming the tests to\nalign with unit-tests' naming conventions.\n\nSince some tests in reftable/readwrite_test.c use the functions\nset_test_hash(), noop_flush() and strbuf_add_void() defined in\nreftable/test_framework.{c,h} but these files are not #included\nin the ported unit test, copy these functions in the new test file.\n\nWhile at it, ensure structs are 0-initialized with '= { 0 }'\ninstead of '= { NULL }'.\n\nMentored-by: Patrick Steinhardt <ps@pks.im>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n---\n Makefile                                      |   2 +-\n reftable/reftable-tests.h                     |   1 -\n t/helper/test-reftable.c                      |   1 -\n .../unit-tests/t-reftable-readwrite.c         | 418 +++++++++---------\n 4 files changed, 210 insertions(+), 212 deletions(-)\n rename reftable/readwrite_test.c => t/unit-tests/t-reftable-readwrite.c (70%)\n\ndiff --git a/Makefile b/Makefile\nindex 3863e60b66..76e4d1c1ec 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1341,6 +1341,7 @@ UNIT_TEST_PROGRAMS += t-oidtree\n UNIT_TEST_PROGRAMS += t-prio-queue\n UNIT_TEST_PROGRAMS += t-reftable-basics\n UNIT_TEST_PROGRAMS += t-reftable-merged\n+UNIT_TEST_PROGRAMS += t-reftable-readwrite\n UNIT_TEST_PROGRAMS += t-reftable-record\n UNIT_TEST_PROGRAMS += t-strbuf\n UNIT_TEST_PROGRAMS += t-strcmp-offset\n@@ -2682,7 +2683,6 @@ REFTABLE_OBJS += reftable/writer.o\n REFTABLE_TEST_OBJS += reftable/block_test.o\n REFTABLE_TEST_OBJS += reftable/dump.o\n REFTABLE_TEST_OBJS += reftable/pq_test.o\n-REFTABLE_TEST_OBJS += reftable/readwrite_test.o\n REFTABLE_TEST_OBJS += reftable/stack_test.o\n REFTABLE_TEST_OBJS += reftable/test_framework.o\n REFTABLE_TEST_OBJS += reftable/tree_test.o\ndiff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h\nindex d5e03dcc1b..7d955393d2 100644\n--- a/reftable/reftable-tests.h\n+++ b/reftable/reftable-tests.h\n@@ -13,7 +13,6 @@ int basics_test_main(int argc, const char **argv);\n int block_test_main(int argc, const char **argv);\n int pq_test_main(int argc, const char **argv);\n int record_test_main(int argc, const char **argv);\n-int readwrite_test_main(int argc, const char **argv);\n int stack_test_main(int argc, const char **argv);\n int tree_test_main(int argc, const char **argv);\n int reftable_dump_main(int argc, char *const *argv);\ndiff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c\nindex 9d378427da..d371e9f9dd 100644\n--- a/t/helper/test-reftable.c\n+++ b/t/helper/test-reftable.c\n@@ -8,7 +8,6 @@ int cmd__reftable(int argc, const char **argv)\n \tblock_test_main(argc, argv);\n \ttree_test_main(argc, argv);\n \tpq_test_main(argc, argv);\n-\treadwrite_test_main(argc, argv);\n \tstack_test_main(argc, argv);\n \treturn 0;\n }\ndiff --git a/reftable/readwrite_test.c b/t/unit-tests/t-reftable-readwrite.c\nsimilarity index 70%\nrename from reftable/readwrite_test.c\nrename to t/unit-tests/t-reftable-readwrite.c\nindex f411abfe9c..d0eb85fc38 100644\n--- a/reftable/readwrite_test.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -6,37 +6,48 @@ license that can be found in the LICENSE file or at\n https://developers.google.com/open-source/licenses/bsd\n */\n \n-#include \"system.h\"\n-\n-#include \"basics.h\"\n-#include \"block.h\"\n-#include \"blocksource.h\"\n-#include \"reader.h\"\n-#include \"record.h\"\n-#include \"test_framework.h\"\n-#include \"reftable-tests.h\"\n-#include \"reftable-writer.h\"\n+#include \"test-lib.h\"\n+#include \"reftable/blocksource.h\"\n+#include \"reftable/reader.h\"\n+#include \"reftable/reftable-error.h\"\n+#include \"reftable/reftable-writer.h\"\n \n static const int update_index = 5;\n \n-static void test_buffer(void)\n+static void set_test_hash(uint8_t *p, int i)\n+{\n+\tmemset(p, (uint8_t)i, hash_size(GIT_SHA1_FORMAT_ID));\n+}\n+\n+static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)\n+{\n+\tstrbuf_add(b, data, sz);\n+\treturn sz;\n+}\n+\n+static int noop_flush(void *arg)\n+{\n+\treturn 0;\n+}\n+\n+static void t_buffer(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_block out = { NULL };\n+\tstruct reftable_block_source source = { 0 };\n+\tstruct reftable_block out = { 0 };\n \tint n;\n \tuint8_t in[] = \"hello\";\n \tstrbuf_add(&buf, in, sizeof(in));\n \tblock_source_from_strbuf(&source, &buf);\n-\tEXPECT(block_source_size(&source) == 6);\n+\tcheck_int(block_source_size(&source), ==, 6);\n \tn = block_source_read_block(&source, &out, 0, sizeof(in));\n-\tEXPECT(n == sizeof(in));\n-\tEXPECT(!memcmp(in, out.data, n));\n+\tcheck_int(n, ==, sizeof(in));\n+\tcheck(!memcmp(in, out.data, n));\n \treftable_block_done(&out);\n \n \tn = block_source_read_block(&source, &out, 1, 2);\n-\tEXPECT(n == 2);\n-\tEXPECT(!memcmp(out.data, \"el\", 2));\n+\tcheck_int(n, ==, 2);\n+\tcheck(!memcmp(out.data, \"el\", 2));\n \n \treftable_block_done(&out);\n \tblock_source_close(&source);\n@@ -52,9 +63,9 @@ static void write_table(char ***names, struct strbuf *buf, int N,\n \t};\n \tstruct reftable_writer *w =\n \t\treftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);\n-\tstruct reftable_ref_record ref = { NULL };\n+\tstruct reftable_ref_record ref = { 0 };\n \tint i = 0, n;\n-\tstruct reftable_log_record log = { NULL };\n+\tstruct reftable_log_record log = { 0 };\n \tconst struct reftable_stats *stats = NULL;\n \n \tREFTABLE_CALLOC_ARRAY(*names, N + 1);\n@@ -73,7 +84,7 @@ static void write_table(char ***names, struct strbuf *buf, int N,\n \t\t(*names)[i] = xstrdup(name);\n \n \t\tn = reftable_writer_add_ref(w, &ref);\n-\t\tEXPECT(n == 0);\n+\t\tcheck_int(n, ==, 0);\n \t}\n \n \tfor (i = 0; i < N; i++) {\n@@ -89,27 +100,25 @@ static void write_table(char ***names, struct strbuf *buf, int N,\n \t\tlog.value.update.message = (char *) \"message\";\n \n \t\tn = reftable_writer_add_log(w, &log);\n-\t\tEXPECT(n == 0);\n+\t\tcheck_int(n, ==, 0);\n \t}\n \n \tn = reftable_writer_close(w);\n-\tEXPECT(n == 0);\n+\tcheck_int(n, ==, 0);\n \n \tstats = reftable_writer_stats(w);\n \tfor (i = 0; i < stats->ref_stats.blocks; i++) {\n \t\tint off = i * opts.block_size;\n-\t\tif (off == 0) {\n-\t\t\toff = header_size(\n-\t\t\t\t(hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);\n-\t\t}\n-\t\tEXPECT(buf->buf[off] == 'r');\n+\t\tif (!off)\n+\t\t\toff = header_size((hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);\n+\t\tcheck_char(buf->buf[off], ==, 'r');\n \t}\n \n-\tEXPECT(stats->log_stats.blocks > 0);\n+\tcheck_int(stats->log_stats.blocks, >, 0);\n \treftable_writer_free(w);\n }\n \n-static void test_log_buffer_size(void)\n+static void t_log_buffer_size(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_write_options opts = {\n@@ -140,14 +149,14 @@ static void test_log_buffer_size(void)\n \t}\n \treftable_writer_set_limits(w, update_index, update_index);\n \terr = reftable_writer_add_log(w, &log);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \terr = reftable_writer_close(w);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n }\n \n-static void test_log_overflow(void)\n+static void t_log_overflow(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tchar msg[256] = { 0 };\n@@ -177,12 +186,12 @@ static void test_log_overflow(void)\n \tmemset(msg, 'x', sizeof(msg) - 1);\n \treftable_writer_set_limits(w, update_index, update_index);\n \terr = reftable_writer_add_log(w, &log);\n-\tEXPECT(err == REFTABLE_ENTRY_TOO_BIG_ERROR);\n+\tcheck_int(err, ==, REFTABLE_ENTRY_TOO_BIG_ERROR);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n }\n \n-static void test_log_write_read(void)\n+static void t_log_write_read(void)\n {\n \tint N = 2;\n \tchar **names = reftable_calloc(N + 1, sizeof(*names));\n@@ -190,13 +199,13 @@ static void test_log_write_read(void)\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 256,\n \t};\n-\tstruct reftable_ref_record ref = { NULL };\n+\tstruct reftable_ref_record ref = { 0 };\n \tint i = 0;\n-\tstruct reftable_log_record log = { NULL };\n+\tstruct reftable_log_record log = { 0 };\n \tint n;\n-\tstruct reftable_iterator it = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n-\tstruct reftable_block_source source = { NULL };\n+\tstruct reftable_iterator it = { 0 };\n+\tstruct reftable_reader rd = { 0 };\n+\tstruct reftable_block_source source = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n \t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n@@ -204,17 +213,17 @@ static void test_log_write_read(void)\n \treftable_writer_set_limits(w, 0, N);\n \tfor (i = 0; i < N; i++) {\n \t\tchar name[256];\n-\t\tstruct reftable_ref_record ref = { NULL };\n+\t\tstruct reftable_ref_record ref = { 0 };\n \t\tsnprintf(name, sizeof(name), \"b%02d%0*d\", i, 130, 7);\n \t\tnames[i] = xstrdup(name);\n \t\tref.refname = name;\n \t\tref.update_index = i;\n \n \t\terr = reftable_writer_add_ref(w, &ref);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \tfor (i = 0; i < N; i++) {\n-\t\tstruct reftable_log_record log = { NULL };\n+\t\tstruct reftable_log_record log = { 0 };\n \n \t\tlog.refname = names[i];\n \t\tlog.update_index = i;\n@@ -223,33 +232,33 @@ static void test_log_write_read(void)\n \t\tset_test_hash(log.value.update.new_hash, i + 1);\n \n \t\terr = reftable_writer_add_log(w, &log);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \n \tn = reftable_writer_close(w);\n-\tEXPECT(n == 0);\n+\tcheck_int(n, ==, 0);\n \n \tstats = reftable_writer_stats(w);\n-\tEXPECT(stats->log_stats.blocks > 0);\n+\tcheck_int(stats->log_stats.blocks, >, 0);\n \treftable_writer_free(w);\n \tw = NULL;\n \n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = init_reader(&rd, &source, \"file.log\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_reader_init_ref_iterator(&rd, &it);\n \n \terr = reftable_iterator_seek_ref(&it, names[N - 1]);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \terr = reftable_iterator_next_ref(&it, &ref);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \t/* end of iteration. */\n \terr = reftable_iterator_next_ref(&it, &ref);\n-\tEXPECT(0 < err);\n+\tcheck_int(err, >, 0);\n \n \treftable_iterator_destroy(&it);\n \treftable_ref_record_release(&ref);\n@@ -257,23 +266,21 @@ static void test_log_write_read(void)\n \treftable_reader_init_log_iterator(&rd, &it);\n \n \terr = reftable_iterator_seek_log(&it, \"\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \ti = 0;\n \twhile (1) {\n \t\tint err = reftable_iterator_next_log(&it, &log);\n-\t\tif (err > 0) {\n+\t\tif (err > 0)\n \t\t\tbreak;\n-\t\t}\n-\n-\t\tEXPECT_ERR(err);\n-\t\tEXPECT_STREQ(names[i], log.refname);\n-\t\tEXPECT(i == log.update_index);\n+\t\tcheck(!err);\n+\t\tcheck_str(names[i], log.refname);\n+\t\tcheck_int(i, ==, log.update_index);\n \t\ti++;\n \t\treftable_log_record_release(&log);\n \t}\n \n-\tEXPECT(i == N);\n+\tcheck_int(i, ==, N);\n \treftable_iterator_destroy(&it);\n \n \t/* cleanup. */\n@@ -282,7 +289,7 @@ static void test_log_write_read(void)\n \treader_close(&rd);\n }\n \n-static void test_log_zlib_corruption(void)\n+static void t_log_zlib_corruption(void)\n {\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 256,\n@@ -316,13 +323,13 @@ static void test_log_zlib_corruption(void)\n \treftable_writer_set_limits(w, 1, 1);\n \n \terr = reftable_writer_add_log(w, &log);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \tn = reftable_writer_close(w);\n-\tEXPECT(n == 0);\n+\tcheck_int(n, ==, 0);\n \n \tstats = reftable_writer_stats(w);\n-\tEXPECT(stats->log_stats.blocks > 0);\n+\tcheck_int(stats->log_stats.blocks, >, 0);\n \treftable_writer_free(w);\n \tw = NULL;\n \n@@ -332,11 +339,11 @@ static void test_log_zlib_corruption(void)\n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = init_reader(&rd, &source, \"file.log\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_reader_init_log_iterator(&rd, &it);\n \terr = reftable_iterator_seek_log(&it, \"refname\");\n-\tEXPECT(err == REFTABLE_ZLIB_ERROR);\n+\tcheck_int(err, ==, REFTABLE_ZLIB_ERROR);\n \n \treftable_iterator_destroy(&it);\n \n@@ -345,14 +352,14 @@ static void test_log_zlib_corruption(void)\n \treader_close(&rd);\n }\n \n-static void test_table_read_write_sequential(void)\n+static void t_table_read_write_sequential(void)\n {\n \tchar **names;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint N = 50;\n-\tstruct reftable_iterator it = { NULL };\n-\tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_iterator it = { 0 };\n+\tstruct reftable_block_source source = { 0 };\n+\tstruct reftable_reader rd = { 0 };\n \tint err = 0;\n \tint j = 0;\n \n@@ -361,26 +368,25 @@ static void test_table_read_write_sequential(void)\n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = init_reader(&rd, &source, \"file.ref\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_reader_init_ref_iterator(&rd, &it);\n \terr = reftable_iterator_seek_ref(&it, \"\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \twhile (1) {\n-\t\tstruct reftable_ref_record ref = { NULL };\n+\t\tstruct reftable_ref_record ref = { 0 };\n \t\tint r = reftable_iterator_next_ref(&it, &ref);\n-\t\tEXPECT(r >= 0);\n-\t\tif (r > 0) {\n+\t\tcheck_int(r, >=, 0);\n+\t\tif (r > 0)\n \t\t\tbreak;\n-\t\t}\n-\t\tEXPECT(0 == strcmp(names[j], ref.refname));\n-\t\tEXPECT(update_index == ref.update_index);\n+\t\tcheck_str(names[j], ref.refname);\n+\t\tcheck_int(update_index, ==, ref.update_index);\n \n \t\tj++;\n \t\treftable_ref_record_release(&ref);\n \t}\n-\tEXPECT(j == N);\n+\tcheck_int(j, ==, N);\n \treftable_iterator_destroy(&it);\n \tstrbuf_release(&buf);\n \tfree_names(names);\n@@ -388,90 +394,88 @@ static void test_table_read_write_sequential(void)\n \treader_close(&rd);\n }\n \n-static void test_table_write_small_table(void)\n+static void t_table_write_small_table(void)\n {\n \tchar **names;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint N = 1;\n \twrite_table(&names, &buf, N, 4096, GIT_SHA1_FORMAT_ID);\n-\tEXPECT(buf.len < 200);\n+\tcheck_int(buf.len, <, 200);\n \tstrbuf_release(&buf);\n \tfree_names(names);\n }\n \n-static void test_table_read_api(void)\n+static void t_table_read_api(void)\n {\n \tchar **names;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint N = 50;\n-\tstruct reftable_reader rd = { NULL };\n-\tstruct reftable_block_source source = { NULL };\n+\tstruct reftable_reader rd = { 0 };\n+\tstruct reftable_block_source source = { 0 };\n \tint err;\n \tint i;\n-\tstruct reftable_log_record log = { NULL };\n-\tstruct reftable_iterator it = { NULL };\n+\tstruct reftable_log_record log = { 0 };\n+\tstruct reftable_iterator it = { 0 };\n \n \twrite_table(&names, &buf, N, 256, GIT_SHA1_FORMAT_ID);\n \n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = init_reader(&rd, &source, \"file.ref\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_reader_init_ref_iterator(&rd, &it);\n \terr = reftable_iterator_seek_ref(&it, names[0]);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \terr = reftable_iterator_next_log(&it, &log);\n-\tEXPECT(err == REFTABLE_API_ERROR);\n+\tcheck_int(err, ==, REFTABLE_API_ERROR);\n \n \tstrbuf_release(&buf);\n-\tfor (i = 0; i < N; i++) {\n+\tfor (i = 0; i < N; i++)\n \t\treftable_free(names[i]);\n-\t}\n \treftable_iterator_destroy(&it);\n \treftable_free(names);\n \treader_close(&rd);\n \tstrbuf_release(&buf);\n }\n \n-static void test_table_read_write_seek(int index, int hash_id)\n+static void t_table_read_write_seek(int index, int hash_id)\n {\n \tchar **names;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint N = 50;\n-\tstruct reftable_reader rd = { NULL };\n-\tstruct reftable_block_source source = { NULL };\n+\tstruct reftable_reader rd = { 0 };\n+\tstruct reftable_block_source source = { 0 };\n \tint err;\n \tint i = 0;\n \n-\tstruct reftable_iterator it = { NULL };\n+\tstruct reftable_iterator it = { 0 };\n \tstruct strbuf pastLast = STRBUF_INIT;\n-\tstruct reftable_ref_record ref = { NULL };\n+\tstruct reftable_ref_record ref = { 0 };\n \n \twrite_table(&names, &buf, N, 256, hash_id);\n \n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = init_reader(&rd, &source, \"file.ref\");\n-\tEXPECT_ERR(err);\n-\tEXPECT(hash_id == reftable_reader_hash_id(&rd));\n+\tcheck(!err);\n+\tcheck_int(hash_id, ==, reftable_reader_hash_id(&rd));\n \n-\tif (!index) {\n+\tif (!index)\n \t\trd.ref_offsets.index_offset = 0;\n-\t} else {\n-\t\tEXPECT(rd.ref_offsets.index_offset > 0);\n-\t}\n+\telse\n+\t\tcheck_int(rd.ref_offsets.index_offset, >, 0);\n \n \tfor (i = 1; i < N; i++) {\n \t\treftable_reader_init_ref_iterator(&rd, &it);\n \t\terr = reftable_iterator_seek_ref(&it, names[i]);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t\terr = reftable_iterator_next_ref(&it, &ref);\n-\t\tEXPECT_ERR(err);\n-\t\tEXPECT(0 == strcmp(names[i], ref.refname));\n-\t\tEXPECT(REFTABLE_REF_VAL1 == ref.value_type);\n-\t\tEXPECT(i == ref.value.val1[0]);\n+\t\tcheck(!err);\n+\t\tcheck_str(names[i], ref.refname);\n+\t\tcheck_int(REFTABLE_REF_VAL1, ==, ref.value_type);\n+\t\tcheck_int(i, ==, ref.value.val1[0]);\n \n \t\treftable_ref_record_release(&ref);\n \t\treftable_iterator_destroy(&it);\n@@ -483,40 +487,39 @@ static void test_table_read_write_seek(int index, int hash_id)\n \treftable_reader_init_ref_iterator(&rd, &it);\n \terr = reftable_iterator_seek_ref(&it, pastLast.buf);\n \tif (err == 0) {\n-\t\tstruct reftable_ref_record ref = { NULL };\n+\t\tstruct reftable_ref_record ref = { 0 };\n \t\tint err = reftable_iterator_next_ref(&it, &ref);\n-\t\tEXPECT(err > 0);\n+\t\tcheck_int(err, >, 0);\n \t} else {\n-\t\tEXPECT(err > 0);\n+\t\tcheck_int(err, >, 0);\n \t}\n \n \tstrbuf_release(&pastLast);\n \treftable_iterator_destroy(&it);\n \n \tstrbuf_release(&buf);\n-\tfor (i = 0; i < N; i++) {\n+\tfor (i = 0; i < N; i++)\n \t\treftable_free(names[i]);\n-\t}\n \treftable_free(names);\n \treader_close(&rd);\n }\n \n-static void test_table_read_write_seek_linear(void)\n+static void t_table_read_write_seek_linear(void)\n {\n-\ttest_table_read_write_seek(0, GIT_SHA1_FORMAT_ID);\n+\tt_table_read_write_seek(0, GIT_SHA1_FORMAT_ID);\n }\n \n-static void test_table_read_write_seek_linear_sha256(void)\n+static void t_table_read_write_seek_linear_sha256(void)\n {\n-\ttest_table_read_write_seek(0, GIT_SHA256_FORMAT_ID);\n+\tt_table_read_write_seek(0, GIT_SHA256_FORMAT_ID);\n }\n \n-static void test_table_read_write_seek_index(void)\n+static void t_table_read_write_seek_index(void)\n {\n-\ttest_table_read_write_seek(1, GIT_SHA1_FORMAT_ID);\n+\tt_table_read_write_seek(1, GIT_SHA1_FORMAT_ID);\n }\n \n-static void test_table_refs_for(int indexed)\n+static void t_table_refs_for(int indexed)\n {\n \tint N = 50;\n \tchar **want_names = reftable_calloc(N + 1, sizeof(*want_names));\n@@ -526,18 +529,18 @@ static void test_table_refs_for(int indexed)\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 256,\n \t};\n-\tstruct reftable_ref_record ref = { NULL };\n+\tstruct reftable_ref_record ref = { 0 };\n \tint i = 0;\n \tint n;\n \tint err;\n \tstruct reftable_reader rd;\n-\tstruct reftable_block_source source = { NULL };\n+\tstruct reftable_block_source source = { 0 };\n \n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n \t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n \n-\tstruct reftable_iterator it = { NULL };\n+\tstruct reftable_iterator it = { 0 };\n \tint j;\n \n \tset_test_hash(want_hash, 4);\n@@ -546,7 +549,7 @@ static void test_table_refs_for(int indexed)\n \t\tuint8_t hash[GIT_SHA1_RAWSZ];\n \t\tchar fill[51] = { 0 };\n \t\tchar name[100];\n-\t\tstruct reftable_ref_record ref = { NULL };\n+\t\tstruct reftable_ref_record ref = { 0 };\n \n \t\tmemset(hash, i, sizeof(hash));\n \t\tmemset(fill, 'x', 50);\n@@ -563,16 +566,15 @@ static void test_table_refs_for(int indexed)\n \t\t */\n \t\t/* blocks. */\n \t\tn = reftable_writer_add_ref(w, &ref);\n-\t\tEXPECT(n == 0);\n+\t\tcheck_int(n, ==, 0);\n \n \t\tif (!memcmp(ref.value.val2.value, want_hash, GIT_SHA1_RAWSZ) ||\n-\t\t    !memcmp(ref.value.val2.target_value, want_hash, GIT_SHA1_RAWSZ)) {\n+\t\t    !memcmp(ref.value.val2.target_value, want_hash, GIT_SHA1_RAWSZ))\n \t\t\twant_names[want_names_len++] = xstrdup(name);\n-\t\t}\n \t}\n \n \tn = reftable_writer_close(w);\n-\tEXPECT(n == 0);\n+\tcheck_int(n, ==, 0);\n \n \treftable_writer_free(w);\n \tw = NULL;\n@@ -580,33 +582,30 @@ static void test_table_refs_for(int indexed)\n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = init_reader(&rd, &source, \"file.ref\");\n-\tEXPECT_ERR(err);\n-\tif (!indexed) {\n+\tcheck(!err);\n+\tif (!indexed)\n \t\trd.obj_offsets.is_present = 0;\n-\t}\n \n \treftable_reader_init_ref_iterator(&rd, &it);\n \terr = reftable_iterator_seek_ref(&it, \"\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \treftable_iterator_destroy(&it);\n \n \terr = reftable_reader_refs_for(&rd, &it, want_hash);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \tj = 0;\n \twhile (1) {\n \t\tint err = reftable_iterator_next_ref(&it, &ref);\n-\t\tEXPECT(err >= 0);\n-\t\tif (err > 0) {\n+\t\tcheck_int(err, >=, 0);\n+\t\tif (err > 0)\n \t\t\tbreak;\n-\t\t}\n-\n-\t\tEXPECT(j < want_names_len);\n-\t\tEXPECT(0 == strcmp(ref.refname, want_names[j]));\n+\t\tcheck_int(j, <, want_names_len);\n+\t\tcheck_str(ref.refname, want_names[j]);\n \t\tj++;\n \t\treftable_ref_record_release(&ref);\n \t}\n-\tEXPECT(j == want_names_len);\n+\tcheck_int(j, ==, want_names_len);\n \n \tstrbuf_release(&buf);\n \tfree_names(want_names);\n@@ -614,54 +613,54 @@ static void test_table_refs_for(int indexed)\n \treader_close(&rd);\n }\n \n-static void test_table_refs_for_no_index(void)\n+static void t_table_refs_for_no_index(void)\n {\n-\ttest_table_refs_for(0);\n+\tt_table_refs_for(0);\n }\n \n-static void test_table_refs_for_obj_index(void)\n+static void t_table_refs_for_obj_index(void)\n {\n-\ttest_table_refs_for(1);\n+\tt_table_refs_for(1);\n }\n \n-static void test_write_empty_table(void)\n+static void t_write_empty_table(void)\n {\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n \t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n-\tstruct reftable_block_source source = { NULL };\n+\tstruct reftable_block_source source = { 0 };\n \tstruct reftable_reader *rd = NULL;\n-\tstruct reftable_ref_record rec = { NULL };\n-\tstruct reftable_iterator it = { NULL };\n+\tstruct reftable_ref_record rec = { 0 };\n+\tstruct reftable_iterator it = { 0 };\n \tint err;\n \n \treftable_writer_set_limits(w, 1, 1);\n \n \terr = reftable_writer_close(w);\n-\tEXPECT(err == REFTABLE_EMPTY_TABLE_ERROR);\n+\tcheck_int(err, ==, REFTABLE_EMPTY_TABLE_ERROR);\n \treftable_writer_free(w);\n \n-\tEXPECT(buf.len == header_size(1) + footer_size(1));\n+\tcheck_int(buf.len, ==, header_size(1) + footer_size(1));\n \n \tblock_source_from_strbuf(&source, &buf);\n \n \terr = reftable_new_reader(&rd, &source, \"filename\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_reader_init_ref_iterator(rd, &it);\n \terr = reftable_iterator_seek_ref(&it, \"\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \terr = reftable_iterator_next_ref(&it, &rec);\n-\tEXPECT(err > 0);\n+\tcheck_int(err, >, 0);\n \n \treftable_iterator_destroy(&it);\n \treftable_reader_free(rd);\n \tstrbuf_release(&buf);\n }\n \n-static void test_write_object_id_min_length(void)\n+static void t_write_object_id_min_length(void)\n {\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 75,\n@@ -686,17 +685,17 @@ static void test_write_object_id_min_length(void)\n \t\tsnprintf(name, sizeof(name), \"ref%05d\", i);\n \t\tref.refname = name;\n \t\terr = reftable_writer_add_ref(w, &ref);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \n \terr = reftable_writer_close(w);\n-\tEXPECT_ERR(err);\n-\tEXPECT(reftable_writer_stats(w)->object_id_len == 2);\n+\tcheck(!err);\n+\tcheck_int(reftable_writer_stats(w)->object_id_len, ==, 2);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n }\n \n-static void test_write_object_id_length(void)\n+static void t_write_object_id_length(void)\n {\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 75,\n@@ -722,17 +721,17 @@ static void test_write_object_id_length(void)\n \t\tref.refname = name;\n \t\tref.value.val1[15] = i;\n \t\terr = reftable_writer_add_ref(w, &ref);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \n \terr = reftable_writer_close(w);\n-\tEXPECT_ERR(err);\n-\tEXPECT(reftable_writer_stats(w)->object_id_len == 16);\n+\tcheck(!err);\n+\tcheck_int(reftable_writer_stats(w)->object_id_len, ==, 16);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n }\n \n-static void test_write_empty_key(void)\n+static void t_write_empty_key(void)\n {\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -747,15 +746,15 @@ static void test_write_empty_key(void)\n \n \treftable_writer_set_limits(w, 1, 1);\n \terr = reftable_writer_add_ref(w, &ref);\n-\tEXPECT(err == REFTABLE_API_ERROR);\n+\tcheck_int(err, ==, REFTABLE_API_ERROR);\n \n \terr = reftable_writer_close(w);\n-\tEXPECT(err == REFTABLE_EMPTY_TABLE_ERROR);\n+\tcheck_int(err, ==, REFTABLE_EMPTY_TABLE_ERROR);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n }\n \n-static void test_write_key_order(void)\n+static void t_write_key_order(void)\n {\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -782,15 +781,15 @@ static void test_write_key_order(void)\n \n \treftable_writer_set_limits(w, 1, 1);\n \terr = reftable_writer_add_ref(w, &refs[0]);\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \terr = reftable_writer_add_ref(w, &refs[1]);\n-\tEXPECT(err == REFTABLE_API_ERROR);\n+\tcheck_int(err, ==, REFTABLE_API_ERROR);\n \treftable_writer_close(w);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n }\n \n-static void test_write_multiple_indices(void)\n+static void t_write_multiple_indices(void)\n {\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 100,\n@@ -817,7 +816,7 @@ static void test_write_multiple_indices(void)\n \t\tref.refname = buf.buf,\n \n \t\terr = reftable_writer_add_ref(writer, &ref);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \n \tfor (i = 0; i < 100; i++) {\n@@ -835,7 +834,7 @@ static void test_write_multiple_indices(void)\n \t\tlog.refname = buf.buf,\n \n \t\terr = reftable_writer_add_log(writer, &log);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \n \treftable_writer_close(writer);\n@@ -845,13 +844,13 @@ static void test_write_multiple_indices(void)\n \t * for each of the block types.\n \t */\n \tstats = reftable_writer_stats(writer);\n-\tEXPECT(stats->ref_stats.index_offset > 0);\n-\tEXPECT(stats->obj_stats.index_offset > 0);\n-\tEXPECT(stats->log_stats.index_offset > 0);\n+\tcheck_int(stats->ref_stats.index_offset, >, 0);\n+\tcheck_int(stats->obj_stats.index_offset, >, 0);\n+\tcheck_int(stats->log_stats.index_offset, >, 0);\n \n \tblock_source_from_strbuf(&source, &writer_buf);\n \terr = reftable_new_reader(&reader, &source, \"filename\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \t/*\n \t * Seeking the log uses the log index now. In case there is any\n@@ -859,7 +858,7 @@ static void test_write_multiple_indices(void)\n \t */\n \treftable_reader_init_log_iterator(reader, &it);\n \terr = reftable_iterator_seek_log(&it, \"\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_iterator_destroy(&it);\n \treftable_writer_free(writer);\n@@ -868,7 +867,7 @@ static void test_write_multiple_indices(void)\n \tstrbuf_release(&buf);\n }\n \n-static void test_write_multi_level_index(void)\n+static void t_write_multi_level_index(void)\n {\n \tstruct reftable_write_options opts = {\n \t\t.block_size = 100,\n@@ -895,7 +894,7 @@ static void test_write_multi_level_index(void)\n \t\tref.refname = buf.buf,\n \n \t\terr = reftable_writer_add_ref(writer, &ref);\n-\t\tEXPECT_ERR(err);\n+\t\tcheck(!err);\n \t}\n \treftable_writer_close(writer);\n \n@@ -904,18 +903,18 @@ static void test_write_multi_level_index(void)\n \t * multi-level index.\n \t */\n \tstats = reftable_writer_stats(writer);\n-\tEXPECT(stats->ref_stats.max_index_level == 2);\n+\tcheck_int(stats->ref_stats.max_index_level, ==, 2);\n \n \tblock_source_from_strbuf(&source, &writer_buf);\n \terr = reftable_new_reader(&reader, &source, \"filename\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \t/*\n \t * Seeking the last ref should work as expected.\n \t */\n \treftable_reader_init_ref_iterator(reader, &it);\n \terr = reftable_iterator_seek_ref(&it, \"refs/heads/199\");\n-\tEXPECT_ERR(err);\n+\tcheck(!err);\n \n \treftable_iterator_destroy(&it);\n \treftable_writer_free(writer);\n@@ -924,56 +923,57 @@ static void test_write_multi_level_index(void)\n \tstrbuf_release(&buf);\n }\n \n-static void test_corrupt_table_empty(void)\n+static void t_corrupt_table_empty(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_block_source source = { 0 };\n+\tstruct reftable_reader rd = { 0 };\n \tint err;\n \n \tblock_source_from_strbuf(&source, &buf);\n \terr = init_reader(&rd, &source, \"file.log\");\n-\tEXPECT(err == REFTABLE_FORMAT_ERROR);\n+\tcheck_int(err, ==, REFTABLE_FORMAT_ERROR);\n }\n \n-static void test_corrupt_table(void)\n+static void t_corrupt_table(void)\n {\n \tuint8_t zeros[1024] = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_block_source source = { 0 };\n+\tstruct reftable_reader rd = { 0 };\n \tint err;\n \tstrbuf_add(&buf, zeros, sizeof(zeros));\n \n \tblock_source_from_strbuf(&source, &buf);\n \terr = init_reader(&rd, &source, \"file.log\");\n-\tEXPECT(err == REFTABLE_FORMAT_ERROR);\n+\tcheck_int(err, ==, REFTABLE_FORMAT_ERROR);\n \tstrbuf_release(&buf);\n }\n \n-int readwrite_test_main(int argc, const char *argv[])\n+int cmd_main(int argc, const char *argv[])\n {\n-\tRUN_TEST(test_log_zlib_corruption);\n-\tRUN_TEST(test_corrupt_table);\n-\tRUN_TEST(test_corrupt_table_empty);\n-\tRUN_TEST(test_log_write_read);\n-\tRUN_TEST(test_write_key_order);\n-\tRUN_TEST(test_table_read_write_seek_linear_sha256);\n-\tRUN_TEST(test_log_buffer_size);\n-\tRUN_TEST(test_table_write_small_table);\n-\tRUN_TEST(test_buffer);\n-\tRUN_TEST(test_table_read_api);\n-\tRUN_TEST(test_table_read_write_sequential);\n-\tRUN_TEST(test_table_read_write_seek_linear);\n-\tRUN_TEST(test_table_read_write_seek_index);\n-\tRUN_TEST(test_table_refs_for_no_index);\n-\tRUN_TEST(test_table_refs_for_obj_index);\n-\tRUN_TEST(test_write_empty_key);\n-\tRUN_TEST(test_write_empty_table);\n-\tRUN_TEST(test_log_overflow);\n-\tRUN_TEST(test_write_object_id_length);\n-\tRUN_TEST(test_write_object_id_min_length);\n-\tRUN_TEST(test_write_multiple_indices);\n-\tRUN_TEST(test_write_multi_level_index);\n-\treturn 0;\n+\tTEST(t_buffer(), \"strbuf works as blocksource\");\n+\tTEST(t_corrupt_table(), \"read-write on corrupted table\");\n+\tTEST(t_corrupt_table_empty(), \"read-write on an empty table\");\n+\tTEST(t_log_buffer_size(), \"buffer extension for log compression\");\n+\tTEST(t_log_overflow(), \"log overflow returns expected error\");\n+\tTEST(t_log_write_read(), \"read-write on log records\");\n+\tTEST(t_log_zlib_corruption(), \"reading corrupted log record returns expected error\");\n+\tTEST(t_table_read_api(), \"read on a table\");\n+\tTEST(t_table_read_write_seek_index(), \"read-write on a table with index\");\n+\tTEST(t_table_read_write_seek_linear(), \"read-write on a table without index (SHA1)\");\n+\tTEST(t_table_read_write_seek_linear_sha256(), \"read-write on a table without index (SHA256)\");\n+\tTEST(t_table_read_write_sequential(), \"sequential read-write on a table\");\n+\tTEST(t_table_refs_for_no_index(), \"refs-only table with no index\");\n+\tTEST(t_table_refs_for_obj_index(), \"refs-only table with index\");\n+\tTEST(t_table_write_small_table(), \"write_table works\");\n+\tTEST(t_write_empty_key(), \"write on refs with empty keys\");\n+\tTEST(t_write_empty_table(), \"read-write on empty tables\");\n+\tTEST(t_write_key_order(), \"refs must be written in increasing order\");\n+\tTEST(t_write_multi_level_index(), \"table with multi-level index\");\n+\tTEST(t_write_multiple_indices(), \"table with indices for multiple block types\");\n+\tTEST(t_write_object_id_length(), \"prefix compression on writing refs\");\n+\tTEST(t_write_object_id_min_length(), \"prefix compression on writing refs\");\n+\n+\treturn test_done();\n }\n-- \n2.45.GIT\n\n"},{"id":"500788","messageId":"20240813144440.4602-3-chandrapratap3519@gmail.com","threadId":"61916","inReplyTo":"20240813144440.4602-1-chandrapratap3519@gmail.com","subject":"[PATCH v3 2/4] t-reftable-readwrite: use free_names() instead of a for loop","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-13T14:34:48Z","receivedAt":"2024-08-13T14:45:08Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"free_names() as defined by reftable/basics.{c,h} frees a NULL\nterminated array of malloced strings along with the array itself.\nUse this function instead of a for loop to free such an array.\n\nMentored-by: Patrick Steinhardt <ps@pks.im>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n---\n t/unit-tests/t-reftable-readwrite.c | 10 +++-------\n 1 file changed, 3 insertions(+), 7 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\nindex d0eb85fc38..8e546b0dd6 100644\n--- a/t/unit-tests/t-reftable-readwrite.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -7,6 +7,7 @@ license that can be found in the LICENSE file or at\n */\n \n #include \"test-lib.h\"\n+#include \"reftable/basics.h\"\n #include \"reftable/blocksource.h\"\n #include \"reftable/reader.h\"\n #include \"reftable/reftable-error.h\"\n@@ -413,7 +414,6 @@ static void t_table_read_api(void)\n \tstruct reftable_reader rd = { 0 };\n \tstruct reftable_block_source source = { 0 };\n \tint err;\n-\tint i;\n \tstruct reftable_log_record log = { 0 };\n \tstruct reftable_iterator it = { 0 };\n \n@@ -432,10 +432,8 @@ static void t_table_read_api(void)\n \tcheck_int(err, ==, REFTABLE_API_ERROR);\n \n \tstrbuf_release(&buf);\n-\tfor (i = 0; i < N; i++)\n-\t\treftable_free(names[i]);\n+\tfree_names(names);\n \treftable_iterator_destroy(&it);\n-\treftable_free(names);\n \treader_close(&rd);\n \tstrbuf_release(&buf);\n }\n@@ -498,9 +496,7 @@ static void t_table_read_write_seek(int index, int hash_id)\n \treftable_iterator_destroy(&it);\n \n \tstrbuf_release(&buf);\n-\tfor (i = 0; i < N; i++)\n-\t\treftable_free(names[i]);\n-\treftable_free(names);\n+\tfree_names(names);\n \treader_close(&rd);\n }\n \n-- \n2.45.GIT\n\n"},{"id":"500789","messageId":"20240813144440.4602-4-chandrapratap3519@gmail.com","threadId":"61916","inReplyTo":"20240813144440.4602-1-chandrapratap3519@gmail.com","subject":"[PATCH v3 3/4] t-reftable-readwrite: use 'for' in place of infinite 'while' loops","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-13T14:34:49Z","receivedAt":"2024-08-13T14:45:10Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"Using a for loop with an empty conditional statement is more concise\nand easier to read than an infinite 'while' loop in instances\nwhere we need a loop variable. Hence, replace such instances of a\n'while' loop with the equivalent 'for' loop.\n\nMentored-by: Patrick Steinhardt <ps@pks.im>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n---\n t/unit-tests/t-reftable-readwrite.c | 12 +++---------\n 1 file changed, 3 insertions(+), 9 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\nindex 8e546b0dd6..9a05dde9d6 100644\n--- a/t/unit-tests/t-reftable-readwrite.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -269,15 +269,13 @@ static void t_log_write_read(void)\n \terr = reftable_iterator_seek_log(&it, \"\");\n \tcheck(!err);\n \n-\ti = 0;\n-\twhile (1) {\n+\tfor (i = 0; ; i++) {\n \t\tint err = reftable_iterator_next_log(&it, &log);\n \t\tif (err > 0)\n \t\t\tbreak;\n \t\tcheck(!err);\n \t\tcheck_str(names[i], log.refname);\n \t\tcheck_int(i, ==, log.update_index);\n-\t\ti++;\n \t\treftable_log_record_release(&log);\n \t}\n \n@@ -375,7 +373,7 @@ static void t_table_read_write_sequential(void)\n \terr = reftable_iterator_seek_ref(&it, \"\");\n \tcheck(!err);\n \n-\twhile (1) {\n+\tfor (j = 0; ; j++) {\n \t\tstruct reftable_ref_record ref = { 0 };\n \t\tint r = reftable_iterator_next_ref(&it, &ref);\n \t\tcheck_int(r, >=, 0);\n@@ -383,8 +381,6 @@ static void t_table_read_write_sequential(void)\n \t\t\tbreak;\n \t\tcheck_str(names[j], ref.refname);\n \t\tcheck_int(update_index, ==, ref.update_index);\n-\n-\t\tj++;\n \t\treftable_ref_record_release(&ref);\n \t}\n \tcheck_int(j, ==, N);\n@@ -590,15 +586,13 @@ static void t_table_refs_for(int indexed)\n \terr = reftable_reader_refs_for(&rd, &it, want_hash);\n \tcheck(!err);\n \n-\tj = 0;\n-\twhile (1) {\n+\tfor (j = 0; ; j++) {\n \t\tint err = reftable_iterator_next_ref(&it, &ref);\n \t\tcheck_int(err, >=, 0);\n \t\tif (err > 0)\n \t\t\tbreak;\n \t\tcheck_int(j, <, want_names_len);\n \t\tcheck_str(ref.refname, want_names[j]);\n-\t\tj++;\n \t\treftable_ref_record_release(&ref);\n \t}\n \tcheck_int(j, ==, want_names_len);\n-- \n2.45.GIT\n\n"},{"id":"500790","messageId":"20240813144440.4602-5-chandrapratap3519@gmail.com","threadId":"61916","inReplyTo":"20240813144440.4602-1-chandrapratap3519@gmail.com","subject":"[PATCH v3 4/4] t-reftable-readwrite: add test for known error","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-13T14:34:50Z","receivedAt":"2024-08-13T14:45:13Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"When using reftable_writer_add_ref() to add a ref record to a\nreftable writer, The update_index of the ref record must be within\nthe limits set by reftable_writer_set_limits(), or REFTABLE_API_ERROR\nis returned. This scenario is currently left untested. Add a test\ncase for the same.\n\nMentored-by: Patrick Steinhardt <ps@pks.im>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n---\n t/unit-tests/t-reftable-readwrite.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\nindex 9a05dde9d6..2ce56a0523 100644\n--- a/t/unit-tests/t-reftable-readwrite.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -774,6 +774,11 @@ static void t_write_key_order(void)\n \tcheck(!err);\n \terr = reftable_writer_add_ref(w, &refs[1]);\n \tcheck_int(err, ==, REFTABLE_API_ERROR);\n+\n+\trefs[0].update_index = 2;\n+\terr = reftable_writer_add_ref(w, &refs[0]);\n+\tcheck_int(err, ==, REFTABLE_API_ERROR);\n+\n \treftable_writer_close(w);\n \treftable_writer_free(w);\n \tstrbuf_release(&buf);\n-- \n2.45.GIT\n\n"},{"id":"500806","messageId":"xmqqsev85oxu.fsf@gitster.g","threadId":"61916","inReplyTo":"20240813144440.4602-1-chandrapratap3519@gmail.com","subject":"Re: [GSoC][PATCH v3 0/4] t: port reftable/readwrite_test.c to the unit testing framework","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-13T17:10:21Z","receivedAt":"2024-08-13T17:10:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chandra Pratap <chandrapratap3519@gmail.com> writes:\n\n> Changes in v3:\n> - Order the header files alphabetically in\n>   t/unit-tests/t-reftable-readwrite.c in patch 1.\n> - use check_char() and check_int() instead of check() at two\n>   instances in patch 1.\n> - #include 'reftable/basics.h' in patch 2 when introducing\n>   free_names() in the test file.\n>\n> CI/PR: https://github.com/gitgitgadget/git/pull/1770\n>\n> Chandra Pratap(4):\n> t: move reftable/readwrite_test.c to the unit testing framework\n> t-reftable-readwrite: use free_names() instead of a for loop\n> t-reftable-readwrite: use 'for' in place of infinite 'while' loops\n> t-reftable-readwrite: add test for known error\n\nWill replace.\n\nLet's see if we get any more comments and otherwise let me mark the\ntopic for 'next' soonish.\n\nThanks.\n"},{"id":"500824","messageId":"2rxxfpzijfmvo65xournnmx4oawzqlhgipje4cxzxvo5aqzt6u@xppoikj262cp","threadId":"61916","inReplyTo":"20240813144440.4602-2-chandrapratap3519@gmail.com","subject":"Re: [PATCH v3 1/4] t: move reftable/readwrite_test.c to the unit testing framework","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-08-13T22:33:04Z","receivedAt":"2024-08-13T22:33:11Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.08.13 20:04, Chandra Pratap wrote:\n> reftable/readwrite_test.c exercises the functions defined in\n> reftable/reader.{c,h} and reftable/writer.{c,h}. Migrate\n> reftable/readwrite_test.c to the unit testing framework. Migration\n> involves refactoring the tests to use the unit testing framework\n> instead of reftable's test framework and renaming the tests to\n> align with unit-tests' naming conventions.\n> \n> Since some tests in reftable/readwrite_test.c use the functions\n> set_test_hash(), noop_flush() and strbuf_add_void() defined in\n> reftable/test_framework.{c,h} but these files are not #included\n> in the ported unit test, copy these functions in the new test file.\n\nI'm assuming that eventually, reftable/test_framework (and all the rest\nof reftable/libreftable_test.a) will be removed after all the tests are\nconverted to the unit test framework, is that correct? Will other tests\nneed these test_framework functions? If so, I'd rather not end up with\nduplicates in each test file, even if these are small functions. Is\nthere a reason why we can't link the reftable/test_framework object (or\nthe whole reftable/libreftable_test.a library)?\n"},{"id":"500874","messageId":"CA+J6zkSAa4ejLtTqNob_JyS8wwx+BX0vsgsWU9mekWcNtVY80g@mail.gmail.com","threadId":"61916","inReplyTo":"2rxxfpzijfmvo65xournnmx4oawzqlhgipje4cxzxvo5aqzt6u@xppoikj262cp","subject":"Re: [PATCH v3 1/4] t: move reftable/readwrite_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-14T11:48:33Z","receivedAt":"2024-08-14T11:49:01Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"On Wed, 14 Aug 2024 at 04:03, Josh Steadmon <steadmon@google.com> wrote:\n>\n> On 2024.08.13 20:04, Chandra Pratap wrote:\n> > reftable/readwrite_test.c exercises the functions defined in\n> > reftable/reader.{c,h} and reftable/writer.{c,h}. Migrate\n> > reftable/readwrite_test.c to the unit testing framework. Migration\n> > involves refactoring the tests to use the unit testing framework\n> > instead of reftable's test framework and renaming the tests to\n> > align with unit-tests' naming conventions.\n> >\n> > Since some tests in reftable/readwrite_test.c use the functions\n> > set_test_hash(), noop_flush() and strbuf_add_void() defined in\n> > reftable/test_framework.{c,h} but these files are not #included\n> > in the ported unit test, copy these functions in the new test file.\n>\n> I'm assuming that eventually, reftable/test_framework (and all the rest\n> of reftable/libreftable_test.a) will be removed after all the tests are\n> converted to the unit test framework, is that correct?\n\nThat hasn't been discussed yet but yes, that seems the most likely\nfate for reftable/test_framework.{c,h}.\n\n> Will other tests need these test_framework functions? If so, I'd rather\n> not end up with duplicates in each test file, even if these are small\n> functions. Is there a reason why we can't link the reftable/test_framework\n> object (or the whole reftable/libreftable_test.a library)?\n\nIf I remember correctly, only stack and merged tests besides readwrite\nutilize these functions.\n\nWe're not #including 'test_framwork.h' in the new test files because in\na way, the point of this GSoC project is to get rid of\nreftable/test_framework.{c,h} so we don't need to carry an entirely\ndifferent testing framework for the reftable sub-project.\n\nAs far as duplicated functions are concerned, we can maybe move\nthem to unit-tests/test-lib.{c,h} or a new file in reftable/. I'll create\na patch for it sometimes later.\n"},{"id":"500890","messageId":"ZrysVg04x_uIdNio@tanuki","threadId":"61916","inReplyTo":"2rxxfpzijfmvo65xournnmx4oawzqlhgipje4cxzxvo5aqzt6u@xppoikj262cp","subject":"Re: [PATCH v3 1/4] t: move reftable/readwrite_test.c to the unit testing framework","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-14T13:08:38Z","receivedAt":"2024-08-14T13:08:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Aug 13, 2024 at 03:33:04PM -0700, Josh Steadmon wrote:\n> On 2024.08.13 20:04, Chandra Pratap wrote:\n> > reftable/readwrite_test.c exercises the functions defined in\n> > reftable/reader.{c,h} and reftable/writer.{c,h}. Migrate\n> > reftable/readwrite_test.c to the unit testing framework. Migration\n> > involves refactoring the tests to use the unit testing framework\n> > instead of reftable's test framework and renaming the tests to\n> > align with unit-tests' naming conventions.\n> > \n> > Since some tests in reftable/readwrite_test.c use the functions\n> > set_test_hash(), noop_flush() and strbuf_add_void() defined in\n> > reftable/test_framework.{c,h} but these files are not #included\n> > in the ported unit test, copy these functions in the new test file.\n> \n> I'm assuming that eventually, reftable/test_framework (and all the rest\n> of reftable/libreftable_test.a) will be removed after all the tests are\n> converted to the unit test framework, is that correct? Will other tests\n> need these test_framework functions? If so, I'd rather not end up with\n> duplicates in each test file, even if these are small functions. Is\n> there a reason why we can't link the reftable/test_framework object (or\n> the whole reftable/libreftable_test.a library)?\n\nThe reason is likely that they use different infra, e.g. `EXPECT()` vs\n`check()`. So instead of linking `libreftable_test.a`, I think it is\nfine to duplicate the functionality in `t/unit-tests`. In not too\ndistant of a future we're going to get rid of everything in the reftable\ntests anyway, including the `libreftable_test.a` library. So avoiding\nthe duplication doesn't make a ton of sense to me.\n\nThat being said, I think we should not duplicate functionality in\n`t/unit-tests`. So if there is functionality used by multiple tests, we\nshould likely move it into a new `t/unit-tests/lib-reftable.c` file.\n\nPatrick\n"}]}