{"thread":{"id":"61590","subject":"[GSoC][PATCH] t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c","startedAt":"2024-06-05T13:44:41Z","lastAt":"2024-06-10T23:36:21Z","messageCount":14,"participants":["Ghanshyam Thakkar","Junio C Hamano","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"496370","messageId":"20240605134400.37309-1-shyamthakkar001@gmail.com","threadId":"61590","inReplyTo":null,"subject":"[GSoC][PATCH] t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-06-05T13:43:52Z","receivedAt":"2024-06-05T13:44:41Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"helper/test-oidtree.c along with t0069-oidtree.sh test the oidtree.h\nlibrary, which is a wrapper around crit-bit tree. Migrate them to\nthe unit testing framework for better debugging and runtime\nperformance.\n\nTo achieve this, introduce a new library called 'lib-oid.h'\nexclusively for the unit tests to use. It currently mainly includes\nutility to generate object_id from an arbitrary hex string\n(i.e. '12a' -> '12a0000000000000000000000000000000000000').\nThis will also be helpful when we port other unit tests such\nas oid-array, oidset etc.\n\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\nSigned-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>\n---\n Makefile                 |  8 +++-\n t/helper/test-oidtree.c  | 54 ------------------------\n t/helper/test-tool.c     |  1 -\n t/helper/test-tool.h     |  1 -\n t/t0069-oidtree.sh       | 50 ----------------------\n t/unit-tests/lib-oid.c   | 52 +++++++++++++++++++++++\n t/unit-tests/lib-oid.h   | 17 ++++++++\n t/unit-tests/t-oidtree.c | 91 ++++++++++++++++++++++++++++++++++++++++\n 8 files changed, 166 insertions(+), 108 deletions(-)\n delete mode 100644 t/helper/test-oidtree.c\n delete mode 100755 t/t0069-oidtree.sh\n create mode 100644 t/unit-tests/lib-oid.c\n create mode 100644 t/unit-tests/lib-oid.h\n create mode 100644 t/unit-tests/t-oidtree.c\n\ndiff --git a/Makefile b/Makefile\nindex 59d98ba688..6c9927afae 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -811,7 +811,6 @@ TEST_BUILTINS_OBJS += test-mergesort.o\n TEST_BUILTINS_OBJS += test-mktemp.o\n TEST_BUILTINS_OBJS += test-oid-array.o\n TEST_BUILTINS_OBJS += test-oidmap.o\n-TEST_BUILTINS_OBJS += test-oidtree.o\n TEST_BUILTINS_OBJS += test-online-cpus.o\n TEST_BUILTINS_OBJS += test-pack-mtimes.o\n TEST_BUILTINS_OBJS += test-parse-options.o\n@@ -1335,6 +1334,7 @@ THIRD_PARTY_SOURCES += sha1dc/%\n \n UNIT_TEST_PROGRAMS += t-ctype\n UNIT_TEST_PROGRAMS += t-mem-pool\n+UNIT_TEST_PROGRAMS += t-oidtree\n UNIT_TEST_PROGRAMS += t-prio-queue\n UNIT_TEST_PROGRAMS += t-strbuf\n UNIT_TEST_PROGRAMS += t-strcmp-offset\n@@ -1342,6 +1342,7 @@ UNIT_TEST_PROGRAMS += t-trailer\n UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o\n+UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-oid.o\n \n # xdiff and reftable libs may in turn depend on what is in libgit.a\n GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)\n@@ -3882,7 +3883,10 @@ $(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS\n \t\t-Wl,--allow-multiple-definition \\\n \t\t$(filter %.o,$^) $(filter %.a,$^) $(LIBS) $(LIB_FUZZING_ENGINE)\n \n-$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS\n+$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o \\\n+\t$(UNIT_TEST_DIR)/test-lib.o \\\n+\t$(UNIT_TEST_DIR)/lib-oid.o \\\n+\t$(GITLIBS) GIT-LDFLAGS\n \t$(call mkdir_p_parent_template)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \\\n \t\t$(filter %.o,$^) $(filter %.a,$^) $(LIBS)\ndiff --git a/t/helper/test-oidtree.c b/t/helper/test-oidtree.c\ndeleted file mode 100644\nindex c7a1d4c642..0000000000\n--- a/t/helper/test-oidtree.c\n+++ /dev/null\n@@ -1,54 +0,0 @@\n-#include \"test-tool.h\"\n-#include \"hex.h\"\n-#include \"oidtree.h\"\n-#include \"setup.h\"\n-#include \"strbuf.h\"\n-\n-static enum cb_next print_oid(const struct object_id *oid, void *data UNUSED)\n-{\n-\tputs(oid_to_hex(oid));\n-\treturn CB_CONTINUE;\n-}\n-\n-int cmd__oidtree(int argc UNUSED, const char **argv UNUSED)\n-{\n-\tstruct oidtree ot;\n-\tstruct strbuf line = STRBUF_INIT;\n-\tint nongit_ok;\n-\tint algo = GIT_HASH_UNKNOWN;\n-\n-\toidtree_init(&ot);\n-\tsetup_git_directory_gently(&nongit_ok);\n-\n-\twhile (strbuf_getline(&line, stdin) != EOF) {\n-\t\tconst char *arg;\n-\t\tstruct object_id oid;\n-\n-\t\tif (skip_prefix(line.buf, \"insert \", &arg)) {\n-\t\t\tif (get_oid_hex_any(arg, &oid) == GIT_HASH_UNKNOWN)\n-\t\t\t\tdie(\"insert not a hexadecimal oid: %s\", arg);\n-\t\t\talgo = oid.algo;\n-\t\t\toidtree_insert(&ot, &oid);\n-\t\t} else if (skip_prefix(line.buf, \"contains \", &arg)) {\n-\t\t\tif (get_oid_hex(arg, &oid))\n-\t\t\t\tdie(\"contains not a hexadecimal oid: %s\", arg);\n-\t\t\tprintf(\"%d\\n\", oidtree_contains(&ot, &oid));\n-\t\t} else if (skip_prefix(line.buf, \"each \", &arg)) {\n-\t\t\tchar buf[GIT_MAX_HEXSZ + 1] = { '0' };\n-\t\t\tmemset(&oid, 0, sizeof(oid));\n-\t\t\tmemcpy(buf, arg, strlen(arg));\n-\t\t\tbuf[hash_algos[algo].hexsz] = '\\0';\n-\t\t\tget_oid_hex_any(buf, &oid);\n-\t\t\toid.algo = algo;\n-\t\t\toidtree_each(&ot, &oid, strlen(arg), print_oid, NULL);\n-\t\t} else if (!strcmp(line.buf, \"clear\")) {\n-\t\t\toidtree_clear(&ot);\n-\t\t} else {\n-\t\t\tdie(\"unknown command: %s\", line.buf);\n-\t\t}\n-\t}\n-\n-\tstrbuf_release(&line);\n-\n-\treturn 0;\n-}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 7ad7d07018..253324a06b 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -46,7 +46,6 @@ static struct test_cmd cmds[] = {\n \t{ \"mktemp\", cmd__mktemp },\n \t{ \"oid-array\", cmd__oid_array },\n \t{ \"oidmap\", cmd__oidmap },\n-\t{ \"oidtree\", cmd__oidtree },\n \t{ \"online-cpus\", cmd__online_cpus },\n \t{ \"pack-mtimes\", cmd__pack_mtimes },\n \t{ \"parse-options\", cmd__parse_options },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex d14b3072bd..460dd7d260 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -39,7 +39,6 @@ int cmd__match_trees(int argc, const char **argv);\n int cmd__mergesort(int argc, const char **argv);\n int cmd__mktemp(int argc, const char **argv);\n int cmd__oidmap(int argc, const char **argv);\n-int cmd__oidtree(int argc, const char **argv);\n int cmd__online_cpus(int argc, const char **argv);\n int cmd__pack_mtimes(int argc, const char **argv);\n int cmd__parse_options(int argc, const char **argv);\ndiff --git a/t/t0069-oidtree.sh b/t/t0069-oidtree.sh\ndeleted file mode 100755\nindex 889db50818..0000000000\n--- a/t/t0069-oidtree.sh\n+++ /dev/null\n@@ -1,50 +0,0 @@\n-#!/bin/sh\n-\n-test_description='basic tests for the oidtree implementation'\n-TEST_PASSES_SANITIZE_LEAK=true\n-. ./test-lib.sh\n-\n-maxhexsz=$(test_oid hexsz)\n-echoid () {\n-\tprefix=\"${1:+$1 }\"\n-\tshift\n-\twhile test $# -gt 0\n-\tdo\n-\t\tshortoid=\"$1\"\n-\t\tshift\n-\t\tdifference=$(($maxhexsz - ${#shortoid}))\n-\t\tprintf \"%s%s%0${difference}d\\\\n\" \"$prefix\" \"$shortoid\" \"0\"\n-\tdone\n-}\n-\n-test_expect_success 'oidtree insert and contains' '\n-\tcat >expect <<-\\EOF &&\n-\t\t0\n-\t\t0\n-\t\t0\n-\t\t1\n-\t\t1\n-\t\t0\n-\tEOF\n-\t{\n-\t\techoid insert 444 1 2 3 4 5 a b c d e &&\n-\t\techoid contains 44 441 440 444 4440 4444 &&\n-\t\techo clear\n-\t} | test-tool oidtree >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'oidtree each' '\n-\techoid \"\" 123 321 321 >expect &&\n-\t{\n-\t\techoid insert f 9 8 123 321 a b c d e &&\n-\t\techo each 12300 &&\n-\t\techo each 3211 &&\n-\t\techo each 3210 &&\n-\t\techo each 32100 &&\n-\t\techo clear\n-\t} | test-tool oidtree >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_done\ndiff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c\nnew file mode 100644\nindex 0000000000..37105f0a8f\n--- /dev/null\n+++ b/t/unit-tests/lib-oid.c\n@@ -0,0 +1,52 @@\n+#include \"test-lib.h\"\n+#include \"lib-oid.h\"\n+#include \"strbuf.h\"\n+#include \"hex.h\"\n+\n+static int init_hash_algo(void)\n+{\n+\tstatic int algo = -1;\n+\n+\tif (algo < 0) {\n+\t\tconst char *algo_name = getenv(\"GIT_TEST_DEFAULT_HASH\");\n+\t\talgo = algo_name ? hash_algo_by_name(algo_name) : GIT_HASH_SHA1;\n+\n+\t\tif (!check(algo != GIT_HASH_UNKNOWN))\n+\t\t\ttest_msg(\"BUG: invalid GIT_TEST_DEFAULT_HASH value ('%s')\",\n+\t\t\t\t algo_name);\n+\t}\n+\treturn algo;\n+}\n+\n+static int get_oid_arbitrary_hex_algop(const char *hex, struct object_id *oid,\n+\t\t\t\t       const struct git_hash_algo *algop)\n+{\n+\tint ret;\n+\tsize_t sz = strlen(hex);\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tif (!check(sz <= algop->hexsz)) {\n+\t\ttest_msg(\"BUG: hex string (%s) bigger than maximum allowed (%lu)\",\n+\t\t\t hex, (unsigned long)algop->hexsz);\n+\t\treturn -1;\n+\t}\n+\n+\tstrbuf_add(&buf, hex, sz);\n+\tstrbuf_addchars(&buf, '0', algop->hexsz - sz);\n+\n+\tret = get_oid_hex_algop(buf.buf, oid, algop);\n+\tif (!check_int(ret, ==, 0))\n+\t\ttest_msg(\"BUG: invalid hex input (%s) provided\", hex);\n+\n+\tstrbuf_release(&buf);\n+\treturn ret;\n+}\n+\n+int get_oid_arbitrary_hex(const char *hex, struct object_id *oid)\n+{\n+\tint hash_algo = init_hash_algo();\n+\n+\tif (!check_int(hash_algo, !=, GIT_HASH_UNKNOWN))\n+\t\treturn -1;\n+\treturn get_oid_arbitrary_hex_algop(hex, oid, &hash_algos[hash_algo]);\n+}\ndiff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h\nnew file mode 100644\nindex 0000000000..bfde639190\n--- /dev/null\n+++ b/t/unit-tests/lib-oid.h\n@@ -0,0 +1,17 @@\n+#ifndef LIB_OID_H\n+#define LIB_OID_H\n+\n+#include \"hash-ll.h\"\n+\n+/*\n+ * Convert arbitrary hex string to object_id.\n+ * For example, passing \"abc12\" will generate\n+ * \"abc1200000000000000000000000000000000000\" hex of length 40 for SHA-1 and\n+ * create object_id with that.\n+ * WARNING: passing a string of length more than the hexsz of respective hash\n+ * algo is not allowed. The hash algo is decided based on GIT_TEST_DEFAULT_HASH\n+ * environment variable.\n+ */\n+int get_oid_arbitrary_hex(const char *s, struct object_id *oid);\n+\n+#endif /* LIB_OID_H */\ndiff --git a/t/unit-tests/t-oidtree.c b/t/unit-tests/t-oidtree.c\nnew file mode 100644\nindex 0000000000..0ebe17d2b9\n--- /dev/null\n+++ b/t/unit-tests/t-oidtree.c\n@@ -0,0 +1,91 @@\n+#include \"test-lib.h\"\n+#include \"lib-oid.h\"\n+#include \"oidtree.h\"\n+#include \"hash.h\"\n+#include \"hex.h\"\n+\n+#define FILL_TREE(tree, ...)                                       \\\n+\tdo {                                                       \\\n+\t\tconst char *hexes[] = { __VA_ARGS__ };             \\\n+\t\tif (fill_tree_loc(tree, hexes, ARRAY_SIZE(hexes))) \\\n+\t\t\treturn;                                    \\\n+\t} while (0)\n+\n+static int fill_tree_loc(struct oidtree *ot, const char *hexes[], int n)\n+{\n+\tfor (size_t i = 0; i < n; i++) {\n+\t\tstruct object_id oid;\n+\t\tif (!check_int(get_oid_arbitrary_hex(hexes[i], &oid), ==, 0))\n+\t\t\treturn -1;\n+\t\toidtree_insert(ot, &oid);\n+\t}\n+\treturn 0;\n+}\n+\n+static void check_contains(struct oidtree *ot, const char *hex, int expected)\n+{\n+\tstruct object_id oid;\n+\n+\tif (!check_int(get_oid_arbitrary_hex(hex, &oid), ==, 0))\n+\t\treturn;\n+\tif (!check_int(oidtree_contains(ot, &oid), ==, expected))\n+\t\ttest_msg(\"oid: %s\", oid_to_hex(&oid));\n+}\n+\n+static enum cb_next check_each_cb(const struct object_id *oid, void *data)\n+{\n+\tconst char *hex = data;\n+\tstruct object_id expected;\n+\n+\tif (!check_int(get_oid_arbitrary_hex(hex, &expected), ==, 0))\n+\t\treturn CB_CONTINUE;\n+\tif (!check(oideq(oid, &expected)))\n+\t\ttest_msg(\"expected: %s\\n       got: %s\",\n+\t\t\t hash_to_hex(expected.hash), hash_to_hex(oid->hash));\n+\treturn CB_CONTINUE;\n+}\n+\n+static void check_each(struct oidtree *ot, char *hex, char *expected)\n+{\n+\tstruct object_id oid;\n+\n+\tif (!check_int(get_oid_arbitrary_hex(hex, &oid), ==, 0))\n+\t\treturn;\n+\toidtree_each(ot, &oid, 40, check_each_cb, expected);\n+}\n+\n+static void setup(void (*f)(struct oidtree *ot))\n+{\n+\tstruct oidtree ot;\n+\n+\toidtree_init(&ot);\n+\tf(&ot);\n+\toidtree_clear(&ot);\n+}\n+\n+static void t_contains(struct oidtree *ot)\n+{\n+\tFILL_TREE(ot, \"444\", \"1\", \"2\", \"3\", \"4\", \"5\", \"a\", \"b\", \"c\", \"d\", \"e\");\n+\tcheck_contains(ot, \"44\", 0);\n+\tcheck_contains(ot, \"441\", 0);\n+\tcheck_contains(ot, \"440\", 0);\n+\tcheck_contains(ot, \"444\", 1);\n+\tcheck_contains(ot, \"4440\", 1);\n+\tcheck_contains(ot, \"4444\", 0);\n+}\n+\n+static void t_each(struct oidtree *ot)\n+{\n+\tFILL_TREE(ot, \"f\", \"9\", \"8\", \"123\", \"321\", \"a\", \"b\", \"c\", \"d\", \"e\");\n+\tcheck_each(ot, \"12300\", \"123\");\n+\tcheck_each(ot, \"3211\", \"\"); /* should not reach callback */\n+\tcheck_each(ot, \"3210\", \"321\");\n+\tcheck_each(ot, \"32100\", \"321\");\n+}\n+\n+int cmd_main(int argc UNUSED, const char **argv UNUSED)\n+{\n+\tTEST(setup(t_contains), \"oidtree insert and contains works\");\n+\tTEST(setup(t_each), \"oidtree each works\");\n+\treturn test_done();\n+}\n-- \n2.45.2\n\n"},{"id":"496564","messageId":"xmqqo78dka99.fsf@gitster.g","threadId":"61590","inReplyTo":"20240605134400.37309-1-shyamthakkar001@gmail.com","subject":"Re: [GSoC][PATCH] t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-06T21:54:26Z","receivedAt":"2024-06-06T21:55:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n\n> helper/test-oidtree.c along with t0069-oidtree.sh test the oidtree.h\n> library, which is a wrapper around crit-bit tree. Migrate them to\n> the unit testing framework for better debugging and runtime\n> performance.\n>\n> To achieve this, introduce a new library called 'lib-oid.h'\n> exclusively for the unit tests to use. It currently mainly includes\n> utility to generate object_id from an arbitrary hex string\n> (i.e. '12a' -> '12a0000000000000000000000000000000000000').\n> This will also be helpful when we port other unit tests such\n> as oid-array, oidset etc.\n\nPerhaps.  With only a single user it is hard to judge if it is worth\ndoing, but once the code is written, it is not worth a code churn to\nmerge it into t-oidtree.c.\n\n> +#define FILL_TREE(tree, ...)                                       \\\n> +\tdo {                                                       \\\n> +\t\tconst char *hexes[] = { __VA_ARGS__ };             \\\n> +\t\tif (fill_tree_loc(tree, hexes, ARRAY_SIZE(hexes))) \\\n> +\t\t\treturn;                                    \\\n> +\t} while (0)\n\nNice.\n\n> +static enum cb_next check_each_cb(const struct object_id *oid, void *data)\n> +{\n> +\tconst char *hex = data;\n> +\tstruct object_id expected;\n> +\n> +\tif (!check_int(get_oid_arbitrary_hex(hex, &expected), ==, 0))\n> +\t\treturn CB_CONTINUE;\n> +\tif (!check(oideq(oid, &expected)))\n> +\t\ttest_msg(\"expected: %s\\n       got: %s\",\n> +\t\t\t hash_to_hex(expected.hash), hash_to_hex(oid->hash));\n> +\treturn CB_CONTINUE;\n> +}\n\nThe control flow looks somewhat strange here.  I would have written:\n\n\tif (!check_int(..., ==, 0))\n\t\t; /* the data is bogus and cannot be used */\n\telse if (!check(oideq(...))\n\t\ttest_msg(... expected and got differ ...);\n\treturn CB_CONTINUE;\n\nbut OK.\n\n> +static void check_each(struct oidtree *ot, char *hex, char *expected)\n> +{\n> +\tstruct object_id oid;\n> +\n> +\tif (!check_int(get_oid_arbitrary_hex(hex, &oid), ==, 0))\n> +\t\treturn;\n> +\toidtree_each(ot, &oid, 40, check_each_cb, expected);\n> +}\n> +\n> +static void setup(void (*f)(struct oidtree *ot))\n> +{\n> +\tstruct oidtree ot;\n> +\n> +\toidtree_init(&ot);\n> +\tf(&ot);\n> +\toidtree_clear(&ot);\n> +}\n> +\n> +static void t_contains(struct oidtree *ot)\n> +{\n> +\tFILL_TREE(ot, \"444\", \"1\", \"2\", \"3\", \"4\", \"5\", \"a\", \"b\", \"c\", \"d\", \"e\");\n> +\tcheck_contains(ot, \"44\", 0);\n> +\tcheck_contains(ot, \"441\", 0);\n> +\tcheck_contains(ot, \"440\", 0);\n> +\tcheck_contains(ot, \"444\", 1);\n> +\tcheck_contains(ot, \"4440\", 1);\n> +\tcheck_contains(ot, \"4444\", 0);\n> +}\n\nOK.\n\nCompared to the original, this makes the correspondence between the\ninput and the expected result slightly easier to see, which is good.\n\n> +static void t_each(struct oidtree *ot)\n> +{\n> +\tFILL_TREE(ot, \"f\", \"9\", \"8\", \"123\", \"321\", \"a\", \"b\", \"c\", \"d\", \"e\");\n> +\tcheck_each(ot, \"12300\", \"123\");\n> +\tcheck_each(ot, \"3211\", \"\"); /* should not reach callback */\n> +\tcheck_each(ot, \"3210\", \"321\");\n> +\tcheck_each(ot, \"32100\", \"321\");\n> +}\n\nTesting \"each\" with test data that yields only at most one response\nsmells iffy.  It is a problem in the original test, and not a\nproblem with the conversion, ...\n\nBUT\n\n... in the original, it is easy to do something like the attached to\ndemonstrate that \"each\" can yield all oid that the shares the query\nprefix.  But the rewritten unit test bakes the assumption that we\nwill only try a query that yields at most one response into the test\nhelper functions.  Shouldn't we do a bit better, perhaps allowing the\ncheck_each() helper to take variable number of parameters, e.g.\n\n\tcheck_each(ot, \"12300\", \"123\", NULL);\n\tcheck_each(ot, \"32\", \"320\", \"321\", NULL);\n\nso the latter invocation asks \"ot\" trie \"I have prefix 32, please\ncall me back with each element you have that match\", and makes sure\nthat we get called back with \"320\" and then \"321\" and never after.\n\nCome to think of it, how is your check_each_cb() ensuring that it is\nonly called once with \"123\" when queried with \"12300\"?  If the\ncallback is made with \"123\" 100 times with the single query with\n\"12300\", would it even notice?  I would imagine that the original\nwould (simply because it dumps each and every callback to a file to\nbe compared with the golden copy).\n\n t/t0069-oidtree.sh | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git c/t/t0069-oidtree.sh w/t/t0069-oidtree.sh\nindex 889db50818..40b836aff5 100755\n--- c/t/t0069-oidtree.sh\n+++ w/t/t0069-oidtree.sh\n@@ -35,13 +35,14 @@ test_expect_success 'oidtree insert and contains' '\n '\n \n test_expect_success 'oidtree each' '\n-\techoid \"\" 123 321 321 >expect &&\n+\techoid \"\" 123 321 321 320 321 >expect &&\n \t{\n-\t\techoid insert f 9 8 123 321 a b c d e &&\n+\t\techoid insert f 9 8 123 321 320 a b c d e &&\n \t\techo each 12300 &&\n \t\techo each 3211 &&\n \t\techo each 3210 &&\n \t\techo each 32100 &&\n+\t\techo each 32 &&\n \t\techo clear\n \t} | test-tool oidtree >actual &&\n \ttest_cmp expect actual\n"},{"id":"496593","messageId":"dohbd64jxuahelut63esztozdozqrhx5rgv5m4t3wt5gz6v6kv@6q2aivlcvxcq","threadId":"61590","inReplyTo":"xmqqo78dka99.fsf@gitster.g","subject":"Re: [GSoC][PATCH] t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-06-06T23:35:40Z","receivedAt":"2024-06-06T23:35:44Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Thu, 06 Jun 2024, Junio C Hamano <gitster@pobox.com> wrote:\n> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n> > +static void check_each(struct oidtree *ot, char *hex, char *expected)\n> > +{\n> > +\tstruct object_id oid;\n> > +\n> > +\tif (!check_int(get_oid_arbitrary_hex(hex, &oid), ==, 0))\n> > +\t\treturn;\n> > +\toidtree_each(ot, &oid, 40, check_each_cb, expected);\n\nI think I mistakenly kept '40' from when I was testing, but it should\nbe strlen(hex). Will correct.\n\n> > +static void t_each(struct oidtree *ot)\n> > +{\n> > +\tFILL_TREE(ot, \"f\", \"9\", \"8\", \"123\", \"321\", \"a\", \"b\", \"c\", \"d\", \"e\");\n> > +\tcheck_each(ot, \"12300\", \"123\");\n> > +\tcheck_each(ot, \"3211\", \"\"); /* should not reach callback */\n> > +\tcheck_each(ot, \"3210\", \"321\");\n> > +\tcheck_each(ot, \"32100\", \"321\");\n> > +}\n> \n> Testing \"each\" with test data that yields only at most one response\n> smells iffy.  It is a problem in the original test, and not a\n> problem with the conversion, ...\n> \n> BUT\n> \n> ... in the original, it is easy to do something like the attached to\n> demonstrate that \"each\" can yield all oid that the shares the query\n> prefix.  But the rewritten unit test bakes the assumption that we\n> will only try a query that yields at most one response into the test\n> helper functions.  Shouldn't we do a bit better, perhaps allowing the\n> check_each() helper to take variable number of parameters, e.g.\n> \n> \tcheck_each(ot, \"12300\", \"123\", NULL);\n> \tcheck_each(ot, \"32\", \"320\", \"321\", NULL);\n> \n> so the latter invocation asks \"ot\" trie \"I have prefix 32, please\n> call me back with each element you have that match\", and makes sure\n> that we get called back with \"320\" and then \"321\" and never after.\n> \n> Come to think of it, how is your check_each_cb() ensuring that it is\n> only called once with \"123\" when queried with \"12300\"?  If the\n> callback is made with \"123\" 100 times with the single query with\n> \"12300\", would it even notice?  I would imagine that the original\n> would (simply because it dumps each and every callback to a file to\n> be compared with the golden copy).\n\nThat's true! I did not think of that. What do you think about something\nlike this then? I will clean it up to send in v2.\n\n---\n\nstruct cb_data {\n\tint *i;\n\tstruct strvec *expected_hexes;\n};\n\nstatic enum cb_next check_each_cb(const struct object_id *oid, void *data)\n{\n\tstruct cb_data *cb_data = data;\n\tstruct object_id expected;\n\n\tif(!check_int(*cb_data->i, <, cb_data->hexes->nr)) {\n\t\ttest_msg(\"error: extraneous callback. found oid: %s\", oid_to_hex(oid));\n\t\treturn CB_BREAK;\n\t}\n\n\tif (!check_int(get_oid_arbitrary_hex(cb_data->expected_hexes->v[*cb_data->i], &expected), ==, 0))\n\t\treturn CB_BREAK;\n\tif (!check(oideq(oid, &expected)))\n\t\ttest_msg(\"expected: %s\\n       got: %s\",\n\t\t\t hash_to_hex(expected.hash), hash_to_hex(oid->hash));\n\n\t*cb_data->i += 1;\n\treturn CB_CONTINUE;\n}\n\nstatic void check_each(struct oidtree *ot, char *query, ...)\n{\n\tstruct object_id oid;\n\tstruct strvec hexes = STRVEC_INIT;\n\tstruct cb_data cb_data;\n\tconst char *arg;\n\tint i = 0;\n\n\tva_list expected;\n\tva_start(expected, query);\n\n\twhile ((arg = va_arg(expected, const char *)))\n\t\tstrvec_push(&hexes, arg);\n\n\tcb_data.i = &i;\n\tcb_data.expected_hexes = &hexes;\n\n\tif (!check_int(get_oid_arbitrary_hex(query, &oid), ==, 0))\n\t\treturn;\n\toidtree_each(ot, &oid, strlen(query), check_each_cb, &cb_data);\n\n\tif (!check_int(*cb_data.i, ==, cb_data.expected_hexes->nr))\n\t\ttest_msg(\"error: could not find some oids\");\n}\n---\n\nThanks for the review.\n"},{"id":"496634","messageId":"CAP8UFD0r+YYxAvN2Ej1mGa2Kt5M2dQgQEGLraB3iQ30cPWuA6Q@mail.gmail.com","threadId":"61590","inReplyTo":"dohbd64jxuahelut63esztozdozqrhx5rgv5m4t3wt5gz6v6kv@6q2aivlcvxcq","subject":"Re: [GSoC][PATCH] t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-06-07T08:31:29Z","receivedAt":"2024-06-07T08:31:43Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Jun 7, 2024 at 1:35 AM Ghanshyam Thakkar\n<shyamthakkar001@gmail.com> wrote:\n\n> > Come to think of it, how is your check_each_cb() ensuring that it is\n> > only called once with \"123\" when queried with \"12300\"?  If the\n> > callback is made with \"123\" 100 times with the single query with\n> > \"12300\", would it even notice?  I would imagine that the original\n> > would (simply because it dumps each and every callback to a file to\n> > be compared with the golden copy).\n>\n> That's true! I did not think of that. What do you think about something\n> like this then? I will clean it up to send in v2.\n>\n> ---\n>\n> struct cb_data {\n>         int *i;\n>         struct strvec *expected_hexes;\n> };\n\nIt might be better to use a more meaningful name for the struct, like\nperhaps 'expected_hex_iter'. Also I think 'i' could be just 'size_t i'\ninstead of 'int *i', and 'expected_hexes' could be just 'hexes'.\n\n> static enum cb_next check_each_cb(const struct object_id *oid, void *data)\n> {\n>         struct cb_data *cb_data = data;\n\nMaybe: `struct expected_hex_iter *hex_iter = data;`\n\n>         struct object_id expected;\n>\n>         if(!check_int(*cb_data->i, <, cb_data->hexes->nr)) {\n\nA space character is missing between 'if' and '('. And by the way you\nuse 'hexes' instead of 'expected_hexes' here.\n\n>                 test_msg(\"error: extraneous callback. found oid: %s\", oid_to_hex(oid));\n>                 return CB_BREAK;\n>         }\n>\n>         if (!check_int(get_oid_arbitrary_hex(cb_data->expected_hexes->v[*cb_data->i], &expected), ==, 0))\n>                 return CB_BREAK;\n>         if (!check(oideq(oid, &expected)))\n>                 test_msg(\"expected: %s\\n       got: %s\",\n>                          hash_to_hex(expected.hash), hash_to_hex(oid->hash));\n>\n>         *cb_data->i += 1;\n>         return CB_CONTINUE;\n> }\n>\n> static void check_each(struct oidtree *ot, char *query, ...)\n> {\n>         struct object_id oid;\n>         struct strvec hexes = STRVEC_INIT;\n>         struct cb_data cb_data;\n>         const char *arg;\n>         int i = 0;\n>\n>         va_list expected;\n>         va_start(expected, query);\n>\n>         while ((arg = va_arg(expected, const char *)))\n>                 strvec_push(&hexes, arg);\n>\n>         cb_data.i = &i;\n>         cb_data.expected_hexes = &hexes;\n\nCan't we just have something like:\n\n        struct expected_hex_iter hex_iter = { .i = 0, .hexes = &hexes };\n\nabove when 'hex_iter' is declared?\n\n>         if (!check_int(get_oid_arbitrary_hex(query, &oid), ==, 0))\n>                 return;\n>         oidtree_each(ot, &oid, strlen(query), check_each_cb, &cb_data);\n>\n>         if (!check_int(*cb_data.i, ==, cb_data.expected_hexes->nr))\n>                 test_msg(\"error: could not find some oids\");\n> }\n\nThanks.\n"},{"id":"496635","messageId":"CAP8UFD2eUxrr2Z0etZKbakXEB5VAfXH63jLhfAYJyOxBwvJPEQ@mail.gmail.com","threadId":"61590","inReplyTo":"CAP8UFD0r+YYxAvN2Ej1mGa2Kt5M2dQgQEGLraB3iQ30cPWuA6Q@mail.gmail.com","subject":"Re: [GSoC][PATCH] t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-06-07T08:36:57Z","receivedAt":"2024-06-07T08:37:11Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Jun 7, 2024 at 10:31 AM Christian Couder\n<christian.couder@gmail.com> wrote:\n>\n> On Fri, Jun 7, 2024 at 1:35 AM Ghanshyam Thakkar\n> <shyamthakkar001@gmail.com> wrote:\n>\n> > > Come to think of it, how is your check_each_cb() ensuring that it is\n> > > only called once with \"123\" when queried with \"12300\"?  If the\n> > > callback is made with \"123\" 100 times with the single query with\n> > > \"12300\", would it even notice?  I would imagine that the original\n> > > would (simply because it dumps each and every callback to a file to\n> > > be compared with the golden copy).\n> >\n> > That's true! I did not think of that. What do you think about something\n> > like this then? I will clean it up to send in v2.\n> >\n> > ---\n> >\n> > struct cb_data {\n> >         int *i;\n> >         struct strvec *expected_hexes;\n> > };\n>\n> It might be better to use a more meaningful name for the struct, like\n> perhaps 'expected_hex_iter'. Also I think 'i' could be just 'size_t i'\n> instead of 'int *i', and 'expected_hexes' could be just 'hexes'.\n\nMaybe even:\n\nstruct expected_hex_iter {\n       size_t i;\n       struct strvec hexes;\n};\n\n(so without any pointer)\n\n...\n\n> > static enum cb_next check_each_cb(const struct object_id *oid, void *data)\n> > {\n> >         struct cb_data *cb_data = data;\n>\n> Maybe: `struct expected_hex_iter *hex_iter = data;`\n>\n> >         struct object_id expected;\n> >\n> >         if(!check_int(*cb_data->i, <, cb_data->hexes->nr)) {\n>\n> A space character is missing between 'if' and '('. And by the way you\n> use 'hexes' instead of 'expected_hexes' here.\n>\n> >                 test_msg(\"error: extraneous callback. found oid: %s\", oid_to_hex(oid));\n> >                 return CB_BREAK;\n> >         }\n> >\n> >         if (!check_int(get_oid_arbitrary_hex(cb_data->expected_hexes->v[*cb_data->i], &expected), ==, 0))\n> >                 return CB_BREAK;\n> >         if (!check(oideq(oid, &expected)))\n> >                 test_msg(\"expected: %s\\n       got: %s\",\n> >                          hash_to_hex(expected.hash), hash_to_hex(oid->hash));\n> >\n> >         *cb_data->i += 1;\n> >         return CB_CONTINUE;\n> > }\n> >\n> > static void check_each(struct oidtree *ot, char *query, ...)\n> > {\n> >         struct object_id oid;\n> >         struct strvec hexes = STRVEC_INIT;\n> >         struct cb_data cb_data;\n> >         const char *arg;\n> >         int i = 0;\n\n... and above only:\n\n        struct object_id oid;\n        const char *arg;\n        struct expected_hex_iter hex_iter = { 0 };\n"},{"id":"496636","messageId":"CAP8UFD3aTC_s8BgXYDcM_ecjMQmRsMiNMJxgXD2FLYvt26OwWQ@mail.gmail.com","threadId":"61590","inReplyTo":"CAP8UFD2eUxrr2Z0etZKbakXEB5VAfXH63jLhfAYJyOxBwvJPEQ@mail.gmail.com","subject":"Re: [GSoC][PATCH] t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-06-07T08:41:46Z","receivedAt":"2024-06-07T08:41:59Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Jun 7, 2024 at 10:36 AM Christian Couder\n<christian.couder@gmail.com> wrote:\n>\n> On Fri, Jun 7, 2024 at 10:31 AM Christian Couder\n> <christian.couder@gmail.com> wrote:\n> >\n> > On Fri, Jun 7, 2024 at 1:35 AM Ghanshyam Thakkar\n> > <shyamthakkar001@gmail.com> wrote:\n\n\n> > > static void check_each(struct oidtree *ot, char *query, ...)\n> > > {\n> > >         struct object_id oid;\n> > >         struct strvec hexes = STRVEC_INIT;\n> > >         struct cb_data cb_data;\n> > >         const char *arg;\n> > >         int i = 0;\n>\n> ... and above only:\n>\n>         struct object_id oid;\n>         const char *arg;\n>         struct expected_hex_iter hex_iter = { 0 };\n\nActually I think it should be:\n\n        struct expected_hex_iter hex_iter = { .hexes = STRVEC_INIT };\n"},{"id":"496655","messageId":"xmqq4ja4g13y.fsf@gitster.g","threadId":"61590","inReplyTo":"dohbd64jxuahelut63esztozdozqrhx5rgv5m4t3wt5gz6v6kv@6q2aivlcvxcq","subject":"Re: [GSoC][PATCH] t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-07T16:37:53Z","receivedAt":"2024-06-07T16:37:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n\n>> Come to think of it, how is your check_each_cb() ensuring that it is\n>> only called once with \"123\" when queried with \"12300\"?  If the\n>> callback is made with \"123\" 100 times with the single query with\n>> \"12300\", would it even notice?  I would imagine that the original\n>> would (simply because it dumps each and every callback to a file to\n>> be compared with the golden copy).\n>\n> That's true! I did not think of that. What do you think about something\n> like this then? I will clean it up to send in v2.\n\nI do not see a strong reason to have a pointer to int in cb_data, as\nthe caller has access to cb_data after the callback finishes using\nit so check_each() can check cb_data.i instead of *cb_data.i (or i)\nat the end.\n\nBut other than that, yes, it is the direction you would want to go,\nI would think.\n\n>\n> ---\n>\n> struct cb_data {\n> \tint *i;\n> \tstruct strvec *expected_hexes;\n> };\n>\n> static enum cb_next check_each_cb(const struct object_id *oid, void *data)\n> {\n> \tstruct cb_data *cb_data = data;\n> \tstruct object_id expected;\n>\n> \tif(!check_int(*cb_data->i, <, cb_data->hexes->nr)) {\n> \t\ttest_msg(\"error: extraneous callback. found oid: %s\", oid_to_hex(oid));\n> \t\treturn CB_BREAK;\n> \t}\n>\n> \tif (!check_int(get_oid_arbitrary_hex(cb_data->expected_hexes->v[*cb_data->i], &expected), ==, 0))\n> \t\treturn CB_BREAK;\n> \tif (!check(oideq(oid, &expected)))\n> \t\ttest_msg(\"expected: %s\\n       got: %s\",\n> \t\t\t hash_to_hex(expected.hash), hash_to_hex(oid->hash));\n>\n> \t*cb_data->i += 1;\n> \treturn CB_CONTINUE;\n> }\n>\n> static void check_each(struct oidtree *ot, char *query, ...)\n> {\n> \tstruct object_id oid;\n> \tstruct strvec hexes = STRVEC_INIT;\n> \tstruct cb_data cb_data;\n> \tconst char *arg;\n> \tint i = 0;\n>\n> \tva_list expected;\n> \tva_start(expected, query);\n>\n> \twhile ((arg = va_arg(expected, const char *)))\n> \t\tstrvec_push(&hexes, arg);\n>\n> \tcb_data.i = &i;\n> \tcb_data.expected_hexes = &hexes;\n>\n> \tif (!check_int(get_oid_arbitrary_hex(query, &oid), ==, 0))\n> \t\treturn;\n> \toidtree_each(ot, &oid, strlen(query), check_each_cb, &cb_data);\n>\n> \tif (!check_int(*cb_data.i, ==, cb_data.expected_hexes->nr))\n> \t\ttest_msg(\"error: could not find some oids\");\n> }\n> ---\n>\n> Thanks for the review.\n"},{"id":"496713","messageId":"20240608165731.29467-1-shyamthakkar001@gmail.com","threadId":"61590","inReplyTo":"20240605134400.37309-1-shyamthakkar001@gmail.com","subject":"[GSoC][PATCH v2] t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-06-08T16:57:09Z","receivedAt":"2024-06-08T16:58:57Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"helper/test-oidtree.c along with t0069-oidtree.sh test the oidtree.h\nlibrary, which is a wrapper around crit-bit tree. Migrate them to\nthe unit testing framework for better debugging and runtime\nperformance. Along with the migration, add an extra check for\noidtree_each() test, which showcases how multiple expected matches can\nbe given to check_each() helper.\n\nTo achieve this, introduce a new library called 'lib-oid.h'\nexclusively for the unit tests to use. It currently mainly includes\nutility to generate object_id from an arbitrary hex string\n(i.e. '12a' -> '12a0000000000000000000000000000000000000'). This also\nhandles the hash algo selection based on GIT_TEST_DEFAULT_HASH.\nThis library will also be helpful when we port other unit tests such\nas oid-array, oidset etc.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\nSigned-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>\n---\nRange-diff against v1:\n1:  ee3df5db33 ! 1:  6d94be745a t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c\n    @@ Commit message\n         helper/test-oidtree.c along with t0069-oidtree.sh test the oidtree.h\n         library, which is a wrapper around crit-bit tree. Migrate them to\n         the unit testing framework for better debugging and runtime\n    -    performance.\n    +    performance. Along with the migration, add an extra check for\n    +    oidtree_each() test, which showcases how multiple expected matches can\n    +    be given to check_each() helper.\n     \n         To achieve this, introduce a new library called 'lib-oid.h'\n         exclusively for the unit tests to use. It currently mainly includes\n         utility to generate object_id from an arbitrary hex string\n    -    (i.e. '12a' -> '12a0000000000000000000000000000000000000').\n    -    This will also be helpful when we port other unit tests such\n    +    (i.e. '12a' -> '12a0000000000000000000000000000000000000'). This also\n    +    handles the hash algo selection based on GIT_TEST_DEFAULT_HASH.\n    +    This library will also be helpful when we port other unit tests such\n         as oid-array, oidset etc.\n     \n         Mentored-by: Christian Couder <chriscool@tuxfamily.org>\n    @@ t/unit-tests/t-oidtree.c (new)\n     +#include \"oidtree.h\"\n     +#include \"hash.h\"\n     +#include \"hex.h\"\n    ++#include \"strvec.h\"\n     +\n     +#define FILL_TREE(tree, ...)                                       \\\n     +\tdo {                                                       \\\n    @@ t/unit-tests/t-oidtree.c (new)\n     +\t\t\treturn;                                    \\\n     +\t} while (0)\n     +\n    -+static int fill_tree_loc(struct oidtree *ot, const char *hexes[], int n)\n    ++static int fill_tree_loc(struct oidtree *ot, const char *hexes[], size_t n)\n     +{\n     +\tfor (size_t i = 0; i < n; i++) {\n     +\t\tstruct object_id oid;\n    @@ t/unit-tests/t-oidtree.c (new)\n     +\t\ttest_msg(\"oid: %s\", oid_to_hex(&oid));\n     +}\n     +\n    ++struct expected_hex_iter {\n    ++\tsize_t i;\n    ++\tstruct strvec expected_hexes;\n    ++\tconst char *query;\n    ++};\n    ++\n     +static enum cb_next check_each_cb(const struct object_id *oid, void *data)\n     +{\n    -+\tconst char *hex = data;\n    ++\tstruct expected_hex_iter *hex_iter = data;\n     +\tstruct object_id expected;\n     +\n    -+\tif (!check_int(get_oid_arbitrary_hex(hex, &expected), ==, 0))\n    -+\t\treturn CB_CONTINUE;\n    -+\tif (!check(oideq(oid, &expected)))\n    -+\t\ttest_msg(\"expected: %s\\n       got: %s\",\n    -+\t\t\t hash_to_hex(expected.hash), hash_to_hex(oid->hash));\n    ++\tif (!check_int(hex_iter->i, <, hex_iter->expected_hexes.nr)) {\n    ++\t\ttest_msg(\"error: extraneous callback for query: ('%s'), object_id: ('%s')\",\n    ++\t\t\t hex_iter->query, oid_to_hex(oid));\n    ++\t\treturn CB_BREAK;\n    ++\t}\n    ++\n    ++\tif (!check_int(get_oid_arbitrary_hex(hex_iter->expected_hexes.v[hex_iter->i],\n    ++\t\t\t\t\t     &expected), ==, 0))\n    ++\t\t; /* the data is bogus and cannot be used */\n    ++\telse if (!check(oideq(oid, &expected)))\n    ++\t\ttest_msg(\"expected: %s\\n       got: %s\\n     query: %s\",\n    ++\t\t\t oid_to_hex(&expected), oid_to_hex(oid), hex_iter->query);\n    ++\n    ++\thex_iter->i += 1;\n     +\treturn CB_CONTINUE;\n     +}\n     +\n    -+static void check_each(struct oidtree *ot, char *hex, char *expected)\n    ++static void check_each(struct oidtree *ot, char *query, ...)\n     +{\n     +\tstruct object_id oid;\n    ++\tstruct expected_hex_iter hex_iter = { .expected_hexes = STRVEC_INIT,\n    ++\t\t\t\t\t      .query = query };\n    ++\tconst char *arg;\n    ++\tva_list hex_args;\n     +\n    -+\tif (!check_int(get_oid_arbitrary_hex(hex, &oid), ==, 0))\n    ++\tva_start(hex_args, query);\n    ++\twhile ((arg = va_arg(hex_args, const char *)))\n    ++\t\tstrvec_push(&hex_iter.expected_hexes, arg);\n    ++\tva_end(hex_args);\n    ++\n    ++\tif (!check_int(get_oid_arbitrary_hex(query, &oid), ==, 0))\n     +\t\treturn;\n    -+\toidtree_each(ot, &oid, 40, check_each_cb, expected);\n    ++\toidtree_each(ot, &oid, strlen(query), check_each_cb, &hex_iter);\n    ++\n    ++\tif (!check_int(hex_iter.i, ==, hex_iter.expected_hexes.nr))\n    ++\t\ttest_msg(\"error: could not find some 'object_id's for query ('%s')\", query);\n    ++\tstrvec_clear(&hex_iter.expected_hexes);\n     +}\n     +\n     +static void setup(void (*f)(struct oidtree *ot))\n    @@ t/unit-tests/t-oidtree.c (new)\n     +\n     +static void t_each(struct oidtree *ot)\n     +{\n    -+\tFILL_TREE(ot, \"f\", \"9\", \"8\", \"123\", \"321\", \"a\", \"b\", \"c\", \"d\", \"e\");\n    -+\tcheck_each(ot, \"12300\", \"123\");\n    -+\tcheck_each(ot, \"3211\", \"\"); /* should not reach callback */\n    -+\tcheck_each(ot, \"3210\", \"321\");\n    -+\tcheck_each(ot, \"32100\", \"321\");\n    ++\tFILL_TREE(ot, \"f\", \"9\", \"8\", \"123\", \"321\", \"320\", \"a\", \"b\", \"c\", \"d\", \"e\");\n    ++\tcheck_each(ot, \"12300\", \"123\", NULL);\n    ++\tcheck_each(ot, \"3211\", NULL); /* should not reach callback */\n    ++\tcheck_each(ot, \"3210\", \"321\", NULL);\n    ++\tcheck_each(ot, \"32100\", \"321\", NULL);\n    ++\tcheck_each(ot, \"32\", \"320\", \"321\", NULL);\n     +}\n     +\n     +int cmd_main(int argc UNUSED, const char **argv UNUSED)\n\n Makefile                 |   8 ++-\n t/helper/test-oidtree.c  |  54 -----------------\n t/helper/test-tool.c     |   1 -\n t/helper/test-tool.h     |   1 -\n t/t0069-oidtree.sh       |  50 ----------------\n t/unit-tests/lib-oid.c   |  52 +++++++++++++++++\n t/unit-tests/lib-oid.h   |  17 ++++++\n t/unit-tests/t-oidtree.c | 121 +++++++++++++++++++++++++++++++++++++++\n 8 files changed, 196 insertions(+), 108 deletions(-)\n delete mode 100644 t/helper/test-oidtree.c\n delete mode 100755 t/t0069-oidtree.sh\n create mode 100644 t/unit-tests/lib-oid.c\n create mode 100644 t/unit-tests/lib-oid.h\n create mode 100644 t/unit-tests/t-oidtree.c\n\ndiff --git a/Makefile b/Makefile\nindex 2f5f16847a..03751e0fc0 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -811,7 +811,6 @@ TEST_BUILTINS_OBJS += test-mergesort.o\n TEST_BUILTINS_OBJS += test-mktemp.o\n TEST_BUILTINS_OBJS += test-oid-array.o\n TEST_BUILTINS_OBJS += test-oidmap.o\n-TEST_BUILTINS_OBJS += test-oidtree.o\n TEST_BUILTINS_OBJS += test-online-cpus.o\n TEST_BUILTINS_OBJS += test-pack-mtimes.o\n TEST_BUILTINS_OBJS += test-parse-options.o\n@@ -1335,6 +1334,7 @@ THIRD_PARTY_SOURCES += sha1dc/%\n \n UNIT_TEST_PROGRAMS += t-ctype\n UNIT_TEST_PROGRAMS += t-mem-pool\n+UNIT_TEST_PROGRAMS += t-oidtree\n UNIT_TEST_PROGRAMS += t-prio-queue\n UNIT_TEST_PROGRAMS += t-strbuf\n UNIT_TEST_PROGRAMS += t-strcmp-offset\n@@ -1343,6 +1343,7 @@ UNIT_TEST_PROGRAMS += t-trailer\n UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o\n+UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-oid.o\n \n # xdiff and reftable libs may in turn depend on what is in libgit.a\n GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)\n@@ -3883,7 +3884,10 @@ $(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS\n \t\t-Wl,--allow-multiple-definition \\\n \t\t$(filter %.o,$^) $(filter %.a,$^) $(LIBS) $(LIB_FUZZING_ENGINE)\n \n-$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS\n+$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o \\\n+\t$(UNIT_TEST_DIR)/test-lib.o \\\n+\t$(UNIT_TEST_DIR)/lib-oid.o \\\n+\t$(GITLIBS) GIT-LDFLAGS\n \t$(call mkdir_p_parent_template)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \\\n \t\t$(filter %.o,$^) $(filter %.a,$^) $(LIBS)\ndiff --git a/t/helper/test-oidtree.c b/t/helper/test-oidtree.c\ndeleted file mode 100644\nindex c7a1d4c642..0000000000\n--- a/t/helper/test-oidtree.c\n+++ /dev/null\n@@ -1,54 +0,0 @@\n-#include \"test-tool.h\"\n-#include \"hex.h\"\n-#include \"oidtree.h\"\n-#include \"setup.h\"\n-#include \"strbuf.h\"\n-\n-static enum cb_next print_oid(const struct object_id *oid, void *data UNUSED)\n-{\n-\tputs(oid_to_hex(oid));\n-\treturn CB_CONTINUE;\n-}\n-\n-int cmd__oidtree(int argc UNUSED, const char **argv UNUSED)\n-{\n-\tstruct oidtree ot;\n-\tstruct strbuf line = STRBUF_INIT;\n-\tint nongit_ok;\n-\tint algo = GIT_HASH_UNKNOWN;\n-\n-\toidtree_init(&ot);\n-\tsetup_git_directory_gently(&nongit_ok);\n-\n-\twhile (strbuf_getline(&line, stdin) != EOF) {\n-\t\tconst char *arg;\n-\t\tstruct object_id oid;\n-\n-\t\tif (skip_prefix(line.buf, \"insert \", &arg)) {\n-\t\t\tif (get_oid_hex_any(arg, &oid) == GIT_HASH_UNKNOWN)\n-\t\t\t\tdie(\"insert not a hexadecimal oid: %s\", arg);\n-\t\t\talgo = oid.algo;\n-\t\t\toidtree_insert(&ot, &oid);\n-\t\t} else if (skip_prefix(line.buf, \"contains \", &arg)) {\n-\t\t\tif (get_oid_hex(arg, &oid))\n-\t\t\t\tdie(\"contains not a hexadecimal oid: %s\", arg);\n-\t\t\tprintf(\"%d\\n\", oidtree_contains(&ot, &oid));\n-\t\t} else if (skip_prefix(line.buf, \"each \", &arg)) {\n-\t\t\tchar buf[GIT_MAX_HEXSZ + 1] = { '0' };\n-\t\t\tmemset(&oid, 0, sizeof(oid));\n-\t\t\tmemcpy(buf, arg, strlen(arg));\n-\t\t\tbuf[hash_algos[algo].hexsz] = '\\0';\n-\t\t\tget_oid_hex_any(buf, &oid);\n-\t\t\toid.algo = algo;\n-\t\t\toidtree_each(&ot, &oid, strlen(arg), print_oid, NULL);\n-\t\t} else if (!strcmp(line.buf, \"clear\")) {\n-\t\t\toidtree_clear(&ot);\n-\t\t} else {\n-\t\t\tdie(\"unknown command: %s\", line.buf);\n-\t\t}\n-\t}\n-\n-\tstrbuf_release(&line);\n-\n-\treturn 0;\n-}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 7ad7d07018..253324a06b 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -46,7 +46,6 @@ static struct test_cmd cmds[] = {\n \t{ \"mktemp\", cmd__mktemp },\n \t{ \"oid-array\", cmd__oid_array },\n \t{ \"oidmap\", cmd__oidmap },\n-\t{ \"oidtree\", cmd__oidtree },\n \t{ \"online-cpus\", cmd__online_cpus },\n \t{ \"pack-mtimes\", cmd__pack_mtimes },\n \t{ \"parse-options\", cmd__parse_options },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex d14b3072bd..460dd7d260 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -39,7 +39,6 @@ int cmd__match_trees(int argc, const char **argv);\n int cmd__mergesort(int argc, const char **argv);\n int cmd__mktemp(int argc, const char **argv);\n int cmd__oidmap(int argc, const char **argv);\n-int cmd__oidtree(int argc, const char **argv);\n int cmd__online_cpus(int argc, const char **argv);\n int cmd__pack_mtimes(int argc, const char **argv);\n int cmd__parse_options(int argc, const char **argv);\ndiff --git a/t/t0069-oidtree.sh b/t/t0069-oidtree.sh\ndeleted file mode 100755\nindex 889db50818..0000000000\n--- a/t/t0069-oidtree.sh\n+++ /dev/null\n@@ -1,50 +0,0 @@\n-#!/bin/sh\n-\n-test_description='basic tests for the oidtree implementation'\n-TEST_PASSES_SANITIZE_LEAK=true\n-. ./test-lib.sh\n-\n-maxhexsz=$(test_oid hexsz)\n-echoid () {\n-\tprefix=\"${1:+$1 }\"\n-\tshift\n-\twhile test $# -gt 0\n-\tdo\n-\t\tshortoid=\"$1\"\n-\t\tshift\n-\t\tdifference=$(($maxhexsz - ${#shortoid}))\n-\t\tprintf \"%s%s%0${difference}d\\\\n\" \"$prefix\" \"$shortoid\" \"0\"\n-\tdone\n-}\n-\n-test_expect_success 'oidtree insert and contains' '\n-\tcat >expect <<-\\EOF &&\n-\t\t0\n-\t\t0\n-\t\t0\n-\t\t1\n-\t\t1\n-\t\t0\n-\tEOF\n-\t{\n-\t\techoid insert 444 1 2 3 4 5 a b c d e &&\n-\t\techoid contains 44 441 440 444 4440 4444 &&\n-\t\techo clear\n-\t} | test-tool oidtree >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'oidtree each' '\n-\techoid \"\" 123 321 321 >expect &&\n-\t{\n-\t\techoid insert f 9 8 123 321 a b c d e &&\n-\t\techo each 12300 &&\n-\t\techo each 3211 &&\n-\t\techo each 3210 &&\n-\t\techo each 32100 &&\n-\t\techo clear\n-\t} | test-tool oidtree >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_done\ndiff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c\nnew file mode 100644\nindex 0000000000..37105f0a8f\n--- /dev/null\n+++ b/t/unit-tests/lib-oid.c\n@@ -0,0 +1,52 @@\n+#include \"test-lib.h\"\n+#include \"lib-oid.h\"\n+#include \"strbuf.h\"\n+#include \"hex.h\"\n+\n+static int init_hash_algo(void)\n+{\n+\tstatic int algo = -1;\n+\n+\tif (algo < 0) {\n+\t\tconst char *algo_name = getenv(\"GIT_TEST_DEFAULT_HASH\");\n+\t\talgo = algo_name ? hash_algo_by_name(algo_name) : GIT_HASH_SHA1;\n+\n+\t\tif (!check(algo != GIT_HASH_UNKNOWN))\n+\t\t\ttest_msg(\"BUG: invalid GIT_TEST_DEFAULT_HASH value ('%s')\",\n+\t\t\t\t algo_name);\n+\t}\n+\treturn algo;\n+}\n+\n+static int get_oid_arbitrary_hex_algop(const char *hex, struct object_id *oid,\n+\t\t\t\t       const struct git_hash_algo *algop)\n+{\n+\tint ret;\n+\tsize_t sz = strlen(hex);\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tif (!check(sz <= algop->hexsz)) {\n+\t\ttest_msg(\"BUG: hex string (%s) bigger than maximum allowed (%lu)\",\n+\t\t\t hex, (unsigned long)algop->hexsz);\n+\t\treturn -1;\n+\t}\n+\n+\tstrbuf_add(&buf, hex, sz);\n+\tstrbuf_addchars(&buf, '0', algop->hexsz - sz);\n+\n+\tret = get_oid_hex_algop(buf.buf, oid, algop);\n+\tif (!check_int(ret, ==, 0))\n+\t\ttest_msg(\"BUG: invalid hex input (%s) provided\", hex);\n+\n+\tstrbuf_release(&buf);\n+\treturn ret;\n+}\n+\n+int get_oid_arbitrary_hex(const char *hex, struct object_id *oid)\n+{\n+\tint hash_algo = init_hash_algo();\n+\n+\tif (!check_int(hash_algo, !=, GIT_HASH_UNKNOWN))\n+\t\treturn -1;\n+\treturn get_oid_arbitrary_hex_algop(hex, oid, &hash_algos[hash_algo]);\n+}\ndiff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h\nnew file mode 100644\nindex 0000000000..bfde639190\n--- /dev/null\n+++ b/t/unit-tests/lib-oid.h\n@@ -0,0 +1,17 @@\n+#ifndef LIB_OID_H\n+#define LIB_OID_H\n+\n+#include \"hash-ll.h\"\n+\n+/*\n+ * Convert arbitrary hex string to object_id.\n+ * For example, passing \"abc12\" will generate\n+ * \"abc1200000000000000000000000000000000000\" hex of length 40 for SHA-1 and\n+ * create object_id with that.\n+ * WARNING: passing a string of length more than the hexsz of respective hash\n+ * algo is not allowed. The hash algo is decided based on GIT_TEST_DEFAULT_HASH\n+ * environment variable.\n+ */\n+int get_oid_arbitrary_hex(const char *s, struct object_id *oid);\n+\n+#endif /* LIB_OID_H */\ndiff --git a/t/unit-tests/t-oidtree.c b/t/unit-tests/t-oidtree.c\nnew file mode 100644\nindex 0000000000..80cbab0394\n--- /dev/null\n+++ b/t/unit-tests/t-oidtree.c\n@@ -0,0 +1,121 @@\n+#include \"test-lib.h\"\n+#include \"lib-oid.h\"\n+#include \"oidtree.h\"\n+#include \"hash.h\"\n+#include \"hex.h\"\n+#include \"strvec.h\"\n+\n+#define FILL_TREE(tree, ...)                                       \\\n+\tdo {                                                       \\\n+\t\tconst char *hexes[] = { __VA_ARGS__ };             \\\n+\t\tif (fill_tree_loc(tree, hexes, ARRAY_SIZE(hexes))) \\\n+\t\t\treturn;                                    \\\n+\t} while (0)\n+\n+static int fill_tree_loc(struct oidtree *ot, const char *hexes[], size_t n)\n+{\n+\tfor (size_t i = 0; i < n; i++) {\n+\t\tstruct object_id oid;\n+\t\tif (!check_int(get_oid_arbitrary_hex(hexes[i], &oid), ==, 0))\n+\t\t\treturn -1;\n+\t\toidtree_insert(ot, &oid);\n+\t}\n+\treturn 0;\n+}\n+\n+static void check_contains(struct oidtree *ot, const char *hex, int expected)\n+{\n+\tstruct object_id oid;\n+\n+\tif (!check_int(get_oid_arbitrary_hex(hex, &oid), ==, 0))\n+\t\treturn;\n+\tif (!check_int(oidtree_contains(ot, &oid), ==, expected))\n+\t\ttest_msg(\"oid: %s\", oid_to_hex(&oid));\n+}\n+\n+struct expected_hex_iter {\n+\tsize_t i;\n+\tstruct strvec expected_hexes;\n+\tconst char *query;\n+};\n+\n+static enum cb_next check_each_cb(const struct object_id *oid, void *data)\n+{\n+\tstruct expected_hex_iter *hex_iter = data;\n+\tstruct object_id expected;\n+\n+\tif (!check_int(hex_iter->i, <, hex_iter->expected_hexes.nr)) {\n+\t\ttest_msg(\"error: extraneous callback for query: ('%s'), object_id: ('%s')\",\n+\t\t\t hex_iter->query, oid_to_hex(oid));\n+\t\treturn CB_BREAK;\n+\t}\n+\n+\tif (!check_int(get_oid_arbitrary_hex(hex_iter->expected_hexes.v[hex_iter->i],\n+\t\t\t\t\t     &expected), ==, 0))\n+\t\t; /* the data is bogus and cannot be used */\n+\telse if (!check(oideq(oid, &expected)))\n+\t\ttest_msg(\"expected: %s\\n       got: %s\\n     query: %s\",\n+\t\t\t oid_to_hex(&expected), oid_to_hex(oid), hex_iter->query);\n+\n+\thex_iter->i += 1;\n+\treturn CB_CONTINUE;\n+}\n+\n+static void check_each(struct oidtree *ot, char *query, ...)\n+{\n+\tstruct object_id oid;\n+\tstruct expected_hex_iter hex_iter = { .expected_hexes = STRVEC_INIT,\n+\t\t\t\t\t      .query = query };\n+\tconst char *arg;\n+\tva_list hex_args;\n+\n+\tva_start(hex_args, query);\n+\twhile ((arg = va_arg(hex_args, const char *)))\n+\t\tstrvec_push(&hex_iter.expected_hexes, arg);\n+\tva_end(hex_args);\n+\n+\tif (!check_int(get_oid_arbitrary_hex(query, &oid), ==, 0))\n+\t\treturn;\n+\toidtree_each(ot, &oid, strlen(query), check_each_cb, &hex_iter);\n+\n+\tif (!check_int(hex_iter.i, ==, hex_iter.expected_hexes.nr))\n+\t\ttest_msg(\"error: could not find some 'object_id's for query ('%s')\", query);\n+\tstrvec_clear(&hex_iter.expected_hexes);\n+}\n+\n+static void setup(void (*f)(struct oidtree *ot))\n+{\n+\tstruct oidtree ot;\n+\n+\toidtree_init(&ot);\n+\tf(&ot);\n+\toidtree_clear(&ot);\n+}\n+\n+static void t_contains(struct oidtree *ot)\n+{\n+\tFILL_TREE(ot, \"444\", \"1\", \"2\", \"3\", \"4\", \"5\", \"a\", \"b\", \"c\", \"d\", \"e\");\n+\tcheck_contains(ot, \"44\", 0);\n+\tcheck_contains(ot, \"441\", 0);\n+\tcheck_contains(ot, \"440\", 0);\n+\tcheck_contains(ot, \"444\", 1);\n+\tcheck_contains(ot, \"4440\", 1);\n+\tcheck_contains(ot, \"4444\", 0);\n+}\n+\n+static void t_each(struct oidtree *ot)\n+{\n+\tFILL_TREE(ot, \"f\", \"9\", \"8\", \"123\", \"321\", \"320\", \"a\", \"b\", \"c\", \"d\", \"e\");\n+\tcheck_each(ot, \"12300\", \"123\", NULL);\n+\tcheck_each(ot, \"3211\", NULL); /* should not reach callback */\n+\tcheck_each(ot, \"3210\", \"321\", NULL);\n+\tcheck_each(ot, \"32100\", \"321\", NULL);\n+\tcheck_each(ot, \"32\", \"320\", \"321\", NULL);\n+}\n+\n+int cmd_main(int argc UNUSED, const char **argv UNUSED)\n+{\n+\tTEST(setup(t_contains), \"oidtree insert and contains works\");\n+\tTEST(setup(t_each), \"oidtree each works\");\n+\treturn test_done();\n+}\n-- \n2.45.2\n\n"},{"id":"496781","messageId":"xmqqed944uq7.fsf@gitster.g","threadId":"61590","inReplyTo":"20240608165731.29467-1-shyamthakkar001@gmail.com","subject":"Re: [GSoC][PATCH v2] t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-10T16:40:16Z","receivedAt":"2024-06-10T16:40:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n\n> helper/test-oidtree.c along with t0069-oidtree.sh test the oidtree.h\n> library, which is a wrapper around crit-bit tree. Migrate them to\n> the unit testing framework for better debugging and runtime\n> performance. Along with the migration, add an extra check for\n> oidtree_each() test, which showcases how multiple expected matches can\n> be given to check_each() helper.\n> ...\n\nUse \"LAST_ARG_MUST_BE_NULL\" here, probably.\n> +static void check_each(struct oidtree *ot, char *query, ...)\n> +{\n> +\tstruct object_id oid;\n> +\tstruct expected_hex_iter hex_iter = { .expected_hexes = STRVEC_INIT,\n> ...\n> +static void t_each(struct oidtree *ot)\n> +{\n> +\tFILL_TREE(ot, \"f\", \"9\", \"8\", \"123\", \"321\", \"320\", \"a\", \"b\", \"c\", \"d\", \"e\");\n> +\tcheck_each(ot, \"12300\", \"123\", NULL);\n> +\tcheck_each(ot, \"3211\", NULL); /* should not reach callback */\n\nThis one truly checks that the callback is never called with this\nversion, which is way better than the previous one (or the\noriginal).\n\n> +\tcheck_each(ot, \"3210\", \"321\", NULL);\n> +\tcheck_each(ot, \"32100\", \"321\", NULL);\n> +\tcheck_each(ot, \"32\", \"320\", \"321\", NULL);\n> +}\n> +\n> +int cmd_main(int argc UNUSED, const char **argv UNUSED)\n> +{\n> +\tTEST(setup(t_contains), \"oidtree insert and contains works\");\n> +\tTEST(setup(t_each), \"oidtree each works\");\n> +\treturn test_done();\n> +}\n"},{"id":"496801","messageId":"72dncmhj2qt6ufh67gbj3ctnwnssnlc3w22x77chcigzxou36f@mnwnrwg4oo5r","threadId":"61590","inReplyTo":"xmqqed944uq7.fsf@gitster.g","subject":"Re: [GSoC][PATCH v2] t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-06-10T20:52:47Z","receivedAt":"2024-06-10T20:52:51Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Mon, 10 Jun 2024, Junio C Hamano <gitster@pobox.com> wrote:\n> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n> \n> > helper/test-oidtree.c along with t0069-oidtree.sh test the oidtree.h\n> > library, which is a wrapper around crit-bit tree. Migrate them to\n> > the unit testing framework for better debugging and runtime\n> > performance. Along with the migration, add an extra check for\n> > oidtree_each() test, which showcases how multiple expected matches can\n> > be given to check_each() helper.\n> > ...\n> \n> Use \"LAST_ARG_MUST_BE_NULL\" here, probably.\n> > +static void check_each(struct oidtree *ot, char *query, ...)\n\nI see that you already made this change in merge-fix/gt/unit-test-oidtree.\nThanks for that.\n\n"},{"id":"496803","messageId":"xmqqr0d4zevq.fsf@gitster.g","threadId":"61590","inReplyTo":"72dncmhj2qt6ufh67gbj3ctnwnssnlc3w22x77chcigzxou36f@mnwnrwg4oo5r","subject":"Re: [GSoC][PATCH v2] t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-10T21:06:49Z","receivedAt":"2024-06-10T21:06:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n\n> On Mon, 10 Jun 2024, Junio C Hamano <gitster@pobox.com> wrote:\n>> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n>> \n>> > helper/test-oidtree.c along with t0069-oidtree.sh test the oidtree.h\n>> > library, which is a wrapper around crit-bit tree. Migrate them to\n>> > the unit testing framework for better debugging and runtime\n>> > performance. Along with the migration, add an extra check for\n>> > oidtree_each() test, which showcases how multiple expected matches can\n>> > be given to check_each() helper.\n>> > ...\n>> \n>> Use \"LAST_ARG_MUST_BE_NULL\" here, probably.\n>> > +static void check_each(struct oidtree *ot, char *query, ...)\n>\n> I see that you already made this change in merge-fix/gt/unit-test-oidtree.\n> Thanks for that.\n\nThat is merely tentative.  LAST_ARG_MUST_BE_NULL must be on the base\ntopic, as it is not something that suddenly becomes required after\ngetting merged to the integration branch (unlike other changes in\nthe merge-fix which became necessary in the world order after Patrick's\nconst string fixes are merged).\n\nI do not know what other fixes are needed, and if there is nothing\nelse that needs to be done in gt/unit-test-oidtree topic, I can do\n\"git commit --amend\" before merging it to 'next' (unless I forget,\nthat is ;-)), but if you are rerolling, please do not forget to add\nthat (you do not need to do the constness changes, which will require\nyou to rebase on top of whatever contains Patrick's work).\n\nThanks.\n"},{"id":"496809","messageId":"7o6fuymnfn6b6buyw3yyctjd4dlwlrazspv3xgxvys6djjivxh@qbhyurorgbtt","threadId":"61590","inReplyTo":"xmqqr0d4zevq.fsf@gitster.g","subject":"Re: [GSoC][PATCH v2] t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-06-10T22:01:59Z","receivedAt":"2024-06-10T22:02:03Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Mon, 10 Jun 2024, Junio C Hamano <gitster@pobox.com> wrote:\n> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n> \n> > On Mon, 10 Jun 2024, Junio C Hamano <gitster@pobox.com> wrote:\n> >> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n> >> \n> >> > helper/test-oidtree.c along with t0069-oidtree.sh test the oidtree.h\n> >> > library, which is a wrapper around crit-bit tree. Migrate them to\n> >> > the unit testing framework for better debugging and runtime\n> >> > performance. Along with the migration, add an extra check for\n> >> > oidtree_each() test, which showcases how multiple expected matches can\n> >> > be given to check_each() helper.\n> >> > ...\n> >> \n> >> Use \"LAST_ARG_MUST_BE_NULL\" here, probably.\n> >> > +static void check_each(struct oidtree *ot, char *query, ...)\n> >\n> > I see that you already made this change in merge-fix/gt/unit-test-oidtree.\n> > Thanks for that.\n> \n> That is merely tentative.  LAST_ARG_MUST_BE_NULL must be on the base\n> topic, as it is not something that suddenly becomes required after\n> getting merged to the integration branch (unlike other changes in\n> the merge-fix which became necessary in the world order after Patrick's\n> const string fixes are merged).\n> \n> I do not know what other fixes are needed, and if there is nothing\n> else that needs to be done in gt/unit-test-oidtree topic, I can do\n> \"git commit --amend\" before merging it to 'next' (unless I forget,\n> that is ;-)), but if you are rerolling, please do not forget to add\n> that (you do not need to do the constness changes, which will require\n> you to rebase on top of whatever contains Patrick's work).\n\nYeah, I'll reroll as rebasing on 'ps/no-writable-strings' did produce some\nerrors but the change required was minimal, so I'll include it anyway:\n\ndiff --git a/t/unit-tests/t-oidtree.c b/t/unit-tests/t-oidtree.c\nindex cecefde899..a38754b066 100644\n--- a/t/unit-tests/t-oidtree.c\n+++ b/t/unit-tests/t-oidtree.c\n@@ -62,7 +62,7 @@ static enum cb_next check_each_cb(const struct object_id *oid, void *data)\n }\n\n LAST_ARG_MUST_BE_NULL\n-static void check_each(struct oidtree *ot, char *query, ...)\n+static void check_each(struct oidtree *ot, const char *query, ...)\n {\n        struct object_id oid;\n        struct expected_hex_iter hex_iter = { .expected_hexes = STRVEC_INIT,\n\nThanks.\n"},{"id":"496813","messageId":"xmqq8qzcz8pd.fsf@gitster.g","threadId":"61590","inReplyTo":"7o6fuymnfn6b6buyw3yyctjd4dlwlrazspv3xgxvys6djjivxh@qbhyurorgbtt","subject":"Re: [GSoC][PATCH v2] t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-10T23:20:14Z","receivedAt":"2024-06-10T23:20:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n\n> Yeah, I'll reroll as rebasing on 'ps/no-writable-strings' did produce some\n> errors but the change required was minimal, so I'll include it anyway:\n>\n> diff --git a/t/unit-tests/t-oidtree.c b/t/unit-tests/t-oidtree.c\n> index cecefde899..a38754b066 100644\n> --- a/t/unit-tests/t-oidtree.c\n> +++ b/t/unit-tests/t-oidtree.c\n> @@ -62,7 +62,7 @@ static enum cb_next check_each_cb(const struct object_id *oid, void *data)\n>  }\n>\n>  LAST_ARG_MUST_BE_NULL\n> -static void check_each(struct oidtree *ot, char *query, ...)\n> +static void check_each(struct oidtree *ot, const char *query, ...)\n>  {\n>         struct object_id oid;\n>         struct expected_hex_iter hex_iter = { .expected_hexes = STRVEC_INIT,\n\nI somehow suspect that you do not even need to depend on the\nPatrick's series---tightening the constness in the function\nsignature by itself is a good thing as you are not writing into\n\"query\" anyway, even without his topic.\n\nThanks.\n"},{"id":"496816","messageId":"ssl4pyng2id3hcp2ssvi4artjxnsdcm7h4mnocidasxggnztqe@c62anllwrded","threadId":"61590","inReplyTo":"xmqq8qzcz8pd.fsf@gitster.g","subject":"Re: [GSoC][PATCH v2] t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-06-10T23:36:17Z","receivedAt":"2024-06-10T23:36:21Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Mon, 10 Jun 2024, Junio C Hamano <gitster@pobox.com> wrote:\n> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n> \n> > Yeah, I'll reroll as rebasing on 'ps/no-writable-strings' did produce some\n> > errors but the change required was minimal, so I'll include it anyway:\n> >\n> > diff --git a/t/unit-tests/t-oidtree.c b/t/unit-tests/t-oidtree.c\n> > index cecefde899..a38754b066 100644\n> > --- a/t/unit-tests/t-oidtree.c\n> > +++ b/t/unit-tests/t-oidtree.c\n> > @@ -62,7 +62,7 @@ static enum cb_next check_each_cb(const struct object_id *oid, void *data)\n> >  }\n> >\n> >  LAST_ARG_MUST_BE_NULL\n> > -static void check_each(struct oidtree *ot, char *query, ...)\n> > +static void check_each(struct oidtree *ot, const char *query, ...)\n> >  {\n> >         struct object_id oid;\n> >         struct expected_hex_iter hex_iter = { .expected_hexes = STRVEC_INIT,\n> \n> I somehow suspect that you do not even need to depend on the\n> Patrick's series---tightening the constness in the function\n> signature by itself is a good thing as you are not writing into\n> \"query\" anyway, even without his topic.\n\nI'll clarify \"I'll reroll as rebasing...\" -> \"I'll reroll, as rebasing...\" \n\nYeah, I meant as not depending on 'ps/no-writable-strings' but only\nincluding the diff above. But since you already did that, I'll refrain from\nsending another version unless some other changes are required. :)\n\nThanks.\n"}]}