{"thread":{"id":"61614","subject":"[GSoC][PATCH 0/4] t: port reftable/tree_test.c to the unit testing framework","startedAt":"2024-06-10T13:11:14Z","lastAt":"2024-08-06T15:35:40Z","messageCount":66,"participants":["Chandra Pratap","Patrick Steinhardt","Junio C Hamano","Karthik Nayak","Justin Tobler"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"496766","messageId":"20240610131017.8321-1-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":null,"subject":"[GSoC][PATCH 0/4] t: port reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-10T13:01:27Z","receivedAt":"2024-06-10T13:11:14Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit\ntesting framework written entirely in C was introduced to the Git project\naimed at simplifying testing and reducing test run times.\nCurrently, tests for the reftable refs-backend are performed by a custom\ntesting framework defined by reftable/test_framework.{c, h}. Port\nreftable/tree_test.c to the unit testing framework and improve upon\nthe ported test.\n\nThe first patch in the series is preparatory cleanup, the second patch\nmoves the test to the unit testing framework, and the rest of the patches\nimprove upon the ported test.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n\n---\nCI/PR: https://github.com/gitgitgadget/git/pull/1740\n\nChandra Pratap (4):\nreftable: remove unnecessary curly braces in\nt: move reftable/tree_test.c to the unit testing\nt-reftable-tree: split test_tree() into two sub-test\nt-reftable-tree: add test for non-existent key\n\nMakefile | 2 +-\nreftable/tree.c | 15 +++--------\nreftable/tree_test.c | 60 -----------------------------------\nt/helper/test-reftable.c | 1 -\nt/unit-tests/t-reftable-tree.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++\n5 files changed, 79 insertions(+), 72 deletions(-)\n"},{"id":"496767","messageId":"20240610131017.8321-2-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240610131017.8321-1-chandrapratap3519@gmail.com","subject":"[PATCH 1/4] reftable: remove unnecessary curly braces in reftable/tree.c","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-10T13:01:28Z","receivedAt":"2024-06-10T13:11:16Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"According to Documentation/CodingGuidelines, single-line control-flow\nstatements must omit curly braces (except for some special cases).\nMake reftable/tree.c adhere to this guideline.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n reftable/tree.c | 15 +++++----------\n 1 file changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/reftable/tree.c b/reftable/tree.c\nindex 528f33ae38..5ffb2e0d69 100644\n--- a/reftable/tree.c\n+++ b/reftable/tree.c\n@@ -39,25 +39,20 @@ struct tree_node *tree_search(void *key, struct tree_node **rootp,\n void infix_walk(struct tree_node *t, void (*action)(void *arg, void *key),\n \t\tvoid *arg)\n {\n-\tif (t->left) {\n+\tif (t->left)\n \t\tinfix_walk(t->left, action, arg);\n-\t}\n \taction(arg, t->key);\n-\tif (t->right) {\n+\tif (t->right)\n \t\tinfix_walk(t->right, action, arg);\n-\t}\n }\n \n void tree_free(struct tree_node *t)\n {\n-\tif (!t) {\n+\tif (!t)\n \t\treturn;\n-\t}\n-\tif (t->left) {\n+\tif (t->left)\n \t\ttree_free(t->left);\n-\t}\n-\tif (t->right) {\n+\tif (t->right)\n \t\ttree_free(t->right);\n-\t}\n \treftable_free(t);\n }\n-- \n2.45.2.404.g9eaef5822c\n\n"},{"id":"496768","messageId":"20240610131017.8321-3-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240610131017.8321-1-chandrapratap3519@gmail.com","subject":"[PATCH 2/4] t: move reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-10T13:01:29Z","receivedAt":"2024-06-10T13:11:19Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"reftable/tree_test.c exercises the functions defined in\nreftable/tree.{c, h}. Migrate reftable/tree_test.c to the\nunit testing framework. Migration involves refactoring the tests\nto use the unit testing framework instead of reftable's test\nframework.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n Makefile | 2 +-\n t/helper/test-reftable.c | 1 -\n .../unit-tests/t-reftable-tree.c | 32 ++++++++-----------\n 3 files changed, 15 insertions(+), 20 deletions(-)\n rename reftable/tree_test.c => t/unit-tests/t-reftable-tree.c (59%)\n\ndiff --git a/Makefile b/Makefile\nindex 2f5f16847a..d736b2f8bd 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1336,6 +1336,7 @@ THIRD_PARTY_SOURCES += sha1dc/%\n UNIT_TEST_PROGRAMS += t-ctype\n UNIT_TEST_PROGRAMS += t-mem-pool\n UNIT_TEST_PROGRAMS += t-prio-queue\n+UNIT_TEST_PROGRAMS += t-reftable-tree\n UNIT_TEST_PROGRAMS += t-strbuf\n UNIT_TEST_PROGRAMS += t-strcmp-offset\n UNIT_TEST_PROGRAMS += t-strvec\n@@ -2681,7 +2682,6 @@ REFTABLE_TEST_OBJS += reftable/record_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\n \n TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))\n \ndiff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c\nindex bae731669c..9475db2f76 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 \tbasics_test_main(argc, argv);\n \trecord_test_main(argc, 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 \tmerged_test_main(argc, argv);\ndiff --git a/reftable/tree_test.c b/t/unit-tests/t-reftable-tree.c\nsimilarity index 59%\nrename from reftable/tree_test.c\nrename to t/unit-tests/t-reftable-tree.c\nindex 6961a657ad..208e7b7874 100644\n--- a/reftable/tree_test.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -6,11 +6,8 @@ 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-#include \"tree.h\"\n-\n-#include \"test_framework.h\"\n-#include \"reftable-tests.h\"\n+#include \"test-lib.h\"\n+#include \"reftable/tree.h\"\n \n static int test_compare(const void *a, const void *b)\n {\n@@ -24,37 +21,36 @@ struct curry {\n static void check_increasing(void *arg, void *key)\n {\n \tstruct curry *c = arg;\n-\tif (c->last) {\n-\t\tEXPECT(test_compare(c->last, key) < 0);\n-\t}\n+\tif (c->last)\n+\t\tcheck_int(test_compare(c->last, key), <, 0);\n \tc->last = key;\n }\n \n static void test_tree(void)\n {\n \tstruct tree_node *root = NULL;\n+\tvoid *values[11] = { 0 };\n+\tstruct tree_node *nodes[11] = { 0 };\n+\tsize_t i = 1;\n+\tstruct curry c = { 0 };\n \n-\tvoid *values[11] = { NULL };\n-\tstruct tree_node *nodes[11] = { NULL };\n-\tint i = 1;\n-\tstruct curry c = { NULL };\n \tdo {\n \t\tnodes[i] = tree_search(values + i, &root, &test_compare, 1);\n \t\ti = (i * 7) % 11;\n \t} while (i != 1);\n \n \tfor (i = 1; i < ARRAY_SIZE(nodes); i++) {\n-\t\tEXPECT(values + i == nodes[i]->key);\n-\t\tEXPECT(nodes[i] ==\n-\t\t tree_search(values + i, &root, &test_compare, 0));\n+\t\tcheck_pointer_eq(values + i, nodes[i]->key);\n+\t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));\n \t}\n \n \tinfix_walk(root, check_increasing, &c);\n \ttree_free(root);\n }\n \n-int tree_test_main(int argc, const char *argv[])\n+int cmd_main(int argc, const char *argv[])\n {\n-\tRUN_TEST(test_tree);\n-\treturn 0;\n+\tTEST(test_tree(), \"tree_search and infix_walk work\");\n+\n+\treturn test_done();\n }\n-- \n2.45.2.404.g9eaef5822c\n\n"},{"id":"496769","messageId":"20240610131017.8321-4-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240610131017.8321-1-chandrapratap3519@gmail.com","subject":"[PATCH 3/4] t-reftable-tree: split test_tree() into two sub-test functions","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-10T13:01:30Z","receivedAt":"2024-06-10T13:11:22Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup, tests for both tree_search() and\ninfix_walk() defined by reftable/tree.{c, h} are performed by\na single test function, test_tree(). Split tree_test() into\ntest_tree_search() and test_infix_walk() responsible for\nindependently testing tree_search() and infix_walk() respectively.\nThis improves the overall readability of the test file as well as\nsimplifies debugging.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 22 +++++++++++++++++++---\n 1 file changed, 19 insertions(+), 3 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex 208e7b7874..78d5caafbe 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -26,7 +26,7 @@ static void check_increasing(void *arg, void *key)\n \tc->last = key;\n }\n \n-static void test_tree(void)\n+static void test_tree_search(void)\n {\n \tstruct tree_node *root = NULL;\n \tvoid *values[11] = { 0 };\n@@ -44,13 +44,29 @@ static void test_tree(void)\n \t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));\n \t}\n \n-\tinfix_walk(root, check_increasing, &c);\n+\ttree_free(root);\n+}\n+\n+static void test_infix_walk(void)\n+{\n+\tstruct tree_node *root = NULL;\n+\tvoid *values[13] = { 0 };\n+\tstruct curry c = { 0 };\n+\tsize_t i = 1;\n+\n+\tdo {\n+\t\ttree_search(values + i, &root, &test_compare, 1);\n+\t\ti = (i * 5) % 13;\n+\t} while (i != 1);\n+\n+\tinfix_walk(root, &check_increasing, &c);\n \ttree_free(root);\n }\n \n int cmd_main(int argc, const char *argv[])\n {\n-\tTEST(test_tree(), \"tree_search and infix_walk work\");\n+\tTEST(test_tree_search(), \"tree_search works\");\n+\tTEST(test_infix_walk(), \"infix_walk works\");\n \n \treturn test_done();\n }\n-- \n2.45.2.404.g9eaef5822c\n\n"},{"id":"496770","messageId":"20240610131017.8321-5-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240610131017.8321-1-chandrapratap3519@gmail.com","subject":"[PATCH 4/4] t-reftable-tree: add test for non-existent key","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-10T13:01:31Z","receivedAt":"2024-06-10T13:11:25Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup for tree_search(), the case for\nnon-existent key is not exercised. Improve this by adding a\ntest-case for the same.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex 78d5caafbe..b2fca0cb5e 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -44,6 +44,7 @@ static void test_tree_search(void)\n \t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));\n \t}\n \n+\tcheck(!tree_search(values, &root, &test_compare, 0));\n \ttree_free(root);\n }\n \n-- \n2.45.2.404.g9eaef5822c\n\n"},{"id":"496772","messageId":"ZmcEaFxZhpyrFd-b@tanuki","threadId":"61614","inReplyTo":"20240610131017.8321-4-chandrapratap3519@gmail.com","subject":"Re: [PATCH 3/4] t-reftable-tree: split test_tree() into two sub-test functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-10T13:49:28Z","receivedAt":"2024-06-10T13:49:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jun 10, 2024 at 06:31:30PM +0530, Chandra Pratap wrote:\n> @@ -44,13 +44,29 @@ static void test_tree(void)\n> \t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));\n> \t}\n> \n> -\tinfix_walk(root, check_increasing, &c);\n> +\ttree_free(root);\n> +}\n> +\n> +static void test_infix_walk(void)\n> +{\n> +\tstruct tree_node *root = NULL;\n> +\tvoid *values[13] = { 0 };\n\nIs there a reason why we have 13 values here while we had 11 values in\nthe test this was split out from?\n\n> +\tstruct curry c = { 0 };\n> +\tsize_t i = 1;\n> +\n> +\tdo {\n> +\t\ttree_search(values + i, &root, &test_compare, 1);\n> +\t\ti = (i * 5) % 13;\n> +\t} while (i != 1);\n\nIt's completely non-obvious that `tree_search()` ends up _inserting_\nnodes into the tree when the entry we're searching for wasn't found (and\nif the last parameter is `1`. I feel like this interface could really\nuse a complete makeover and split up its concerns. In any case, that\ndoes not need to happen as part of this patch seriesr\n\nWhat I think would help though is if the commit message itself mentioned\nthis unorthodox way of inserting values into the tree.\n\n> +\tinfix_walk(root, &check_increasing, &c);\n\nNot a fault of this commit, but this test certainly isn't great. It\nwould succeed even if `infix_walk()` didn't do anything as we do not\nverify at all whether all nodes have been traversed (and traversed once,\nexactly).\n\nPatrick\n"},{"id":"496832","messageId":"CA+J6zkS8zkyienEDm9m1Z6bEBzbPzC_Lo5gvy03vFfzTHhLFjQ@mail.gmail.com","threadId":"61614","inReplyTo":"ZmcEaFxZhpyrFd-b@tanuki","subject":"Re: [PATCH 3/4] t-reftable-tree: split test_tree() into two sub-test functions","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-11T06:44:14Z","receivedAt":"2024-06-11T06:44:27Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"On Mon, 10 Jun 2024 at 19:19, Patrick Steinhardt wrote:\n>\n> On Mon, Jun 10, 2024 at 06:31:30PM +0530, Chandra Pratap wrote:\n> > @@ -44,13 +44,29 @@ static void test_tree(void)\n> > check_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));\n> > }\n> >\n> > - infix_walk(root, check_increasing, &c);\n> > + tree_free(root);\n> > +}\n> > +\n> > +static void test_infix_walk(void)\n> > +{\n> > + struct tree_node *root = NULL;\n> > + void *values[13] = { 0 };\n>\n> Is there a reason why we have 13 values here while we had 11 values in\n> the test this was split out from?\n\nI did that to introduce some variety between the test cases, but now that you\nmention it, this change doesn't go well with the objective of this patch.\n\n> > + struct curry c = { 0 };\n> > + size_t i = 1;\n> > +\n> > + do {\n> > + tree_search(values + i, &root, &test_compare, 1);\n> > + i = (i * 5) % 13;\n> > + } while (i != 1);\n>\n> It's completely non-obvious that `tree_search()` ends up _inserting_\n> nodes into the tree when the entry we're searching for wasn't found (and\n> if the last parameter is `1`. I feel like this interface could really\n> use a complete makeover and split up its concerns. In any case, that\n> does not need to happen as part of this patch seriesr\n\nI don't really mind it because all tree operations are only used in\nreftable/writer.c and only in a couple of places, so it would make sense\nfor the original authors to focus their efforts on other parts of the codebase.\nStill, I do agree that the readability takes a hit 'cause of that.\n\n> What I think would help though is if the commit message itself mentioned\n> this unorthodox way of inserting values into the tree.\n\nSure thing.\n\n> > + infix_walk(root, &check_increasing, &c);\n>\n> Not a fault of this commit, but this test certainly isn't great. It\n> would succeed even if `infix_walk()` didn't do anything as we do not\n> verify at all whether all nodes have been traversed (and traversed once,\n> exactly).\n\nI'll try to modify the test to check for these properties of an infix\nwalk as well.\n"},{"id":"496978","messageId":"20240612055031.3607-1-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240610131017.8321-1-chandrapratap3519@gmail.com","subject":"[GSoC][PATCH v2 0/5] t: port reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-12T05:38:09Z","receivedAt":"2024-06-12T05:51:02Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"\nIn the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit\ntesting framework written entirely in C was introduced to the Git project\naimed at simplifying testing and reducing test run times.\nCurrently, tests for the reftable refs-backend are performed by a custom\ntesting framework defined by reftable/test_framework.{c, h}. Port\nreftable/tree_test.c to the unit testing framework and improve upon\nthe ported test.\n\nThe first patch in the series is preparatory cleanup, the second patch\nmoves the test to the unit testing framework, and the rest of the patches\nimprove upon the ported test.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n\n---\nChanges in v2:\n- Add more context in the commit message of the third patch\n- Add an improvement patch for test_infix_walk()\n- Small refactor changes\n\nCI/PR: https://github.com/gitgitgadget/git/pull/1740\n\nChandra Pratap(5):\nreftable: remove unnecessary curly braces in\nt: move reftable/tree_test.c to the unit testing\nt-reftable-tree: split test_tree() into two sub-test\nt-reftable-tree: add test for non-existent key\nt-reftable-tree: improve the test for infix_walk()\n\nMakefile | 2 +-\nreftable/tree.c | 15 +++--------\nreftable/tree_test.c | 60 -------------------------------\nt/helper/test-reftable.c | 1 -\nt/unit-tests/t-reftable-tree.c | 76 ++++++++++++++++++++++++++++++++++++++++++++\n5 files changed, 82 insertions(+), 72 deletions(-)\n\nRange-diff against v1:\n1: 161d8892d6 ! 1: 542e497334 t-reftable-tree: split test_tree() into two sub-test functions\n @@ Commit message\n This improves the overall readability of the test file as well as\n simplifies debugging.\n \n + Note that the last parameter in the tree_search() functiom is\n + 'int insert' which when set, inserts the key if it is not found\n + in the tree. Otherwise, the function returns NULL for such cases.\n +\n Mentored-by: Patrick Steinhardt \n Mentored-by: Christian Couder \n Signed-off-by: Chandra Pratap \n @@ t/unit-tests/t-reftable-tree.c: static void check_increasing(void *arg, void *ke\n {\n \tstruct tree_node *root = NULL;\n \tvoid *values[11] = { 0 };\n + \tstruct tree_node *nodes[11] = { 0 };\n + \tsize_t i = 1;\n +-\tstruct curry c = { 0 };\n + \n + \tdo {\n + \t\tnodes[i] = tree_search(values + i, &root, &test_compare, 1);\n @@ t/unit-tests/t-reftable-tree.c: static void test_tree(void)\n \t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));\n \t}\n @@ t/unit-tests/t-reftable-tree.c: static void test_tree(void)\n +static void test_infix_walk(void)\n +{\n +\tstruct tree_node *root = NULL;\n -+\tvoid *values[13] = { 0 };\n ++\tvoid *values[11] = { 0 };\n +\tstruct curry c = { 0 };\n +\tsize_t i = 1;\n +\n +\tdo {\n +\t\ttree_search(values + i, &root, &test_compare, 1);\n -+\t\ti = (i * 5) % 13;\n ++\t\ti = (i * 7) % 11;\n +\t} while (i != 1);\n +\n +\tinfix_walk(root, &check_increasing, &c);\n2: d649c4a193 = 2: c976a37cbc t-reftable-tree: add test for non-existent key\n-: ---------- > 3: 3010c8f01a t-reftable-tree: improve the test for infix_walk()\n"},{"id":"496979","messageId":"20240612055031.3607-2-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240612055031.3607-1-chandrapratap3519@gmail.com","subject":"[PATCH v2 1/5] reftable: remove unnecessary curly braces in reftable/tree.c","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-12T05:38:10Z","receivedAt":"2024-06-12T05:51:05Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"According to Documentation/CodingGuidelines, single-line control-flow\nstatements must omit curly braces (except for some special cases).\nMake reftable/tree.c adhere to this guideline.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n reftable/tree.c | 15 +++++----------\n 1 file changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/reftable/tree.c b/reftable/tree.c\nindex 528f33ae38..5ffb2e0d69 100644\n--- a/reftable/tree.c\n+++ b/reftable/tree.c\n@@ -39,25 +39,20 @@ struct tree_node *tree_search(void *key, struct tree_node **rootp,\n void infix_walk(struct tree_node *t, void (*action)(void *arg, void *key),\n \t\tvoid *arg)\n {\n-\tif (t->left) {\n+\tif (t->left)\n \t\tinfix_walk(t->left, action, arg);\n-\t}\n \taction(arg, t->key);\n-\tif (t->right) {\n+\tif (t->right)\n \t\tinfix_walk(t->right, action, arg);\n-\t}\n }\n \n void tree_free(struct tree_node *t)\n {\n-\tif (!t) {\n+\tif (!t)\n \t\treturn;\n-\t}\n-\tif (t->left) {\n+\tif (t->left)\n \t\ttree_free(t->left);\n-\t}\n-\tif (t->right) {\n+\tif (t->right)\n \t\ttree_free(t->right);\n-\t}\n \treftable_free(t);\n }\n-- \n2.45.2.404.g9eaef5822c\n\n"},{"id":"496980","messageId":"20240612055031.3607-3-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240612055031.3607-1-chandrapratap3519@gmail.com","subject":"[PATCH v2 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-12T05:38:11Z","receivedAt":"2024-06-12T05:51:08Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"reftable/tree_test.c exercises the functions defined in\nreftable/tree.{c, h}. Migrate reftable/tree_test.c to the\nunit testing framework. Migration involves refactoring the tests\nto use the unit testing framework instead of reftable's test\nframework.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n Makefile | 2 +-\n t/helper/test-reftable.c | 1 -\n .../unit-tests/t-reftable-tree.c | 32 ++++++++-----------\n 3 files changed, 15 insertions(+), 20 deletions(-)\n rename reftable/tree_test.c => t/unit-tests/t-reftable-tree.c (59%)\n\ndiff --git a/Makefile b/Makefile\nindex 2f5f16847a..d736b2f8bd 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1336,6 +1336,7 @@ THIRD_PARTY_SOURCES += sha1dc/%\n UNIT_TEST_PROGRAMS += t-ctype\n UNIT_TEST_PROGRAMS += t-mem-pool\n UNIT_TEST_PROGRAMS += t-prio-queue\n+UNIT_TEST_PROGRAMS += t-reftable-tree\n UNIT_TEST_PROGRAMS += t-strbuf\n UNIT_TEST_PROGRAMS += t-strcmp-offset\n UNIT_TEST_PROGRAMS += t-strvec\n@@ -2681,7 +2682,6 @@ REFTABLE_TEST_OBJS += reftable/record_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\n \n TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))\n \ndiff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c\nindex bae731669c..9475db2f76 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 \tbasics_test_main(argc, argv);\n \trecord_test_main(argc, 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 \tmerged_test_main(argc, argv);\ndiff --git a/reftable/tree_test.c b/t/unit-tests/t-reftable-tree.c\nsimilarity index 59%\nrename from reftable/tree_test.c\nrename to t/unit-tests/t-reftable-tree.c\nindex 6961a657ad..208e7b7874 100644\n--- a/reftable/tree_test.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -6,11 +6,8 @@ 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-#include \"tree.h\"\n-\n-#include \"test_framework.h\"\n-#include \"reftable-tests.h\"\n+#include \"test-lib.h\"\n+#include \"reftable/tree.h\"\n \n static int test_compare(const void *a, const void *b)\n {\n@@ -24,37 +21,36 @@ struct curry {\n static void check_increasing(void *arg, void *key)\n {\n \tstruct curry *c = arg;\n-\tif (c->last) {\n-\t\tEXPECT(test_compare(c->last, key) < 0);\n-\t}\n+\tif (c->last)\n+\t\tcheck_int(test_compare(c->last, key), <, 0);\n \tc->last = key;\n }\n \n static void test_tree(void)\n {\n \tstruct tree_node *root = NULL;\n+\tvoid *values[11] = { 0 };\n+\tstruct tree_node *nodes[11] = { 0 };\n+\tsize_t i = 1;\n+\tstruct curry c = { 0 };\n \n-\tvoid *values[11] = { NULL };\n-\tstruct tree_node *nodes[11] = { NULL };\n-\tint i = 1;\n-\tstruct curry c = { NULL };\n \tdo {\n \t\tnodes[i] = tree_search(values + i, &root, &test_compare, 1);\n \t\ti = (i * 7) % 11;\n \t} while (i != 1);\n \n \tfor (i = 1; i < ARRAY_SIZE(nodes); i++) {\n-\t\tEXPECT(values + i == nodes[i]->key);\n-\t\tEXPECT(nodes[i] ==\n-\t\t tree_search(values + i, &root, &test_compare, 0));\n+\t\tcheck_pointer_eq(values + i, nodes[i]->key);\n+\t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));\n \t}\n \n \tinfix_walk(root, check_increasing, &c);\n \ttree_free(root);\n }\n \n-int tree_test_main(int argc, const char *argv[])\n+int cmd_main(int argc, const char *argv[])\n {\n-\tRUN_TEST(test_tree);\n-\treturn 0;\n+\tTEST(test_tree(), \"tree_search and infix_walk work\");\n+\n+\treturn test_done();\n }\n-- \n2.45.2.404.g9eaef5822c\n\n"},{"id":"496981","messageId":"20240612055031.3607-4-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240612055031.3607-1-chandrapratap3519@gmail.com","subject":"[PATCH v2 3/5] t-reftable-tree: split test_tree() into two sub-test functions","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-12T05:38:12Z","receivedAt":"2024-06-12T05:51:12Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup, tests for both tree_search() and\ninfix_walk() defined by reftable/tree.{c, h} are performed by\na single test function, test_tree(). Split tree_test() into\ntest_tree_search() and test_infix_walk() responsible for\nindependently testing tree_search() and infix_walk() respectively.\nThis improves the overall readability of the test file as well as\nsimplifies debugging.\n\nNote that the last parameter in the tree_search() functiom is\n'int insert' which when set, inserts the key if it is not found\nin the tree. Otherwise, the function returns NULL for such cases.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 23 +++++++++++++++++++----\n 1 file changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex 208e7b7874..cb721b377a 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -26,13 +26,12 @@ static void check_increasing(void *arg, void *key)\n \tc->last = key;\n }\n \n-static void test_tree(void)\n+static void test_tree_search(void)\n {\n \tstruct tree_node *root = NULL;\n \tvoid *values[11] = { 0 };\n \tstruct tree_node *nodes[11] = { 0 };\n \tsize_t i = 1;\n-\tstruct curry c = { 0 };\n \n \tdo {\n \t\tnodes[i] = tree_search(values + i, &root, &test_compare, 1);\n@@ -44,13 +43,29 @@ static void test_tree(void)\n \t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));\n \t}\n \n-\tinfix_walk(root, check_increasing, &c);\n+\ttree_free(root);\n+}\n+\n+static void test_infix_walk(void)\n+{\n+\tstruct tree_node *root = NULL;\n+\tvoid *values[11] = { 0 };\n+\tstruct curry c = { 0 };\n+\tsize_t i = 1;\n+\n+\tdo {\n+\t\ttree_search(values + i, &root, &test_compare, 1);\n+\t\ti = (i * 7) % 11;\n+\t} while (i != 1);\n+\n+\tinfix_walk(root, &check_increasing, &c);\n \ttree_free(root);\n }\n \n int cmd_main(int argc, const char *argv[])\n {\n-\tTEST(test_tree(), \"tree_search and infix_walk work\");\n+\tTEST(test_tree_search(), \"tree_search works\");\n+\tTEST(test_infix_walk(), \"infix_walk works\");\n \n \treturn test_done();\n }\n-- \n2.45.2.404.g9eaef5822c\n\n"},{"id":"496982","messageId":"20240612055031.3607-5-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240612055031.3607-1-chandrapratap3519@gmail.com","subject":"[PATCH v2 4/5] t-reftable-tree: add test for non-existent key","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-12T05:38:13Z","receivedAt":"2024-06-12T05:51:15Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup for tree_search(), the case for\nnon-existent key is not exercised. Improve this by adding a\ntest-case for the same.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex cb721b377a..f1adab4458 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -43,6 +43,7 @@ static void test_tree_search(void)\n \t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));\n \t}\n \n+\tcheck(!tree_search(values, &root, &test_compare, 0));\n \ttree_free(root);\n }\n \n-- \n2.45.2.404.g9eaef5822c\n\n"},{"id":"496983","messageId":"20240612055031.3607-6-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240612055031.3607-1-chandrapratap3519@gmail.com","subject":"[PATCH v2 5/5] t-reftable-tree: improve the test for infix_walk()","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-12T05:38:14Z","receivedAt":"2024-06-12T05:51:17Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup for infix_walk(), the following\nproperties of an infix traversal of a tree remain untested:\n- every node of the tree must be visited\n- every node must be visited exactly only\nand only the property 'traversal in increasing order' is tested.\nModify test_infix_walk() to check for all the properties above.\n\nThis can be achieved by storing the nodes' keys linearly, in a nullified\nbuffer, as we visit them and then checking the input keys against this\nbuffer in increasing order. By checking that the element just after\nthe last input key is 'NULL' in the output buffer, we ensure that\nevery node is traversed exactly once.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 16 ++++++++++------\n 1 file changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex f1adab4458..917a64a7d1 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -15,15 +15,14 @@ static int test_compare(const void *a, const void *b)\n }\n \n struct curry {\n-\tvoid *last;\n+\tvoid **arr;\n+\tsize_t i;\n };\n \n-static void check_increasing(void *arg, void *key)\n+static void store(void *arg, void *key)\n {\n \tstruct curry *c = arg;\n-\tif (c->last)\n-\t\tcheck_int(test_compare(c->last, key), <, 0);\n-\tc->last = key;\n+\tc->arr[c->i++] = key;\n }\n \n static void test_tree_search(void)\n@@ -51,6 +50,7 @@ static void test_infix_walk(void)\n {\n \tstruct tree_node *root = NULL;\n \tvoid *values[11] = { 0 };\n+\tvoid *out[20] = { 0 };\n \tstruct curry c = { 0 };\n \tsize_t i = 1;\n \n@@ -59,7 +59,11 @@ static void test_infix_walk(void)\n \t\ti = (i * 7) % 11;\n \t} while (i != 1);\n \n-\tinfix_walk(root, &check_increasing, &c);\n+\tc.arr = (void **) &out;\n+\tinfix_walk(root, &store, &c);\n+\tfor (i = 1; i < ARRAY_SIZE(values); i++)\n+\t\tcheck_pointer_eq(values + i, out[i - 1]);\n+\tcheck(!out[i]);\n \ttree_free(root);\n }\n \n-- \n2.45.2.404.g9eaef5822c\n\n"},{"id":"496986","messageId":"ZmlHZXN8_7rKLTYk@tanuki","threadId":"61614","inReplyTo":"20240612055031.3607-6-chandrapratap3519@gmail.com","subject":"Re: [PATCH v2 5/5] t-reftable-tree: improve the test for infix_walk()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-12T06:59:49Z","receivedAt":"2024-06-12T06:59:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jun 12, 2024 at 11:08:14AM +0530, Chandra Pratap wrote:\n> In the current testing setup for infix_walk(), the following\n> properties of an infix traversal of a tree remain untested:\n> - every node of the tree must be visited\n> - every node must be visited exactly only\n\ns/only/once\n\n> and only the property 'traversal in increasing order' is tested.\n\nNit: this reads a bit awkward. How about \"In fact, we only verify that\nthe traversal happens in increasing order.\"\n\n> @@ -51,6 +50,7 @@ static void test_infix_walk(void)\n> {\n> \tstruct tree_node *root = NULL;\n> \tvoid *values[11] = { 0 };\n> +\tvoid *out[20] = { 0 };\n\nFrom the test below it looks like we only expect 11 values to be added\nto `out`. Why does this array have length 20?\n\nWe could of course also use something like `ALLOC_GROW()` to grow the\narray dynamically. But that'd likely be overkill.\n\n> \tstruct curry c = { 0 };\n> \tsize_t i = 1;\n> \n> @@ -59,7 +59,11 @@ static void test_infix_walk(void)\n> \t\ti = (i * 7) % 11;\n> \t} while (i != 1);\n> \n> -\tinfix_walk(root, &check_increasing, &c);\n> +\tc.arr = (void **) &out;\n\nWe can initialize this variable directly when declaring `c`:\n\n struct curry c = {\n .arr = &out;\n };\n\nAlso, is the cast necessary? This is the only site where we use `struct\ncurry` if I'm not mistaken, so I'd expect that the type of `arr` should\nmatch our expectations.\n\n> +\tinfix_walk(root, &store, &c);\n> +\tfor (i = 1; i < ARRAY_SIZE(values); i++)\n> +\t\tcheck_pointer_eq(values + i, out[i - 1]);\n\nLet's also verify that `c.len` matches the expected number of nodes\nvisited.\n\nPatrick\n\n> +\tcheck(!out[i]);\n> \ttree_free(root);\n> }\n"},{"id":"497005","messageId":"CA+J6zkQANGzX=+=U9=DYfVujV68yE3-LvvrgF+bnYtgAskFRHg@mail.gmail.com","threadId":"61614","inReplyTo":"ZmlHZXN8_7rKLTYk@tanuki","subject":"Re: [PATCH v2 5/5] t-reftable-tree: improve the test for infix_walk()","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-12T09:05:19Z","receivedAt":"2024-06-12T09:05:32Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"On Wed, 12 Jun 2024 at 12:29, Patrick Steinhardt wrote:\n>\n> On Wed, Jun 12, 2024 at 11:08:14AM +0530, Chandra Pratap wrote:\n> > In the current testing setup for infix_walk(), the following\n> > properties of an infix traversal of a tree remain untested:\n> > - every node of the tree must be visited\n> > - every node must be visited exactly only\n>\n> s/only/once\n>\n> > and only the property 'traversal in increasing order' is tested.\n>\n> Nit: this reads a bit awkward. How about \"In fact, we only verify that\n> the traversal happens in increasing order.\"\n>\n> > @@ -51,6 +50,7 @@ static void test_infix_walk(void)\n> > {\n> > struct tree_node *root = NULL;\n> > void *values[11] = { 0 };\n> > + void *out[20] = { 0 };\n>\n> From the test below it looks like we only expect 11 values to be added\n> to `out`. Why does this array have length 20?\n\nThat's an error. I'll correct it in the next iteration.\n\n> We could of course also use something like `ALLOC_GROW()` to grow the\n> array dynamically. But that'd likely be overkill.\n>\n> > struct curry c = { 0 };\n> > size_t i = 1;\n> >\n> > @@ -59,7 +59,11 @@ static void test_infix_walk(void)\n> > i = (i * 7) % 11;\n> > } while (i != 1);\n> >\n> > - infix_walk(root, &check_increasing, &c);\n> > + c.arr = (void **) &out;\n>\n> We can initialize this variable directly when declaring `c`:\n>\n> struct curry c = {\n> .arr = &out;\n> };\n\nRight, this seems more concise.\n\n> Also, is the cast necessary? This is the only site where we use `struct\n> curry` if I'm not mistaken, so I'd expect that the type of `arr` should\n> match our expectations.\n\nTrying to do this without a cast produces the following compilation error\nfor me:\ninitialization of ‘void **’ from incompatible pointer type ‘void * (*)[11]’\n\n> > + infix_walk(root, &store, &c);\n> > + for (i = 1; i < ARRAY_SIZE(values); i++)\n> > + check_pointer_eq(values + i, out[i - 1]);\n>\n> Let's also verify that `c.len` matches the expected number of nodes\n> visited.\n\nSure.\n"},{"id":"497031","messageId":"20240612130217.8877-1-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240612055031.3607-1-chandrapratap3519@gmail.com","subject":"[GSoC][PATCH v3 0/5] t: port reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-12T12:52:58Z","receivedAt":"2024-06-12T13:02:45Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit\ntesting framework written entirely in C was introduced to the Git project\naimed at simplifying testing and reducing test run times.\nCurrently, tests for the reftable refs-backend are performed by a custom\ntesting framework defined by reftable/test_framework.{c, h}. Port\nreftable/tree_test.c to the unit testing framework and improve upon\nthe ported test.\n\nThe first patch in the series is preparatory cleanup, the second patch\nmoves the test to the unit testing framework, and the rest of the patches\nimprove upon the ported test.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n\n---\nChanges in v3:\n- Fix a typo in the commit message of the fifth patch\n- Add a check for number of input & output elements in the fifth patch\n- Small refactor changes\n\nCI/PR: https://github.com/gitgitgadget/git/pull/1740\n\nChandra Pratap(5):\nreftable: remove unnecessary curly braces in\nt: move reftable/tree_test.c to the unit testing\nt-reftable-tree: split test_tree() into two sub-test\nt-reftable-tree: add test for non-existent key\nt-reftable-tree: improve the test for infix_walk()\n\nMakefile | 2 +-\nreftable/tree.c | 15 +++--------\nreftable/tree_test.c | 60 -------------------------------\nt/helper/test-reftable.c | 1 -\nt/unit-tests/t-reftable-tree.c | 76 ++++++++++++++++++++++++++++++++++++++++++++\n5 files changed, 86 insertions(+), 72 deletions(-)\n\nRange-diff against v2:\n1: 3010c8f01a ! 1: 44800ad205 t-reftable-tree: improve the test for infix_walk()\n @@ Commit message\n In the current testing setup for infix_walk(), the following\n properties of an infix traversal of a tree remain untested:\n - every node of the tree must be visited\n - - every node must be visited exactly only\n - and only the property 'traversal in increasing order' is tested.\n + - every node must be visited exactly once\n + In fact, only the property 'traversal in increasing order' is tested.\n Modify test_infix_walk() to check for all the properties above.\n\n This can be achieved by storing the nodes' keys linearly, in a nullified\n @@ t/unit-tests/t-reftable-tree.c: static int test_compare(const void *a, const voi\n struct curry {\n -\tvoid *last;\n +\tvoid **arr;\n -+\tsize_t i;\n ++\tsize_t len;\n };\n\n -static void check_increasing(void *arg, void *key)\n @@ t/unit-tests/t-reftable-tree.c: static int test_compare(const void *a, const voi\n -\tif (c->last)\n -\t\tcheck_int(test_compare(c->last, key), <, 0);\n -\tc->last = key;\n -+\tc->arr[c->i++] = key;\n ++\tc->arr[c->len++] = key;\n }\n\n static void test_tree_search(void)\n @@ t/unit-tests/t-reftable-tree.c: static void test_infix_walk(void)\n {\n \tstruct tree_node *root = NULL;\n \tvoid *values[11] = { 0 };\n -+\tvoid *out[20] = { 0 };\n - \tstruct curry c = { 0 };\n +-\tstruct curry c = { 0 };\n ++\tvoid *out[11] = { 0 };\n ++\tstruct curry c = {\n ++\t\t.arr = (void **) &out,\n ++\t};\n \tsize_t i = 1;\n ++\tsize_t count = 0;\n\n -@@ t/unit-tests/t-reftable-tree.c: static void test_infix_walk(void)\n + \tdo {\n + \t\ttree_search(values + i, &root, &test_compare, 1);\n \t\ti = (i * 7) % 11;\n ++\t\tcount++;\n \t} while (i != 1);\n\n -\tinfix_walk(root, &check_increasing, &c);\n -+\tc.arr = (void **) &out;\n +\tinfix_walk(root, &store, &c);\n +\tfor (i = 1; i < ARRAY_SIZE(values); i++)\n +\t\tcheck_pointer_eq(values + i, out[i - 1]);\n -+\tcheck(!out[i]);\n ++\tcheck(!out[i - 1]);\n ++\tcheck_int(c.len, ==, count);\n \ttree_free(root);\n }\n\n"},{"id":"497032","messageId":"20240612130217.8877-2-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240612130217.8877-1-chandrapratap3519@gmail.com","subject":"[PATCH v3 1/5] reftable: remove unnecessary curly braces in reftable/tree.c","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-12T12:52:59Z","receivedAt":"2024-06-12T13:02:48Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"According to Documentation/CodingGuidelines, single-line control-flow\nstatements must omit curly braces (except for some special cases).\nMake reftable/tree.c adhere to this guideline.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n reftable/tree.c | 15 +++++----------\n 1 file changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/reftable/tree.c b/reftable/tree.c\nindex 528f33ae38..5ffb2e0d69 100644\n--- a/reftable/tree.c\n+++ b/reftable/tree.c\n@@ -39,25 +39,20 @@ struct tree_node *tree_search(void *key, struct tree_node **rootp,\n void infix_walk(struct tree_node *t, void (*action)(void *arg, void *key),\n \t\tvoid *arg)\n {\n-\tif (t->left) {\n+\tif (t->left)\n \t\tinfix_walk(t->left, action, arg);\n-\t}\n \taction(arg, t->key);\n-\tif (t->right) {\n+\tif (t->right)\n \t\tinfix_walk(t->right, action, arg);\n-\t}\n }\n \n void tree_free(struct tree_node *t)\n {\n-\tif (!t) {\n+\tif (!t)\n \t\treturn;\n-\t}\n-\tif (t->left) {\n+\tif (t->left)\n \t\ttree_free(t->left);\n-\t}\n-\tif (t->right) {\n+\tif (t->right)\n \t\ttree_free(t->right);\n-\t}\n \treftable_free(t);\n }\n-- \n2.45.2.404.g9eaef5822c\n\n"},{"id":"497033","messageId":"20240612130217.8877-3-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240612130217.8877-1-chandrapratap3519@gmail.com","subject":"[PATCH v3 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-12T12:53:00Z","receivedAt":"2024-06-12T13:02:51Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"reftable/tree_test.c exercises the functions defined in\nreftable/tree.{c, h}. Migrate reftable/tree_test.c to the\nunit testing framework. Migration involves refactoring the tests\nto use the unit testing framework instead of reftable's test\nframework.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n Makefile | 2 +-\n t/helper/test-reftable.c | 1 -\n .../unit-tests/t-reftable-tree.c | 32 ++++++++-----------\n 3 files changed, 15 insertions(+), 20 deletions(-)\n rename reftable/tree_test.c => t/unit-tests/t-reftable-tree.c (59%)\n\ndiff --git a/Makefile b/Makefile\nindex 2f5f16847a..d736b2f8bd 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1336,6 +1336,7 @@ THIRD_PARTY_SOURCES += sha1dc/%\n UNIT_TEST_PROGRAMS += t-ctype\n UNIT_TEST_PROGRAMS += t-mem-pool\n UNIT_TEST_PROGRAMS += t-prio-queue\n+UNIT_TEST_PROGRAMS += t-reftable-tree\n UNIT_TEST_PROGRAMS += t-strbuf\n UNIT_TEST_PROGRAMS += t-strcmp-offset\n UNIT_TEST_PROGRAMS += t-strvec\n@@ -2681,7 +2682,6 @@ REFTABLE_TEST_OBJS += reftable/record_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\n \n TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))\n \ndiff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c\nindex bae731669c..9475db2f76 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 \tbasics_test_main(argc, argv);\n \trecord_test_main(argc, 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 \tmerged_test_main(argc, argv);\ndiff --git a/reftable/tree_test.c b/t/unit-tests/t-reftable-tree.c\nsimilarity index 59%\nrename from reftable/tree_test.c\nrename to t/unit-tests/t-reftable-tree.c\nindex 6961a657ad..208e7b7874 100644\n--- a/reftable/tree_test.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -6,11 +6,8 @@ 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-#include \"tree.h\"\n-\n-#include \"test_framework.h\"\n-#include \"reftable-tests.h\"\n+#include \"test-lib.h\"\n+#include \"reftable/tree.h\"\n \n static int test_compare(const void *a, const void *b)\n {\n@@ -24,37 +21,36 @@ struct curry {\n static void check_increasing(void *arg, void *key)\n {\n \tstruct curry *c = arg;\n-\tif (c->last) {\n-\t\tEXPECT(test_compare(c->last, key) < 0);\n-\t}\n+\tif (c->last)\n+\t\tcheck_int(test_compare(c->last, key), <, 0);\n \tc->last = key;\n }\n \n static void test_tree(void)\n {\n \tstruct tree_node *root = NULL;\n+\tvoid *values[11] = { 0 };\n+\tstruct tree_node *nodes[11] = { 0 };\n+\tsize_t i = 1;\n+\tstruct curry c = { 0 };\n \n-\tvoid *values[11] = { NULL };\n-\tstruct tree_node *nodes[11] = { NULL };\n-\tint i = 1;\n-\tstruct curry c = { NULL };\n \tdo {\n \t\tnodes[i] = tree_search(values + i, &root, &test_compare, 1);\n \t\ti = (i * 7) % 11;\n \t} while (i != 1);\n \n \tfor (i = 1; i < ARRAY_SIZE(nodes); i++) {\n-\t\tEXPECT(values + i == nodes[i]->key);\n-\t\tEXPECT(nodes[i] ==\n-\t\t tree_search(values + i, &root, &test_compare, 0));\n+\t\tcheck_pointer_eq(values + i, nodes[i]->key);\n+\t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));\n \t}\n \n \tinfix_walk(root, check_increasing, &c);\n \ttree_free(root);\n }\n \n-int tree_test_main(int argc, const char *argv[])\n+int cmd_main(int argc, const char *argv[])\n {\n-\tRUN_TEST(test_tree);\n-\treturn 0;\n+\tTEST(test_tree(), \"tree_search and infix_walk work\");\n+\n+\treturn test_done();\n }\n-- \n2.45.2.404.g9eaef5822c\n\n"},{"id":"497034","messageId":"20240612130217.8877-4-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240612130217.8877-1-chandrapratap3519@gmail.com","subject":"[PATCH v3 3/5] t-reftable-tree: split test_tree() into two sub-test functions","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-12T12:53:01Z","receivedAt":"2024-06-12T13:02:54Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup, tests for both tree_search() and\ninfix_walk() defined by reftable/tree.{c, h} are performed by\na single test function, test_tree(). Split tree_test() into\ntest_tree_search() and test_infix_walk() responsible for\nindependently testing tree_search() and infix_walk() respectively.\nThis improves the overall readability of the test file as well as\nsimplifies debugging.\n\nNote that the last parameter in the tree_search() functiom is\n'int insert' which when set, inserts the key if it is not found\nin the tree. Otherwise, the function returns NULL for such cases.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 23 +++++++++++++++++++----\n 1 file changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex 208e7b7874..cb721b377a 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -26,13 +26,12 @@ static void check_increasing(void *arg, void *key)\n \tc->last = key;\n }\n \n-static void test_tree(void)\n+static void test_tree_search(void)\n {\n \tstruct tree_node *root = NULL;\n \tvoid *values[11] = { 0 };\n \tstruct tree_node *nodes[11] = { 0 };\n \tsize_t i = 1;\n-\tstruct curry c = { 0 };\n \n \tdo {\n \t\tnodes[i] = tree_search(values + i, &root, &test_compare, 1);\n@@ -44,13 +43,29 @@ static void test_tree(void)\n \t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));\n \t}\n \n-\tinfix_walk(root, check_increasing, &c);\n+\ttree_free(root);\n+}\n+\n+static void test_infix_walk(void)\n+{\n+\tstruct tree_node *root = NULL;\n+\tvoid *values[11] = { 0 };\n+\tstruct curry c = { 0 };\n+\tsize_t i = 1;\n+\n+\tdo {\n+\t\ttree_search(values + i, &root, &test_compare, 1);\n+\t\ti = (i * 7) % 11;\n+\t} while (i != 1);\n+\n+\tinfix_walk(root, &check_increasing, &c);\n \ttree_free(root);\n }\n \n int cmd_main(int argc, const char *argv[])\n {\n-\tTEST(test_tree(), \"tree_search and infix_walk work\");\n+\tTEST(test_tree_search(), \"tree_search works\");\n+\tTEST(test_infix_walk(), \"infix_walk works\");\n \n \treturn test_done();\n }\n-- \n2.45.2.404.g9eaef5822c\n\n"},{"id":"497035","messageId":"20240612130217.8877-5-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240612130217.8877-1-chandrapratap3519@gmail.com","subject":"[PATCH v3 4/5] t-reftable-tree: add test for non-existent key","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-12T12:53:02Z","receivedAt":"2024-06-12T13:02:56Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup for tree_search(), the case for\nnon-existent key is not exercised. Improve this by adding a\ntest-case for the same.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex cb721b377a..f1adab4458 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -43,6 +43,7 @@ static void test_tree_search(void)\n \t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));\n \t}\n \n+\tcheck(!tree_search(values, &root, &test_compare, 0));\n \ttree_free(root);\n }\n \n-- \n2.45.2.404.g9eaef5822c\n\n"},{"id":"497036","messageId":"20240612130217.8877-6-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240612130217.8877-1-chandrapratap3519@gmail.com","subject":"[PATCH v3 5/5] t-reftable-tree: improve the test for infix_walk()","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-06-12T12:53:03Z","receivedAt":"2024-06-12T13:02:59Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup for infix_walk(), the following\nproperties of an infix traversal of a tree remain untested:\n- every node of the tree must be visited\n- every node must be visited exactly once\nIn fact, only the property 'traversal in increasing order' is tested.\nModify test_infix_walk() to check for all the properties above.\n\nThis can be achieved by storing the nodes' keys linearly, in a nullified\nbuffer, as we visit them and then checking the input keys against this\nbuffer in increasing order. By checking that the element just after\nthe last input key is 'NULL' in the output buffer, we ensure that\nevery node is traversed exactly once.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 22 +++++++++++++++-------\n 1 file changed, 15 insertions(+), 7 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex f1adab4458..79c6bfd49a 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -15,15 +15,14 @@ static int test_compare(const void *a, const void *b)\n }\n \n struct curry {\n-\tvoid *last;\n+\tvoid **arr;\n+\tsize_t len;\n };\n \n-static void check_increasing(void *arg, void *key)\n+static void store(void *arg, void *key)\n {\n \tstruct curry *c = arg;\n-\tif (c->last)\n-\t\tcheck_int(test_compare(c->last, key), <, 0);\n-\tc->last = key;\n+\tc->arr[c->len++] = key;\n }\n \n static void test_tree_search(void)\n@@ -51,15 +50,24 @@ static void test_infix_walk(void)\n {\n \tstruct tree_node *root = NULL;\n \tvoid *values[11] = { 0 };\n-\tstruct curry c = { 0 };\n+\tvoid *out[11] = { 0 };\n+\tstruct curry c = {\n+\t\t.arr = (void **) &out,\n+\t};\n \tsize_t i = 1;\n+\tsize_t count = 0;\n \n \tdo {\n \t\ttree_search(values + i, &root, &test_compare, 1);\n \t\ti = (i * 7) % 11;\n+\t\tcount++;\n \t} while (i != 1);\n \n-\tinfix_walk(root, &check_increasing, &c);\n+\tinfix_walk(root, &store, &c);\n+\tfor (i = 1; i < ARRAY_SIZE(values); i++)\n+\t\tcheck_pointer_eq(values + i, out[i - 1]);\n+\tcheck(!out[i - 1]);\n+\tcheck_int(c.len, ==, count);\n \ttree_free(root);\n }\n \n-- \n2.45.2.404.g9eaef5822c\n\n"},{"id":"498773","messageId":"20240716075641.4264-1-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240612130217.8877-1-chandrapratap3519@gmail.com","subject":"[GSoC][PATCH v4 0/5] t: port reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-07-16T07:48:12Z","receivedAt":"2024-07-16T07:57:23Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit\ntesting framework written entirely in C was introduced to the Git project\naimed at simplifying testing and reducing test run times.\nCurrently, tests for the reftable refs-backend are performed by a custom\ntesting framework defined by reftable/test_framework.{c, h}. Port\nreftable/tree_test.c to the unit testing framework and improve upon\nthe ported test.\n\nThe first patch in the series is preparatory cleanup, the second patch\nmoves the test to the unit testing framework, and the rest of the patches\nimprove upon the ported test.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n\n---\nChanges in v4:\n- Rename the tests to be in-line with unit-tests' standards\n\nCI/PR: https://github.com/gitgitgadget/git/pull/1740\n\nChandra Pratap(5):\nreftable: remove unnecessary curly braces in reftable/tree.c\nt: move reftable/tree_test.c to the unit testing framework\nt-reftable-tree: split test_tree() into two sub-test\nt-reftable-tree: add test for non-existent key\nt-reftable-tree: improve the test for infix_walk()\n\nMakefile | 2 +-\nreftable/reftable-tests.h | 1 -\nreftable/tree.c | 15 +++-------\nreftable/tree_test.c | 60 ----------------------\nt/helper/test-reftable.c | 1 -\nt/unit-tests/t-reftable-tree.c | 80 +++++++++++++++++++++++++++++++++++++\n6 files changed, 86 insertions(+), 73 deletions(-)\n\nRange-diff against v3:\n1: ffabd3e411 = 1: 2be2a35b7f reftable: remove unnecessary curly braces in reftable/tree.c\n2: 17937233fb < -: ---------- t: move reftable/tree_test.c to the unit testing framework\n-: ---------- > 2: de49698ea7 t: move reftable/tree_test.c to the unit testing framework\n3: c3992091db ! 3: c733776054 t-reftable-tree: split test_tree() into two sub-test functions\n @@ t/unit-tests/t-reftable-tree.c: static void check_increasing(void *arg, void *ke\n \tc->last = key;\n }\n \n --static void test_tree(void)\n -+static void test_tree_search(void)\n +-static void t_tree(void)\n ++static void t_tree_search(void)\n {\n \tstruct tree_node *root = NULL;\n \tvoid *values[11] = { 0 };\n @@ t/unit-tests/t-reftable-tree.c: static void check_increasing(void *arg, void *ke\n -\tstruct curry c = { 0 };\n \n \tdo {\n - \t\tnodes[i] = tree_search(values + i, &root, &test_compare, 1);\n -@@ t/unit-tests/t-reftable-tree.c: static void test_tree(void)\n - \t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));\n + \t\tnodes[i] = tree_search(values + i, &root, &t_compare, 1);\n +@@ t/unit-tests/t-reftable-tree.c: static void t_tree(void)\n + \t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));\n \t}\n \n -\tinfix_walk(root, check_increasing, &c);\n +\ttree_free(root);\n +}\n +\n -+static void test_infix_walk(void)\n ++static void t_infix_walk(void)\n +{\n +\tstruct tree_node *root = NULL;\n +\tvoid *values[11] = { 0 };\n @@ t/unit-tests/t-reftable-tree.c: static void test_tree(void)\n +\tsize_t i = 1;\n +\n +\tdo {\n -+\t\ttree_search(values + i, &root, &test_compare, 1);\n ++\t\ttree_search(values + i, &root, &t_compare, 1);\n +\t\ti = (i * 7) % 11;\n +\t} while (i != 1);\n +\n @@ t/unit-tests/t-reftable-tree.c: static void test_tree(void)\n \n int cmd_main(int argc, const char *argv[])\n {\n --\tTEST(test_tree(), \"tree_search and infix_walk work\");\n -+\tTEST(test_tree_search(), \"tree_search works\");\n -+\tTEST(test_infix_walk(), \"infix_walk works\");\n +-\tTEST(t_tree(), \"tree_search and infix_walk work\");\n ++\tTEST(t_tree_search(), \"tree_search works\");\n ++\tTEST(t_infix_walk(), \"infix_walk works\");\n \n \treturn test_done();\n }\n4: 99a0ed484e ! 4: f1a9325bb3 t-reftable-tree: add test for non-existent key\n @@ Commit message\n Signed-off-by: Chandra Pratap \n \n ## t/unit-tests/t-reftable-tree.c ##\n -@@ t/unit-tests/t-reftable-tree.c: static void test_tree_search(void)\n - \t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));\n +@@ t/unit-tests/t-reftable-tree.c: static void t_tree_search(void)\n + \t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));\n \t}\n \n +\tcheck(!tree_search(values, &root, &test_compare, 0));\n5: 4fce9a8bd8 ! 5: c6b7a3d646 t-reftable-tree: improve the test for infix_walk()\n @@ Commit message\n Signed-off-by: Chandra Pratap \n \n ## t/unit-tests/t-reftable-tree.c ##\n -@@ t/unit-tests/t-reftable-tree.c: static int test_compare(const void *a, const void *b)\n +@@ t/unit-tests/t-reftable-tree.c: static int t_compare(const void *a, const void *b)\n }\n \n struct curry {\n @@ t/unit-tests/t-reftable-tree.c: static int test_compare(const void *a, const voi\n {\n \tstruct curry *c = arg;\n -\tif (c->last)\n --\t\tcheck_int(test_compare(c->last, key), <, 0);\n +-\t\tcheck_int(t_compare(c->last, key), <, 0);\n -\tc->last = key;\n +\tc->arr[c->len++] = key;\n }\n \n - static void test_tree_search(void)\n -@@ t/unit-tests/t-reftable-tree.c: static void test_infix_walk(void)\n + static void t_tree_search(void)\n +@@ t/unit-tests/t-reftable-tree.c: static void t_infix_walk(void)\n {\n \tstruct tree_node *root = NULL;\n \tvoid *values[11] = { 0 };\n @@ t/unit-tests/t-reftable-tree.c: static void test_infix_walk(void)\n +\tsize_t count = 0;\n \n \tdo {\n - \t\ttree_search(values + i, &root, &test_compare, 1);\n + \t\ttree_search(values + i, &root, &t_compare, 1);\n \t\ti = (i * 7) % 11;\n +\t\tcount++;\n \t} while (i != 1);\n"},{"id":"498774","messageId":"20240716075641.4264-2-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240716075641.4264-1-chandrapratap3519@gmail.com","subject":"[PATCH v4 1/5] reftable: remove unnecessary curly braces in reftable/tree.c","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-07-16T07:48:13Z","receivedAt":"2024-07-16T07:57:25Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"According to Documentation/CodingGuidelines, single-line control-flow\nstatements must omit curly braces (except for some special cases).\nMake reftable/tree.c adhere to this guideline.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n reftable/tree.c | 15 +++++----------\n 1 file changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/reftable/tree.c b/reftable/tree.c\nindex 528f33ae38..5ffb2e0d69 100644\n--- a/reftable/tree.c\n+++ b/reftable/tree.c\n@@ -39,25 +39,20 @@ struct tree_node *tree_search(void *key, struct tree_node **rootp,\n void infix_walk(struct tree_node *t, void (*action)(void *arg, void *key),\n \t\tvoid *arg)\n {\n-\tif (t->left) {\n+\tif (t->left)\n \t\tinfix_walk(t->left, action, arg);\n-\t}\n \taction(arg, t->key);\n-\tif (t->right) {\n+\tif (t->right)\n \t\tinfix_walk(t->right, action, arg);\n-\t}\n }\n \n void tree_free(struct tree_node *t)\n {\n-\tif (!t) {\n+\tif (!t)\n \t\treturn;\n-\t}\n-\tif (t->left) {\n+\tif (t->left)\n \t\ttree_free(t->left);\n-\t}\n-\tif (t->right) {\n+\tif (t->right)\n \t\ttree_free(t->right);\n-\t}\n \treftable_free(t);\n }\n-- \n2.45.GIT\n\n"},{"id":"498775","messageId":"20240716075641.4264-3-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240716075641.4264-1-chandrapratap3519@gmail.com","subject":"[PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-07-16T07:48:14Z","receivedAt":"2024-07-16T07:57:28Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"reftable/tree_test.c exercises the functions defined in\nreftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit\ntesting framework. Migration involves refactoring the tests to use\nthe unit testing framework instead of reftable's test framework and\nrenaming the tests to align with unit-tests' standards.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n Makefile | 2 +-\n reftable/reftable-tests.h | 1 -\n reftable/tree_test.c | 60 ----------------------------------\n t/helper/test-reftable.c | 1 -\n t/unit-tests/t-reftable-tree.c | 56 +++++++++++++++++++++++++++++++\n 5 files changed, 57 insertions(+), 63 deletions(-)\n delete mode 100644 reftable/tree_test.c\n create mode 100644 t/unit-tests/t-reftable-tree.c\n\ndiff --git a/Makefile b/Makefile\nindex 3eab701b10..79e86ddf53 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1340,6 +1340,7 @@ UNIT_TEST_PROGRAMS += t-mem-pool\n 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-tree\n UNIT_TEST_PROGRAMS += t-strbuf\n UNIT_TEST_PROGRAMS += t-strcmp-offset\n UNIT_TEST_PROGRAMS += t-strvec\n@@ -2685,7 +2686,6 @@ REFTABLE_TEST_OBJS += reftable/record_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\n \n TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))\n \ndiff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h\nindex 114cc3d053..d0abcc51e2 100644\n--- a/reftable/reftable-tests.h\n+++ b/reftable/reftable-tests.h\n@@ -16,7 +16,6 @@ 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);\n \n #endif\ndiff --git a/reftable/tree_test.c b/reftable/tree_test.c\ndeleted file mode 100644\nindex 6961a657ad..0000000000\n--- a/reftable/tree_test.c\n+++ /dev/null\n@@ -1,60 +0,0 @@\n-/*\n-Copyright 2020 Google LLC\n-\n-Use of this source code is governed by a BSD-style\n-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-#include \"tree.h\"\n-\n-#include \"test_framework.h\"\n-#include \"reftable-tests.h\"\n-\n-static int test_compare(const void *a, const void *b)\n-{\n-\treturn (char *)a - (char *)b;\n-}\n-\n-struct curry {\n-\tvoid *last;\n-};\n-\n-static void check_increasing(void *arg, void *key)\n-{\n-\tstruct curry *c = arg;\n-\tif (c->last) {\n-\t\tEXPECT(test_compare(c->last, key) < 0);\n-\t}\n-\tc->last = key;\n-}\n-\n-static void test_tree(void)\n-{\n-\tstruct tree_node *root = NULL;\n-\n-\tvoid *values[11] = { NULL };\n-\tstruct tree_node *nodes[11] = { NULL };\n-\tint i = 1;\n-\tstruct curry c = { NULL };\n-\tdo {\n-\t\tnodes[i] = tree_search(values + i, &root, &test_compare, 1);\n-\t\ti = (i * 7) % 11;\n-\t} while (i != 1);\n-\n-\tfor (i = 1; i < ARRAY_SIZE(nodes); i++) {\n-\t\tEXPECT(values + i == nodes[i]->key);\n-\t\tEXPECT(nodes[i] ==\n-\t\t tree_search(values + i, &root, &test_compare, 0));\n-\t}\n-\n-\tinfix_walk(root, check_increasing, &c);\n-\ttree_free(root);\n-}\n-\n-int tree_test_main(int argc, const char *argv[])\n-{\n-\tRUN_TEST(test_tree);\n-\treturn 0;\n-}\ndiff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c\nindex 9160bc5da6..245b674a3c 100644\n--- a/t/helper/test-reftable.c\n+++ b/t/helper/test-reftable.c\n@@ -7,7 +7,6 @@ int cmd__reftable(int argc, const char **argv)\n \t/* test from simple to complex. */\n \trecord_test_main(argc, 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 \tmerged_test_main(argc, argv);\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nnew file mode 100644\nindex 0000000000..5df814d983\n--- /dev/null\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -0,0 +1,56 @@\n+/*\n+Copyright 2020 Google LLC\n+\n+Use of this source code is governed by a BSD-style\n+license that can be found in the LICENSE file or at\n+https://developers.google.com/open-source/licenses/bsd\n+*/\n+\n+#include \"test-lib.h\"\n+#include \"reftable/tree.h\"\n+\n+static int t_compare(const void *a, const void *b)\n+{\n+\treturn (char *)a - (char *)b;\n+}\n+\n+struct curry {\n+\tvoid *last;\n+};\n+\n+static void check_increasing(void *arg, void *key)\n+{\n+\tstruct curry *c = arg;\n+\tif (c->last)\n+\t\tcheck_int(t_compare(c->last, key), <, 0);\n+\tc->last = key;\n+}\n+\n+static void t_tree(void)\n+{\n+\tstruct tree_node *root = NULL;\n+\tvoid *values[11] = { 0 };\n+\tstruct tree_node *nodes[11] = { 0 };\n+\tsize_t i = 1;\n+\tstruct curry c = { 0 };\n+\n+\tdo {\n+\t\tnodes[i] = tree_search(values + i, &root, &t_compare, 1);\n+\t\ti = (i * 7) % 11;\n+\t} while (i != 1);\n+\n+\tfor (i = 1; i < ARRAY_SIZE(nodes); i++) {\n+\t\tcheck_pointer_eq(values + i, nodes[i]->key);\n+\t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));\n+\t}\n+\n+\tinfix_walk(root, check_increasing, &c);\n+\ttree_free(root);\n+}\n+\n+int cmd_main(int argc, const char *argv[])\n+{\n+\tTEST(t_tree(), \"tree_search and infix_walk work\");\n+\n+\treturn test_done();\n+}\n-- \n2.45.GIT\n\n"},{"id":"498776","messageId":"20240716075641.4264-4-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240716075641.4264-1-chandrapratap3519@gmail.com","subject":"[PATCH v4 3/5] t-reftable-tree: split test_tree() into two sub-test functions","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-07-16T07:48:15Z","receivedAt":"2024-07-16T07:57:31Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup, tests for both tree_search() and\ninfix_walk() defined by reftable/tree.{c, h} are performed by\na single test function, test_tree(). Split tree_test() into\ntest_tree_search() and test_infix_walk() responsible for\nindependently testing tree_search() and infix_walk() respectively.\nThis improves the overall readability of the test file as well as\nsimplifies debugging.\n\nNote that the last parameter in the tree_search() functiom is\n'int insert' which when set, inserts the key if it is not found\nin the tree. Otherwise, the function returns NULL for such cases.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 23 +++++++++++++++++++----\n 1 file changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex 5df814d983..68b1b31176 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -26,13 +26,12 @@ static void check_increasing(void *arg, void *key)\n \tc->last = key;\n }\n \n-static void t_tree(void)\n+static void t_tree_search(void)\n {\n \tstruct tree_node *root = NULL;\n \tvoid *values[11] = { 0 };\n \tstruct tree_node *nodes[11] = { 0 };\n \tsize_t i = 1;\n-\tstruct curry c = { 0 };\n \n \tdo {\n \t\tnodes[i] = tree_search(values + i, &root, &t_compare, 1);\n@@ -44,13 +43,29 @@ static void t_tree(void)\n \t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));\n \t}\n \n-\tinfix_walk(root, check_increasing, &c);\n+\ttree_free(root);\n+}\n+\n+static void t_infix_walk(void)\n+{\n+\tstruct tree_node *root = NULL;\n+\tvoid *values[11] = { 0 };\n+\tstruct curry c = { 0 };\n+\tsize_t i = 1;\n+\n+\tdo {\n+\t\ttree_search(values + i, &root, &t_compare, 1);\n+\t\ti = (i * 7) % 11;\n+\t} while (i != 1);\n+\n+\tinfix_walk(root, &check_increasing, &c);\n \ttree_free(root);\n }\n \n int cmd_main(int argc, const char *argv[])\n {\n-\tTEST(t_tree(), \"tree_search and infix_walk work\");\n+\tTEST(t_tree_search(), \"tree_search works\");\n+\tTEST(t_infix_walk(), \"infix_walk works\");\n \n \treturn test_done();\n }\n-- \n2.45.GIT\n\n"},{"id":"498777","messageId":"20240716075641.4264-5-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240716075641.4264-1-chandrapratap3519@gmail.com","subject":"[PATCH v4 4/5] t-reftable-tree: add test for non-existent key","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-07-16T07:48:16Z","receivedAt":"2024-07-16T07:57:33Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup for tree_search(), the case for\nnon-existent key is not exercised. Improve this by adding a\ntest-case for the same.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex 68b1b31176..e04e555509 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -43,6 +43,7 @@ static void t_tree_search(void)\n \t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));\n \t}\n \n+\tcheck(!tree_search(values, &root, &test_compare, 0));\n \ttree_free(root);\n }\n \n-- \n2.45.GIT\n\n"},{"id":"498778","messageId":"20240716075641.4264-6-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240716075641.4264-1-chandrapratap3519@gmail.com","subject":"[PATCH v4 5/5] t-reftable-tree: improve the test for infix_walk()","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-07-16T07:48:17Z","receivedAt":"2024-07-16T07:57:36Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup for infix_walk(), the following\nproperties of an infix traversal of a tree remain untested:\n- every node of the tree must be visited\n- every node must be visited exactly once\nIn fact, only the property 'traversal in increasing order' is tested.\nModify test_infix_walk() to check for all the properties above.\n\nThis can be achieved by storing the nodes' keys linearly, in a nullified\nbuffer, as we visit them and then checking the input keys against this\nbuffer in increasing order. By checking that the element just after\nthe last input key is 'NULL' in the output buffer, we ensure that\nevery node is traversed exactly once.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 22 +++++++++++++++-------\n 1 file changed, 15 insertions(+), 7 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex e04e555509..b3d4008e5c 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -15,15 +15,14 @@ static int t_compare(const void *a, const void *b)\n }\n \n struct curry {\n-\tvoid *last;\n+\tvoid **arr;\n+\tsize_t len;\n };\n \n-static void check_increasing(void *arg, void *key)\n+static void store(void *arg, void *key)\n {\n \tstruct curry *c = arg;\n-\tif (c->last)\n-\t\tcheck_int(t_compare(c->last, key), <, 0);\n-\tc->last = key;\n+\tc->arr[c->len++] = key;\n }\n \n static void t_tree_search(void)\n@@ -51,15 +50,24 @@ static void t_infix_walk(void)\n {\n \tstruct tree_node *root = NULL;\n \tvoid *values[11] = { 0 };\n-\tstruct curry c = { 0 };\n+\tvoid *out[11] = { 0 };\n+\tstruct curry c = {\n+\t\t.arr = (void **) &out,\n+\t};\n \tsize_t i = 1;\n+\tsize_t count = 0;\n \n \tdo {\n \t\ttree_search(values + i, &root, &t_compare, 1);\n \t\ti = (i * 7) % 11;\n+\t\tcount++;\n \t} while (i != 1);\n \n-\tinfix_walk(root, &check_increasing, &c);\n+\tinfix_walk(root, &store, &c);\n+\tfor (i = 1; i < ARRAY_SIZE(values); i++)\n+\t\tcheck_pointer_eq(values + i, out[i - 1]);\n+\tcheck(!out[i - 1]);\n+\tcheck_int(c.len, ==, count);\n \ttree_free(root);\n }\n \n-- \n2.45.GIT\n\n"},{"id":"498801","messageId":"xmqqh6cp8688.fsf@gitster.g","threadId":"61614","inReplyTo":"20240716075641.4264-1-chandrapratap3519@gmail.com","subject":"Re: [GSoC][PATCH v4 0/5] t: port reftable/tree_test.c to the unit testing framework","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-16T19:52:23Z","receivedAt":"2024-07-16T19:52:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chandra Pratap writes:\n\n> In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit\n\nConsistently refer to an existing commit with --format=reference.\n\n\t$ git show -s --format=reference 8bf6fbd\n\t8bf6fbd00d (Merge branch 'js/doc-unit-tests', 2023-12-09)\n\n> testing framework written entirely in C was introduced to the Git project\n> aimed at simplifying testing and reducing test run times.\n\nI doubt that the reason why \"unit-tests\" written entirely in C was\nintroduced is because we wanted to simplify testing and to reduce\ntest run times to begin with. The traditional tests to observe the\neffect visible to end-users by actually running commands that would\nbe run by end-users and unit tests serve two separate purposes. The\nlatter does not \"simplify\", or \"reduce time\"---you cannot write a\ntest \"entirely in C\" to make sure, say, that \"git push --force\"\nallows a non-fast-forward update to happen using unit-test\nframework. The unit-tests cannot replace end-to-end tests. They\nare complementary.\n\nThe statement may need to be rethought.\n\nOr just stop at saying something like\n\n The reftable library comes with self tests, which are exercised\n as part of the usual end-to-end tests that are designed to\n observe the end-user visible effects of Git commands. What it\n exercises, however, is a better match to the unit-testing\n framework, merged at 8bf6fbd0 (Merge branch 'js/doc-unit-tests',\n 2023-12-09), that are designed to observe how low level\n implementation details, at the level of sequences of individual\n function calls, behave.\n\nwhich already covers the next paragraph while at it.\n\n> Currently, tests for the reftable refs-backend are performed by a custom\n> testing framework defined by reftable/test_framework.{c, h}. Port\n> reftable/tree_test.c to the unit testing framework and improve upon\n> the ported test.\n>\n> The first patch in the series is preparatory cleanup, the second patch\n> moves the test to the unit testing framework, and the rest of the patches\n> improve upon the ported test.\n>\n> Mentored-by: Patrick Steinhardt \n> Mentored-by: Christian Couder \n> Signed-off-by: Chandra Pratap \n>\n> ---\n> Changes in v4:\n> - Rename the tests to be in-line with unit-tests' standards\n>\n> CI/PR: https://github.com/gitgitgadget/git/pull/1740\n\nBy the way, did you rebase the patches? On which commit is this\niteration based? Judging from the second patch, it seems to assume\nthat Makefile does not yet have t-reftable-record in it, and it\napplies cleanly to 'master' before 9118e46e (Merge branch\n'cp/unit-test-reftable-record', 2024-07-15). Newer 'master' has\ntextual conflicts (nothing that I cannot resolve, but it shows that\napparently that anything newer than 9118e46e are not commits that\nyou developed this series on).\n\nHas this series even been compile-tested? I do not think that even\nthe unit test added by this series has been run (or compiled).\n\n$ make -j32 t/unit-tests/t-reftable-tree.o\nGIT_VERSION = 2.46.0.rc0.27.g0c2075a7c5\n * new build flags\n CC t/unit-tests/t-reftable-tree.o\nIn file included from t/unit-tests/t-reftable-tree.c:9:\nt/unit-tests/t-reftable-tree.c: In function ‘t_tree_search’:\nt/unit-tests/t-reftable-tree.c:45:44: error: ‘test_compare’ undeclared (first use in this function); did you mean ‘t_compare’?\n 45 | check(!tree_search(values, &root, &test_compare, 0));\n | ^~~~~~~~~~~~\nt/unit-tests/test-lib.h:75:45: note: in definition of macro ‘check’\n 75 | check_bool_loc(TEST_LOCATION(), #x, x)\n | ^\nt/unit-tests/t-reftable-tree.c:45:44: note: each undeclared identifier is reported only once for each function it appears in\n 45 | check(!tree_search(values, &root, &test_compare, 0));\n | ^~~~~~~~~~~~\nt/unit-tests/test-lib.h:75:45: note: in definition of macro ‘check’\n 75 | check_bool_loc(TEST_LOCATION(), #x, x)\n | ^\nmake: *** [Makefile:2754: t/unit-tests/t-reftable-tree.o] Error 1\n\n\n\n\n\nLet's concentrate on quality, not quantity; too many topics with the\nsame prefix cp/unit-test* seem to be left unreviewed on the list.\n\nIn the meantime, I'll queue the following fix-up on top. In this\ncodebase, it is preferred to write a pointer to a function whose\nname is \"func\" as just \"func\", not \"&func\".\n\n\n t/unit-tests/t-reftable-tree.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex b3d4008e5c..107f1f69bf 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -33,16 +33,16 @@ static void t_tree_search(void)\n \tsize_t i = 1;\n \n \tdo {\n-\t\tnodes[i] = tree_search(values + i, &root, &t_compare, 1);\n+\t\tnodes[i] = tree_search(values + i, &root, t_compare, 1);\n \t\ti = (i * 7) % 11;\n \t} while (i != 1);\n \n \tfor (i = 1; i < ARRAY_SIZE(nodes); i++) {\n \t\tcheck_pointer_eq(values + i, nodes[i]->key);\n-\t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));\n+\t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, t_compare, 0));\n \t}\n \n-\tcheck(!tree_search(values, &root, &test_compare, 0));\n+\tcheck(!tree_search(values, &root, t_compare, 0));\n \ttree_free(root);\n }\n \n@@ -58,7 +58,7 @@ static void t_infix_walk(void)\n \tsize_t count = 0;\n \n \tdo {\n-\t\ttree_search(values + i, &root, &t_compare, 1);\n+\t\ttree_search(values + i, &root, t_compare, 1);\n \t\ti = (i * 7) % 11;\n \t\tcount++;\n \t} while (i != 1);\n-- \n2.46.0-rc0-137-g851401d64b\n\n"},{"id":"498823","messageId":"CAOLa=ZQ7xQFKZ9Oeo0WyrgzvjCvNm4dbgatp0JTvP33sUQ_3fw@mail.gmail.com","threadId":"61614","inReplyTo":"20240716075641.4264-3-chandrapratap3519@gmail.com","subject":"Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-17T11:49:00Z","receivedAt":"2024-07-17T11:49:03Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Chandra Pratap writes:\n\n> reftable/tree_test.c exercises the functions defined in\n> reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit\n> testing framework. Migration involves refactoring the tests to use\n> the unit testing framework instead of reftable's test framework and\n> renaming the tests to align with unit-tests' standards.\n>\n\nNit: it would be nice to mention that this commit copies it over as-is\n(mostly) and the upcoming commits do refactoring. This would really help\nreviewers.\n\n> Mentored-by: Patrick Steinhardt \n> Mentored-by: Christian Couder \n> Signed-off-by: Chandra Pratap \n> ---\n> Makefile | 2 +-\n> reftable/reftable-tests.h | 1 -\n> reftable/tree_test.c | 60 ----------------------------------\n> t/helper/test-reftable.c | 1 -\n> t/unit-tests/t-reftable-tree.c | 56 +++++++++++++++++++++++++++++++\n> 5 files changed, 57 insertions(+), 63 deletions(-)\n> delete mode 100644 reftable/tree_test.c\n> create mode 100644 t/unit-tests/t-reftable-tree.c\n>\n> diff --git a/Makefile b/Makefile\n> index 3eab701b10..79e86ddf53 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1340,6 +1340,7 @@ UNIT_TEST_PROGRAMS += t-mem-pool\n> 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-tree\n> UNIT_TEST_PROGRAMS += t-strbuf\n> UNIT_TEST_PROGRAMS += t-strcmp-offset\n> UNIT_TEST_PROGRAMS += t-strvec\n> @@ -2685,7 +2686,6 @@ REFTABLE_TEST_OBJS += reftable/record_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\n>\n> TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))\n>\n> diff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h\n> index 114cc3d053..d0abcc51e2 100644\n> --- a/reftable/reftable-tests.h\n> +++ b/reftable/reftable-tests.h\n> @@ -16,7 +16,6 @@ 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);\n>\n> #endif\n> diff --git a/reftable/tree_test.c b/reftable/tree_test.c\n> deleted file mode 100644\n> index 6961a657ad..0000000000\n> --- a/reftable/tree_test.c\n> +++ /dev/null\n> @@ -1,60 +0,0 @@\n> -/*\n> -Copyright 2020 Google LLC\n> -\n> -Use of this source code is governed by a BSD-style\n> -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> -#include \"tree.h\"\n> -\n> -#include \"test_framework.h\"\n> -#include \"reftable-tests.h\"\n> -\n> -static int test_compare(const void *a, const void *b)\n> -{\n> -\treturn (char *)a - (char *)b;\n> -}\n> -\n> -struct curry {\n> -\tvoid *last;\n> -};\n> -\n> -static void check_increasing(void *arg, void *key)\n> -{\n> -\tstruct curry *c = arg;\n> -\tif (c->last) {\n> -\t\tEXPECT(test_compare(c->last, key) < 0);\n> -\t}\n> -\tc->last = key;\n> -}\n> -\n> -static void test_tree(void)\n> -{\n> -\tstruct tree_node *root = NULL;\n> -\n> -\tvoid *values[11] = { NULL };\n> -\tstruct tree_node *nodes[11] = { NULL };\n> -\tint i = 1;\n> -\tstruct curry c = { NULL };\n> -\tdo {\n> -\t\tnodes[i] = tree_search(values + i, &root, &test_compare, 1);\n> -\t\ti = (i * 7) % 11;\n> -\t} while (i != 1);\n> -\n> -\tfor (i = 1; i < ARRAY_SIZE(nodes); i++) {\n> -\t\tEXPECT(values + i == nodes[i]->key);\n> -\t\tEXPECT(nodes[i] ==\n> -\t\t tree_search(values + i, &root, &test_compare, 0));\n> -\t}\n> -\n> -\tinfix_walk(root, check_increasing, &c);\n> -\ttree_free(root);\n> -}\n> -\n> -int tree_test_main(int argc, const char *argv[])\n> -{\n> -\tRUN_TEST(test_tree);\n> -\treturn 0;\n> -}\n> diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c\n> index 9160bc5da6..245b674a3c 100644\n> --- a/t/helper/test-reftable.c\n> +++ b/t/helper/test-reftable.c\n> @@ -7,7 +7,6 @@ int cmd__reftable(int argc, const char **argv)\n> \t/* test from simple to complex. */\n> \trecord_test_main(argc, 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> \tmerged_test_main(argc, argv);\n> diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\n> new file mode 100644\n> index 0000000000..5df814d983\n> --- /dev/null\n> +++ b/t/unit-tests/t-reftable-tree.c\n> @@ -0,0 +1,56 @@\n> +/*\n> +Copyright 2020 Google LLC\n> +\n> +Use of this source code is governed by a BSD-style\n> +license that can be found in the LICENSE file or at\n> +https://developers.google.com/open-source/licenses/bsd\n> +*/\n> +\n> +#include \"test-lib.h\"\n> +#include \"reftable/tree.h\"\n> +\n> +static int t_compare(const void *a, const void *b)\n> +{\n> +\treturn (char *)a - (char *)b;\n> +}\n> +\n> +struct curry {\n> +\tvoid *last;\n> +};\n> +\n> +static void check_increasing(void *arg, void *key)\n> +{\n> +\tstruct curry *c = arg;\n> +\tif (c->last)\n> +\t\tcheck_int(t_compare(c->last, key), <, 0);\n> +\tc->last = key;\n> +}\n> +\n> +static void t_tree(void)\n> +{\n> +\tstruct tree_node *root = NULL;\n> +\tvoid *values[11] = { 0 };\n> +\tstruct tree_node *nodes[11] = { 0 };\n> +\tsize_t i = 1;\n> +\tstruct curry c = { 0 };\n> +\n> +\tdo {\n> +\t\tnodes[i] = tree_search(values + i, &root, &t_compare, 1);\n> +\t\ti = (i * 7) % 11;\n> +\t} while (i != 1);\n> +\n> +\tfor (i = 1; i < ARRAY_SIZE(nodes); i++) {\n> +\t\tcheck_pointer_eq(values + i, nodes[i]->key);\n> +\t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));\n> +\t}\n> +\n> +\tinfix_walk(root, check_increasing, &c);\n> +\ttree_free(root);\n> +}\n> +\n> +int cmd_main(int argc, const char *argv[])\n> +{\n> +\tTEST(t_tree(), \"tree_search and infix_walk work\");\n> +\n> +\treturn test_done();\n> +}\n> --\n> 2.45.GIT\n"},{"id":"498824","messageId":"CAOLa=ZQxDXDDWXQrt9kpykuMr6nxSA8uf2U2nu0ChTf3yuH8sQ@mail.gmail.com","threadId":"61614","inReplyTo":"20240716075641.4264-3-chandrapratap3519@gmail.com","subject":"Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-17T12:39:13Z","receivedAt":"2024-07-17T12:39:15Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Chandra Pratap writes:\n\n> reftable/tree_test.c exercises the functions defined in\n> reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit\n> testing framework. Migration involves refactoring the tests to use\n> the unit testing framework instead of reftable's test framework and\n> renaming the tests to align with unit-tests' standards.\n>\n\nOn second thought, it's easier for me to review here for the existing\nstate of the code. So let me do that..\n\n[snip]\n\n> diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\n> new file mode 100644\n> index 0000000000..5df814d983\n> --- /dev/null\n> +++ b/t/unit-tests/t-reftable-tree.c\n> @@ -0,0 +1,56 @@\n> +/*\n> +Copyright 2020 Google LLC\n> +\n> +Use of this source code is governed by a BSD-style\n> +license that can be found in the LICENSE file or at\n> +https://developers.google.com/open-source/licenses/bsd\n> +*/\n> +\n> +#include \"test-lib.h\"\n> +#include \"reftable/tree.h\"\n> +\n> +static int t_compare(const void *a, const void *b)\n> +{\n> +\treturn (char *)a - (char *)b;\n> +}\n> +\n\nSo this is the comparison code, and we're expecting the values to be a\ncharacter. Okay.\n\n> +struct curry {\n> +\tvoid *last;\n> +};\n> +\n> +static void check_increasing(void *arg, void *key)\n> +{\n> +\tstruct curry *c = arg;\n> +\tif (c->last)\n> +\t\tcheck_int(t_compare(c->last, key), <, 0);\n> +\tc->last = key;\n> +}\n> +\n> +static void t_tree(void)\n> +{\n> +\tstruct tree_node *root = NULL;\n> +\tvoid *values[11] = { 0 };\n\nAlthough we were comparing 'char' above, here we have a 'void *' array.\nWhy?\n\n> +\tstruct tree_node *nodes[11] = { 0 };\n> +\tsize_t i = 1;\n> +\tstruct curry c = { 0 };\n> +\n> +\tdo {\n> +\t\tnodes[i] = tree_search(values + i, &root, &t_compare, 1);\n> +\t\ti = (i * 7) % 11;\n\nIt gets weirder, we calculate 'i' as {7, 5, 2, 3, 10, 4, 6, 9, 8, 1}. We\nuse that to index 'values', but values is '0' initialized, so we always\nsend '0' to tree_search? Doesn't that make this whole thing a moot? Or\ndid I miss something?\n\n> +\t} while (i != 1);\n> +\n> +\tfor (i = 1; i < ARRAY_SIZE(nodes); i++) {\n> +\t\tcheck_pointer_eq(values + i, nodes[i]->key);\n> +\t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));\n> +\t}\n> +\n> +\tinfix_walk(root, check_increasing, &c);\n> +\ttree_free(root);\n> +}\n> +\n> +int cmd_main(int argc, const char *argv[])\n> +{\n> +\tTEST(t_tree(), \"tree_search and infix_walk work\");\n> +\n> +\treturn test_done();\n> +}\n> --\n> 2.45.GIT\n"},{"id":"498827","messageId":"CA+J6zkQpNiM1UpGRzkNP9o04cEL4ip-YFkQvXk2zgSjpE4uGBw@mail.gmail.com","threadId":"61614","inReplyTo":"CAOLa=ZQ7xQFKZ9Oeo0WyrgzvjCvNm4dbgatp0JTvP33sUQ_3fw@mail.gmail.com","subject":"Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-07-17T13:50:43Z","receivedAt":"2024-07-17T13:50:55Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"On Wed, 17 Jul 2024 at 17:19, Karthik Nayak wrote:\n>\n> Chandra Pratap writes:\n>\n> > reftable/tree_test.c exercises the functions defined in\n> > reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit\n> > testing framework. Migration involves refactoring the tests to use\n> > the unit testing framework instead of reftable's test framework and\n> > renaming the tests to align with unit-tests' standards.\n> >\n>\n> Nit: it would be nice to mention that this commit copies it over as-is\n> (mostly) and the upcoming commits do refactoring. This would really help\n> reviewers.\n\nI do mention that in the patch cover letter, specifically this paragraph:\n\n> The first patch in the series is preparatory cleanup, the second patch\n> moves the test to the unit testing framework, and the rest of the patches\n> improve upon the ported test.\n\nWould it be better if I transport that here?\n\n----[snip]----\n"},{"id":"498828","messageId":"CA+J6zkQLXsDdSa5xjizr82bPUCng0-XZJRNQ1CAV7ttDbE03xA@mail.gmail.com","threadId":"61614","inReplyTo":"CAOLa=ZQxDXDDWXQrt9kpykuMr6nxSA8uf2U2nu0ChTf3yuH8sQ@mail.gmail.com","subject":"Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-07-17T14:30:35Z","receivedAt":"2024-07-17T14:30:47Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"On Wed, 17 Jul 2024 at 18:09, Karthik Nayak wrote:\n>\n> Chandra Pratap writes:\n>\n> > reftable/tree_test.c exercises the functions defined in\n> > reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit\n> > testing framework. Migration involves refactoring the tests to use\n> > the unit testing framework instead of reftable's test framework and\n> > renaming the tests to align with unit-tests' standards.\n> >\n>\n> On second thought, it's easier for me to review here for the existing\n> state of the code. So let me do that..\n>\n> [snip]\n>\n> > diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\n> > new file mode 100644\n> > index 0000000000..5df814d983\n> > --- /dev/null\n> > +++ b/t/unit-tests/t-reftable-tree.c\n> > @@ -0,0 +1,56 @@\n> > +/*\n> > +Copyright 2020 Google LLC\n> > +\n> > +Use of this source code is governed by a BSD-style\n> > +license that can be found in the LICENSE file or at\n> > +https://developers.google.com/open-source/licenses/bsd\n> > +*/\n> > +\n> > +#include \"test-lib.h\"\n> > +#include \"reftable/tree.h\"\n> > +\n> > +static int t_compare(const void *a, const void *b)\n> > +{\n> > + return (char *)a - (char *)b;\n> > +}\n> > +\n>\n> So this is the comparison code, and we're expecting the values to be a\n> character. Okay.\n\nWe're actually expecting the values 'a' and 'b' to be of the type (char *),\nwhich is a pointer to a character, and thus we perform the comparison on\nthe basis of pointer arithmetic.\n\n> > +struct curry {\n> > + void *last;\n> > +};\n> > +\n> > +static void check_increasing(void *arg, void *key)\n> > +{\n> > + struct curry *c = arg;\n> > + if (c->last)\n> > + check_int(t_compare(c->last, key), <, 0);\n> > + c->last = key;\n> > +}\n> > +\n> > +static void t_tree(void)\n> > +{\n> > + struct tree_node *root = NULL;\n> > + void *values[11] = { 0 };\n>\n> Although we were comparing 'char' above, here we have a 'void *' array.\n> Why?\n\nThe array is passed as a parameter to the 'tree_search()' function which\nrequires a void * parameter (i.e. a generic pointer). In the comparison\nfunction (also passed as a parameter), we cast it to our expected type\n(a character pointer) and then perform the required comparison.\n\n> > + struct tree_node *nodes[11] = { 0 };\n> > + size_t i = 1;\n> > + struct curry c = { 0 };\n> > +\n> > + do {\n> > + nodes[i] = tree_search(values + i, &root, &t_compare, 1);\n> > + i = (i * 7) % 11;\n>\n> It gets weirder, we calculate 'i' as {7, 5, 2, 3, 10, 4, 6, 9, 8, 1}. We\n> use that to index 'values', but values is '0' initialized, so we always\n> send '0' to tree_search? Doesn't that make this whole thing a moot? Or\n> did I miss something?\n\nWe don't use 'i' to index 'values[]', we use it to calculate the next pointer\naddress to be passed to the 'tree_search()' function (the pointer that is 'i'\nahead of the pointer 'values'), which isn't 0.\n\n> > + } while (i != 1);\n> > +\n> > + for (i = 1; i < ARRAY_SIZE(nodes); i++) {\n> > + check_pointer_eq(values + i, nodes[i]->key);\n> > + check_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));\n> > + }\n> > +\n> > + infix_walk(root, check_increasing, &c);\n> > + tree_free(root);\n> > +}\n> > +\n> > +int cmd_main(int argc, const char *argv[])\n> > +{\n> > + TEST(t_tree(), \"tree_search and infix_walk work\");\n> > +\n> > + return test_done();\n> > +}\n> > --\n> > 2.45.GIT\n"},{"id":"498881","messageId":"irlg4rgsbfedphbxemj6pns35aceuwqrth6gyj6hi56fmr25n6@yvp4od2ydc26","threadId":"61614","inReplyTo":"CA+J6zkQLXsDdSa5xjizr82bPUCng0-XZJRNQ1CAV7ttDbE03xA@mail.gmail.com","subject":"Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-07-17T22:14:50Z","receivedAt":"2024-07-17T22:15:29Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/07/17 08:00PM, Chandra Pratap wrote:\n> On Wed, 17 Jul 2024 at 18:09, Karthik Nayak wrote:\n> >\n> > Chandra Pratap writes:\n> >\n> > > +struct curry {\n> > > + void *last;\n> > > +};\n> > > +\n> > > +static void check_increasing(void *arg, void *key)\n> > > +{\n> > > + struct curry *c = arg;\n> > > + if (c->last)\n> > > + check_int(t_compare(c->last, key), <, 0);\n> > > + c->last = key;\n> > > +}\n> > > +\n> > > +static void t_tree(void)\n> > > +{\n> > > + struct tree_node *root = NULL;\n> > > + void *values[11] = { 0 };\n> >\n> > Although we were comparing 'char' above, here we have a 'void *' array.\n> > Why?\n> \n> The array is passed as a parameter to the 'tree_search()' function which\n> requires a void * parameter (i.e. a generic pointer). In the comparison\n> function (also passed as a parameter), we cast it to our expected type\n> (a character pointer) and then perform the required comparison.\n\nThe point of `values` is to provide a set of values of type `void **` to\nbe inserted in the tree. As far as I can tell, there is no reason for\n`values` to be initialized to begin with and is a bit misleading. Might\nbe reasonable to remove its initialization here.\n\n> > > + struct tree_node *nodes[11] = { 0 };\n> > > + size_t i = 1;\n> > > + struct curry c = { 0 };\n> > > +\n> > > + do {\n> > > + nodes[i] = tree_search(values + i, &root, &t_compare, 1);\n> > > + i = (i * 7) % 11;\n> >\n> > It gets weirder, we calculate 'i' as {7, 5, 2, 3, 10, 4, 6, 9, 8, 1}. We\n> > use that to index 'values', but values is '0' initialized, so we always\n> > send '0' to tree_search? Doesn't that make this whole thing a moot? Or\n> > did I miss something?\n> \n> We don't use 'i' to index 'values[]', we use it to calculate the next pointer\n> address to be passed to the 'tree_search()' function (the pointer that is 'i'\n> ahead of the pointer 'values'), which isn't 0.\n\nThe `i = (i * 7) % 11;` is used to deterministically generate numbers\n1-10 in a psuedo-random fashion. These numbers are used as memory\noffsets to be inserted into the tree. I suspect the psuedo-randomness is\nuseful keys should be ordered when inserted into the tree and that is\nlater validated as part of the in-order traversal that is performed.\n\nWhile rather compact, I find the test setup here to rather difficult to\nparse. It might be a good idea to either provide comments explaining\nthis test setup or consider refactoring it. Honestly, I'd personally\nperfer the tree setup be done more explicitly as I think it would make\nunderstanding the test much easier.\n\n-Justin\n"},{"id":"498898","messageId":"CA+J6zkRWzq_XNf-E7Nx4_D7rOOjN4Bwi4g3zgzV6YCiVoCe8rA@mail.gmail.com","threadId":"61614","inReplyTo":"irlg4rgsbfedphbxemj6pns35aceuwqrth6gyj6hi56fmr25n6@yvp4od2ydc26","subject":"Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-07-18T04:58:07Z","receivedAt":"2024-07-18T04:58:20Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"On Thu, 18 Jul 2024 at 03:45, Justin Tobler wrote:\n>\n> On 24/07/17 08:00PM, Chandra Pratap wrote:\n> > On Wed, 17 Jul 2024 at 18:09, Karthik Nayak wrote:\n> > >\n> > > Chandra Pratap writes:\n> > >\n> > > > +struct curry {\n> > > > + void *last;\n> > > > +};\n> > > > +\n> > > > +static void check_increasing(void *arg, void *key)\n> > > > +{\n> > > > + struct curry *c = arg;\n> > > > + if (c->last)\n> > > > + check_int(t_compare(c->last, key), <, 0);\n> > > > + c->last = key;\n> > > > +}\n> > > > +\n> > > > +static void t_tree(void)\n> > > > +{\n> > > > + struct tree_node *root = NULL;\n> > > > + void *values[11] = { 0 };\n> > >\n> > > Although we were comparing 'char' above, here we have a 'void *' array.\n> > > Why?\n> >\n> > The array is passed as a parameter to the 'tree_search()' function which\n> > requires a void * parameter (i.e. a generic pointer). In the comparison\n> > function (also passed as a parameter), we cast it to our expected type\n> > (a character pointer) and then perform the required comparison.\n>\n> The point of `values` is to provide a set of values of type `void **` to\n> be inserted in the tree. As far as I can tell, there is no reason for\n> `values` to be initialized to begin with and is a bit misleading. Might\n> be reasonable to remove its initialization here.\n\nThe thing is, the values[] array being 0-initialized makes debugging\na lot easier in the case of a test failure, so I'm not very sure about\ngetting rid of the initialization here.\n\n> > > > + struct tree_node *nodes[11] = { 0 };\n> > > > + size_t i = 1;\n> > > > + struct curry c = { 0 };\n> > > > +\n> > > > + do {\n> > > > + nodes[i] = tree_search(values + i, &root, &t_compare, 1);\n> > > > + i = (i * 7) % 11;\n> > >\n> > > It gets weirder, we calculate 'i' as {7, 5, 2, 3, 10, 4, 6, 9, 8, 1}. We\n> > > use that to index 'values', but values is '0' initialized, so we always\n> > > send '0' to tree_search? Doesn't that make this whole thing a moot? Or\n> > > did I miss something?\n> >\n> > We don't use 'i' to index 'values[]', we use it to calculate the next pointer\n> > address to be passed to the 'tree_search()' function (the pointer that is 'i'\n> > ahead of the pointer 'values'), which isn't 0.\n>\n> The `i = (i * 7) % 11;` is used to deterministically generate numbers\n> 1-10 in a psuedo-random fashion. These numbers are used as memory\n> offsets to be inserted into the tree. I suspect the psuedo-randomness is\n> useful keys should be ordered when inserted into the tree and that is\n> later validated as part of the in-order traversal that is performed.\n\nThat's right, the randomness of the insertion order is helpful in validating\nthat the tree-functions 'tree_search()' and 'infix_walk()' work according\nto their defined behaviour.\n\n> While rather compact, I find the test setup here to rather difficult to\n> parse. It might be a good idea to either provide comments explaining\n> this test setup or consider refactoring it. Honestly, I'd personally\n> perfer the tree setup be done more explicitly as I think it would make\n> understanding the test much easier.\n\nThis probably ties in with the comments by Patrick on the previous iteration\nof this patch, that using 'tree_search()' to insert tree nodes leads to\nconfusion. Solving that would require efforts outside the scope of this\npatch series though, so I'm more inclined towards providing comments\nand other ways of simplifying this subroutine.\n"},{"id":"498900","messageId":"CAOLa=ZTJ_oyqmR0ktJa=AABy290Z7RNLc0_ES9UNB=zXY0-5zA@mail.gmail.com","threadId":"61614","inReplyTo":"CA+J6zkQpNiM1UpGRzkNP9o04cEL4ip-YFkQvXk2zgSjpE4uGBw@mail.gmail.com","subject":"Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-18T08:04:02Z","receivedAt":"2024-07-18T08:04:04Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Chandra Pratap writes:\n\n> On Wed, 17 Jul 2024 at 17:19, Karthik Nayak wrote:\n>>\n>> Chandra Pratap writes:\n>>\n>> > reftable/tree_test.c exercises the functions defined in\n>> > reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit\n>> > testing framework. Migration involves refactoring the tests to use\n>> > the unit testing framework instead of reftable's test framework and\n>> > renaming the tests to align with unit-tests' standards.\n>> >\n>>\n>> Nit: it would be nice to mention that this commit copies it over as-is\n>> (mostly) and the upcoming commits do refactoring. This would really help\n>> reviewers.\n>\n> I do mention that in the patch cover letter, specifically this paragraph:\n>\n>> The first patch in the series is preparatory cleanup, the second patch\n>> moves the test to the unit testing framework, and the rest of the patches\n>> improve upon the ported test.\n>\n> Would it be better if I transport that here?\n>\n> ----[snip]----\n\nI think that would be nice!\n"},{"id":"498901","messageId":"CAOLa=ZRGV1-3cDxgJd9zENTCEPqz04AFwWmkQnYwj5Cbg=EmfA@mail.gmail.com","threadId":"61614","inReplyTo":"CA+J6zkRWzq_XNf-E7Nx4_D7rOOjN4Bwi4g3zgzV6YCiVoCe8rA@mail.gmail.com","subject":"Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-18T08:10:31Z","receivedAt":"2024-07-18T08:10:32Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Chandra Pratap writes:\n\n> On Thu, 18 Jul 2024 at 03:45, Justin Tobler wrote:\n>>\n>> On 24/07/17 08:00PM, Chandra Pratap wrote:\n>> > On Wed, 17 Jul 2024 at 18:09, Karthik Nayak wrote:\n>> > >\n>> > > Chandra Pratap writes:\n>> > >\n>> > > > +struct curry {\n>> > > > + void *last;\n>> > > > +};\n>> > > > +\n>> > > > +static void check_increasing(void *arg, void *key)\n>> > > > +{\n>> > > > + struct curry *c = arg;\n>> > > > + if (c->last)\n>> > > > + check_int(t_compare(c->last, key), <, 0);\n>> > > > + c->last = key;\n>> > > > +}\n>> > > > +\n>> > > > +static void t_tree(void)\n>> > > > +{\n>> > > > + struct tree_node *root = NULL;\n>> > > > + void *values[11] = { 0 };\n>> > >\n>> > > Although we were comparing 'char' above, here we have a 'void *' array.\n>> > > Why?\n>> >\n>> > The array is passed as a parameter to the 'tree_search()' function which\n>> > requires a void * parameter (i.e. a generic pointer). In the comparison\n>> > function (also passed as a parameter), we cast it to our expected type\n>> > (a character pointer) and then perform the required comparison.\n>>\n>> The point of `values` is to provide a set of values of type `void **` to\n>> be inserted in the tree. As far as I can tell, there is no reason for\n>> `values` to be initialized to begin with and is a bit misleading. Might\n>> be reasonable to remove its initialization here.\n>\n> The thing is, the values[] array being 0-initialized makes debugging\n> a lot easier in the case of a test failure, so I'm not very sure about\n> getting rid of the initialization here.\n>\n>> > > > + struct tree_node *nodes[11] = { 0 };\n>> > > > + size_t i = 1;\n>> > > > + struct curry c = { 0 };\n>> > > > +\n>> > > > + do {\n>> > > > + nodes[i] = tree_search(values + i, &root, &t_compare, 1);\n>> > > > + i = (i * 7) % 11;\n>> > >\n>> > > It gets weirder, we calculate 'i' as {7, 5, 2, 3, 10, 4, 6, 9, 8, 1}. We\n>> > > use that to index 'values', but values is '0' initialized, so we always\n>> > > send '0' to tree_search? Doesn't that make this whole thing a moot? Or\n>> > > did I miss something?\n>> >\n>> > We don't use 'i' to index 'values[]', we use it to calculate the next pointer\n>> > address to be passed to the 'tree_search()' function (the pointer that is 'i'\n>> > ahead of the pointer 'values'), which isn't 0.\n>>\n>> The `i = (i * 7) % 11;` is used to deterministically generate numbers\n>> 1-10 in a psuedo-random fashion. These numbers are used as memory\n>> offsets to be inserted into the tree. I suspect the psuedo-randomness is\n>> useful keys should be ordered when inserted into the tree and that is\n>> later validated as part of the in-order traversal that is performed.\n>\n> That's right, the randomness of the insertion order is helpful in validating\n> that the tree-functions 'tree_search()' and 'infix_walk()' work according\n> to their defined behaviour.\n>\n>> While rather compact, I find the test setup here to rather difficult to\n>> parse. It might be a good idea to either provide comments explaining\n>> this test setup or consider refactoring it. Honestly, I'd personally\n>> perfer the tree setup be done more explicitly as I think it would make\n>> understanding the test much easier.\n>\n> This probably ties in with the comments by Patrick on the previous iteration\n> of this patch, that using 'tree_search()' to insert tree nodes leads to\n> confusion. Solving that would require efforts outside the scope of this\n> patch series though, so I'm more inclined towards providing comments\n> and other ways of simplifying this subroutine.\n\nAgreed that refactoring `tree_search()` probably is out of scope here.\nBut rewriting the test is definitely something we can do.\n\nPerhaps:\n\nstatic void t_tree(void)\n{\n\tstruct tree_node *root = NULL;\n\tint values[11] = {7, 5, 2, 3, 10, 4, 6, 9, 8, 1};\n\tstruct tree_node *nodes[11] = { 0 };\n\tsize_t i = 1;\n\tstruct curry c = { 0 };\n\n // Insert values to the tree by passing '1' as the last argument.\n for (i = 1; i < ARRAY_SIZE(values); i++) {\n\t\tnodes[i] = tree_search(&values[i], &root, &t_compare, 1);\n }\n\t\n\tfor (i = 1; i < ARRAY_SIZE(nodes); i++) {\n\t\tcheck_pointer_eq(values[i], nodes[i]->key);\n\t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));\n\t}\n\n\tinfix_walk(root, check_increasing, &c);\n\ttree_free(root);\n}\n\nWouldn't this have the same effect while making it much easier to read?\n"},{"id":"498910","messageId":"CA+J6zkSBap2OehD+fbK3cMtVBJgU4kb5C9-4Jk4kafqTgum14Q@mail.gmail.com","threadId":"61614","inReplyTo":"CAOLa=ZRGV1-3cDxgJd9zENTCEPqz04AFwWmkQnYwj5Cbg=EmfA@mail.gmail.com","subject":"Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-07-18T08:23:01Z","receivedAt":"2024-07-18T08:23:13Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"On Thu, 18 Jul 2024 at 13:40, Karthik Nayak wrote:\n>\n> Chandra Pratap writes:\n>\n> > On Thu, 18 Jul 2024 at 03:45, Justin Tobler wrote:\n> >>\n> >> On 24/07/17 08:00PM, Chandra Pratap wrote:\n> >> > On Wed, 17 Jul 2024 at 18:09, Karthik Nayak wrote:\n> >> > >\n> >> > > Chandra Pratap writes:\n> >> > >\n> >> > > > +struct curry {\n> >> > > > + void *last;\n> >> > > > +};\n> >> > > > +\n> >> > > > +static void check_increasing(void *arg, void *key)\n> >> > > > +{\n> >> > > > + struct curry *c = arg;\n> >> > > > + if (c->last)\n> >> > > > + check_int(t_compare(c->last, key), <, 0);\n> >> > > > + c->last = key;\n> >> > > > +}\n> >> > > > +\n> >> > > > +static void t_tree(void)\n> >> > > > +{\n> >> > > > + struct tree_node *root = NULL;\n> >> > > > + void *values[11] = { 0 };\n> >> > >\n> >> > > Although we were comparing 'char' above, here we have a 'void *' array.\n> >> > > Why?\n> >> >\n> >> > The array is passed as a parameter to the 'tree_search()' function which\n> >> > requires a void * parameter (i.e. a generic pointer). In the comparison\n> >> > function (also passed as a parameter), we cast it to our expected type\n> >> > (a character pointer) and then perform the required comparison.\n> >>\n> >> The point of `values` is to provide a set of values of type `void **` to\n> >> be inserted in the tree. As far as I can tell, there is no reason for\n> >> `values` to be initialized to begin with and is a bit misleading. Might\n> >> be reasonable to remove its initialization here.\n> >\n> > The thing is, the values[] array being 0-initialized makes debugging\n> > a lot easier in the case of a test failure, so I'm not very sure about\n> > getting rid of the initialization here.\n> >\n> >> > > > + struct tree_node *nodes[11] = { 0 };\n> >> > > > + size_t i = 1;\n> >> > > > + struct curry c = { 0 };\n> >> > > > +\n> >> > > > + do {\n> >> > > > + nodes[i] = tree_search(values + i, &root, &t_compare, 1);\n> >> > > > + i = (i * 7) % 11;\n> >> > >\n> >> > > It gets weirder, we calculate 'i' as {7, 5, 2, 3, 10, 4, 6, 9, 8, 1}. We\n> >> > > use that to index 'values', but values is '0' initialized, so we always\n> >> > > send '0' to tree_search? Doesn't that make this whole thing a moot? Or\n> >> > > did I miss something?\n> >> >\n> >> > We don't use 'i' to index 'values[]', we use it to calculate the next pointer\n> >> > address to be passed to the 'tree_search()' function (the pointer that is 'i'\n> >> > ahead of the pointer 'values'), which isn't 0.\n> >>\n> >> The `i = (i * 7) % 11;` is used to deterministically generate numbers\n> >> 1-10 in a psuedo-random fashion. These numbers are used as memory\n> >> offsets to be inserted into the tree. I suspect the psuedo-randomness is\n> >> useful keys should be ordered when inserted into the tree and that is\n> >> later validated as part of the in-order traversal that is performed.\n> >\n> > That's right, the randomness of the insertion order is helpful in validating\n> > that the tree-functions 'tree_search()' and 'infix_walk()' work according\n> > to their defined behaviour.\n> >\n> >> While rather compact, I find the test setup here to rather difficult to\n> >> parse. It might be a good idea to either provide comments explaining\n> >> this test setup or consider refactoring it. Honestly, I'd personally\n> >> perfer the tree setup be done more explicitly as I think it would make\n> >> understanding the test much easier.\n> >\n> > This probably ties in with the comments by Patrick on the previous iteration\n> > of this patch, that using 'tree_search()' to insert tree nodes leads to\n> > confusion. Solving that would require efforts outside the scope of this\n> > patch series though, so I'm more inclined towards providing comments\n> > and other ways of simplifying this subroutine.\n>\n> Agreed that refactoring `tree_search()` probably is out of scope here.\n> But rewriting the test is definitely something we can do.\n>\n> Perhaps:\n>\n> static void t_tree(void)\n> {\n> struct tree_node *root = NULL;\n> int values[11] = {7, 5, 2, 3, 10, 4, 6, 9, 8, 1};\n> struct tree_node *nodes[11] = { 0 };\n> size_t i = 1;\n> struct curry c = { 0 };\n>\n> // Insert values to the tree by passing '1' as the last argument.\n> for (i = 1; i < ARRAY_SIZE(values); i++) {\n> nodes[i] = tree_search(&values[i], &root, &t_compare, 1);\n> }\n>\n> for (i = 1; i < ARRAY_SIZE(nodes); i++) {\n> check_pointer_eq(values[i], nodes[i]->key);\n> check_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));\n> }\n>\n> infix_walk(root, check_increasing, &c);\n> tree_free(root);\n> }\n>\n> Wouldn't this have the same effect while making it much easier to read?\n\nI agree that the change 'values + i -> &values[i]' is a net positive, I had this\nchange in mind as well. This comment on the other hand,\n> // Insert values to the tree by passing '1' as the last argument.\nhas already been stated in the commit message of the 3rd patch\nas was suggested by Patrick earlier:\n\n'Note that the last parameter in the tree_search() function is\n'int insert' which when set, inserts the key if it is not found\nin the tree. Otherwise, the function returns NULL for such cases.'\n\nSo I was thinking of adding something along the lines of:\n'// pseudo-randomly insert pointers to elements between values[1]\nand values[10] in the tree'\n"},{"id":"498927","messageId":"zjaj4aqjp6aa2vevgmhrrxegaum4nvv26kq3olru5z26bqd33l@4mgzgenvib4q","threadId":"61614","inReplyTo":"CAOLa=ZRGV1-3cDxgJd9zENTCEPqz04AFwWmkQnYwj5Cbg=EmfA@mail.gmail.com","subject":"Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-07-18T15:26:00Z","receivedAt":"2024-07-18T15:26:36Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/07/18 01:10AM, Karthik Nayak wrote:\n> Chandra Pratap writes:\n> \n> > On Thu, 18 Jul 2024 at 03:45, Justin Tobler wrote:\n> >>\n> >> On 24/07/17 08:00PM, Chandra Pratap wrote:\n>\n> >> The `i = (i * 7) % 11;` is used to deterministically generate numbers\n> >> 1-10 in a psuedo-random fashion. These numbers are used as memory\n> >> offsets to be inserted into the tree. I suspect the psuedo-randomness is\n> >> useful keys should be ordered when inserted into the tree and that is\n> >> later validated as part of the in-order traversal that is performed.\n> >\n> > That's right, the randomness of the insertion order is helpful in validating\n> > that the tree-functions 'tree_search()' and 'infix_walk()' work according\n> > to their defined behaviour.\n> >\n> >> While rather compact, I find the test setup here to rather difficult to\n> >> parse. It might be a good idea to either provide comments explaining\n> >> this test setup or consider refactoring it. Honestly, I'd personally\n> >> perfer the tree setup be done more explicitly as I think it would make\n> >> understanding the test much easier.\n> >\n> > This probably ties in with the comments by Patrick on the previous iteration\n> > of this patch, that using 'tree_search()' to insert tree nodes leads to\n> > confusion. Solving that would require efforts outside the scope of this\n> > patch series though, so I'm more inclined towards providing comments\n> > and other ways of simplifying this subroutine.\n> \n> Agreed that refactoring `tree_search()` probably is out of scope here.\n> But rewriting the test is definitely something we can do.\n> \n> Perhaps:\n> \n> static void t_tree(void)\n> {\n> \tstruct tree_node *root = NULL;\n> \tint values[11] = {7, 5, 2, 3, 10, 4, 6, 9, 8, 1};\n> \tstruct tree_node *nodes[11] = { 0 };\n> \tsize_t i = 1;\n> \tstruct curry c = { 0 };\n> \n> // Insert values to the tree by passing '1' as the last argument.\n> for (i = 1; i < ARRAY_SIZE(values); i++) {\n> \t\tnodes[i] = tree_search(&values[i], &root, &t_compare, 1);\n> }\n> \t\n> \tfor (i = 1; i < ARRAY_SIZE(nodes); i++) {\n> \t\tcheck_pointer_eq(values[i], nodes[i]->key);\n> \t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));\n> \t}\n> \n> \tinfix_walk(root, check_increasing, &c);\n> \ttree_free(root);\n> }\n> \n> Wouldn't this have the same effect while making it much easier to read?\n\nPersonally, I quite like this approach. It's more up front with what its\ndoing and ultimately accoplishes the same thing.\n"},{"id":"499034","messageId":"20240722061836.4176-1-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240716075641.4264-1-chandrapratap3519@gmail.com","subject":"[GSoC][PATCH v5 0/5] t: port reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-07-22T05:57:53Z","receivedAt":"2024-07-22T06:19: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/tree_test.c to the unit testing framework and\nimprove upon the ported test. The first patch in the series is\npreparatory cleanup, the second patch moves the test to the unit\ntesting framework, and the rest of the patches improve upon the\nported test.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n\n---\nChanges in v5:\n- Rebase the branch on top of the latest master\n- Add more explanation in the commit message of patch 2\n- Refer to function pointers as 'func' and not '&func'\n- Add comments and refactor the test in patch 2 for easier\n comprehension\n\nCI/PR: https://github.com/gitgitgadget/git/pull/1740\n\nChandra Pratap(5):\nreftable: remove unnecessary curly braces in reftable/tree.c\nt: move reftable/tree_test.c to the unit testing framework\nt-reftable-tree: split test_tree() into two sub-test\nt-reftable-tree: add test for non-existent key\nt-reftable-tree: improve the test for infix_walk()\n\nMakefile | 2 +-\nreftable/reftable-tests.h | 1 -\nreftable/tree.c | 15 +++-------\nreftable/tree_test.c | 60 ----------------------\nt/helper/test-reftable.c | 1 -\nt/unit-tests/t-reftable-tree.c | 83 +++++++++++++++++++++++++++++++++++++\n6 files changed, 89 insertions(+), 73 deletions(-)\n\nRange-diff against v4:\n\n1: 2be2a35b7f = 45: 1d637f7686 reftable: remove unnecessary curly braces in reftable/tree.c\n2: de49698ea7 ! 46: 7401e2409f t: move reftable/tree_test.c to the unit testing framework\n @@ Commit message\n the unit testing framework instead of reftable's test framework and\n renaming the tests to align with unit-tests' standards.\n\n + Also add a comment to help understand the test routine.\n +\n + Note that this commit mostly moves the test from reftable/ to\n + t/unit-tests/ and most of the refactoring is performed by the\n + trailing commits.\n +\n Mentored-by: Patrick Steinhardt \n Mentored-by: Christian Couder \n Signed-off-by: Chandra Pratap \n\n ## Makefile ##\n -@@ Makefile: UNIT_TEST_PROGRAMS += t-mem-pool\n - UNIT_TEST_PROGRAMS += t-oidtree\n +@@ Makefile: 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-record\n +UNIT_TEST_PROGRAMS += t-reftable-tree\n UNIT_TEST_PROGRAMS += t-strbuf\n UNIT_TEST_PROGRAMS += t-strcmp-offset\n UNIT_TEST_PROGRAMS += t-strvec\n -@@ Makefile: REFTABLE_TEST_OBJS += reftable/record_test.o\n +@@ Makefile: 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/tree_test.c (deleted)\n\n ## t/helper/test-reftable.c ##\n @@ t/helper/test-reftable.c: int cmd__reftable(int argc, const char **argv)\n + {\n \t/* test from simple to complex. */\n - \trecord_test_main(argc, argv);\n \tblock_test_main(argc, argv);\n -\ttree_test_main(argc, argv);\n \tpq_test_main(argc, argv);\n @@ t/unit-tests/t-reftable-tree.c (new)\n +\tsize_t i = 1;\n +\tstruct curry c = { 0 };\n +\n ++\t/* pseudo-randomly insert the pointers for elements between\n ++\t * values[1] and values[10] (included) in the tree.\n ++\t */\n +\tdo {\n -+\t\tnodes[i] = tree_search(values + i, &root, &t_compare, 1);\n ++\t\tnodes[i] = tree_search(&values[i], &root, &t_compare, 1);\n +\t\ti = (i * 7) % 11;\n +\t} while (i != 1);\n +\n +\tfor (i = 1; i < ARRAY_SIZE(nodes); i++) {\n -+\t\tcheck_pointer_eq(values + i, nodes[i]->key);\n -+\t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));\n ++\t\tcheck_pointer_eq(&values[i], nodes[i]->key);\n ++\t\tcheck_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));\n +\t}\n +\n +\tinfix_walk(root, check_increasing, &c);\n 3: c733776054 ! 47: 59d5c17d5e t-reftable-tree: split test_tree() into two sub-test functions\n @@ Commit message\n 'int insert' which when set, inserts the key if it is not found\n in the tree. Otherwise, the function returns NULL for such cases.\n\n + While at it, use 'func' to pass function pointers and not '&func'.\n +\n Mentored-by: Patrick Steinhardt \n Mentored-by: Christian Couder \n Signed-off-by: Chandra Pratap \n @@ t/unit-tests/t-reftable-tree.c: static void check_increasing(void *arg, void *ke\n \tsize_t i = 1;\n -\tstruct curry c = { 0 };\n\n - \tdo {\n - \t\tnodes[i] = tree_search(values + i, &root, &t_compare, 1);\n + \t/* pseudo-randomly insert the pointers for elements between\n + \t * values[1] and values[10] (included) in the tree.\n @@ t/unit-tests/t-reftable-tree.c: static void t_tree(void)\n - \t\tcheck_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));\n + \t\tcheck_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));\n \t}\n\n -\tinfix_walk(root, check_increasing, &c);\n @@ t/unit-tests/t-reftable-tree.c: static void t_tree(void)\n +\tsize_t i = 1;\n +\n +\tdo {\n -+\t\ttree_search(values + i, &root, &t_compare, 1);\n ++\t\ttree_search(&values[i], &root, t_compare, 1);\n +\t\ti = (i * 7) % 11;\n +\t} while (i != 1);\n +\n 4: f1a9325bb3 < -: ---------- t-reftable-tree: add test for non-existent key\n -: ---------- > 48: c1ce79916b t-reftable-tree: add test for non-existent key\n 5: c6b7a3d646 ! 49: d1a5ced526 t-reftable-tree: improve the test for infix_walk()\n @@ t/unit-tests/t-reftable-tree.c: static void t_infix_walk(void)\n +\tsize_t count = 0;\n\n \tdo {\n - \t\ttree_search(values + i, &root, &t_compare, 1);\n + \t\ttree_search(&values[i], &root, t_compare, 1);\n \t\ti = (i * 7) % 11;\n +\t\tcount++;\n \t} while (i != 1);\n @@ t/unit-tests/t-reftable-tree.c: static void t_infix_walk(void)\n -\tinfix_walk(root, &check_increasing, &c);\n +\tinfix_walk(root, &store, &c);\n +\tfor (i = 1; i < ARRAY_SIZE(values); i++)\n -+\t\tcheck_pointer_eq(values + i, out[i - 1]);\n ++\t\tcheck_pointer_eq(&values[i], out[i - 1]);\n +\tcheck(!out[i - 1]);\n +\tcheck_int(c.len, ==, count);\n \ttree_free(root);\n"},{"id":"499035","messageId":"20240722061836.4176-2-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240722061836.4176-1-chandrapratap3519@gmail.com","subject":"[PATCH v5 1/5] reftable: remove unnecessary curly braces in reftable/tree.c","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-07-22T05:57:54Z","receivedAt":"2024-07-22T06:19:42Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"According to Documentation/CodingGuidelines, single-line control-flow\nstatements must omit curly braces (except for some special cases).\nMake reftable/tree.c adhere to this guideline.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n reftable/tree.c | 15 +++++----------\n 1 file changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/reftable/tree.c b/reftable/tree.c\nindex 528f33ae38..5ffb2e0d69 100644\n--- a/reftable/tree.c\n+++ b/reftable/tree.c\n@@ -39,25 +39,20 @@ struct tree_node *tree_search(void *key, struct tree_node **rootp,\n void infix_walk(struct tree_node *t, void (*action)(void *arg, void *key),\n \t\tvoid *arg)\n {\n-\tif (t->left) {\n+\tif (t->left)\n \t\tinfix_walk(t->left, action, arg);\n-\t}\n \taction(arg, t->key);\n-\tif (t->right) {\n+\tif (t->right)\n \t\tinfix_walk(t->right, action, arg);\n-\t}\n }\n \n void tree_free(struct tree_node *t)\n {\n-\tif (!t) {\n+\tif (!t)\n \t\treturn;\n-\t}\n-\tif (t->left) {\n+\tif (t->left)\n \t\ttree_free(t->left);\n-\t}\n-\tif (t->right) {\n+\tif (t->right)\n \t\ttree_free(t->right);\n-\t}\n \treftable_free(t);\n }\n-- \n2.45.GIT\n\n"},{"id":"499036","messageId":"20240722061836.4176-3-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240722061836.4176-1-chandrapratap3519@gmail.com","subject":"[PATCH v5 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-07-22T05:57:55Z","receivedAt":"2024-07-22T06:19:45Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"reftable/tree_test.c exercises the functions defined in\nreftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit\ntesting framework. Migration involves refactoring the tests to use\nthe unit testing framework instead of reftable's test framework and\nrenaming the tests to align with unit-tests' standards.\n\nAlso add a comment to help understand the test routine.\n\nNote that this commit mostly moves the test from reftable/ to\nt/unit-tests/ and most of the refactoring is performed by the\ntrailing commits.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n Makefile | 2 +-\n reftable/reftable-tests.h | 1 -\n reftable/tree_test.c | 60 ----------------------------------\n t/helper/test-reftable.c | 1 -\n t/unit-tests/t-reftable-tree.c | 59 +++++++++++++++++++++++++++++++++\n 5 files changed, 60 insertions(+), 63 deletions(-)\n delete mode 100644 reftable/tree_test.c\n create mode 100644 t/unit-tests/t-reftable-tree.c\n\ndiff --git a/Makefile b/Makefile\nindex d6479092a0..6f423a2a1e 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-record\n+UNIT_TEST_PROGRAMS += t-reftable-tree\n UNIT_TEST_PROGRAMS += t-strbuf\n UNIT_TEST_PROGRAMS += t-strcmp-offset\n UNIT_TEST_PROGRAMS += t-strvec\n@@ -2685,7 +2686,6 @@ 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\n \n TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))\n \ndiff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h\nindex 114cc3d053..d0abcc51e2 100644\n--- a/reftable/reftable-tests.h\n+++ b/reftable/reftable-tests.h\n@@ -16,7 +16,6 @@ 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);\n \n #endif\ndiff --git a/reftable/tree_test.c b/reftable/tree_test.c\ndeleted file mode 100644\nindex 6961a657ad..0000000000\n--- a/reftable/tree_test.c\n+++ /dev/null\n@@ -1,60 +0,0 @@\n-/*\n-Copyright 2020 Google LLC\n-\n-Use of this source code is governed by a BSD-style\n-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-#include \"tree.h\"\n-\n-#include \"test_framework.h\"\n-#include \"reftable-tests.h\"\n-\n-static int test_compare(const void *a, const void *b)\n-{\n-\treturn (char *)a - (char *)b;\n-}\n-\n-struct curry {\n-\tvoid *last;\n-};\n-\n-static void check_increasing(void *arg, void *key)\n-{\n-\tstruct curry *c = arg;\n-\tif (c->last) {\n-\t\tEXPECT(test_compare(c->last, key) < 0);\n-\t}\n-\tc->last = key;\n-}\n-\n-static void test_tree(void)\n-{\n-\tstruct tree_node *root = NULL;\n-\n-\tvoid *values[11] = { NULL };\n-\tstruct tree_node *nodes[11] = { NULL };\n-\tint i = 1;\n-\tstruct curry c = { NULL };\n-\tdo {\n-\t\tnodes[i] = tree_search(values + i, &root, &test_compare, 1);\n-\t\ti = (i * 7) % 11;\n-\t} while (i != 1);\n-\n-\tfor (i = 1; i < ARRAY_SIZE(nodes); i++) {\n-\t\tEXPECT(values + i == nodes[i]->key);\n-\t\tEXPECT(nodes[i] ==\n-\t\t tree_search(values + i, &root, &test_compare, 0));\n-\t}\n-\n-\tinfix_walk(root, check_increasing, &c);\n-\ttree_free(root);\n-}\n-\n-int tree_test_main(int argc, const char *argv[])\n-{\n-\tRUN_TEST(test_tree);\n-\treturn 0;\n-}\ndiff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c\nindex aa6538a8da..8c41ef7c9d 100644\n--- a/t/helper/test-reftable.c\n+++ b/t/helper/test-reftable.c\n@@ -6,7 +6,6 @@ int cmd__reftable(int argc, const char **argv)\n {\n \t/* test from simple to complex. */\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 \tmerged_test_main(argc, argv);\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nnew file mode 100644\nindex 0000000000..08f1873ad3\n--- /dev/null\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -0,0 +1,59 @@\n+/*\n+Copyright 2020 Google LLC\n+\n+Use of this source code is governed by a BSD-style\n+license that can be found in the LICENSE file or at\n+https://developers.google.com/open-source/licenses/bsd\n+*/\n+\n+#include \"test-lib.h\"\n+#include \"reftable/tree.h\"\n+\n+static int t_compare(const void *a, const void *b)\n+{\n+\treturn (char *)a - (char *)b;\n+}\n+\n+struct curry {\n+\tvoid *last;\n+};\n+\n+static void check_increasing(void *arg, void *key)\n+{\n+\tstruct curry *c = arg;\n+\tif (c->last)\n+\t\tcheck_int(t_compare(c->last, key), <, 0);\n+\tc->last = key;\n+}\n+\n+static void t_tree(void)\n+{\n+\tstruct tree_node *root = NULL;\n+\tvoid *values[11] = { 0 };\n+\tstruct tree_node *nodes[11] = { 0 };\n+\tsize_t i = 1;\n+\tstruct curry c = { 0 };\n+\n+\t/* pseudo-randomly insert the pointers for elements between\n+\t * values[1] and values[10] (included) in the tree.\n+\t */\n+\tdo {\n+\t\tnodes[i] = tree_search(&values[i], &root, &t_compare, 1);\n+\t\ti = (i * 7) % 11;\n+\t} while (i != 1);\n+\n+\tfor (i = 1; i < ARRAY_SIZE(nodes); i++) {\n+\t\tcheck_pointer_eq(&values[i], nodes[i]->key);\n+\t\tcheck_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));\n+\t}\n+\n+\tinfix_walk(root, check_increasing, &c);\n+\ttree_free(root);\n+}\n+\n+int cmd_main(int argc, const char *argv[])\n+{\n+\tTEST(t_tree(), \"tree_search and infix_walk work\");\n+\n+\treturn test_done();\n+}\n-- \n2.45.GIT\n\n"},{"id":"499037","messageId":"20240722061836.4176-4-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240722061836.4176-1-chandrapratap3519@gmail.com","subject":"[PATCH v5 3/5] t-reftable-tree: split test_tree() into two sub-test functions","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-07-22T05:57:56Z","receivedAt":"2024-07-22T06:19:47Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup, tests for both tree_search() and\ninfix_walk() defined by reftable/tree.{c, h} are performed by\na single test function, test_tree(). Split tree_test() into\ntest_tree_search() and test_infix_walk() responsible for\nindependently testing tree_search() and infix_walk() respectively.\nThis improves the overall readability of the test file as well as\nsimplifies debugging.\n\nNote that the last parameter in the tree_search() functiom is\n'int insert' which when set, inserts the key if it is not found\nin the tree. Otherwise, the function returns NULL for such cases.\n\nWhile at it, use 'func' to pass function pointers and not '&func'.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 23 +++++++++++++++++++----\n 1 file changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex 08f1873ad3..07c6c6dce5 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -26,13 +26,12 @@ static void check_increasing(void *arg, void *key)\n \tc->last = key;\n }\n \n-static void t_tree(void)\n+static void t_tree_search(void)\n {\n \tstruct tree_node *root = NULL;\n \tvoid *values[11] = { 0 };\n \tstruct tree_node *nodes[11] = { 0 };\n \tsize_t i = 1;\n-\tstruct curry c = { 0 };\n \n \t/* pseudo-randomly insert the pointers for elements between\n \t * values[1] and values[10] (included) in the tree.\n@@ -47,13 +46,29 @@ static void t_tree(void)\n \t\tcheck_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));\n \t}\n \n-\tinfix_walk(root, check_increasing, &c);\n+\ttree_free(root);\n+}\n+\n+static void t_infix_walk(void)\n+{\n+\tstruct tree_node *root = NULL;\n+\tvoid *values[11] = { 0 };\n+\tstruct curry c = { 0 };\n+\tsize_t i = 1;\n+\n+\tdo {\n+\t\ttree_search(&values[i], &root, t_compare, 1);\n+\t\ti = (i * 7) % 11;\n+\t} while (i != 1);\n+\n+\tinfix_walk(root, &check_increasing, &c);\n \ttree_free(root);\n }\n \n int cmd_main(int argc, const char *argv[])\n {\n-\tTEST(t_tree(), \"tree_search and infix_walk work\");\n+\tTEST(t_tree_search(), \"tree_search works\");\n+\tTEST(t_infix_walk(), \"infix_walk works\");\n \n \treturn test_done();\n }\n-- \n2.45.GIT\n\n"},{"id":"499038","messageId":"20240722061836.4176-5-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240722061836.4176-1-chandrapratap3519@gmail.com","subject":"[PATCH v5 4/5] t-reftable-tree: add test for non-existent key","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-07-22T05:57:57Z","receivedAt":"2024-07-22T06:19:49Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup for tree_search(), the case for\nnon-existent key is not exercised. Improve this by adding a\ntest-case for the same.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex 07c6c6dce5..6cd35b0ea0 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -46,6 +46,7 @@ static void t_tree_search(void)\n \t\tcheck_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));\n \t}\n \n+\tcheck(!tree_search(values, &root, t_compare, 0));\n \ttree_free(root);\n }\n \n-- \n2.45.GIT\n\n"},{"id":"499039","messageId":"20240722061836.4176-6-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240722061836.4176-1-chandrapratap3519@gmail.com","subject":"[PATCH v5 5/5] t-reftable-tree: improve the test for infix_walk()","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-07-22T05:57:58Z","receivedAt":"2024-07-22T06:19:52Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup for infix_walk(), the following\nproperties of an infix traversal of a tree remain untested:\n- every node of the tree must be visited\n- every node must be visited exactly once\nIn fact, only the property 'traversal in increasing order' is tested.\nModify test_infix_walk() to check for all the properties above.\n\nThis can be achieved by storing the nodes' keys linearly, in a nullified\nbuffer, as we visit them and then checking the input keys against this\nbuffer in increasing order. By checking that the element just after\nthe last input key is 'NULL' in the output buffer, we ensure that\nevery node is traversed exactly once.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 22 +++++++++++++++-------\n 1 file changed, 15 insertions(+), 7 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex 6cd35b0ea0..d7d530f2f7 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -15,15 +15,14 @@ static int t_compare(const void *a, const void *b)\n }\n \n struct curry {\n-\tvoid *last;\n+\tvoid **arr;\n+\tsize_t len;\n };\n \n-static void check_increasing(void *arg, void *key)\n+static void store(void *arg, void *key)\n {\n \tstruct curry *c = arg;\n-\tif (c->last)\n-\t\tcheck_int(t_compare(c->last, key), <, 0);\n-\tc->last = key;\n+\tc->arr[c->len++] = key;\n }\n \n static void t_tree_search(void)\n@@ -54,15 +53,24 @@ static void t_infix_walk(void)\n {\n \tstruct tree_node *root = NULL;\n \tvoid *values[11] = { 0 };\n-\tstruct curry c = { 0 };\n+\tvoid *out[11] = { 0 };\n+\tstruct curry c = {\n+\t\t.arr = (void **) &out,\n+\t};\n \tsize_t i = 1;\n+\tsize_t count = 0;\n \n \tdo {\n \t\ttree_search(&values[i], &root, t_compare, 1);\n \t\ti = (i * 7) % 11;\n+\t\tcount++;\n \t} while (i != 1);\n \n-\tinfix_walk(root, &check_increasing, &c);\n+\tinfix_walk(root, &store, &c);\n+\tfor (i = 1; i < ARRAY_SIZE(values); i++)\n+\t\tcheck_pointer_eq(&values[i], out[i - 1]);\n+\tcheck(!out[i - 1]);\n+\tcheck_int(c.len, ==, count);\n \ttree_free(root);\n }\n \n-- \n2.45.GIT\n\n"},{"id":"499068","messageId":"xmqqikwxco1c.fsf@gitster.g","threadId":"61614","inReplyTo":"20240722061836.4176-3-chandrapratap3519@gmail.com","subject":"Re: [PATCH v5 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-22T17:52:15Z","receivedAt":"2024-07-22T17:52:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chandra Pratap writes:\n\n> reftable/tree_test.c exercises the functions defined in\n> reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit\n> testing framework. Migration involves refactoring the tests to use\n> the unit testing framework instead of reftable's test framework and\n> renaming the tests to align with unit-tests' standards.\n>\n> Also add a comment to help understand the test routine.\n>\n> Note that this commit mostly moves the test from reftable/ to\n> t/unit-tests/ and most of the refactoring is performed by the\n> trailing commits.\n>\n> Mentored-by: Patrick Steinhardt \n> Mentored-by: Christian Couder \n> Signed-off-by: Chandra Pratap \n> ---\n\nLooking good.\n\n\"git show -M30\" matches the moved file up correctly and makes it\neasy to see that there is no serious changes snuck in (please do not\ntake this as a suggestion to run format-patch with -M30---I am just\nsaying that it was a good way to give an extra validation).\n\nThanks.\n"},{"id":"499069","messageId":"xmqqcyn5co04.fsf@gitster.g","threadId":"61614","inReplyTo":"20240722061836.4176-3-chandrapratap3519@gmail.com","subject":"Re: [PATCH v5 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-22T17:52:59Z","receivedAt":"2024-07-22T17:53:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chandra Pratap writes:\n\n> +\t/* pseudo-randomly insert the pointers for elements between\n> +\t * values[1] and values[10] (included) in the tree.\n> +\t */\n\nStyle?\n"},{"id":"499070","messageId":"xmqq8qxtcnu2.fsf@gitster.g","threadId":"61614","inReplyTo":"xmqqcyn5co04.fsf@gitster.g","subject":"Re: [PATCH v5 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-22T17:56:37Z","receivedAt":"2024-07-22T17:56:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Chandra Pratap writes:\n>\n>> +\t/* pseudo-randomly insert the pointers for elements between\n>> +\t * values[1] and values[10] (included) in the tree.\n>> +\t */\n>\n> Style?\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex d7d530f2f7..e7d774d774 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -32,8 +32,9 @@ static void t_tree_search(void)\n \tstruct tree_node *nodes[11] = { 0 };\n \tsize_t i = 1;\n \n-\t/* pseudo-randomly insert the pointers for elements between\n-\t * values[1] and values[10] (included) in the tree.\n+\t/*\n+\t * Pseudo-randomly insert the pointers for elements between\n+\t * values[1] and values[10] (inclusive) in the tree.\n \t */\n \tdo {\n \t\tnodes[i] = tree_search(&values[i], &root, &t_compare, 1);\n-- \n2.46.0-rc1-48-g0900f1888e\n\n"},{"id":"499838","messageId":"CA+J6zkT55DVh28w2Pb=PVs3cBAeoQw4eYc2QbD7Bn0BvPjjH1Q@mail.gmail.com","threadId":"61614","inReplyTo":"20240722061836.4176-1-chandrapratap3519@gmail.com","subject":"Re: [GSoC][PATCH v5 0/5] t: port reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-01T11:21:44Z","receivedAt":"2024-08-01T11:21:56Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"A reminder for reviews/acks.\n"},{"id":"499840","messageId":"Zqt1WTT_eJKEuO1z@tanuki","threadId":"61614","inReplyTo":"CA+J6zkT55DVh28w2Pb=PVs3cBAeoQw4eYc2QbD7Bn0BvPjjH1Q@mail.gmail.com","subject":"Re: [GSoC][PATCH v5 0/5] t: port reftable/tree_test.c to the unit testing framework","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-01T11:45:29Z","receivedAt":"2024-08-01T11:45:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Aug 01, 2024 at 04:51:44PM +0530, Chandra Pratap wrote:\n> A reminder for reviews/acks.\n\nThere was one style nit from Junio already that you should probably\naddress. Other than that I didn't really have anything else to add to\nthis series.\n\nPatrick\n"},{"id":"499940","messageId":"20240802121318.4583-1-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240722061836.4176-1-chandrapratap3519@gmail.com","subject":"[GSoC][PATCH v6 0/5] t: port reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-02T12:08:03Z","receivedAt":"2024-08-02T12:14:26Z","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/tree_test.c to the unit testing framework and\nimprove upon the ported test. The first patch in the series is\npreparatory cleanup, the second patch moves the test to the unit\ntesting framework, and the rest of the patches improve upon the\nported test.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n\n---\nChanges in v6:\n- Fix a style issue in a comment introduced in patch 3\n\nCI/PR: https://github.com/gitgitgadget/git/pull/1740\n\nChandra Pratap(5):\nreftable: remove unnecessary curly braces in reftable/tree.c\nt: move reftable/tree_test.c to the unit testing framework\nt-reftable-tree: split test_tree() into two sub-test\nt-reftable-tree: add test for non-existent key\nt-reftable-tree: improve the test for infix_walk()\n\nMakefile | 2 +-\nreftable/reftable-tests.h | 1 -\nreftable/tree.c | 15 +++-------\nreftable/tree_test.c | 60 ----------------------\nt/helper/test-reftable.c | 1 -\nt/unit-tests/t-reftable-tree.c | 83 +++++++++++++++++++++++++++++++++++++\n6 files changed, 89 insertions(+), 73 deletions(-)\n\nRange-diff against v5:\n1: 59d5c17d5e ! 1: 501ea6c05d t-reftable-tree: split test_tree() into two sub-test functions\n @@ t/unit-tests/t-reftable-tree.c: static void check_increasing(void *arg, void *ke\n \tsize_t i = 1;\n -\tstruct curry c = { 0 };\n\n - \t/* pseudo-randomly insert the pointers for elements between\n - \t * values[1] and values[10] (included) in the tree.\n +-\t/* pseudo-randomly insert the pointers for elements between\n +-\t * values[1] and values[10] (included) in the tree.\n ++\t/* Pseudo-randomly insert the pointers for elements between\n ++\t * values[1] and values[10] (inclusive) in the tree.\n + \t */\n + \tdo {\n + \t\tnodes[i] = tree_search(&values[i], &root, &t_compare, 1);\n @@ t/unit-tests/t-reftable-tree.c: static void t_tree(void)\n \t\tcheck_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));\n \t}\n2: c1ce79916b = 2: ecadc1833e t-reftable-tree: add test for non-existent key\n3: d1a5ced526 = 3: cda7509281 t-reftable-tree: improve the test for infix_walk()\n\n"},{"id":"499941","messageId":"20240802121318.4583-2-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240802121318.4583-1-chandrapratap3519@gmail.com","subject":"[PATCH v6 1/5] reftable: remove unnecessary curly braces in reftable/tree.c","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-02T12:08:04Z","receivedAt":"2024-08-02T12:14:29Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"According to Documentation/CodingGuidelines, single-line control-flow\nstatements must omit curly braces (except for some special cases).\nMake reftable/tree.c adhere to this guideline.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n reftable/tree.c | 15 +++++----------\n 1 file changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/reftable/tree.c b/reftable/tree.c\nindex 528f33ae38..5ffb2e0d69 100644\n--- a/reftable/tree.c\n+++ b/reftable/tree.c\n@@ -39,25 +39,20 @@ struct tree_node *tree_search(void *key, struct tree_node **rootp,\n void infix_walk(struct tree_node *t, void (*action)(void *arg, void *key),\n \t\tvoid *arg)\n {\n-\tif (t->left) {\n+\tif (t->left)\n \t\tinfix_walk(t->left, action, arg);\n-\t}\n \taction(arg, t->key);\n-\tif (t->right) {\n+\tif (t->right)\n \t\tinfix_walk(t->right, action, arg);\n-\t}\n }\n \n void tree_free(struct tree_node *t)\n {\n-\tif (!t) {\n+\tif (!t)\n \t\treturn;\n-\t}\n-\tif (t->left) {\n+\tif (t->left)\n \t\ttree_free(t->left);\n-\t}\n-\tif (t->right) {\n+\tif (t->right)\n \t\ttree_free(t->right);\n-\t}\n \treftable_free(t);\n }\n-- \n2.45.GIT\n\n"},{"id":"499942","messageId":"20240802121318.4583-3-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240802121318.4583-1-chandrapratap3519@gmail.com","subject":"[PATCH v6 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-02T12:08:05Z","receivedAt":"2024-08-02T12:14:32Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"reftable/tree_test.c exercises the functions defined in\nreftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit\ntesting framework. Migration involves refactoring the tests to use\nthe unit testing framework instead of reftable's test framework and\nrenaming the tests to align with unit-tests' standards.\n\nAlso add a comment to help understand the test routine.\n\nNote that this commit mostly moves the test from reftable/ to\nt/unit-tests/ and most of the refactoring is performed by the\ntrailing commits.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n Makefile | 2 +-\n reftable/reftable-tests.h | 1 -\n reftable/tree_test.c | 60 ----------------------------------\n t/helper/test-reftable.c | 1 -\n t/unit-tests/t-reftable-tree.c | 59 +++++++++++++++++++++++++++++++++\n 5 files changed, 60 insertions(+), 63 deletions(-)\n delete mode 100644 reftable/tree_test.c\n create mode 100644 t/unit-tests/t-reftable-tree.c\n\ndiff --git a/Makefile b/Makefile\nindex 3863e60b66..5499f7bcbd 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1342,6 +1342,7 @@ 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-record\n+UNIT_TEST_PROGRAMS += t-reftable-tree\n UNIT_TEST_PROGRAMS += t-strbuf\n UNIT_TEST_PROGRAMS += t-strcmp-offset\n UNIT_TEST_PROGRAMS += t-strvec\n@@ -2685,7 +2686,6 @@ 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\n \n TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))\n \ndiff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h\nindex d5e03dcc1b..8516b1f923 100644\n--- a/reftable/reftable-tests.h\n+++ b/reftable/reftable-tests.h\n@@ -15,7 +15,6 @@ 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);\n \n #endif\ndiff --git a/reftable/tree_test.c b/reftable/tree_test.c\ndeleted file mode 100644\nindex 6961a657ad..0000000000\n--- a/reftable/tree_test.c\n+++ /dev/null\n@@ -1,60 +0,0 @@\n-/*\n-Copyright 2020 Google LLC\n-\n-Use of this source code is governed by a BSD-style\n-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-#include \"tree.h\"\n-\n-#include \"test_framework.h\"\n-#include \"reftable-tests.h\"\n-\n-static int test_compare(const void *a, const void *b)\n-{\n-\treturn (char *)a - (char *)b;\n-}\n-\n-struct curry {\n-\tvoid *last;\n-};\n-\n-static void check_increasing(void *arg, void *key)\n-{\n-\tstruct curry *c = arg;\n-\tif (c->last) {\n-\t\tEXPECT(test_compare(c->last, key) < 0);\n-\t}\n-\tc->last = key;\n-}\n-\n-static void test_tree(void)\n-{\n-\tstruct tree_node *root = NULL;\n-\n-\tvoid *values[11] = { NULL };\n-\tstruct tree_node *nodes[11] = { NULL };\n-\tint i = 1;\n-\tstruct curry c = { NULL };\n-\tdo {\n-\t\tnodes[i] = tree_search(values + i, &root, &test_compare, 1);\n-\t\ti = (i * 7) % 11;\n-\t} while (i != 1);\n-\n-\tfor (i = 1; i < ARRAY_SIZE(nodes); i++) {\n-\t\tEXPECT(values + i == nodes[i]->key);\n-\t\tEXPECT(nodes[i] ==\n-\t\t tree_search(values + i, &root, &test_compare, 0));\n-\t}\n-\n-\tinfix_walk(root, check_increasing, &c);\n-\ttree_free(root);\n-}\n-\n-int tree_test_main(int argc, const char *argv[])\n-{\n-\tRUN_TEST(test_tree);\n-\treturn 0;\n-}\ndiff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c\nindex 9d378427da..0acaf85494 100644\n--- a/t/helper/test-reftable.c\n+++ b/t/helper/test-reftable.c\n@@ -6,7 +6,6 @@ int cmd__reftable(int argc, const char **argv)\n {\n \t/* test from simple to complex. */\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);\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nnew file mode 100644\nindex 0000000000..08f1873ad3\n--- /dev/null\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -0,0 +1,59 @@\n+/*\n+Copyright 2020 Google LLC\n+\n+Use of this source code is governed by a BSD-style\n+license that can be found in the LICENSE file or at\n+https://developers.google.com/open-source/licenses/bsd\n+*/\n+\n+#include \"test-lib.h\"\n+#include \"reftable/tree.h\"\n+\n+static int t_compare(const void *a, const void *b)\n+{\n+\treturn (char *)a - (char *)b;\n+}\n+\n+struct curry {\n+\tvoid *last;\n+};\n+\n+static void check_increasing(void *arg, void *key)\n+{\n+\tstruct curry *c = arg;\n+\tif (c->last)\n+\t\tcheck_int(t_compare(c->last, key), <, 0);\n+\tc->last = key;\n+}\n+\n+static void t_tree(void)\n+{\n+\tstruct tree_node *root = NULL;\n+\tvoid *values[11] = { 0 };\n+\tstruct tree_node *nodes[11] = { 0 };\n+\tsize_t i = 1;\n+\tstruct curry c = { 0 };\n+\n+\t/* pseudo-randomly insert the pointers for elements between\n+\t * values[1] and values[10] (included) in the tree.\n+\t */\n+\tdo {\n+\t\tnodes[i] = tree_search(&values[i], &root, &t_compare, 1);\n+\t\ti = (i * 7) % 11;\n+\t} while (i != 1);\n+\n+\tfor (i = 1; i < ARRAY_SIZE(nodes); i++) {\n+\t\tcheck_pointer_eq(&values[i], nodes[i]->key);\n+\t\tcheck_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));\n+\t}\n+\n+\tinfix_walk(root, check_increasing, &c);\n+\ttree_free(root);\n+}\n+\n+int cmd_main(int argc, const char *argv[])\n+{\n+\tTEST(t_tree(), \"tree_search and infix_walk work\");\n+\n+\treturn test_done();\n+}\n-- \n2.45.GIT\n\n"},{"id":"499943","messageId":"20240802121318.4583-4-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240802121318.4583-1-chandrapratap3519@gmail.com","subject":"[PATCH v6 3/5] t-reftable-tree: split test_tree() into two sub-test functions","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-02T12:08:06Z","receivedAt":"2024-08-02T12:14:35Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup, tests for both tree_search() and\ninfix_walk() defined by reftable/tree.{c, h} are performed by\na single test function, test_tree(). Split tree_test() into\ntest_tree_search() and test_infix_walk() responsible for\nindependently testing tree_search() and infix_walk() respectively.\nThis improves the overall readability of the test file as well as\nsimplifies debugging.\n\nNote that the last parameter in the tree_search() functiom is\n'int insert' which when set, inserts the key if it is not found\nin the tree. Otherwise, the function returns NULL for such cases.\n\nWhile at it, use 'func' to pass function pointers and not '&func'.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 27 +++++++++++++++++++++------\n 1 file changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex 08f1873ad3..5efe34835e 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -26,16 +26,15 @@ static void check_increasing(void *arg, void *key)\n \tc->last = key;\n }\n \n-static void t_tree(void)\n+static void t_tree_search(void)\n {\n \tstruct tree_node *root = NULL;\n \tvoid *values[11] = { 0 };\n \tstruct tree_node *nodes[11] = { 0 };\n \tsize_t i = 1;\n-\tstruct curry c = { 0 };\n \n-\t/* pseudo-randomly insert the pointers for elements between\n-\t * values[1] and values[10] (included) in the tree.\n+\t/* Pseudo-randomly insert the pointers for elements between\n+\t * values[1] and values[10] (inclusive) in the tree.\n \t */\n \tdo {\n \t\tnodes[i] = tree_search(&values[i], &root, &t_compare, 1);\n@@ -47,13 +46,29 @@ static void t_tree(void)\n \t\tcheck_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));\n \t}\n \n-\tinfix_walk(root, check_increasing, &c);\n+\ttree_free(root);\n+}\n+\n+static void t_infix_walk(void)\n+{\n+\tstruct tree_node *root = NULL;\n+\tvoid *values[11] = { 0 };\n+\tstruct curry c = { 0 };\n+\tsize_t i = 1;\n+\n+\tdo {\n+\t\ttree_search(&values[i], &root, t_compare, 1);\n+\t\ti = (i * 7) % 11;\n+\t} while (i != 1);\n+\n+\tinfix_walk(root, &check_increasing, &c);\n \ttree_free(root);\n }\n \n int cmd_main(int argc, const char *argv[])\n {\n-\tTEST(t_tree(), \"tree_search and infix_walk work\");\n+\tTEST(t_tree_search(), \"tree_search works\");\n+\tTEST(t_infix_walk(), \"infix_walk works\");\n \n \treturn test_done();\n }\n-- \n2.45.GIT\n\n"},{"id":"499944","messageId":"20240802121318.4583-5-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240802121318.4583-1-chandrapratap3519@gmail.com","subject":"[PATCH v6 4/5] t-reftable-tree: add test for non-existent key","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-02T12:08:07Z","receivedAt":"2024-08-02T12:14:38Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup for tree_search(), the case for\nnon-existent key is not exercised. Improve this by adding a\ntest-case for the same.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex 5efe34835e..0f00a31819 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -46,6 +46,7 @@ static void t_tree_search(void)\n \t\tcheck_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));\n \t}\n \n+\tcheck(!tree_search(values, &root, t_compare, 0));\n \ttree_free(root);\n }\n \n-- \n2.45.GIT\n\n"},{"id":"499945","messageId":"20240802121318.4583-6-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240802121318.4583-1-chandrapratap3519@gmail.com","subject":"[PATCH v6 5/5] t-reftable-tree: improve the test for infix_walk()","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-02T12:08:08Z","receivedAt":"2024-08-02T12:14:41Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup for infix_walk(), the following\nproperties of an infix traversal of a tree remain untested:\n- every node of the tree must be visited\n- every node must be visited exactly once\nIn fact, only the property 'traversal in increasing order' is tested.\nModify test_infix_walk() to check for all the properties above.\n\nThis can be achieved by storing the nodes' keys linearly, in a nullified\nbuffer, as we visit them and then checking the input keys against this\nbuffer in increasing order. By checking that the element just after\nthe last input key is 'NULL' in the output buffer, we ensure that\nevery node is traversed exactly once.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 22 +++++++++++++++-------\n 1 file changed, 15 insertions(+), 7 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex 0f00a31819..2fc6a34008 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -15,15 +15,14 @@ static int t_compare(const void *a, const void *b)\n }\n \n struct curry {\n-\tvoid *last;\n+\tvoid **arr;\n+\tsize_t len;\n };\n \n-static void check_increasing(void *arg, void *key)\n+static void store(void *arg, void *key)\n {\n \tstruct curry *c = arg;\n-\tif (c->last)\n-\t\tcheck_int(t_compare(c->last, key), <, 0);\n-\tc->last = key;\n+\tc->arr[c->len++] = key;\n }\n \n static void t_tree_search(void)\n@@ -54,15 +53,24 @@ static void t_infix_walk(void)\n {\n \tstruct tree_node *root = NULL;\n \tvoid *values[11] = { 0 };\n-\tstruct curry c = { 0 };\n+\tvoid *out[11] = { 0 };\n+\tstruct curry c = {\n+\t\t.arr = (void **) &out,\n+\t};\n \tsize_t i = 1;\n+\tsize_t count = 0;\n \n \tdo {\n \t\ttree_search(&values[i], &root, t_compare, 1);\n \t\ti = (i * 7) % 11;\n+\t\tcount++;\n \t} while (i != 1);\n \n-\tinfix_walk(root, &check_increasing, &c);\n+\tinfix_walk(root, &store, &c);\n+\tfor (i = 1; i < ARRAY_SIZE(values); i++)\n+\t\tcheck_pointer_eq(&values[i], out[i - 1]);\n+\tcheck(!out[i - 1]);\n+\tcheck_int(c.len, ==, count);\n \ttree_free(root);\n }\n \n-- \n2.45.GIT\n\n"},{"id":"499958","messageId":"xmqqikwievto.fsf@gitster.g","threadId":"61614","inReplyTo":"20240802121318.4583-4-chandrapratap3519@gmail.com","subject":"Re: [PATCH v6 3/5] t-reftable-tree: split test_tree() into two sub-test functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-02T16:25:07Z","receivedAt":"2024-08-02T16:25:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chandra Pratap writes:\n\n> -\t/* pseudo-randomly insert the pointers for elements between\n> -\t * values[1] and values[10] (included) in the tree.\n> +\t/* Pseudo-randomly insert the pointers for elements between\n> +\t * values[1] and values[10] (inclusive) in the tree.\n> \t */\n\nIf you are \"fixing\" the comment, let's fix its style as well.\n\n\t/*\n * Our multi-line comments begin with slash-asterisk, and\n\t * end with asterisk-slash, on their own line.\n\t */\n"},{"id":"500019","messageId":"20240804141105.4268-1-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240802121318.4583-1-chandrapratap3519@gmail.com","subject":"[GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-04T14:06:44Z","receivedAt":"2024-08-04T14:11:29Z","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/tree_test.c to the unit testing framework and\nimprove upon the ported test. The first patch in the series is\npreparatory cleanup, the second patch moves the test to the unit\ntesting framework, and the rest of the patches improve upon the\nported test.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n\n---\nChanges in v7:\n- Fix style issues in a comment introduced in patch 3\n of the previous series\n\nCI/PR: https://github.com/gitgitgadget/git/pull/1740\n\nChandra Pratap(5):\nreftable: remove unnecessary curly braces in reftable/tree.c\nt: move reftable/tree_test.c to the unit testing framework\nt-reftable-tree: split test_tree() into two sub-test\nt-reftable-tree: add test for non-existent key\nt-reftable-tree: improve the test for infix_walk()\n\nMakefile | 2 +-\nreftable/reftable-tests.h | 1 -\nreftable/tree.c | 15 +++-------\nreftable/tree_test.c | 60 ----------------------\nt/helper/test-reftable.c | 1 -\nt/unit-tests/t-reftable-tree.c | 83 +++++++++++++++++++++++++++++++++++++\n6 files changed, 89 insertions(+), 73 deletions(-)\n\nRange-diff against v6:\n\n1: d4a45e602c = 1: d738bf57e2 reftable: remove unnecessary curly braces in reftable/tree.c\n2: 9148e740e8 ! 2: f090ace685 t: move reftable/tree_test.c to the unit testing framework\n @@ t/unit-tests/t-reftable-tree.c (new)\n +\tsize_t i = 1;\n +\tstruct curry c = { 0 };\n +\n -+\t/* pseudo-randomly insert the pointers for elements between\n -+\t * values[1] and values[10] (included) in the tree.\n ++\t/*\n ++\t * Pseudo-randomly insert the pointers for elements between\n ++\t * values[1] and values[10] (inclusive) in the tree.\n +\t */\n +\tdo {\n +\t\tnodes[i] = tree_search(&values[i], &root, &t_compare, 1);\n3: f73ad11238 ! 3: 22256e77b3 t-reftable-tree: split test_tree() into two sub-test functions\n @@ t/unit-tests/t-reftable-tree.c: static void check_increasing(void *arg, void *ke\n \tsize_t i = 1;\n -\tstruct curry c = { 0 };\n\n --\t/* pseudo-randomly insert the pointers for elements between\n --\t * values[1] and values[10] (included) in the tree.\n -+\t/* Pseudo-randomly insert the pointers for elements between\n -+\t * values[1] and values[10] (inclusive) in the tree.\n - \t */\n - \tdo {\n - \t\tnodes[i] = tree_search(&values[i], &root, &t_compare, 1);\n + \t/*\n + \t * Pseudo-randomly insert the pointers for elements between\n @@ t/unit-tests/t-reftable-tree.c: static void t_tree(void)\n \t\tcheck_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));\n \t}\n4: edb02d2e84 = 4: 0d04daad28 t-reftable-tree: add test for non-existent key\n5: 6aecd4e374 = 5: 80d4aa2a66 t-reftable-tree: improve the test for infix_walk()\n"},{"id":"500020","messageId":"20240804141105.4268-2-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240804141105.4268-1-chandrapratap3519@gmail.com","subject":"[PATCH v7 1/5] reftable: remove unnecessary curly braces in reftable/tree.c","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-04T14:06:45Z","receivedAt":"2024-08-04T14:11:32Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"According to Documentation/CodingGuidelines, single-line control-flow\nstatements must omit curly braces (except for some special cases).\nMake reftable/tree.c adhere to this guideline.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n reftable/tree.c | 15 +++++----------\n 1 file changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/reftable/tree.c b/reftable/tree.c\nindex 528f33ae38..5ffb2e0d69 100644\n--- a/reftable/tree.c\n+++ b/reftable/tree.c\n@@ -39,25 +39,20 @@ struct tree_node *tree_search(void *key, struct tree_node **rootp,\n void infix_walk(struct tree_node *t, void (*action)(void *arg, void *key),\n \t\tvoid *arg)\n {\n-\tif (t->left) {\n+\tif (t->left)\n \t\tinfix_walk(t->left, action, arg);\n-\t}\n \taction(arg, t->key);\n-\tif (t->right) {\n+\tif (t->right)\n \t\tinfix_walk(t->right, action, arg);\n-\t}\n }\n \n void tree_free(struct tree_node *t)\n {\n-\tif (!t) {\n+\tif (!t)\n \t\treturn;\n-\t}\n-\tif (t->left) {\n+\tif (t->left)\n \t\ttree_free(t->left);\n-\t}\n-\tif (t->right) {\n+\tif (t->right)\n \t\ttree_free(t->right);\n-\t}\n \treftable_free(t);\n }\n-- \n2.45.GIT\n\n"},{"id":"500021","messageId":"20240804141105.4268-3-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240804141105.4268-1-chandrapratap3519@gmail.com","subject":"[PATCH v7 2/5] t: move reftable/tree_test.c to the unit testing framework","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-04T14:06:46Z","receivedAt":"2024-08-04T14:11:36Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"reftable/tree_test.c exercises the functions defined in\nreftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit\ntesting framework. Migration involves refactoring the tests to use\nthe unit testing framework instead of reftable's test framework and\nrenaming the tests to align with unit-tests' standards.\n\nAlso add a comment to help understand the test routine.\n\nNote that this commit mostly moves the test from reftable/ to\nt/unit-tests/ and most of the refactoring is performed by the\ntrailing commits.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n Makefile | 2 +-\n reftable/reftable-tests.h | 1 -\n reftable/tree_test.c | 60 ----------------------------------\n t/helper/test-reftable.c | 1 -\n t/unit-tests/t-reftable-tree.c | 60 ++++++++++++++++++++++++++++++++++\n 5 files changed, 61 insertions(+), 63 deletions(-)\n delete mode 100644 reftable/tree_test.c\n create mode 100644 t/unit-tests/t-reftable-tree.c\n\ndiff --git a/Makefile b/Makefile\nindex 3863e60b66..5499f7bcbd 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1342,6 +1342,7 @@ 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-record\n+UNIT_TEST_PROGRAMS += t-reftable-tree\n UNIT_TEST_PROGRAMS += t-strbuf\n UNIT_TEST_PROGRAMS += t-strcmp-offset\n UNIT_TEST_PROGRAMS += t-strvec\n@@ -2685,7 +2686,6 @@ 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\n \n TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))\n \ndiff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h\nindex d5e03dcc1b..8516b1f923 100644\n--- a/reftable/reftable-tests.h\n+++ b/reftable/reftable-tests.h\n@@ -15,7 +15,6 @@ 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);\n \n #endif\ndiff --git a/reftable/tree_test.c b/reftable/tree_test.c\ndeleted file mode 100644\nindex 6961a657ad..0000000000\n--- a/reftable/tree_test.c\n+++ /dev/null\n@@ -1,60 +0,0 @@\n-/*\n-Copyright 2020 Google LLC\n-\n-Use of this source code is governed by a BSD-style\n-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-#include \"tree.h\"\n-\n-#include \"test_framework.h\"\n-#include \"reftable-tests.h\"\n-\n-static int test_compare(const void *a, const void *b)\n-{\n-\treturn (char *)a - (char *)b;\n-}\n-\n-struct curry {\n-\tvoid *last;\n-};\n-\n-static void check_increasing(void *arg, void *key)\n-{\n-\tstruct curry *c = arg;\n-\tif (c->last) {\n-\t\tEXPECT(test_compare(c->last, key) < 0);\n-\t}\n-\tc->last = key;\n-}\n-\n-static void test_tree(void)\n-{\n-\tstruct tree_node *root = NULL;\n-\n-\tvoid *values[11] = { NULL };\n-\tstruct tree_node *nodes[11] = { NULL };\n-\tint i = 1;\n-\tstruct curry c = { NULL };\n-\tdo {\n-\t\tnodes[i] = tree_search(values + i, &root, &test_compare, 1);\n-\t\ti = (i * 7) % 11;\n-\t} while (i != 1);\n-\n-\tfor (i = 1; i < ARRAY_SIZE(nodes); i++) {\n-\t\tEXPECT(values + i == nodes[i]->key);\n-\t\tEXPECT(nodes[i] ==\n-\t\t tree_search(values + i, &root, &test_compare, 0));\n-\t}\n-\n-\tinfix_walk(root, check_increasing, &c);\n-\ttree_free(root);\n-}\n-\n-int tree_test_main(int argc, const char *argv[])\n-{\n-\tRUN_TEST(test_tree);\n-\treturn 0;\n-}\ndiff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c\nindex 9d378427da..0acaf85494 100644\n--- a/t/helper/test-reftable.c\n+++ b/t/helper/test-reftable.c\n@@ -6,7 +6,6 @@ int cmd__reftable(int argc, const char **argv)\n {\n \t/* test from simple to complex. */\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);\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nnew file mode 100644\nindex 0000000000..8b1f9a66a0\n--- /dev/null\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -0,0 +1,60 @@\n+/*\n+Copyright 2020 Google LLC\n+\n+Use of this source code is governed by a BSD-style\n+license that can be found in the LICENSE file or at\n+https://developers.google.com/open-source/licenses/bsd\n+*/\n+\n+#include \"test-lib.h\"\n+#include \"reftable/tree.h\"\n+\n+static int t_compare(const void *a, const void *b)\n+{\n+\treturn (char *)a - (char *)b;\n+}\n+\n+struct curry {\n+\tvoid *last;\n+};\n+\n+static void check_increasing(void *arg, void *key)\n+{\n+\tstruct curry *c = arg;\n+\tif (c->last)\n+\t\tcheck_int(t_compare(c->last, key), <, 0);\n+\tc->last = key;\n+}\n+\n+static void t_tree(void)\n+{\n+\tstruct tree_node *root = NULL;\n+\tvoid *values[11] = { 0 };\n+\tstruct tree_node *nodes[11] = { 0 };\n+\tsize_t i = 1;\n+\tstruct curry c = { 0 };\n+\n+\t/*\n+\t * Pseudo-randomly insert the pointers for elements between\n+\t * values[1] and values[10] (inclusive) in the tree.\n+\t */\n+\tdo {\n+\t\tnodes[i] = tree_search(&values[i], &root, &t_compare, 1);\n+\t\ti = (i * 7) % 11;\n+\t} while (i != 1);\n+\n+\tfor (i = 1; i < ARRAY_SIZE(nodes); i++) {\n+\t\tcheck_pointer_eq(&values[i], nodes[i]->key);\n+\t\tcheck_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));\n+\t}\n+\n+\tinfix_walk(root, check_increasing, &c);\n+\ttree_free(root);\n+}\n+\n+int cmd_main(int argc, const char *argv[])\n+{\n+\tTEST(t_tree(), \"tree_search and infix_walk work\");\n+\n+\treturn test_done();\n+}\n-- \n2.45.GIT\n\n"},{"id":"500022","messageId":"20240804141105.4268-4-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240804141105.4268-1-chandrapratap3519@gmail.com","subject":"[PATCH v7 3/5] t-reftable-tree: split test_tree() into two sub-test functions","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-04T14:06:47Z","receivedAt":"2024-08-04T14:11:39Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup, tests for both tree_search() and\ninfix_walk() defined by reftable/tree.{c, h} are performed by\na single test function, test_tree(). Split tree_test() into\ntest_tree_search() and test_infix_walk() responsible for\nindependently testing tree_search() and infix_walk() respectively.\nThis improves the overall readability of the test file as well as\nsimplifies debugging.\n\nNote that the last parameter in the tree_search() functiom is\n'int insert' which when set, inserts the key if it is not found\nin the tree. Otherwise, the function returns NULL for such cases.\n\nWhile at it, use 'func' to pass function pointers and not '&func'.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 23 +++++++++++++++++++----\n 1 file changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex 8b1f9a66a0..7cc52a1925 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -26,13 +26,12 @@ static void check_increasing(void *arg, void *key)\n \tc->last = key;\n }\n \n-static void t_tree(void)\n+static void t_tree_search(void)\n {\n \tstruct tree_node *root = NULL;\n \tvoid *values[11] = { 0 };\n \tstruct tree_node *nodes[11] = { 0 };\n \tsize_t i = 1;\n-\tstruct curry c = { 0 };\n \n \t/*\n \t * Pseudo-randomly insert the pointers for elements between\n@@ -48,13 +47,29 @@ static void t_tree(void)\n \t\tcheck_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));\n \t}\n \n-\tinfix_walk(root, check_increasing, &c);\n+\ttree_free(root);\n+}\n+\n+static void t_infix_walk(void)\n+{\n+\tstruct tree_node *root = NULL;\n+\tvoid *values[11] = { 0 };\n+\tstruct curry c = { 0 };\n+\tsize_t i = 1;\n+\n+\tdo {\n+\t\ttree_search(&values[i], &root, t_compare, 1);\n+\t\ti = (i * 7) % 11;\n+\t} while (i != 1);\n+\n+\tinfix_walk(root, &check_increasing, &c);\n \ttree_free(root);\n }\n \n int cmd_main(int argc, const char *argv[])\n {\n-\tTEST(t_tree(), \"tree_search and infix_walk work\");\n+\tTEST(t_tree_search(), \"tree_search works\");\n+\tTEST(t_infix_walk(), \"infix_walk works\");\n \n \treturn test_done();\n }\n-- \n2.45.GIT\n\n"},{"id":"500023","messageId":"20240804141105.4268-5-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240804141105.4268-1-chandrapratap3519@gmail.com","subject":"[PATCH v7 4/5] t-reftable-tree: add test for non-existent key","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-04T14:06:48Z","receivedAt":"2024-08-04T14:11:42Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup for tree_search(), the case for\nnon-existent key is not exercised. Improve this by adding a\ntest-case for the same.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex 7cc52a1925..2220414a18 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -47,6 +47,7 @@ static void t_tree_search(void)\n \t\tcheck_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));\n \t}\n \n+\tcheck(!tree_search(values, &root, t_compare, 0));\n \ttree_free(root);\n }\n \n-- \n2.45.GIT\n\n"},{"id":"500024","messageId":"20240804141105.4268-6-chandrapratap3519@gmail.com","threadId":"61614","inReplyTo":"20240804141105.4268-1-chandrapratap3519@gmail.com","subject":"[PATCH v7 5/5] t-reftable-tree: improve the test for infix_walk()","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-08-04T14:06:49Z","receivedAt":"2024-08-04T14:11:45Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"In the current testing setup for infix_walk(), the following\nproperties of an infix traversal of a tree remain untested:\n- every node of the tree must be visited\n- every node must be visited exactly once\nIn fact, only the property 'traversal in increasing order' is tested.\nModify test_infix_walk() to check for all the properties above.\n\nThis can be achieved by storing the nodes' keys linearly, in a nullified\nbuffer, as we visit them and then checking the input keys against this\nbuffer in increasing order. By checking that the element just after\nthe last input key is 'NULL' in the output buffer, we ensure that\nevery node is traversed exactly once.\n\nMentored-by: Patrick Steinhardt \nMentored-by: Christian Couder \nSigned-off-by: Chandra Pratap \n---\n t/unit-tests/t-reftable-tree.c | 22 +++++++++++++++-------\n 1 file changed, 15 insertions(+), 7 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c\nindex 2220414a18..e7d774d774 100644\n--- a/t/unit-tests/t-reftable-tree.c\n+++ b/t/unit-tests/t-reftable-tree.c\n@@ -15,15 +15,14 @@ static int t_compare(const void *a, const void *b)\n }\n \n struct curry {\n-\tvoid *last;\n+\tvoid **arr;\n+\tsize_t len;\n };\n \n-static void check_increasing(void *arg, void *key)\n+static void store(void *arg, void *key)\n {\n \tstruct curry *c = arg;\n-\tif (c->last)\n-\t\tcheck_int(t_compare(c->last, key), <, 0);\n-\tc->last = key;\n+\tc->arr[c->len++] = key;\n }\n \n static void t_tree_search(void)\n@@ -55,15 +54,24 @@ static void t_infix_walk(void)\n {\n \tstruct tree_node *root = NULL;\n \tvoid *values[11] = { 0 };\n-\tstruct curry c = { 0 };\n+\tvoid *out[11] = { 0 };\n+\tstruct curry c = {\n+\t\t.arr = (void **) &out,\n+\t};\n \tsize_t i = 1;\n+\tsize_t count = 0;\n \n \tdo {\n \t\ttree_search(&values[i], &root, t_compare, 1);\n \t\ti = (i * 7) % 11;\n+\t\tcount++;\n \t} while (i != 1);\n \n-\tinfix_walk(root, &check_increasing, &c);\n+\tinfix_walk(root, &store, &c);\n+\tfor (i = 1; i < ARRAY_SIZE(values); i++)\n+\t\tcheck_pointer_eq(&values[i], out[i - 1]);\n+\tcheck(!out[i - 1]);\n+\tcheck_int(c.len, ==, count);\n \ttree_free(root);\n }\n \n-- \n2.45.GIT\n\n"},{"id":"500058","messageId":"ZrCx0NWRbFOOReki@tanuki","threadId":"61614","inReplyTo":"20240804141105.4268-1-chandrapratap3519@gmail.com","subject":"Re: [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-05T11:04:48Z","receivedAt":"2024-08-05T11:04:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Aug 04, 2024 at 07:36:44PM +0530, Chandra Pratap wrote:\n> The reftable library comes with self tests, which are exercised\n> as part of the usual end-to-end tests and are designed to\n> observe the end-user visible effects of Git commands. What it\n> exercises, however, is a better match for the unit-testing\n> framework, merged at 8bf6fbd0 (Merge branch 'js/doc-unit-tests',\n> 2023-12-09), which is designed to observe how low level\n> implementation details, at the level of sequences of individual\n> function calls, behave.\n> \n> Hence, port reftable/tree_test.c to the unit testing framework and\n> improve upon the ported test. The first patch in the series is\n> preparatory cleanup, the second patch moves the test to the unit\n> testing framework, and the rest of the patches improve upon the\n> ported test.\n> \n> Mentored-by: Patrick Steinhardt \n> Mentored-by: Christian Couder \n> Signed-off-by: Chandra Pratap \n\nOnly a single change compared to v6, addressing the only feedback on\nthat version. So this looks good to me, thanks!\n\nPatrick\n"},{"id":"500093","messageId":"xmqqr0b33r16.fsf@gitster.g","threadId":"61614","inReplyTo":"ZrCx0NWRbFOOReki@tanuki","subject":"Re: [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-05T15:53:09Z","receivedAt":"2024-08-05T15:53:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Sun, Aug 04, 2024 at 07:36:44PM +0530, Chandra Pratap wrote:\n>> The reftable library comes with self tests, which are exercised\n>> as part of the usual end-to-end tests and are designed to\n>> observe the end-user visible effects of Git commands. What it\n>> exercises, however, is a better match for the unit-testing\n>> framework, merged at 8bf6fbd0 (Merge branch 'js/doc-unit-tests',\n>> 2023-12-09), which is designed to observe how low level\n>> implementation details, at the level of sequences of individual\n>> function calls, behave.\n>> \n>> Hence, port reftable/tree_test.c to the unit testing framework and\n>> improve upon the ported test. The first patch in the series is\n>> preparatory cleanup, the second patch moves the test to the unit\n>> testing framework, and the rest of the patches improve upon the\n>> ported test.\n>> \n>> Mentored-by: Patrick Steinhardt \n>> Mentored-by: Christian Couder \n>> Signed-off-by: Chandra Pratap \n>\n> Only a single change compared to v6, addressing the only feedback on\n> that version. So this looks good to me, thanks!\n\nFWIW, I didn't have other feedback not because I found the rest\nperfect, but because I didn't read the series myself carefully,\nhoping others are sharing the burden.\n\nThanks.\n"},{"id":"500158","messageId":"ZrHDB1-yTv_gD1Mx@tanuki","threadId":"61614","inReplyTo":"xmqqr0b33r16.fsf@gitster.g","subject":"Re: [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-06T06:30:31Z","receivedAt":"2024-08-06T06:30:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Aug 05, 2024 at 08:53:09AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > On Sun, Aug 04, 2024 at 07:36:44PM +0530, Chandra Pratap wrote:\n> >> The reftable library comes with self tests, which are exercised\n> >> as part of the usual end-to-end tests and are designed to\n> >> observe the end-user visible effects of Git commands. What it\n> >> exercises, however, is a better match for the unit-testing\n> >> framework, merged at 8bf6fbd0 (Merge branch 'js/doc-unit-tests',\n> >> 2023-12-09), which is designed to observe how low level\n> >> implementation details, at the level of sequences of individual\n> >> function calls, behave.\n> >> \n> >> Hence, port reftable/tree_test.c to the unit testing framework and\n> >> improve upon the ported test. The first patch in the series is\n> >> preparatory cleanup, the second patch moves the test to the unit\n> >> testing framework, and the rest of the patches improve upon the\n> >> ported test.\n> >> \n> >> Mentored-by: Patrick Steinhardt \n> >> Mentored-by: Christian Couder \n> >> Signed-off-by: Chandra Pratap \n> >\n> > Only a single change compared to v6, addressing the only feedback on\n> > that version. So this looks good to me, thanks!\n> \n> FWIW, I didn't have other feedback not because I found the rest\n> perfect, but because I didn't read the series myself carefully,\n> hoping others are sharing the burden.\n\nOh, yes. I didn't mean to say that I relied on your feedback being\naddressed exclusively. I already reviewed v5/v6 of this patch series and\nfound it to be good, and given that there was only a single change\nproposed by you that I'm happy with it translates into me being in favor\nof v7, as well.\n\nPatrick\n"},{"id":"500209","messageId":"xmqqplqlzmt7.fsf@gitster.g","threadId":"61614","inReplyTo":"ZrHDB1-yTv_gD1Mx@tanuki","subject":"Re: [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-06T15:35:32Z","receivedAt":"2024-08-06T15:35:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Oh, yes. I didn't mean to say that I relied on your feedback being\n> addressed exclusively. I already reviewed v5/v6 of this patch series and\n> found it to be good, and given that there was only a single change\n> proposed by you that I'm happy with it translates into me being in favor\n> of v7, as well.\n\nYes, after sending the message you are responding to, I went back to\nthe original thread and figured that much---the topic is marked for\n'next'.\n\nThanks for your help.\n"}]}