{"thread":{"id":"61721","subject":"[GSoC][PATCH] t: port helper/test-oid-array.c to unit-tests/t-oid-array.c","startedAt":"2024-07-03T03:48:30Z","lastAt":"2024-09-04T15:01:39Z","messageCount":12,"participants":["Ghanshyam Thakkar","Phillip Wood","Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"498012","messageId":"20240703034638.8019-2-shyamthakkar001@gmail.com","threadId":"61721","inReplyTo":null,"subject":"[GSoC][PATCH] t: port helper/test-oid-array.c to unit-tests/t-oid-array.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-07-03T03:46:33Z","receivedAt":"2024-07-03T03:48:30Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"helper/test-oid-array.c along with t0064-oid-array.sh test the\noid-array.h library, which provides storage and processing\nefficiency over large lists of object identifiers.\n\nMigrate them to the unit testing framework for better runtime\nperformance and efficiency. Also 'the_hash_algo' is used internally in\noid_array_lookup(), but we do not initialize a repository directory,\ntherefore initialize the_hash_algo manually. And\ninit_hash_algo():lib-oid.c can aid in this process, so make it public.\n\nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nSigned-off-by: Ghanshyam Thakkar \n---\nNote: Once 'rs/unit-tests-test-run' is merged to atleast next, I plan to\nreplace these internal function used in TEST_LOOKUP() with TEST_RUN().\n\n Makefile | 2 +-\n t/helper/test-oid-array.c | 49 -------------\n t/helper/test-tool.c | 1 -\n t/helper/test-tool.h | 1 -\n t/t0064-oid-array.sh | 122 --------------------------------\n t/unit-tests/lib-oid.c | 2 +-\n t/unit-tests/lib-oid.h | 6 ++\n t/unit-tests/t-oid-array.c | 138 +++++++++++++++++++++++++++++++++++++\n 8 files changed, 146 insertions(+), 175 deletions(-)\n delete mode 100644 t/helper/test-oid-array.c\n delete mode 100755 t/t0064-oid-array.sh\n create mode 100644 t/unit-tests/t-oid-array.c\n\ndiff --git a/Makefile b/Makefile\nindex 3eab701b10..26a521c027 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -808,7 +808,6 @@ TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o\n TEST_BUILTINS_OBJS += test-match-trees.o\n 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-online-cpus.o\n TEST_BUILTINS_OBJS += test-pack-mtimes.o\n@@ -1337,6 +1336,7 @@ UNIT_TEST_PROGRAMS += t-ctype\n UNIT_TEST_PROGRAMS += t-example-decorate\n UNIT_TEST_PROGRAMS += t-hash\n UNIT_TEST_PROGRAMS += t-mem-pool\n+UNIT_TEST_PROGRAMS += t-oid-array\n UNIT_TEST_PROGRAMS += t-oidtree\n UNIT_TEST_PROGRAMS += t-prio-queue\n UNIT_TEST_PROGRAMS += t-reftable-basics\ndiff --git a/t/helper/test-oid-array.c b/t/helper/test-oid-array.c\ndeleted file mode 100644\nindex 076b849cbf..0000000000\n--- a/t/helper/test-oid-array.c\n+++ /dev/null\n@@ -1,49 +0,0 @@\n-#define USE_THE_REPOSITORY_VARIABLE\n-\n-#include \"test-tool.h\"\n-#include \"hex.h\"\n-#include \"oid-array.h\"\n-#include \"setup.h\"\n-#include \"strbuf.h\"\n-\n-static int print_oid(const struct object_id *oid, void *data UNUSED)\n-{\n-\tputs(oid_to_hex(oid));\n-\treturn 0;\n-}\n-\n-int cmd__oid_array(int argc UNUSED, const char **argv UNUSED)\n-{\n-\tstruct oid_array array = OID_ARRAY_INIT;\n-\tstruct strbuf line = STRBUF_INIT;\n-\tint nongit_ok;\n-\n-\tsetup_git_directory_gently(&nongit_ok);\n-\tif (nongit_ok)\n-\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\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, \"append \", &arg)) {\n-\t\t\tif (get_oid_hex(arg, &oid))\n-\t\t\t\tdie(\"not a hexadecimal oid: %s\", arg);\n-\t\t\toid_array_append(&array, &oid);\n-\t\t} else if (skip_prefix(line.buf, \"lookup \", &arg)) {\n-\t\t\tif (get_oid_hex(arg, &oid))\n-\t\t\t\tdie(\"not a hexadecimal oid: %s\", arg);\n-\t\t\tprintf(\"%d\\n\", oid_array_lookup(&array, &oid));\n-\t\t} else if (!strcmp(line.buf, \"clear\"))\n-\t\t\toid_array_clear(&array);\n-\t\telse if (!strcmp(line.buf, \"for_each_unique\"))\n-\t\t\toid_array_for_each_unique(&array, print_oid, NULL);\n-\t\telse\n-\t\t\tdie(\"unknown command: %s\", line.buf);\n-\t}\n-\n-\tstrbuf_release(&line);\n-\toid_array_clear(&array);\n-\n-\treturn 0;\n-}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 93436a82ae..fdbf755fb0 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -43,7 +43,6 @@ static struct test_cmd cmds[] = {\n \t{ \"match-trees\", cmd__match_trees },\n \t{ \"mergesort\", cmd__mergesort },\n \t{ \"mktemp\", cmd__mktemp },\n-\t{ \"oid-array\", cmd__oid_array },\n \t{ \"oidmap\", cmd__oidmap },\n \t{ \"online-cpus\", cmd__online_cpus },\n \t{ \"pack-mtimes\", cmd__pack_mtimes },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex d9033d14e1..0d9b9f6583 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -65,7 +65,6 @@ int cmd__scrap_cache_tree(int argc, const char **argv);\n int cmd__serve_v2(int argc, const char **argv);\n int cmd__sha1(int argc, const char **argv);\n int cmd__sha1_is_sha1dc(int argc, const char **argv);\n-int cmd__oid_array(int argc, const char **argv);\n int cmd__sha256(int argc, const char **argv);\n int cmd__sigchain(int argc, const char **argv);\n int cmd__simple_ipc(int argc, const char **argv);\ndiff --git a/t/t0064-oid-array.sh b/t/t0064-oid-array.sh\ndeleted file mode 100755\nindex de74b692d0..0000000000\n--- a/t/t0064-oid-array.sh\n+++ /dev/null\n@@ -1,122 +0,0 @@\n-#!/bin/sh\n-\n-test_description='basic tests for the oid array implementation'\n-\n-TEST_PASSES_SANITIZE_LEAK=true\n-. ./test-lib.sh\n-\n-echoid () {\n-\tprefix=\"${1:+$1 }\"\n-\tshift\n-\twhile test $# -gt 0\n-\tdo\n-\t\techo \"$prefix$ZERO_OID\" | sed -e \"s/00/$1/g\"\n-\t\tshift\n-\tdone\n-}\n-\n-test_expect_success 'without repository' '\n-\tcat >expect <<-EOF &&\n-\t4444444444444444444444444444444444444444\n-\t5555555555555555555555555555555555555555\n-\t8888888888888888888888888888888888888888\n-\taaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n-\tEOF\n-\tcat >input <<-EOF &&\n-\tappend 4444444444444444444444444444444444444444\n-\tappend 5555555555555555555555555555555555555555\n-\tappend 8888888888888888888888888888888888888888\n-\tappend aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n-\tfor_each_unique\n-\tEOF\n-\tnongit test-tool oid-array actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'ordered enumeration' '\n-\techoid \"\" 44 55 88 aa >expect &&\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techo for_each_unique\n-\t} | test-tool oid-array >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'ordered enumeration with duplicate suppression' '\n-\techoid \"\" 44 55 88 aa >expect &&\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techo for_each_unique\n-\t} | test-tool oid-array >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'lookup' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -eq 1\n-'\n-\n-test_expect_success 'lookup non-existing entry' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 33\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -lt 0\n-'\n-\n-test_expect_success 'lookup with duplicates' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -ge 3 &&\n-\ttest \"$n\" -le 5\n-'\n-\n-test_expect_success 'lookup non-existing entry with duplicates' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 66\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -lt 0\n-'\n-\n-test_expect_success 'lookup with almost duplicate values' '\n-\t# n-1 5s\n-\troot=$(echoid \"\" 55) &&\n-\troot=${root%5} &&\n-\t{\n-\t\tid1=\"${root}5\" &&\n-\t\tid2=\"${root}f\" &&\n-\t\techo \"append $id1\" &&\n-\t\techo \"append $id2\" &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -eq 0\n-'\n-\n-test_expect_success 'lookup with single duplicate value' '\n-\t{\n-\t\techoid append 55 55 &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -ge 0 &&\n-\ttest \"$n\" -le 1\n-'\n-\n-test_done\ndiff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c\nindex 37105f0a8f..8f0ccac532 100644\n--- a/t/unit-tests/lib-oid.c\n+++ b/t/unit-tests/lib-oid.c\n@@ -3,7 +3,7 @@\n #include \"strbuf.h\"\n #include \"hex.h\"\n \n-static int init_hash_algo(void)\n+int init_hash_algo(void)\n {\n \tstatic int algo = -1;\n \ndiff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h\nindex 8d2acca768..011a2d88de 100644\n--- a/t/unit-tests/lib-oid.h\n+++ b/t/unit-tests/lib-oid.h\n@@ -13,5 +13,11 @@\n * environment variable.\n */\n int get_oid_arbitrary_hex(const char *s, struct object_id *oid);\n+/*\n+ * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of\n+ * GIT_TEST_DEFAULT_HASH. The fallback value in case of absence of\n+ * GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1.\n+ */\n+int init_hash_algo(void);\n \n #endif /* LIB_OID_H */\ndiff --git a/t/unit-tests/t-oid-array.c b/t/unit-tests/t-oid-array.c\nnew file mode 100644\nindex 0000000000..0a506fab07\n--- /dev/null\n+++ b/t/unit-tests/t-oid-array.c\n@@ -0,0 +1,138 @@\n+#define USE_THE_REPOSITORY_VARIABLE\n+\n+#include \"test-lib.h\"\n+#include \"lib-oid.h\"\n+#include \"oid-array.h\"\n+#include \"hex.h\"\n+\n+static inline int test_min(int a, int b)\n+{\n+\treturn a <= b ? a : b;\n+}\n+\n+static int fill_array(struct oid_array *array, const char *hexes[], size_t n)\n+{\n+\tfor (size_t i = 0; i < n; i++) {\n+\t\tstruct object_id oid;\n+\n+\t\tif (get_oid_arbitrary_hex(hexes[i], &oid))\n+\t\t\treturn -1;\n+\t\toid_array_append(array, &oid);\n+\t}\n+\tif (!check_int(array->nr, ==, n))\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n+static int add_to_oid_array(const struct object_id *oid, void *data)\n+{\n+\tstruct oid_array *array = data;\n+\toid_array_append(array, oid);\n+\treturn 0;\n+}\n+\n+static void t_enumeration(const char *input_args[], size_t input_sz,\n+\t\t\t const char *result[], size_t result_sz)\n+{\n+\tstruct oid_array input = OID_ARRAY_INIT, expect = OID_ARRAY_INIT,\n+\t\t\t actual = OID_ARRAY_INIT;\n+\tsize_t i;\n+\n+\tif (fill_array(&input, input_args, input_sz))\n+\t\treturn;\n+\tif (fill_array(&expect, result, result_sz))\n+\t\treturn;\n+\n+\toid_array_for_each_unique(&input, add_to_oid_array, &actual);\n+\tcheck_int(actual.nr, ==, expect.nr);\n+\n+\tfor (i = 0; i < test_min(actual.nr, expect.nr); i++) {\n+\t\tif (!check(oideq(&actual.oid[i], &expect.oid[i])))\n+\t\t\ttest_msg(\"expected: %s\\n got: %s\\n index: %\" PRIuMAX,\n+\t\t\t\t oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]),\n+\t\t\t\t (uintmax_t)i);\n+\t}\n+\tcheck_int(i, ==, result_sz);\n+\n+\toid_array_clear(&actual);\n+\toid_array_clear(&input);\n+\toid_array_clear(&expect);\n+}\n+\n+#define TEST_ENUMERATION(input, result, desc) \\\n+\tTEST(t_enumeration(input, ARRAY_SIZE(input), result, ARRAY_SIZE(result)), \\\n+\t\t\t desc \" works\")\n+\n+static int t_lookup(struct object_id *oid_query, const char *query,\n+\t\t const char *hexes[], size_t n)\n+{\n+\tstruct oid_array array = OID_ARRAY_INIT;\n+\tint ret;\n+\n+\tif (get_oid_arbitrary_hex(query, oid_query))\n+\t\treturn INT_MIN;\n+\tif (fill_array(&array, hexes, n))\n+\t\treturn INT_MIN;\n+\tret = oid_array_lookup(&array, oid_query);\n+\n+\toid_array_clear(&array);\n+\treturn ret;\n+}\n+\n+#define TEST_LOOKUP(input_args, query, condition, desc) \\\n+\tdo { \\\n+\t\tint skip = test__run_begin(); \\\n+\t\tif (!skip) { \\\n+\t\t\tstruct object_id oid_query; \\\n+\t\t\tint ret = t_lookup(&oid_query, query, input_args, \\\n+\t\t\t\t\t ARRAY_SIZE(input_args)); \\\n+ \\\n+\t\t\tif (ret != INT_MIN && !check(condition)) \\\n+\t\t\t\ttest_msg(\"oid query for lookup: %s\", \\\n+\t\t\t\t\t oid_to_hex(&oid_query)); \\\n+\t\t} \\\n+\t\ttest__run_end(!skip, TEST_LOCATION(), desc \" works\"); \\\n+\t} while (0)\n+\n+static void setup(void)\n+{\n+\tint algo = init_hash_algo();\n+\t/* because the_hash_algo is used by oid_array_lookup() internally */\n+\tif (check_int(algo, !=, GIT_HASH_UNKNOWN))\n+\t\trepo_set_hash_algo(the_repository, algo);\n+}\n+\n+int cmd_main(int argc UNUSED, const char **argv UNUSED)\n+{\n+\tconst char *arr_input[] = { \"88\", \"44\", \"aa\", \"55\" };\n+\tconst char *arr_input_dup[] = { \"88\", \"44\", \"aa\", \"55\",\n+\t\t\t\t\t\"88\", \"44\", \"aa\", \"55\",\n+\t\t\t\t\t\"88\", \"44\", \"aa\", \"55\" };\n+\tconst char *res_sorted[] = { \"44\", \"55\", \"88\", \"aa\" };\n+\n+\tif (!TEST(setup(), \"setup\"))\n+\t\ttest_skip_all(\"hash algo initialization failed\");\n+\n+\tTEST_ENUMERATION(arr_input, res_sorted, \"ordered enumeration\");\n+\tTEST_ENUMERATION(arr_input_dup, res_sorted,\n+\t\t\t \"ordered enumeration with duplicate suppression\");\n+\n+\t/* ret is the return value of oid_array_lookup() */\n+\tTEST_LOOKUP(arr_input, \"55\", ret == 1, \"lookup\");\n+\tTEST_LOOKUP(arr_input, \"33\", ret < 0, \"lookup non-existent entry\");\n+\tTEST_LOOKUP(arr_input_dup, \"55\", ret >= 3 && ret <= 5,\n+\t\t \"lookup with duplicates\");\n+\tTEST_LOOKUP(arr_input_dup, \"66\", ret < 0,\n+\t\t \"lookup non-existent entry with duplicates\");\n+\n+\tTEST_LOOKUP(((const char *[]){\n+\t\t \"55\",\n+\t\t init_hash_algo() == GIT_HASH_SHA1 ?\n+\t\t\t\t\"5500000000000000000000000000000000000001\" :\n+\t\t\t\t\"5500000000000000000000000000000000000000000000000000000000000001\" }),\n+\t\t \"55\", ret == 0, \"lookup with almost duplicate values\");\n+\tTEST_LOOKUP(((const char *[]){ \"55\", \"55\" }), \"55\",\n+\t\t ret >= 0 && ret <= 1, \"lookup with single duplicate value\");\n+\n+\treturn test_done();\n+}\n-- \n2.45.2\n\n"},{"id":"498087","messageId":"db2f97b6-a06e-470f-b1f9-60a78a0a2a7f@gmail.com","threadId":"61721","inReplyTo":"20240703034638.8019-2-shyamthakkar001@gmail.com","subject":"Re: [GSoC][PATCH] t: port helper/test-oid-array.c to unit-tests/t-oid-array.c","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-04T16:33:48Z","receivedAt":"2024-07-04T16:33:51Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ghanshyam\n\nOverall this looks like a faithful conversion, I've left a few comments \nbelow.\n\nOn 03/07/2024 04:46, Ghanshyam Thakkar wrote:\n> helper/test-oid-array.c along with t0064-oid-array.sh test the\n> oid-array.h library, which provides storage and processing\n> efficiency over large lists of object identifiers.\n> \n> Migrate them to the unit testing framework for better runtime\n> performance and efficiency. Also 'the_hash_algo' is used internally in\n> oid_array_lookup(), but we do not initialize a repository directory,\n> therefore initialize the_hash_algo manually. And\n> init_hash_algo():lib-oid.c can aid in this process, so make it public.\n> \n> Mentored-by: Christian Couder \n> Mentored-by: Kaartic Sivaraam \n> Signed-off-by: Ghanshyam Thakkar \n> ---\n> Note: Once 'rs/unit-tests-test-run' is merged to atleast next, I plan to\n> replace these internal function used in TEST_LOOKUP() with TEST_RUN().\n\nNice idea\n\n> diff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c\n> index 37105f0a8f..8f0ccac532 100644\n> --- a/t/unit-tests/lib-oid.c\n> +++ b/t/unit-tests/lib-oid.c\n> @@ -3,7 +3,7 @@\n> #include \"strbuf.h\"\n> #include \"hex.h\"\n> \n> -static int init_hash_algo(void)\n> +int init_hash_algo(void)\n> {\n> \tstatic int algo = -1;\n> \n> diff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h\n> index 8d2acca768..011a2d88de 100644\n> --- a/t/unit-tests/lib-oid.h\n> +++ b/t/unit-tests/lib-oid.h\n> @@ -13,5 +13,11 @@\n> * environment variable.\n> */\n> int get_oid_arbitrary_hex(const char *s, struct object_id *oid);\n> +/*\n> + * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of\n> + * GIT_TEST_DEFAULT_HASH. The fallback value in case of absence of\n> + * GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1.\n> + */\n> +int init_hash_algo(void);\n> \n> #endif /* LIB_OID_H */\n> diff --git a/t/unit-tests/t-oid-array.c b/t/unit-tests/t-oid-array.c\n> new file mode 100644\n> index 0000000000..0a506fab07\n> --- /dev/null\n> +++ b/t/unit-tests/t-oid-array.c\n> @@ -0,0 +1,138 @@\n> +#define USE_THE_REPOSITORY_VARIABLE\n> +\n> +#include \"test-lib.h\"\n> +#include \"lib-oid.h\"\n> +#include \"oid-array.h\"\n> +#include \"hex.h\"\n> +\n> +static inline int test_min(int a, int b)\n> +{\n> +\treturn a <= b ? a : b;\n> +}\n> +\n> +static int fill_array(struct oid_array *array, const char *hexes[], size_t n)\n> +{\n> +\tfor (size_t i = 0; i < n; i++) {\n> +\t\tstruct object_id oid;\n> +\n> +\t\tif (get_oid_arbitrary_hex(hexes[i], &oid))\n> +\t\t\treturn -1;\n> +\t\toid_array_append(array, &oid);\n> +\t}\n> +\tif (!check_int(array->nr, ==, n))\n\nThis should probably use check_uint() as the arguments are unsigned \nintegers.\n\n> +\t\treturn -1;\n> +\treturn 0;\n> +}\n> +\n> +static int add_to_oid_array(const struct object_id *oid, void *data)\n> +{\n> +\tstruct oid_array *array = data;\n\nstyle: we leave a blank line after variable declarations at the start of \na block.\n\n> +\toid_array_append(array, oid);\n> +\treturn 0;\n> +}\n> +\n> +static void t_enumeration(const char *input_args[], size_t input_sz,\n> +\t\t\t const char *result[], size_t result_sz)\n\nstyle: we use \"const char **arg\" rather than \"const char *arg[]\"\n\n> +{\n> +\tstruct oid_array input = OID_ARRAY_INIT, expect = OID_ARRAY_INIT,\n> +\t\t\t actual = OID_ARRAY_INIT;\n> +\tsize_t i;\n> +\n> +\tif (fill_array(&input, input_args, input_sz))\n> +\t\treturn;\n> +\tif (fill_array(&expect, result, result_sz))\n> +\t\treturn;\n> +\n> +\toid_array_for_each_unique(&input, add_to_oid_array, &actual);\n> +\tcheck_int(actual.nr, ==, expect.nr);\n> +\n> +\tfor (i = 0; i < test_min(actual.nr, expect.nr); i++) {\n> +\t\tif (!check(oideq(&actual.oid[i], &expect.oid[i])))\n> +\t\t\ttest_msg(\"expected: %s\\n got: %s\\n index: %\" PRIuMAX,\n> +\t\t\t\t oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]),\n> +\t\t\t\t (uintmax_t)i);\n> +\t}\n> +\tcheck_int(i, ==, result_sz);\n> +\n> +\toid_array_clear(&actual);\n> +\toid_array_clear(&input);\n> +\toid_array_clear(&expect);\n> +}\n> +\n> +#define TEST_ENUMERATION(input, result, desc) \\\n> +\tTEST(t_enumeration(input, ARRAY_SIZE(input), result, ARRAY_SIZE(result)), \\\n> +\t\t\t desc \" works\")\n\nThis macro and its helper function look good.\n\n> +static int t_lookup(struct object_id *oid_query, const char *query,\n> +\t\t const char *hexes[], size_t n)\n> +{\n> +\tstruct oid_array array = OID_ARRAY_INIT;\n> +\tint ret;\n> +\n> +\tif (get_oid_arbitrary_hex(query, oid_query))\n> +\t\treturn INT_MIN;\n> +\tif (fill_array(&array, hexes, n))\n> +\t\treturn INT_MIN;\n> +\tret = oid_array_lookup(&array, oid_query);\n> +\n> +\toid_array_clear(&array);\n> +\treturn ret;\n> +}\n> +\n> +#define TEST_LOOKUP(input_args, query, condition, desc) \\\n\nPassing in the condition is a bit unfortunate as it means that the \ncaller has to know which variable name to use. It might be nicer to have \na function instead that takes the upper and lower bounds of the expected \nresult and then does\n\n\tcheck_int(res, >=, expected_lower);\n\tcheck_int(res, <=, expected_upper);\n\nIt might be worth checking that array[res] matches the expected entry as \nwell.\n\n> +\tdo { \\\n> +\t\tint skip = test__run_begin(); \\\n> +\t\tif (!skip) { \\\n> +\t\t\tstruct object_id oid_query; \\\n> +\t\t\tint ret = t_lookup(&oid_query, query, input_args, \\\n> +\t\t\t\t\t ARRAY_SIZE(input_args)); \\\n> + \\\n> +\t\t\tif (ret != INT_MIN && !check(condition)) \\\n> +\t\t\t\ttest_msg(\"oid query for lookup: %s\", \\\n> +\t\t\t\t\t oid_to_hex(&oid_query)); \\\n> +\t\t} \\\n> +\t\ttest__run_end(!skip, TEST_LOCATION(), desc \" works\"); \\\n> +\t} while (0)\n> +\n> +static void setup(void)\n> +{\n> +\tint algo = init_hash_algo();\n> +\t/* because the_hash_algo is used by oid_array_lookup() internally */\n> +\tif (check_int(algo, !=, GIT_HASH_UNKNOWN))\n> +\t\trepo_set_hash_algo(the_repository, algo);\n> +}\n> +\n> +int cmd_main(int argc UNUSED, const char **argv UNUSED)\n> +{\n> +\tconst char *arr_input[] = { \"88\", \"44\", \"aa\", \"55\" };\n> +\tconst char *arr_input_dup[] = { \"88\", \"44\", \"aa\", \"55\",\n> +\t\t\t\t\t\"88\", \"44\", \"aa\", \"55\",\n> +\t\t\t\t\t\"88\", \"44\", \"aa\", \"55\" };\n> +\tconst char *res_sorted[] = { \"44\", \"55\", \"88\", \"aa\" };\n> +\n> +\tif (!TEST(setup(), \"setup\"))\n> +\t\ttest_skip_all(\"hash algo initialization failed\");\n> +\n> +\tTEST_ENUMERATION(arr_input, res_sorted, \"ordered enumeration\");\n> +\tTEST_ENUMERATION(arr_input_dup, res_sorted,\n> +\t\t\t \"ordered enumeration with duplicate suppression\");\n> +\n> +\t/* ret is the return value of oid_array_lookup() */\n> +\tTEST_LOOKUP(arr_input, \"55\", ret == 1, \"lookup\");\n> +\tTEST_LOOKUP(arr_input, \"33\", ret < 0, \"lookup non-existent entry\");\n> +\tTEST_LOOKUP(arr_input_dup, \"55\", ret >= 3 && ret <= 5,\n> +\t\t \"lookup with duplicates\");\n> +\tTEST_LOOKUP(arr_input_dup, \"66\", ret < 0,\n> +\t\t \"lookup non-existent entry with duplicates\");\n> +\n> +\tTEST_LOOKUP(((const char *[]){\n> +\t\t \"55\",\n> +\t\t init_hash_algo() == GIT_HASH_SHA1 ?\n> +\t\t\t\t\"5500000000000000000000000000000000000001\" :\n> +\t\t\t\t\"5500000000000000000000000000000000000000000000000000000000000001\" }),\n\n> +\t\t \"55\", ret == 0, \"lookup with almost duplicate values\");\n\nThis might be slightly more readable if we stored the oids in a separate \nvariable at the beginning of this function and then it would look \nsomething like\n\n\tTEST_LOOKUP(((const char *[]) {\"55\", nearly_55}), \"55\", 0, 0,\n\t\t \"lookup with almost duplicate values\");\n\nHaving said that it is kind of unfortunate that we have all the variable \ndefinitions at the start as it makes it harder to see what's going on in \neach test. We could avoid that by using TEST_RUN() and declaring the \nvariables in the test block. For example the first test would look like\n\n\tTEST_RUN(\"ordered enumeration\") {\n\t\tconst char *input[] = { \"88\", \"44\", \"aa\", \"55\" };\n\t\tconst char *expected[] = { \"44\", \"55\", \"88\", \"aa\" };\n\n\t\tTEST_ENUMERATION(input, expected)\n\t}\n\nwhere TEST_ENUMERATION is adjusted so it does not call TEST(). It's more \nverbose but it is clearer what the input and expected values actually are.\n\nBest Wishes\n\nPhillip\n\n> +\tTEST_LOOKUP(((const char *[]){ \"55\", \"55\" }), \"55\",\n> +\t\t ret >= 0 && ret <= 1, \"lookup with single duplicate value\");\n> +\n> +\treturn test_done();\n> +}\n"},{"id":"500000","messageId":"20240803132206.72166-1-shyamthakkar001@gmail.com","threadId":"61721","inReplyTo":"20240703034638.8019-2-shyamthakkar001@gmail.com","subject":"[GSoC][PATCH v2] t: port helper/test-oid-array.c to unit-tests/t-oid-array.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-08-03T13:21:52Z","receivedAt":"2024-08-03T13:22:56Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"helper/test-oid-array.c along with t0064-oid-array.sh test the\noid-array.h library, which provides storage and processing\nefficiency over large lists of object identifiers.\n\nMigrate them to the unit testing framework for better runtime\nperformance and efficiency. Also 'the_hash_algo' is used internally in\noid_array_lookup(), but we do not initialize a repository directory,\ntherefore initialize the_hash_algo manually. And\ninit_hash_algo():lib-oid.c can aid in this process, so make it public.\n\nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nHelped-by: Phillip Wood \nSigned-off-by: Ghanshyam Thakkar \n---\nChanges in v2:\n- removed the use of internal test__run_*() functions.\n- TEST_LOOKUP() is entirely changed where it now accepts a lower bound\n and an upper bound for checking return values of oid_array_lookup(),\n instead of passing the condition as a whole literal. (i.e.\n v1: TEST_LOOKUP(..., ret < 0, ...)\n v2: TEST_LOOKUP(..., INT_MIN, -1, ...)\n )\n- TEST_ENUMERATION() remains unchanged.\n\nI didn't include the range-diff because the TEST_LOOKUP() has changed\nquite a lot, so it would have to be reviewed from the beginning\nanyways and I also rebased it on top of latest master since the last\nversion was sent 2 months ago.\n\nThanks.\n\n Makefile | 2 +-\n t/helper/test-oid-array.c | 49 --------------\n t/helper/test-tool.c | 1 -\n t/helper/test-tool.h | 1 -\n t/t0064-oid-array.sh | 122 ----------------------------------\n t/unit-tests/lib-oid.c | 2 +-\n t/unit-tests/lib-oid.h | 6 ++\n t/unit-tests/t-oid-array.c | 132 +++++++++++++++++++++++++++++++++++++\n 8 files changed, 140 insertions(+), 175 deletions(-)\n delete mode 100644 t/helper/test-oid-array.c\n delete mode 100755 t/t0064-oid-array.sh\n create mode 100644 t/unit-tests/t-oid-array.c\n\ndiff --git a/Makefile b/Makefile\nindex d6479092a0..c6f11dc453 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -808,7 +808,6 @@ TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o\n TEST_BUILTINS_OBJS += test-match-trees.o\n 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-online-cpus.o\n TEST_BUILTINS_OBJS += test-pack-mtimes.o\n TEST_BUILTINS_OBJS += test-parse-options.o\n@@ -1336,6 +1335,7 @@ UNIT_TEST_PROGRAMS += t-ctype\n UNIT_TEST_PROGRAMS += t-example-decorate\n UNIT_TEST_PROGRAMS += t-hash\n UNIT_TEST_PROGRAMS += t-mem-pool\n+UNIT_TEST_PROGRAMS += t-oid-array\n UNIT_TEST_PROGRAMS += t-oidmap\n UNIT_TEST_PROGRAMS += t-oidtree\n UNIT_TEST_PROGRAMS += t-prio-queue\ndiff --git a/t/helper/test-oid-array.c b/t/helper/test-oid-array.c\ndeleted file mode 100644\nindex 076b849cbf..0000000000\n--- a/t/helper/test-oid-array.c\n+++ /dev/null\n@@ -1,49 +0,0 @@\n-#define USE_THE_REPOSITORY_VARIABLE\n-\n-#include \"test-tool.h\"\n-#include \"hex.h\"\n-#include \"oid-array.h\"\n-#include \"setup.h\"\n-#include \"strbuf.h\"\n-\n-static int print_oid(const struct object_id *oid, void *data UNUSED)\n-{\n-\tputs(oid_to_hex(oid));\n-\treturn 0;\n-}\n-\n-int cmd__oid_array(int argc UNUSED, const char **argv UNUSED)\n-{\n-\tstruct oid_array array = OID_ARRAY_INIT;\n-\tstruct strbuf line = STRBUF_INIT;\n-\tint nongit_ok;\n-\n-\tsetup_git_directory_gently(&nongit_ok);\n-\tif (nongit_ok)\n-\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\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, \"append \", &arg)) {\n-\t\t\tif (get_oid_hex(arg, &oid))\n-\t\t\t\tdie(\"not a hexadecimal oid: %s\", arg);\n-\t\t\toid_array_append(&array, &oid);\n-\t\t} else if (skip_prefix(line.buf, \"lookup \", &arg)) {\n-\t\t\tif (get_oid_hex(arg, &oid))\n-\t\t\t\tdie(\"not a hexadecimal oid: %s\", arg);\n-\t\t\tprintf(\"%d\\n\", oid_array_lookup(&array, &oid));\n-\t\t} else if (!strcmp(line.buf, \"clear\"))\n-\t\t\toid_array_clear(&array);\n-\t\telse if (!strcmp(line.buf, \"for_each_unique\"))\n-\t\t\toid_array_for_each_unique(&array, print_oid, NULL);\n-\t\telse\n-\t\t\tdie(\"unknown command: %s\", line.buf);\n-\t}\n-\n-\tstrbuf_release(&line);\n-\toid_array_clear(&array);\n-\n-\treturn 0;\n-}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex da3e69128a..353d2aaaa4 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -43,7 +43,6 @@ static struct test_cmd cmds[] = {\n \t{ \"match-trees\", cmd__match_trees },\n \t{ \"mergesort\", cmd__mergesort },\n \t{ \"mktemp\", cmd__mktemp },\n-\t{ \"oid-array\", cmd__oid_array },\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 642a34578c..d3d8aa28e0 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -64,7 +64,6 @@ int cmd__scrap_cache_tree(int argc, const char **argv);\n int cmd__serve_v2(int argc, const char **argv);\n int cmd__sha1(int argc, const char **argv);\n int cmd__sha1_is_sha1dc(int argc, const char **argv);\n-int cmd__oid_array(int argc, const char **argv);\n int cmd__sha256(int argc, const char **argv);\n int cmd__sigchain(int argc, const char **argv);\n int cmd__simple_ipc(int argc, const char **argv);\ndiff --git a/t/t0064-oid-array.sh b/t/t0064-oid-array.sh\ndeleted file mode 100755\nindex de74b692d0..0000000000\n--- a/t/t0064-oid-array.sh\n+++ /dev/null\n@@ -1,122 +0,0 @@\n-#!/bin/sh\n-\n-test_description='basic tests for the oid array implementation'\n-\n-TEST_PASSES_SANITIZE_LEAK=true\n-. ./test-lib.sh\n-\n-echoid () {\n-\tprefix=\"${1:+$1 }\"\n-\tshift\n-\twhile test $# -gt 0\n-\tdo\n-\t\techo \"$prefix$ZERO_OID\" | sed -e \"s/00/$1/g\"\n-\t\tshift\n-\tdone\n-}\n-\n-test_expect_success 'without repository' '\n-\tcat >expect <<-EOF &&\n-\t4444444444444444444444444444444444444444\n-\t5555555555555555555555555555555555555555\n-\t8888888888888888888888888888888888888888\n-\taaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n-\tEOF\n-\tcat >input <<-EOF &&\n-\tappend 4444444444444444444444444444444444444444\n-\tappend 5555555555555555555555555555555555555555\n-\tappend 8888888888888888888888888888888888888888\n-\tappend aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n-\tfor_each_unique\n-\tEOF\n-\tnongit test-tool oid-array actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'ordered enumeration' '\n-\techoid \"\" 44 55 88 aa >expect &&\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techo for_each_unique\n-\t} | test-tool oid-array >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'ordered enumeration with duplicate suppression' '\n-\techoid \"\" 44 55 88 aa >expect &&\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techo for_each_unique\n-\t} | test-tool oid-array >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'lookup' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -eq 1\n-'\n-\n-test_expect_success 'lookup non-existing entry' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 33\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -lt 0\n-'\n-\n-test_expect_success 'lookup with duplicates' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -ge 3 &&\n-\ttest \"$n\" -le 5\n-'\n-\n-test_expect_success 'lookup non-existing entry with duplicates' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 66\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -lt 0\n-'\n-\n-test_expect_success 'lookup with almost duplicate values' '\n-\t# n-1 5s\n-\troot=$(echoid \"\" 55) &&\n-\troot=${root%5} &&\n-\t{\n-\t\tid1=\"${root}5\" &&\n-\t\tid2=\"${root}f\" &&\n-\t\techo \"append $id1\" &&\n-\t\techo \"append $id2\" &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -eq 0\n-'\n-\n-test_expect_success 'lookup with single duplicate value' '\n-\t{\n-\t\techoid append 55 55 &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -ge 0 &&\n-\ttest \"$n\" -le 1\n-'\n-\n-test_done\ndiff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c\nindex 37105f0a8f..8f0ccac532 100644\n--- a/t/unit-tests/lib-oid.c\n+++ b/t/unit-tests/lib-oid.c\n@@ -3,7 +3,7 @@\n #include \"strbuf.h\"\n #include \"hex.h\"\n \n-static int init_hash_algo(void)\n+int init_hash_algo(void)\n {\n \tstatic int algo = -1;\n \ndiff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h\nindex 8d2acca768..011a2d88de 100644\n--- a/t/unit-tests/lib-oid.h\n+++ b/t/unit-tests/lib-oid.h\n@@ -13,5 +13,11 @@\n * environment variable.\n */\n int get_oid_arbitrary_hex(const char *s, struct object_id *oid);\n+/*\n+ * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of\n+ * GIT_TEST_DEFAULT_HASH. The fallback value in case of absence of\n+ * GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1.\n+ */\n+int init_hash_algo(void);\n \n #endif /* LIB_OID_H */\ndiff --git a/t/unit-tests/t-oid-array.c b/t/unit-tests/t-oid-array.c\nnew file mode 100644\nindex 0000000000..baa7c68d7b\n--- /dev/null\n+++ b/t/unit-tests/t-oid-array.c\n@@ -0,0 +1,132 @@\n+#define USE_THE_REPOSITORY_VARIABLE\n+\n+#include \"test-lib.h\"\n+#include \"lib-oid.h\"\n+#include \"oid-array.h\"\n+#include \"hex.h\"\n+\n+static inline size_t test_min(size_t a, size_t b)\n+{\n+\treturn a <= b ? a : b;\n+}\n+\n+static int fill_array(struct oid_array *array, const char *hexes[], size_t n)\n+{\n+\tfor (size_t i = 0; i < n; i++) {\n+\t\tstruct object_id oid;\n+\n+\t\tif (!check_int(get_oid_arbitrary_hex(hexes[i], &oid), ==, 0))\n+\t\t\treturn -1;\n+\t\toid_array_append(array, &oid);\n+\t}\n+\tif (!check_uint(array->nr, ==, n))\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n+static int add_to_oid_array(const struct object_id *oid, void *data)\n+{\n+\tstruct oid_array *array = data;\n+\n+\toid_array_append(array, oid);\n+\treturn 0;\n+}\n+\n+static void t_enumeration(const char **input_args, size_t input_sz,\n+\t\t\t const char **result, size_t result_sz)\n+{\n+\tstruct oid_array input = OID_ARRAY_INIT, expect = OID_ARRAY_INIT,\n+\t\t\t actual = OID_ARRAY_INIT;\n+\tsize_t i;\n+\n+\tif (fill_array(&input, input_args, input_sz))\n+\t\treturn;\n+\tif (fill_array(&expect, result, result_sz))\n+\t\treturn;\n+\n+\toid_array_for_each_unique(&input, add_to_oid_array, &actual);\n+\tcheck_uint(actual.nr, ==, expect.nr);\n+\n+\tfor (i = 0; i < test_min(actual.nr, expect.nr); i++) {\n+\t\tif (!check(oideq(&actual.oid[i], &expect.oid[i])))\n+\t\t\ttest_msg(\"expected: %s\\n got: %s\\n index: %\" PRIuMAX,\n+\t\t\t\t oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]),\n+\t\t\t\t (uintmax_t)i);\n+\t}\n+\tcheck_uint(i, ==, result_sz);\n+\n+\toid_array_clear(&actual);\n+\toid_array_clear(&input);\n+\toid_array_clear(&expect);\n+}\n+\n+#define TEST_ENUMERATION(input, result, desc) \\\n+\tTEST(t_enumeration(input, ARRAY_SIZE(input), result, ARRAY_SIZE(result)), \\\n+\t\t\t desc \" works\")\n+\n+static void t_lookup(const char **input_hexes, size_t n, const char *query_hex,\n+\t\t int lower_bound, int upper_bound)\n+{\n+\tstruct oid_array array = OID_ARRAY_INIT;\n+\tstruct object_id oid_query;\n+\tint ret;\n+\n+\tif (get_oid_arbitrary_hex(query_hex, &oid_query))\n+\t\treturn;\n+\tif (fill_array(&array, input_hexes, n))\n+\t\treturn;\n+\tret = oid_array_lookup(&array, &oid_query);\n+\n+\tif (!check_int(ret, <=, upper_bound) ||\n+\t !check_int(ret, >=, lower_bound))\n+\t\ttest_msg(\"oid query for lookup: %s\", oid_to_hex(&oid_query));\n+\n+\toid_array_clear(&array);\n+}\n+\n+#define TEST_LOOKUP(input_hexes, query, lower_bound, upper_bound, desc) \\\n+\tTEST(t_lookup(input_hexes, ARRAY_SIZE(input_hexes), query, \\\n+\t\t lower_bound, upper_bound), \\\n+\t desc \" works\")\n+\n+static void setup(void)\n+{\n+\tint algo = init_hash_algo();\n+\t/* because the_hash_algo is used by oid_array_lookup() internally */\n+\tif (check_int(algo, !=, GIT_HASH_UNKNOWN))\n+\t\trepo_set_hash_algo(the_repository, algo);\n+}\n+\n+int cmd_main(int argc UNUSED, const char **argv UNUSED)\n+{\n+\tconst char *arr_input[] = { \"88\", \"44\", \"aa\", \"55\" };\n+\tconst char *arr_input_dup[] = { \"88\", \"44\", \"aa\", \"55\",\n+\t\t\t\t\t\"88\", \"44\", \"aa\", \"55\",\n+\t\t\t\t\t\"88\", \"44\", \"aa\", \"55\" };\n+\tconst char *res_sorted[] = { \"44\", \"55\", \"88\", \"aa\" };\n+\tconst char *nearly_55;\n+\n+\tif (!TEST(setup(), \"setup\"))\n+\t\ttest_skip_all(\"hash algo initialization failed\");\n+\n+\tTEST_ENUMERATION(arr_input, res_sorted, \"ordered enumeration\");\n+\tTEST_ENUMERATION(arr_input_dup, res_sorted,\n+\t\t\t \"ordered enumeration with duplicate suppression\");\n+\n+\t/* ret is the return value of oid_array_lookup() */\n+\tTEST_LOOKUP(arr_input, \"55\", 1, 1, \"lookup\");\n+\tTEST_LOOKUP(arr_input, \"33\", INT_MIN, -1, \"lookup non-existent entry\");\n+\tTEST_LOOKUP(arr_input_dup, \"55\", 3, 5, \"lookup with duplicates\");\n+\tTEST_LOOKUP(arr_input_dup, \"66\", INT_MIN, -1,\n+\t\t \"lookup non-existent entry with duplicates\");\n+\n+\tnearly_55 = init_hash_algo() == GIT_HASH_SHA1 ?\n+\t\t\t\"5500000000000000000000000000000000000001\" :\n+\t\t\t\"5500000000000000000000000000000000000000000000000000000000000001\";\n+\tTEST_LOOKUP(((const char *[]){ \"55\", nearly_55 }), \"55\", 0, 0,\n+\t\t \"lookup with almost duplicate values\");\n+\tTEST_LOOKUP(((const char *[]){ \"55\", \"55\" }), \"55\", 0, 1,\n+\t\t \"lookup with single duplicate value\");\n+\n+\treturn test_done();\n+}\n-- \n2.46.0\n\n"},{"id":"500001","messageId":"D36BBC5EWOVX.1CERSXF01H5JI@gmail.com","threadId":"61721","inReplyTo":"db2f97b6-a06e-470f-b1f9-60a78a0a2a7f@gmail.com","subject":"Re: [GSoC][PATCH] t: port helper/test-oid-array.c to unit-tests/t-oid-array.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-08-03T13:31:45Z","receivedAt":"2024-08-03T13:31:52Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Hi Phillip,\n\nPhillip Wood wrote:\n> On 03/07/2024 04:46, Ghanshyam Thakkar wrote:\n> > Note: Once 'rs/unit-tests-test-run' is merged to atleast next, I plan to\n> > replace these internal function used in TEST_LOOKUP() with TEST_RUN().\n>\n> Nice idea\n\nI think the consensus on the 'TEST_RUN()' (which is now 'if_test()')\nwould take a bit more time and since the v2 removes the use of\ninternal functions anyways, I think we should not have to wait for\n'if_test()' anymore (other things like declaring input varibles inside\nthe 'if_test()' block can be addressed as an incremental patch once\n'if_test()' gets merged). And I've addressed all your other review\npoints in v2 as well.\n\nLink to v2: https://lore.kernel.org/git/20240803132206.72166-1-shyamthakkar001@gmail.com/\n\nThanks.\n\n"},{"id":"501289","messageId":"CAP8UFD3E2idN6mUYzEyh11Fzmj07q+BQuyVCtUkPP=cuxsUODw@mail.gmail.com","threadId":"61721","inReplyTo":"20240803132206.72166-1-shyamthakkar001@gmail.com","subject":"Re: [GSoC][PATCH v2] t: port helper/test-oid-array.c to unit-tests/t-oid-array.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-08-19T16:55:29Z","receivedAt":"2024-08-19T16:55:43Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Aug 3, 2024 at 3:22 PM Ghanshyam Thakkar\n wrote:\n>\n> helper/test-oid-array.c along with t0064-oid-array.sh test the\n> oid-array.h library, which provides storage and processing\n\nNit: I think \"oid-array.h\" is more an API than a library.\n\n> efficiency over large lists of object identifiers.\n>\n> Migrate them to the unit testing framework for better runtime\n> performance and efficiency. Also 'the_hash_algo' is used internally in\n\nIt doesn't seem to me that a variable called 'the_hash_algo' is used\ninternally in oid_array_lookup() anymore.\n\n> oid_array_lookup(), but we do not initialize a repository directory,\n> therefore initialize the_hash_algo manually. And\n> init_hash_algo():lib-oid.c can aid in this process, so make it public.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Kaartic Sivaraam \n> Helped-by: Phillip Wood \n> Signed-off-by: Ghanshyam Thakkar \n> ---\n> Changes in v2:\n> - removed the use of internal test__run_*() functions.\n> - TEST_LOOKUP() is entirely changed where it now accepts a lower bound\n> and an upper bound for checking return values of oid_array_lookup(),\n> instead of passing the condition as a whole literal. (i.e.\n> v1: TEST_LOOKUP(..., ret < 0, ...)\n> v2: TEST_LOOKUP(..., INT_MIN, -1, ...)\n> )\n\nNice improvements.\n\n> - TEST_ENUMERATION() remains unchanged.\n\n[...]\n\n> +/*\n> + * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of\n> + * GIT_TEST_DEFAULT_HASH. The fallback value in case of absence of\n> + * GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1.\n> + */\n\nIn this comment, it might be helpful to say that GIT_TEST_DEFAULT_HASH\nis an environment variable (while GIT_HASH_* are #defined values).\nAlso while at it, it might be helpful to say that the function uses\ncheck(algo != GIT_HASH_UNKNOWN) before returning to verify that\nGIT_TEST_DEFAULT_HASH is either unset or properly set.\n\n> +int init_hash_algo(void);\n\n[...]\n\n> +static void t_enumeration(const char **input_args, size_t input_sz,\n> + const char **result, size_t result_sz)\n> +{\n> + struct oid_array input = OID_ARRAY_INIT, expect = OID_ARRAY_INIT,\n> + actual = OID_ARRAY_INIT;\n> + size_t i;\n> +\n> + if (fill_array(&input, input_args, input_sz))\n> + return;\n> + if (fill_array(&expect, result, result_sz))\n> + return;\n\nIt would have been nice if the arguments were called 'expect_args' and\n'expect_sz' in the same way as for 'input'. Is there a reason why we\ncouldn't just use 'expect' (or maybe 'expected') everywhere instead of\n'result'?\n\nAlso after the above 'input.nr' is equal to 'input_sz' and 'expect.nr'\nis equal to 'result_sz' otherwise we would have already returned fron\nthe current function.\n\n> + oid_array_for_each_unique(&input, add_to_oid_array, &actual);\n> + check_uint(actual.nr, ==, expect.nr);\n\nI think it might be better to return if this check fails. Otherwise it\nmeans that we likely messed something up in the 'input_args' or\n'result' arguments we passed to the function, and then...\n\n> + for (i = 0; i < test_min(actual.nr, expect.nr); i++) {\n> + if (!check(oideq(&actual.oid[i], &expect.oid[i])))\n\n...we might not compare here the input oid with the corresponding\nresult oid we intended to compare it to. This might result in a lot of\nnot very relevant output.\n\nReturning if check_uint(actual.nr, ==, expect.nr) fails would avoid\nsuch output and also enable us to just use 'actual.nr' instead of\n'test_min(actual.nr, expect.nr)' in the 'for' loop above.\n\n> + test_msg(\"expected: %s\\n got: %s\\n index: %\" PRIuMAX,\n> + oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]),\n> + (uintmax_t)i);\n> + }\n> + check_uint(i, ==, result_sz);\n\nAs we saw above that 'expect.nr' is equal to 'result_sz', this check\ncan fail only if 'actual.nr' is different from 'expect.nr' which we\nalready checked above. So I think this check is redundant and we might\nwant to get rid of it.\n\n> + oid_array_clear(&actual);\n> + oid_array_clear(&input);\n> + oid_array_clear(&expect);\n> +}\n\n[...]\n\n> +static void t_lookup(const char **input_hexes, size_t n, const char *query_hex,\n> + int lower_bound, int upper_bound)\n> +{\n> + struct oid_array array = OID_ARRAY_INIT;\n> + struct object_id oid_query;\n> + int ret;\n> +\n> + if (get_oid_arbitrary_hex(query_hex, &oid_query))\n> + return;\n\nIn fill_array() above, we use check_int() to check the result of\nget_oid_arbitrary_hex() like this:\n\n if (!check_int(get_oid_arbitrary_hex(hexes[i], &oid), ==, 0))\n\nIt doesn't look consistent to not use check_int() to check the result\nof get_oid_arbitrary_hex() here. Or is there a specific reason to do\nit in one place but not in another?\n\n> + if (fill_array(&array, input_hexes, n))\n> + return;\n> + ret = oid_array_lookup(&array, &oid_query);\n> +\n> + if (!check_int(ret, <=, upper_bound) ||\n> + !check_int(ret, >=, lower_bound))\n> + test_msg(\"oid query for lookup: %s\", oid_to_hex(&oid_query));\n> +\n> + oid_array_clear(&array);\n> +}\n> +\n> +#define TEST_LOOKUP(input_hexes, query, lower_bound, upper_bound, desc) \\\n> + TEST(t_lookup(input_hexes, ARRAY_SIZE(input_hexes), query, \\\n> + lower_bound, upper_bound), \\\n> + desc \" works\")\n> +\n> +static void setup(void)\n> +{\n> + int algo = init_hash_algo();\n> + /* because the_hash_algo is used by oid_array_lookup() internally */\n\nI think this comment should be above the first line in this function\nas it also explains why we need to use init_hash_algo().\n\nAlso something like \"/* The hash algo is used by oid_array_lookup()\ninternally */\" seems better to me as there is no 'the_hash_algo'\nglobal variable used by oid_array_lookup().\n\n> + if (check_int(algo, !=, GIT_HASH_UNKNOWN))\n> + repo_set_hash_algo(the_repository, algo);\n> +}\n> +\n> +int cmd_main(int argc UNUSED, const char **argv UNUSED)\n> +{\n> + const char *arr_input[] = { \"88\", \"44\", \"aa\", \"55\" };\n> + const char *arr_input_dup[] = { \"88\", \"44\", \"aa\", \"55\",\n> + \"88\", \"44\", \"aa\", \"55\",\n> + \"88\", \"44\", \"aa\", \"55\" };\n> + const char *res_sorted[] = { \"44\", \"55\", \"88\", \"aa\" };\n> + const char *nearly_55;\n> +\n> + if (!TEST(setup(), \"setup\"))\n> + test_skip_all(\"hash algo initialization failed\");\n\nNice that we skip all the other tests with a helpful message if setup() fails.\n\n> + TEST_ENUMERATION(arr_input, res_sorted, \"ordered enumeration\");\n> + TEST_ENUMERATION(arr_input_dup, res_sorted,\n> + \"ordered enumeration with duplicate suppression\");\n> +\n> + /* ret is the return value of oid_array_lookup() */\n\nThis comment is not relevant anymore in this version.\n\n> + TEST_LOOKUP(arr_input, \"55\", 1, 1, \"lookup\");\n> + TEST_LOOKUP(arr_input, \"33\", INT_MIN, -1, \"lookup non-existent entry\");\n> + TEST_LOOKUP(arr_input_dup, \"55\", 3, 5, \"lookup with duplicates\");\n> + TEST_LOOKUP(arr_input_dup, \"66\", INT_MIN, -1,\n> + \"lookup non-existent entry with duplicates\");\n> +\n> + nearly_55 = init_hash_algo() == GIT_HASH_SHA1 ?\n> + \"5500000000000000000000000000000000000001\" :\n> + \"5500000000000000000000000000000000000000000000000000000000000001\";\n> + TEST_LOOKUP(((const char *[]){ \"55\", nearly_55 }), \"55\", 0, 0,\n> + \"lookup with almost duplicate values\");\n> + TEST_LOOKUP(((const char *[]){ \"55\", \"55\" }), \"55\", 0, 1,\n> + \"lookup with single duplicate value\");\n> +\n> + return test_done();\n> +}\n> --\n> 2.46.0\n>\n"},{"id":"501626","messageId":"D3OAWJKG9PX9.6MOABOQ77MOB@gmail.com","threadId":"61721","inReplyTo":"CAP8UFD3E2idN6mUYzEyh11Fzmj07q+BQuyVCtUkPP=cuxsUODw@mail.gmail.com","subject":"Re: [GSoC][PATCH v2] t: port helper/test-oid-array.c to unit-tests/t-oid-array.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-08-24T17:00:25Z","receivedAt":"2024-08-24T17:00:32Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Christian Couder wrote:\n> On Sat, Aug 3, 2024 at 3:22 PM Ghanshyam Thakkar\n> wrote:\n> > Migrate them to the unit testing framework for better runtime\n> > performance and efficiency. Also 'the_hash_algo' is used internally in\n>\n> It doesn't seem to me that a variable called 'the_hash_algo' is used\n> internally in oid_array_lookup() anymore.\n\nIt is. oid_array_lookup() uses oid_pos():hash-lookup.c, which uses\n'the_hash_algo'.\n\n> > +static void t_enumeration(const char **input_args, size_t input_sz,\n> > + const char **result, size_t result_sz)\n> > +{\n> > + struct oid_array input = OID_ARRAY_INIT, expect = OID_ARRAY_INIT,\n> > + actual = OID_ARRAY_INIT;\n> > + size_t i;\n> > +\n> > + if (fill_array(&input, input_args, input_sz))\n> > + return;\n> > + if (fill_array(&expect, result, result_sz))\n> > + return;\n>\n> It would have been nice if the arguments were called 'expect_args' and\n> 'expect_sz' in the same way as for 'input'. Is there a reason why we\n> couldn't just use 'expect' (or maybe 'expected') everywhere instead of\n> 'result'?\n\nI have changed them to 'expect' in v3.\n\n> Also after the above 'input.nr' is equal to 'input_sz' and 'expect.nr'\n> is equal to 'result_sz' otherwise we would have already returned fron\n> the current function.\n>\n> > + oid_array_for_each_unique(&input, add_to_oid_array, &actual);\n> > + check_uint(actual.nr, ==, expect.nr);\n>\n> I think it might be better to return if this check fails. Otherwise it\n> means that we likely messed something up in the 'input_args' or\n> 'result' arguments we passed to the function, and then...\n>\n> > + for (i = 0; i < test_min(actual.nr, expect.nr); i++) {\n> > + if (!check(oideq(&actual.oid[i], &expect.oid[i])))\n>\n> ...we might not compare here the input oid with the corresponding\n> result oid we intended to compare it to. This might result in a lot of\n> not very relevant output.\n>\n> Returning if check_uint(actual.nr, ==, expect.nr) fails would avoid\n> such output and also enable us to just use 'actual.nr' instead of\n> 'test_min(actual.nr, expect.nr)' in the 'for' loop above.\n\nChanged this in v3.\n\n>\n> > + test_msg(\"expected: %s\\n got: %s\\n index: %\" PRIuMAX,\n> > + oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]),\n> > + (uintmax_t)i);\n> > + }\n> > + check_uint(i, ==, result_sz);\n>\n> As we saw above that 'expect.nr' is equal to 'result_sz', this check\n> can fail only if 'actual.nr' is different from 'expect.nr' which we\n> already checked above. So I think this check is redundant and we might\n> want to get rid of it.\n\nRemoved in v3.\n\n>\n> In fill_array() above, we use check_int() to check the result of\n> get_oid_arbitrary_hex() like this:\n>\n> if (!check_int(get_oid_arbitrary_hex(hexes[i], &oid), ==, 0))\n>\n> It doesn't look consistent to not use check_int() to check the result\n> of get_oid_arbitrary_hex() here. Or is there a specific reason to do\n> it in one place but not in another?\n\nNot in particular. Added check_int() in v3.\n\nThanks for the review.\n"},{"id":"501627","messageId":"20240824170223.36080-1-shyamthakkar001@gmail.com","threadId":"61721","inReplyTo":"20240803132206.72166-1-shyamthakkar001@gmail.com","subject":"[GSoC][PATCH v3] t: port helper/test-oid-array.c to unit-tests/t-oid-array.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-08-24T17:02:13Z","receivedAt":"2024-08-24T17:02:59Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"helper/test-oid-array.c along with t0064-oid-array.sh test the\noid-array.h API, which provides storage and processing\nefficiency over large lists of object identifiers.\n\nMigrate them to the unit testing framework for better runtime\nperformance and efficiency. Also 'the_hash_algo' is used internally in\noid_array_lookup(), but we do not initialize a repository directory,\ntherefore initialize the_hash_algo manually. And\ninit_hash_algo():lib-oid.c can aid in this process, so make it public.\n\nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nHelped-by: Phillip Wood \nSigned-off-by: Ghanshyam Thakkar \n---\nChanges in v3:\n - changed commmit message and comments for more accurate description\n - removed test_min() and return early when actual.nr and expect.nr\n don't match\n - rename result to expect for more accurate description\n - removed a redundant check in t_enumeration()\n - add check_int() around one of calls of get_oid_arbitrary_hex()\n - rebased to latest master\n\n Makefile | 2 +-\n t/helper/test-oid-array.c | 49 ---------------\n t/helper/test-tool.c | 1 -\n t/helper/test-tool.h | 1 -\n t/t0064-oid-array.sh | 122 -----------------------------------\n t/unit-tests/lib-oid.c | 2 +-\n t/unit-tests/lib-oid.h | 8 +++\n t/unit-tests/t-oid-array.c | 126 +++++++++++++++++++++++++++++++++++++\n 8 files changed, 136 insertions(+), 175 deletions(-)\n delete mode 100644 t/helper/test-oid-array.c\n delete mode 100755 t/t0064-oid-array.sh\n create mode 100644 t/unit-tests/t-oid-array.c\n\ndiff --git a/Makefile b/Makefile\nindex e298c8b55e..8813753d99 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -808,7 +808,6 @@ TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o\n TEST_BUILTINS_OBJS += test-match-trees.o\n 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-online-cpus.o\n TEST_BUILTINS_OBJS += test-pack-mtimes.o\n TEST_BUILTINS_OBJS += test-parse-options.o\n@@ -1337,6 +1336,7 @@ UNIT_TEST_PROGRAMS += t-example-decorate\n UNIT_TEST_PROGRAMS += t-hash\n UNIT_TEST_PROGRAMS += t-hashmap\n UNIT_TEST_PROGRAMS += t-mem-pool\n+UNIT_TEST_PROGRAMS += t-oid-array\n UNIT_TEST_PROGRAMS += t-oidmap\n UNIT_TEST_PROGRAMS += t-oidtree\n UNIT_TEST_PROGRAMS += t-prio-queue\ndiff --git a/t/helper/test-oid-array.c b/t/helper/test-oid-array.c\ndeleted file mode 100644\nindex 076b849cbf..0000000000\n--- a/t/helper/test-oid-array.c\n+++ /dev/null\n@@ -1,49 +0,0 @@\n-#define USE_THE_REPOSITORY_VARIABLE\n-\n-#include \"test-tool.h\"\n-#include \"hex.h\"\n-#include \"oid-array.h\"\n-#include \"setup.h\"\n-#include \"strbuf.h\"\n-\n-static int print_oid(const struct object_id *oid, void *data UNUSED)\n-{\n-\tputs(oid_to_hex(oid));\n-\treturn 0;\n-}\n-\n-int cmd__oid_array(int argc UNUSED, const char **argv UNUSED)\n-{\n-\tstruct oid_array array = OID_ARRAY_INIT;\n-\tstruct strbuf line = STRBUF_INIT;\n-\tint nongit_ok;\n-\n-\tsetup_git_directory_gently(&nongit_ok);\n-\tif (nongit_ok)\n-\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\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, \"append \", &arg)) {\n-\t\t\tif (get_oid_hex(arg, &oid))\n-\t\t\t\tdie(\"not a hexadecimal oid: %s\", arg);\n-\t\t\toid_array_append(&array, &oid);\n-\t\t} else if (skip_prefix(line.buf, \"lookup \", &arg)) {\n-\t\t\tif (get_oid_hex(arg, &oid))\n-\t\t\t\tdie(\"not a hexadecimal oid: %s\", arg);\n-\t\t\tprintf(\"%d\\n\", oid_array_lookup(&array, &oid));\n-\t\t} else if (!strcmp(line.buf, \"clear\"))\n-\t\t\toid_array_clear(&array);\n-\t\telse if (!strcmp(line.buf, \"for_each_unique\"))\n-\t\t\toid_array_for_each_unique(&array, print_oid, NULL);\n-\t\telse\n-\t\t\tdie(\"unknown command: %s\", line.buf);\n-\t}\n-\n-\tstrbuf_release(&line);\n-\toid_array_clear(&array);\n-\n-\treturn 0;\n-}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex da3e69128a..353d2aaaa4 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -43,7 +43,6 @@ static struct test_cmd cmds[] = {\n \t{ \"match-trees\", cmd__match_trees },\n \t{ \"mergesort\", cmd__mergesort },\n \t{ \"mktemp\", cmd__mktemp },\n-\t{ \"oid-array\", cmd__oid_array },\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 642a34578c..d3d8aa28e0 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -64,7 +64,6 @@ int cmd__scrap_cache_tree(int argc, const char **argv);\n int cmd__serve_v2(int argc, const char **argv);\n int cmd__sha1(int argc, const char **argv);\n int cmd__sha1_is_sha1dc(int argc, const char **argv);\n-int cmd__oid_array(int argc, const char **argv);\n int cmd__sha256(int argc, const char **argv);\n int cmd__sigchain(int argc, const char **argv);\n int cmd__simple_ipc(int argc, const char **argv);\ndiff --git a/t/t0064-oid-array.sh b/t/t0064-oid-array.sh\ndeleted file mode 100755\nindex de74b692d0..0000000000\n--- a/t/t0064-oid-array.sh\n+++ /dev/null\n@@ -1,122 +0,0 @@\n-#!/bin/sh\n-\n-test_description='basic tests for the oid array implementation'\n-\n-TEST_PASSES_SANITIZE_LEAK=true\n-. ./test-lib.sh\n-\n-echoid () {\n-\tprefix=\"${1:+$1 }\"\n-\tshift\n-\twhile test $# -gt 0\n-\tdo\n-\t\techo \"$prefix$ZERO_OID\" | sed -e \"s/00/$1/g\"\n-\t\tshift\n-\tdone\n-}\n-\n-test_expect_success 'without repository' '\n-\tcat >expect <<-EOF &&\n-\t4444444444444444444444444444444444444444\n-\t5555555555555555555555555555555555555555\n-\t8888888888888888888888888888888888888888\n-\taaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n-\tEOF\n-\tcat >input <<-EOF &&\n-\tappend 4444444444444444444444444444444444444444\n-\tappend 5555555555555555555555555555555555555555\n-\tappend 8888888888888888888888888888888888888888\n-\tappend aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n-\tfor_each_unique\n-\tEOF\n-\tnongit test-tool oid-array actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'ordered enumeration' '\n-\techoid \"\" 44 55 88 aa >expect &&\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techo for_each_unique\n-\t} | test-tool oid-array >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'ordered enumeration with duplicate suppression' '\n-\techoid \"\" 44 55 88 aa >expect &&\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techo for_each_unique\n-\t} | test-tool oid-array >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'lookup' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -eq 1\n-'\n-\n-test_expect_success 'lookup non-existing entry' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 33\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -lt 0\n-'\n-\n-test_expect_success 'lookup with duplicates' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -ge 3 &&\n-\ttest \"$n\" -le 5\n-'\n-\n-test_expect_success 'lookup non-existing entry with duplicates' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 66\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -lt 0\n-'\n-\n-test_expect_success 'lookup with almost duplicate values' '\n-\t# n-1 5s\n-\troot=$(echoid \"\" 55) &&\n-\troot=${root%5} &&\n-\t{\n-\t\tid1=\"${root}5\" &&\n-\t\tid2=\"${root}f\" &&\n-\t\techo \"append $id1\" &&\n-\t\techo \"append $id2\" &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -eq 0\n-'\n-\n-test_expect_success 'lookup with single duplicate value' '\n-\t{\n-\t\techoid append 55 55 &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -ge 0 &&\n-\ttest \"$n\" -le 1\n-'\n-\n-test_done\ndiff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c\nindex 37105f0a8f..8f0ccac532 100644\n--- a/t/unit-tests/lib-oid.c\n+++ b/t/unit-tests/lib-oid.c\n@@ -3,7 +3,7 @@\n #include \"strbuf.h\"\n #include \"hex.h\"\n \n-static int init_hash_algo(void)\n+int init_hash_algo(void)\n {\n \tstatic int algo = -1;\n \ndiff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h\nindex 8d2acca768..c949af082c 100644\n--- a/t/unit-tests/lib-oid.h\n+++ b/t/unit-tests/lib-oid.h\n@@ -13,5 +13,13 @@\n * environment variable.\n */\n int get_oid_arbitrary_hex(const char *s, struct object_id *oid);\n+/*\n+ * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of\n+ * GIT_TEST_DEFAULT_HASH environment variable. The fallback value in case\n+ * of absence of GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1. It also uses\n+ * check(algo != GIT_HASH_UNKNOWN) before returning to verify if the\n+ * GIT_TEST_DEFAULT_HASH's value is valid or not.\n+ */\n+int init_hash_algo(void);\n \n #endif /* LIB_OID_H */\ndiff --git a/t/unit-tests/t-oid-array.c b/t/unit-tests/t-oid-array.c\nnew file mode 100644\nindex 0000000000..99e3de9dc8\n--- /dev/null\n+++ b/t/unit-tests/t-oid-array.c\n@@ -0,0 +1,126 @@\n+#define USE_THE_REPOSITORY_VARIABLE\n+\n+#include \"test-lib.h\"\n+#include \"lib-oid.h\"\n+#include \"oid-array.h\"\n+#include \"hex.h\"\n+\n+static int fill_array(struct oid_array *array, const char *hexes[], size_t n)\n+{\n+\tfor (size_t i = 0; i < n; i++) {\n+\t\tstruct object_id oid;\n+\n+\t\tif (!check_int(get_oid_arbitrary_hex(hexes[i], &oid), ==, 0))\n+\t\t\treturn -1;\n+\t\toid_array_append(array, &oid);\n+\t}\n+\tif (!check_uint(array->nr, ==, n))\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n+static int add_to_oid_array(const struct object_id *oid, void *data)\n+{\n+\tstruct oid_array *array = data;\n+\n+\toid_array_append(array, oid);\n+\treturn 0;\n+}\n+\n+static void t_enumeration(const char **input_args, size_t input_sz,\n+\t\t\t const char **expect_args, size_t expect_sz)\n+{\n+\tstruct oid_array input = OID_ARRAY_INIT, expect = OID_ARRAY_INIT,\n+\t\t\t actual = OID_ARRAY_INIT;\n+\tsize_t i;\n+\n+\tif (fill_array(&input, input_args, input_sz))\n+\t\treturn;\n+\tif (fill_array(&expect, expect_args, expect_sz))\n+\t\treturn;\n+\n+\toid_array_for_each_unique(&input, add_to_oid_array, &actual);\n+\tif(!check_uint(actual.nr, ==, expect.nr))\n+\t\treturn;\n+\n+\tfor (i = 0; i < actual.nr; i++) {\n+\t\tif (!check(oideq(&actual.oid[i], &expect.oid[i])))\n+\t\t\ttest_msg(\"expected: %s\\n got: %s\\n index: %\" PRIuMAX,\n+\t\t\t\t oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]),\n+\t\t\t\t (uintmax_t)i);\n+\t}\n+\n+\toid_array_clear(&actual);\n+\toid_array_clear(&input);\n+\toid_array_clear(&expect);\n+}\n+\n+#define TEST_ENUMERATION(input, expect, desc) \\\n+\tTEST(t_enumeration(input, ARRAY_SIZE(input), expect, ARRAY_SIZE(expect)), \\\n+\t\t\t desc \" works\")\n+\n+static void t_lookup(const char **input_hexes, size_t n, const char *query_hex,\n+\t\t int lower_bound, int upper_bound)\n+{\n+\tstruct oid_array array = OID_ARRAY_INIT;\n+\tstruct object_id oid_query;\n+\tint ret;\n+\n+\tif (!check_int(get_oid_arbitrary_hex(query_hex, &oid_query), ==, 0))\n+\t\treturn;\n+\tif (fill_array(&array, input_hexes, n))\n+\t\treturn;\n+\tret = oid_array_lookup(&array, &oid_query);\n+\n+\tif (!check_int(ret, <=, upper_bound) ||\n+\t !check_int(ret, >=, lower_bound))\n+\t\ttest_msg(\"oid query for lookup: %s\", oid_to_hex(&oid_query));\n+\n+\toid_array_clear(&array);\n+}\n+\n+#define TEST_LOOKUP(input_hexes, query, lower_bound, upper_bound, desc) \\\n+\tTEST(t_lookup(input_hexes, ARRAY_SIZE(input_hexes), query, \\\n+\t\t lower_bound, upper_bound), \\\n+\t desc \" works\")\n+\n+static void setup(void)\n+{\n+\t/* The hash algo is used by oid_array_lookup() internally */\n+\tint algo = init_hash_algo();\n+\tif (check_int(algo, !=, GIT_HASH_UNKNOWN))\n+\t\trepo_set_hash_algo(the_repository, algo);\n+}\n+\n+int cmd_main(int argc UNUSED, const char **argv UNUSED)\n+{\n+\tconst char *arr_input[] = { \"88\", \"44\", \"aa\", \"55\" };\n+\tconst char *arr_input_dup[] = { \"88\", \"44\", \"aa\", \"55\",\n+\t\t\t\t\t\"88\", \"44\", \"aa\", \"55\",\n+\t\t\t\t\t\"88\", \"44\", \"aa\", \"55\" };\n+\tconst char *res_sorted[] = { \"44\", \"55\", \"88\", \"aa\" };\n+\tconst char *nearly_55;\n+\n+\tif (!TEST(setup(), \"setup\"))\n+\t\ttest_skip_all(\"hash algo initialization failed\");\n+\n+\tTEST_ENUMERATION(arr_input, res_sorted, \"ordered enumeration\");\n+\tTEST_ENUMERATION(arr_input_dup, res_sorted,\n+\t\t\t \"ordered enumeration with duplicate suppression\");\n+\n+\tTEST_LOOKUP(arr_input, \"55\", 1, 1, \"lookup\");\n+\tTEST_LOOKUP(arr_input, \"33\", INT_MIN, -1, \"lookup non-existent entry\");\n+\tTEST_LOOKUP(arr_input_dup, \"55\", 3, 5, \"lookup with duplicates\");\n+\tTEST_LOOKUP(arr_input_dup, \"66\", INT_MIN, -1,\n+\t\t \"lookup non-existent entry with duplicates\");\n+\n+\tnearly_55 = init_hash_algo() == GIT_HASH_SHA1 ?\n+\t\t\t\"5500000000000000000000000000000000000001\" :\n+\t\t\t\"5500000000000000000000000000000000000000000000000000000000000001\";\n+\tTEST_LOOKUP(((const char *[]){ \"55\", nearly_55 }), \"55\", 0, 0,\n+\t\t \"lookup with almost duplicate values\");\n+\tTEST_LOOKUP(((const char *[]){ \"55\", \"55\" }), \"55\", 0, 1,\n+\t\t \"lookup with single duplicate value\");\n+\n+\treturn test_done();\n+}\n\nRange-diff against v2:\n1: 27124bbb00 ! 1: 408a179736 t: port helper/test-oid-array.c to unit-tests/t-oid-array.c\n @@ Commit message\n t: port helper/test-oid-array.c to unit-tests/t-oid-array.c\n \n helper/test-oid-array.c along with t0064-oid-array.sh test the\n - oid-array.h library, which provides storage and processing\n + oid-array.h API, which provides storage and processing\n efficiency over large lists of object identifiers.\n \n Migrate them to the unit testing framework for better runtime\n @@ Makefile: TEST_BUILTINS_OBJS += test-lazy-init-name-hash.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 -@@ Makefile: UNIT_TEST_PROGRAMS += t-ctype\n - UNIT_TEST_PROGRAMS += t-example-decorate\n +@@ Makefile: UNIT_TEST_PROGRAMS += t-example-decorate\n UNIT_TEST_PROGRAMS += t-hash\n + UNIT_TEST_PROGRAMS += t-hashmap\n UNIT_TEST_PROGRAMS += t-mem-pool\n +UNIT_TEST_PROGRAMS += t-oid-array\n UNIT_TEST_PROGRAMS += t-oidmap\n @@ t/unit-tests/lib-oid.h\n int get_oid_arbitrary_hex(const char *s, struct object_id *oid);\n +/*\n + * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of\n -+ * GIT_TEST_DEFAULT_HASH. The fallback value in case of absence of\n -+ * GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1.\n ++ * GIT_TEST_DEFAULT_HASH environment variable. The fallback value in case\n ++ * of absence of GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1. It also uses\n ++ * check(algo != GIT_HASH_UNKNOWN) before returning to verify if the\n ++ * GIT_TEST_DEFAULT_HASH's value is valid or not.\n + */\n +int init_hash_algo(void);\n \n @@ t/unit-tests/t-oid-array.c (new)\n +#include \"oid-array.h\"\n +#include \"hex.h\"\n +\n -+static inline size_t test_min(size_t a, size_t b)\n -+{\n -+\treturn a <= b ? a : b;\n -+}\n -+\n +static int fill_array(struct oid_array *array, const char *hexes[], size_t n)\n +{\n +\tfor (size_t i = 0; i < n; i++) {\n @@ t/unit-tests/t-oid-array.c (new)\n +}\n +\n +static void t_enumeration(const char **input_args, size_t input_sz,\n -+\t\t\t const char **result, size_t result_sz)\n ++\t\t\t const char **expect_args, size_t expect_sz)\n +{\n +\tstruct oid_array input = OID_ARRAY_INIT, expect = OID_ARRAY_INIT,\n +\t\t\t actual = OID_ARRAY_INIT;\n @@ t/unit-tests/t-oid-array.c (new)\n +\n +\tif (fill_array(&input, input_args, input_sz))\n +\t\treturn;\n -+\tif (fill_array(&expect, result, result_sz))\n ++\tif (fill_array(&expect, expect_args, expect_sz))\n +\t\treturn;\n +\n +\toid_array_for_each_unique(&input, add_to_oid_array, &actual);\n -+\tcheck_uint(actual.nr, ==, expect.nr);\n ++\tif(!check_uint(actual.nr, ==, expect.nr))\n ++\t\treturn;\n +\n -+\tfor (i = 0; i < test_min(actual.nr, expect.nr); i++) {\n ++\tfor (i = 0; i < actual.nr; i++) {\n +\t\tif (!check(oideq(&actual.oid[i], &expect.oid[i])))\n +\t\t\ttest_msg(\"expected: %s\\n got: %s\\n index: %\" PRIuMAX,\n +\t\t\t\t oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]),\n +\t\t\t\t (uintmax_t)i);\n +\t}\n -+\tcheck_uint(i, ==, result_sz);\n +\n +\toid_array_clear(&actual);\n +\toid_array_clear(&input);\n +\toid_array_clear(&expect);\n +}\n +\n -+#define TEST_ENUMERATION(input, result, desc) \\\n -+\tTEST(t_enumeration(input, ARRAY_SIZE(input), result, ARRAY_SIZE(result)), \\\n ++#define TEST_ENUMERATION(input, expect, desc) \\\n ++\tTEST(t_enumeration(input, ARRAY_SIZE(input), expect, ARRAY_SIZE(expect)), \\\n +\t\t\t desc \" works\")\n +\n +static void t_lookup(const char **input_hexes, size_t n, const char *query_hex,\n @@ t/unit-tests/t-oid-array.c (new)\n +\tstruct object_id oid_query;\n +\tint ret;\n +\n -+\tif (get_oid_arbitrary_hex(query_hex, &oid_query))\n ++\tif (!check_int(get_oid_arbitrary_hex(query_hex, &oid_query), ==, 0))\n +\t\treturn;\n +\tif (fill_array(&array, input_hexes, n))\n +\t\treturn;\n @@ t/unit-tests/t-oid-array.c (new)\n +\n +static void setup(void)\n +{\n ++\t/* The hash algo is used by oid_array_lookup() internally */\n +\tint algo = init_hash_algo();\n -+\t/* because the_hash_algo is used by oid_array_lookup() internally */\n +\tif (check_int(algo, !=, GIT_HASH_UNKNOWN))\n +\t\trepo_set_hash_algo(the_repository, algo);\n +}\n @@ t/unit-tests/t-oid-array.c (new)\n +\tTEST_ENUMERATION(arr_input_dup, res_sorted,\n +\t\t\t \"ordered enumeration with duplicate suppression\");\n +\n -+\t/* ret is the return value of oid_array_lookup() */\n +\tTEST_LOOKUP(arr_input, \"55\", 1, 1, \"lookup\");\n +\tTEST_LOOKUP(arr_input, \"33\", INT_MIN, -1, \"lookup non-existent entry\");\n +\tTEST_LOOKUP(arr_input_dup, \"55\", 3, 5, \"lookup with duplicates\");\n-- \n2.46.0\n\n"},{"id":"501629","messageId":"CAP8UFD3mq+k8QXDrFAp5bfoCN+sNgm3vJvuhryxVYDaj-SZU0g@mail.gmail.com","threadId":"61721","inReplyTo":"20240824170223.36080-1-shyamthakkar001@gmail.com","subject":"Re: [GSoC][PATCH v3] t: port helper/test-oid-array.c to unit-tests/t-oid-array.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-08-25T06:38:22Z","receivedAt":"2024-08-25T06:38:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Aug 24, 2024 at 7:02 PM Ghanshyam Thakkar\n wrote:\n>\n> helper/test-oid-array.c along with t0064-oid-array.sh test the\n> oid-array.h API, which provides storage and processing\n> efficiency over large lists of object identifiers.\n>\n> Migrate them to the unit testing framework for better runtime\n> performance and efficiency. Also 'the_hash_algo' is used internally in\n> oid_array_lookup(), but we do not initialize a repository directory,\n> therefore initialize the_hash_algo manually.\n\nEven if 'the_hash_algo' is used internally in oid_array_lookup()\nthrough oid_pos(), this patch initializes the hash algo for the repo\nusing repo_set_hash_algo(), which contains the following single\ninstruction:\n\n repo->hash_algo = &hash_algos[hash_algo];\n\nSo \"therefore initialize the_hash_algo manually\" is not quite true, as\nit doesn't look like 'the_hash_algo' is even used.\n\nAlso I think it's not clear how initializing a repository directory is\nrelated to the hash algo.\n\nSo maybe something like the following would be better:\n\n\"As we don't initialize a repository in these tests, the hash algo\nthat functions like oid_array_lookup() use is not initialized,\ntherefore call repo_set_hash_algo() to initialize it.\"\n\n> And\n> init_hash_algo():lib-oid.c can aid in this process, so make it public.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Kaartic Sivaraam \n> Helped-by: Phillip Wood \n> Signed-off-by: Ghanshyam Thakkar \n> ---\n> Changes in v3:\n> - changed commmit message and comments for more accurate description\n> - removed test_min() and return early when actual.nr and expect.nr\n> don't match\n> - rename result to expect for more accurate description\n> - removed a redundant check in t_enumeration()\n> - add check_int() around one of calls of get_oid_arbitrary_hex()\n\nThis looks good.\n\n> - rebased to latest master\n\nIt's nice to say it was rebased, but it's better to tell the reason\nwhy it was rebased.\n\n> diff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h\n> index 8d2acca768..c949af082c 100644\n> --- a/t/unit-tests/lib-oid.h\n> +++ b/t/unit-tests/lib-oid.h\n> @@ -13,5 +13,13 @@\n> * environment variable.\n> */\n> int get_oid_arbitrary_hex(const char *s, struct object_id *oid);\n> +/*\n> + * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of\n> + * GIT_TEST_DEFAULT_HASH environment variable. The fallback value in case\n> + * of absence of GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1. It also uses\n\nNit: maybe: s/in case of absence/in the absence/\n\n> + * check(algo != GIT_HASH_UNKNOWN) before returning to verify if the\n> + * GIT_TEST_DEFAULT_HASH's value is valid or not.\n> + */\n> +int init_hash_algo(void);\n\n[...]\n\n> +static void t_enumeration(const char **input_args, size_t input_sz,\n> + const char **expect_args, size_t expect_sz)\n> +{\n> + struct oid_array input = OID_ARRAY_INIT, expect = OID_ARRAY_INIT,\n> + actual = OID_ARRAY_INIT;\n> + size_t i;\n> +\n> + if (fill_array(&input, input_args, input_sz))\n> + return;\n> + if (fill_array(&expect, expect_args, expect_sz))\n> + return;\n> +\n> + oid_array_for_each_unique(&input, add_to_oid_array, &actual);\n> + if(!check_uint(actual.nr, ==, expect.nr))\n\nMissing space between 'if' and '('.\n\n> + return;\n\nThe rest of the patch looks good to me. Thanks.\n"},{"id":"501636","messageId":"D3OXJZTMW1BP.1EYWBW4CTQVES@gmail.com","threadId":"61721","inReplyTo":"CAP8UFD3mq+k8QXDrFAp5bfoCN+sNgm3vJvuhryxVYDaj-SZU0g@mail.gmail.com","subject":"Re: [GSoC][PATCH v3] t: port helper/test-oid-array.c to unit-tests/t-oid-array.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-08-25T10:45:27Z","receivedAt":"2024-08-25T10:45:33Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Christian Couder wrote:\n> On Sat, Aug 24, 2024 at 7:02 PM Ghanshyam Thakkar\n> wrote:\n> >\n> > helper/test-oid-array.c along with t0064-oid-array.sh test the\n> > oid-array.h API, which provides storage and processing\n> > efficiency over large lists of object identifiers.\n> >\n> > Migrate them to the unit testing framework for better runtime\n> > performance and efficiency. Also 'the_hash_algo' is used internally in\n> > oid_array_lookup(), but we do not initialize a repository directory,\n> > therefore initialize the_hash_algo manually.\n>\n> Even if 'the_hash_algo' is used internally in oid_array_lookup()\n> through oid_pos(), this patch initializes the hash algo for the repo\n> using repo_set_hash_algo(), which contains the following single\n> instruction:\n>\n> repo->hash_algo = &hash_algos[hash_algo];\n>\n> So \"therefore initialize the_hash_algo manually\" is not quite true, as\n> it doesn't look like 'the_hash_algo' is even used.\n\nthe_hash_algo is just:\n define the_hash_algo the_repository->hash_algo\n\nand we do initialize the_repository->hash_algo manually.\n\n>\n> Also I think it's not clear how initializing a repository directory is\n> related to the hash algo.\n\nThat is mentioned because the old code in helper/test-oid-array.c used\nto call setup_git_directory_gently() to setup a git directory, which\nwould also initialize the_repository->hash_algo.\n\n>\n> So maybe something like the following would be better:\n>\n> \"As we don't initialize a repository in these tests, the hash algo\n> that functions like oid_array_lookup() use is not initialized,\n> therefore call repo_set_hash_algo() to initialize it.\"\n\nWill change.\n\n>\n> > And\n> > init_hash_algo():lib-oid.c can aid in this process, so make it public.\n> >\n> > Mentored-by: Christian Couder \n> > Mentored-by: Kaartic Sivaraam \n> > Helped-by: Phillip Wood \n> > Signed-off-by: Ghanshyam Thakkar \n> > ---\n> > Changes in v3:\n> > - changed commmit message and comments for more accurate description\n> > - removed test_min() and return early when actual.nr and expect.nr\n> > don't match\n> > - rename result to expect for more accurate description\n> > - removed a redundant check in t_enumeration()\n> > - add check_int() around one of calls of get_oid_arbitrary_hex()\n>\n> This looks good.\n>\n> > - rebased to latest master\n>\n> It's nice to say it was rebased, but it's better to tell the reason\n> why it was rebased.\n\nNo particular reason other than the fact that v2 was posted 20 days ago.\nAnd it is not merged into 'seen' or 'next', so it shouldn't be a\nproblem.\n\nThanks.\n"},{"id":"501965","messageId":"20240901212649.4910-1-shyamthakkar001@gmail.com","threadId":"61721","inReplyTo":"20240824170223.36080-1-shyamthakkar001@gmail.com","subject":"[PATCH v4] t: port helper/test-oid-array.c to unit-tests/t-oid-array.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-09-01T21:26:29Z","receivedAt":"2024-09-01T21:27:18Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"helper/test-oid-array.c along with t0064-oid-array.sh test the\noid-array.h API, which provides storage and processing\nefficiency over large lists of object identifiers.\n\nMigrate them to the unit testing framework for better runtime\nperformance and efficiency. As we don't initialize a repository\nin these tests, the hash algo that functions like oid_array_lookup()\nuse is not initialized, therefore call repo_set_hash_algo() to\ninitialize it. And init_hash_algo():lib-oid.c can aid in this\nprocess, so make it public.\n\nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nHelped-by: Phillip Wood \nSigned-off-by: Ghanshyam Thakkar \n---\n Makefile | 2 +-\n t/helper/test-oid-array.c | 49 ---------------\n t/helper/test-tool.c | 1 -\n t/helper/test-tool.h | 1 -\n t/t0064-oid-array.sh | 122 -----------------------------------\n t/unit-tests/lib-oid.c | 2 +-\n t/unit-tests/lib-oid.h | 8 +++\n t/unit-tests/t-oid-array.c | 126 +++++++++++++++++++++++++++++++++++++\n 8 files changed, 136 insertions(+), 175 deletions(-)\n delete mode 100644 t/helper/test-oid-array.c\n delete mode 100755 t/t0064-oid-array.sh\n create mode 100644 t/unit-tests/t-oid-array.c\n\ndiff --git a/Makefile b/Makefile\nindex e298c8b55e..8813753d99 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -808,7 +808,6 @@ TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o\n TEST_BUILTINS_OBJS += test-match-trees.o\n 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-online-cpus.o\n TEST_BUILTINS_OBJS += test-pack-mtimes.o\n TEST_BUILTINS_OBJS += test-parse-options.o\n@@ -1337,6 +1336,7 @@ UNIT_TEST_PROGRAMS += t-example-decorate\n UNIT_TEST_PROGRAMS += t-hash\n UNIT_TEST_PROGRAMS += t-hashmap\n UNIT_TEST_PROGRAMS += t-mem-pool\n+UNIT_TEST_PROGRAMS += t-oid-array\n UNIT_TEST_PROGRAMS += t-oidmap\n UNIT_TEST_PROGRAMS += t-oidtree\n UNIT_TEST_PROGRAMS += t-prio-queue\ndiff --git a/t/helper/test-oid-array.c b/t/helper/test-oid-array.c\ndeleted file mode 100644\nindex 076b849cbf..0000000000\n--- a/t/helper/test-oid-array.c\n+++ /dev/null\n@@ -1,49 +0,0 @@\n-#define USE_THE_REPOSITORY_VARIABLE\n-\n-#include \"test-tool.h\"\n-#include \"hex.h\"\n-#include \"oid-array.h\"\n-#include \"setup.h\"\n-#include \"strbuf.h\"\n-\n-static int print_oid(const struct object_id *oid, void *data UNUSED)\n-{\n-\tputs(oid_to_hex(oid));\n-\treturn 0;\n-}\n-\n-int cmd__oid_array(int argc UNUSED, const char **argv UNUSED)\n-{\n-\tstruct oid_array array = OID_ARRAY_INIT;\n-\tstruct strbuf line = STRBUF_INIT;\n-\tint nongit_ok;\n-\n-\tsetup_git_directory_gently(&nongit_ok);\n-\tif (nongit_ok)\n-\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\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, \"append \", &arg)) {\n-\t\t\tif (get_oid_hex(arg, &oid))\n-\t\t\t\tdie(\"not a hexadecimal oid: %s\", arg);\n-\t\t\toid_array_append(&array, &oid);\n-\t\t} else if (skip_prefix(line.buf, \"lookup \", &arg)) {\n-\t\t\tif (get_oid_hex(arg, &oid))\n-\t\t\t\tdie(\"not a hexadecimal oid: %s\", arg);\n-\t\t\tprintf(\"%d\\n\", oid_array_lookup(&array, &oid));\n-\t\t} else if (!strcmp(line.buf, \"clear\"))\n-\t\t\toid_array_clear(&array);\n-\t\telse if (!strcmp(line.buf, \"for_each_unique\"))\n-\t\t\toid_array_for_each_unique(&array, print_oid, NULL);\n-\t\telse\n-\t\t\tdie(\"unknown command: %s\", line.buf);\n-\t}\n-\n-\tstrbuf_release(&line);\n-\toid_array_clear(&array);\n-\n-\treturn 0;\n-}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex da3e69128a..353d2aaaa4 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -43,7 +43,6 @@ static struct test_cmd cmds[] = {\n \t{ \"match-trees\", cmd__match_trees },\n \t{ \"mergesort\", cmd__mergesort },\n \t{ \"mktemp\", cmd__mktemp },\n-\t{ \"oid-array\", cmd__oid_array },\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 642a34578c..d3d8aa28e0 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -64,7 +64,6 @@ int cmd__scrap_cache_tree(int argc, const char **argv);\n int cmd__serve_v2(int argc, const char **argv);\n int cmd__sha1(int argc, const char **argv);\n int cmd__sha1_is_sha1dc(int argc, const char **argv);\n-int cmd__oid_array(int argc, const char **argv);\n int cmd__sha256(int argc, const char **argv);\n int cmd__sigchain(int argc, const char **argv);\n int cmd__simple_ipc(int argc, const char **argv);\ndiff --git a/t/t0064-oid-array.sh b/t/t0064-oid-array.sh\ndeleted file mode 100755\nindex de74b692d0..0000000000\n--- a/t/t0064-oid-array.sh\n+++ /dev/null\n@@ -1,122 +0,0 @@\n-#!/bin/sh\n-\n-test_description='basic tests for the oid array implementation'\n-\n-TEST_PASSES_SANITIZE_LEAK=true\n-. ./test-lib.sh\n-\n-echoid () {\n-\tprefix=\"${1:+$1 }\"\n-\tshift\n-\twhile test $# -gt 0\n-\tdo\n-\t\techo \"$prefix$ZERO_OID\" | sed -e \"s/00/$1/g\"\n-\t\tshift\n-\tdone\n-}\n-\n-test_expect_success 'without repository' '\n-\tcat >expect <<-EOF &&\n-\t4444444444444444444444444444444444444444\n-\t5555555555555555555555555555555555555555\n-\t8888888888888888888888888888888888888888\n-\taaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n-\tEOF\n-\tcat >input <<-EOF &&\n-\tappend 4444444444444444444444444444444444444444\n-\tappend 5555555555555555555555555555555555555555\n-\tappend 8888888888888888888888888888888888888888\n-\tappend aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n-\tfor_each_unique\n-\tEOF\n-\tnongit test-tool oid-array actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'ordered enumeration' '\n-\techoid \"\" 44 55 88 aa >expect &&\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techo for_each_unique\n-\t} | test-tool oid-array >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'ordered enumeration with duplicate suppression' '\n-\techoid \"\" 44 55 88 aa >expect &&\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techo for_each_unique\n-\t} | test-tool oid-array >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'lookup' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -eq 1\n-'\n-\n-test_expect_success 'lookup non-existing entry' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 33\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -lt 0\n-'\n-\n-test_expect_success 'lookup with duplicates' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -ge 3 &&\n-\ttest \"$n\" -le 5\n-'\n-\n-test_expect_success 'lookup non-existing entry with duplicates' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 66\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -lt 0\n-'\n-\n-test_expect_success 'lookup with almost duplicate values' '\n-\t# n-1 5s\n-\troot=$(echoid \"\" 55) &&\n-\troot=${root%5} &&\n-\t{\n-\t\tid1=\"${root}5\" &&\n-\t\tid2=\"${root}f\" &&\n-\t\techo \"append $id1\" &&\n-\t\techo \"append $id2\" &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -eq 0\n-'\n-\n-test_expect_success 'lookup with single duplicate value' '\n-\t{\n-\t\techoid append 55 55 &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -ge 0 &&\n-\ttest \"$n\" -le 1\n-'\n-\n-test_done\ndiff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c\nindex 37105f0a8f..8f0ccac532 100644\n--- a/t/unit-tests/lib-oid.c\n+++ b/t/unit-tests/lib-oid.c\n@@ -3,7 +3,7 @@\n #include \"strbuf.h\"\n #include \"hex.h\"\n \n-static int init_hash_algo(void)\n+int init_hash_algo(void)\n {\n \tstatic int algo = -1;\n \ndiff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h\nindex 8d2acca768..4e77c04bd2 100644\n--- a/t/unit-tests/lib-oid.h\n+++ b/t/unit-tests/lib-oid.h\n@@ -13,5 +13,13 @@\n * environment variable.\n */\n int get_oid_arbitrary_hex(const char *s, struct object_id *oid);\n+/*\n+ * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of\n+ * GIT_TEST_DEFAULT_HASH environment variable. The fallback value in the\n+ * absence of GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1. It also uses\n+ * check(algo != GIT_HASH_UNKNOWN) before returning to verify if the\n+ * GIT_TEST_DEFAULT_HASH's value is valid or not.\n+ */\n+int init_hash_algo(void);\n \n #endif /* LIB_OID_H */\ndiff --git a/t/unit-tests/t-oid-array.c b/t/unit-tests/t-oid-array.c\nnew file mode 100644\nindex 0000000000..45b59a2a51\n--- /dev/null\n+++ b/t/unit-tests/t-oid-array.c\n@@ -0,0 +1,126 @@\n+#define USE_THE_REPOSITORY_VARIABLE\n+\n+#include \"test-lib.h\"\n+#include \"lib-oid.h\"\n+#include \"oid-array.h\"\n+#include \"hex.h\"\n+\n+static int fill_array(struct oid_array *array, const char *hexes[], size_t n)\n+{\n+\tfor (size_t i = 0; i < n; i++) {\n+\t\tstruct object_id oid;\n+\n+\t\tif (!check_int(get_oid_arbitrary_hex(hexes[i], &oid), ==, 0))\n+\t\t\treturn -1;\n+\t\toid_array_append(array, &oid);\n+\t}\n+\tif (!check_uint(array->nr, ==, n))\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n+static int add_to_oid_array(const struct object_id *oid, void *data)\n+{\n+\tstruct oid_array *array = data;\n+\n+\toid_array_append(array, oid);\n+\treturn 0;\n+}\n+\n+static void t_enumeration(const char **input_args, size_t input_sz,\n+\t\t\t const char **expect_args, size_t expect_sz)\n+{\n+\tstruct oid_array input = OID_ARRAY_INIT, expect = OID_ARRAY_INIT,\n+\t\t\t actual = OID_ARRAY_INIT;\n+\tsize_t i;\n+\n+\tif (fill_array(&input, input_args, input_sz))\n+\t\treturn;\n+\tif (fill_array(&expect, expect_args, expect_sz))\n+\t\treturn;\n+\n+\toid_array_for_each_unique(&input, add_to_oid_array, &actual);\n+\tif (!check_uint(actual.nr, ==, expect.nr))\n+\t\treturn;\n+\n+\tfor (i = 0; i < actual.nr; i++) {\n+\t\tif (!check(oideq(&actual.oid[i], &expect.oid[i])))\n+\t\t\ttest_msg(\"expected: %s\\n got: %s\\n index: %\" PRIuMAX,\n+\t\t\t\t oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]),\n+\t\t\t\t (uintmax_t)i);\n+\t}\n+\n+\toid_array_clear(&actual);\n+\toid_array_clear(&input);\n+\toid_array_clear(&expect);\n+}\n+\n+#define TEST_ENUMERATION(input, expect, desc) \\\n+\tTEST(t_enumeration(input, ARRAY_SIZE(input), expect, ARRAY_SIZE(expect)), \\\n+\t\t\t desc \" works\")\n+\n+static void t_lookup(const char **input_hexes, size_t n, const char *query_hex,\n+\t\t int lower_bound, int upper_bound)\n+{\n+\tstruct oid_array array = OID_ARRAY_INIT;\n+\tstruct object_id oid_query;\n+\tint ret;\n+\n+\tif (!check_int(get_oid_arbitrary_hex(query_hex, &oid_query), ==, 0))\n+\t\treturn;\n+\tif (fill_array(&array, input_hexes, n))\n+\t\treturn;\n+\tret = oid_array_lookup(&array, &oid_query);\n+\n+\tif (!check_int(ret, <=, upper_bound) ||\n+\t !check_int(ret, >=, lower_bound))\n+\t\ttest_msg(\"oid query for lookup: %s\", oid_to_hex(&oid_query));\n+\n+\toid_array_clear(&array);\n+}\n+\n+#define TEST_LOOKUP(input_hexes, query, lower_bound, upper_bound, desc) \\\n+\tTEST(t_lookup(input_hexes, ARRAY_SIZE(input_hexes), query, \\\n+\t\t lower_bound, upper_bound), \\\n+\t desc \" works\")\n+\n+static void setup(void)\n+{\n+\t/* The hash algo is used by oid_array_lookup() internally */\n+\tint algo = init_hash_algo();\n+\tif (check_int(algo, !=, GIT_HASH_UNKNOWN))\n+\t\trepo_set_hash_algo(the_repository, algo);\n+}\n+\n+int cmd_main(int argc UNUSED, const char **argv UNUSED)\n+{\n+\tconst char *arr_input[] = { \"88\", \"44\", \"aa\", \"55\" };\n+\tconst char *arr_input_dup[] = { \"88\", \"44\", \"aa\", \"55\",\n+\t\t\t\t\t\"88\", \"44\", \"aa\", \"55\",\n+\t\t\t\t\t\"88\", \"44\", \"aa\", \"55\" };\n+\tconst char *res_sorted[] = { \"44\", \"55\", \"88\", \"aa\" };\n+\tconst char *nearly_55;\n+\n+\tif (!TEST(setup(), \"setup\"))\n+\t\ttest_skip_all(\"hash algo initialization failed\");\n+\n+\tTEST_ENUMERATION(arr_input, res_sorted, \"ordered enumeration\");\n+\tTEST_ENUMERATION(arr_input_dup, res_sorted,\n+\t\t\t \"ordered enumeration with duplicate suppression\");\n+\n+\tTEST_LOOKUP(arr_input, \"55\", 1, 1, \"lookup\");\n+\tTEST_LOOKUP(arr_input, \"33\", INT_MIN, -1, \"lookup non-existent entry\");\n+\tTEST_LOOKUP(arr_input_dup, \"55\", 3, 5, \"lookup with duplicates\");\n+\tTEST_LOOKUP(arr_input_dup, \"66\", INT_MIN, -1,\n+\t\t \"lookup non-existent entry with duplicates\");\n+\n+\tnearly_55 = init_hash_algo() == GIT_HASH_SHA1 ?\n+\t\t\t\"5500000000000000000000000000000000000001\" :\n+\t\t\t\"5500000000000000000000000000000000000000000000000000000000000001\";\n+\tTEST_LOOKUP(((const char *[]){ \"55\", nearly_55 }), \"55\", 0, 0,\n+\t\t \"lookup with almost duplicate values\");\n+\tTEST_LOOKUP(((const char *[]){ \"55\", \"55\" }), \"55\", 0, 1,\n+\t\t \"lookup with single duplicate value\");\n+\n+\treturn test_done();\n+}\n\nRange-diff against v3:\n1: 408a179736 ! 1: 58ca6aefca t: port helper/test-oid-array.c to unit-tests/t-oid-array.c\n @@ Commit message\n efficiency over large lists of object identifiers.\n \n Migrate them to the unit testing framework for better runtime\n - performance and efficiency. Also 'the_hash_algo' is used internally in\n - oid_array_lookup(), but we do not initialize a repository directory,\n - therefore initialize the_hash_algo manually. And\n - init_hash_algo():lib-oid.c can aid in this process, so make it public.\n + performance and efficiency. As we don't initialize a repository\n + in these tests, the hash algo that functions like oid_array_lookup()\n + use is not initialized, therefore call repo_set_hash_algo() to\n + initialize it. And init_hash_algo():lib-oid.c can aid in this\n + process, so make it public.\n \n Mentored-by: Christian Couder \n Mentored-by: Kaartic Sivaraam \n @@ t/unit-tests/lib-oid.h\n int get_oid_arbitrary_hex(const char *s, struct object_id *oid);\n +/*\n + * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of\n -+ * GIT_TEST_DEFAULT_HASH environment variable. The fallback value in case\n -+ * of absence of GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1. It also uses\n ++ * GIT_TEST_DEFAULT_HASH environment variable. The fallback value in the\n ++ * absence of GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1. It also uses\n + * check(algo != GIT_HASH_UNKNOWN) before returning to verify if the\n + * GIT_TEST_DEFAULT_HASH's value is valid or not.\n + */\n @@ t/unit-tests/t-oid-array.c (new)\n +\t\treturn;\n +\n +\toid_array_for_each_unique(&input, add_to_oid_array, &actual);\n -+\tif(!check_uint(actual.nr, ==, expect.nr))\n ++\tif (!check_uint(actual.nr, ==, expect.nr))\n +\t\treturn;\n +\n +\tfor (i = 0; i < actual.nr; i++) {\n-- \n2.46.0\n\n"},{"id":"502099","messageId":"CAP8UFD0NMCUeFpQmLzXZmTUQQjQh5Dk79QxxMH_GN62w8ZC6YQ@mail.gmail.com","threadId":"61721","inReplyTo":"20240901212649.4910-1-shyamthakkar001@gmail.com","subject":"Re: [PATCH v4] t: port helper/test-oid-array.c to unit-tests/t-oid-array.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-09-04T07:42:58Z","receivedAt":"2024-09-04T07:43:12Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Sep 1, 2024 at 11:27 PM Ghanshyam Thakkar\n wrote:\n>\n> helper/test-oid-array.c along with t0064-oid-array.sh test the\n> oid-array.h API, which provides storage and processing\n> efficiency over large lists of object identifiers.\n>\n> Migrate them to the unit testing framework for better runtime\n> performance and efficiency. As we don't initialize a repository\n> in these tests, the hash algo that functions like oid_array_lookup()\n> use is not initialized, therefore call repo_set_hash_algo() to\n> initialize it. And init_hash_algo():lib-oid.c can aid in this\n> process, so make it public.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Kaartic Sivaraam \n> Helped-by: Phillip Wood \n> Signed-off-by: Ghanshyam Thakkar \n> ---\n\nIt would have been nice to briefly summarize here the changes compared\nto v3. On the other hand they are small enough and this version\naddresses all the suggestions that were made previously and looks good\nto me, so I think it is good to go.\n\nThanks!\n"},{"id":"502138","messageId":"xmqq7cbrea40.fsf@gitster.g","threadId":"61721","inReplyTo":"CAP8UFD0NMCUeFpQmLzXZmTUQQjQh5Dk79QxxMH_GN62w8ZC6YQ@mail.gmail.com","subject":"Re: [PATCH v4] t: port helper/test-oid-array.c to unit-tests/t-oid-array.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-04T15:01:35Z","receivedAt":"2024-09-04T15:01:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Sun, Sep 1, 2024 at 11:27 PM Ghanshyam Thakkar\n> wrote:\n>>\n>> helper/test-oid-array.c along with t0064-oid-array.sh test the\n>> oid-array.h API, which provides storage and processing\n>> efficiency over large lists of object identifiers.\n>>\n>> Migrate them to the unit testing framework for better runtime\n>> performance and efficiency. As we don't initialize a repository\n>> in these tests, the hash algo that functions like oid_array_lookup()\n>> use is not initialized, therefore call repo_set_hash_algo() to\n>> initialize it. And init_hash_algo():lib-oid.c can aid in this\n>> process, so make it public.\n>>\n>> Mentored-by: Christian Couder \n>> Mentored-by: Kaartic Sivaraam \n>> Helped-by: Phillip Wood \n>> Signed-off-by: Ghanshyam Thakkar \n>> ---\n>\n> It would have been nice to briefly summarize here the changes compared\n> to v3. On the other hand they are small enough and this version\n> addresses all the suggestions that were made previously and looks good\n> to me, so I think it is good to go.\n\nI only checked the changes sine the previous round myself, and\ndidn't see anything questionable.\n\nLet me mark the topic for 'next' soonish.\n\nThanks for polishing the topic, both of you.\n"}]}