{"thread":{"id":"61651","subject":"[GSoC][PATCH] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c","startedAt":"2024-06-19T17:50:51Z","lastAt":"2024-07-02T18:55:20Z","messageCount":16,"participants":["Ghanshyam Thakkar","Jonathan Nieder","Phillip Wood","phillip.wood123@gmail.com","Josh Steadmon","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"497366","messageId":"20240619175036.64291-1-shyamthakkar001@gmail.com","threadId":"61651","inReplyTo":null,"subject":"[GSoC][PATCH] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-06-19T17:50:29Z","receivedAt":"2024-06-19T17:50:51Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h\nlibrary which is built on top of hashmap.h to store arbitrary\ndatastructure (which must contain oidmap_entry, which is a wrapper\naround object_id). These entries can be accessed by querying their\nassociated object_id.\n\nMigrate them to the unit testing framework for better performance,\nconcise code and better debugging. Along with the migration also plug\nmemory leaks and make the test logic independent for all the tests.\nThe migration removes 'put' tests from t0016, because it is used as\nsetup to all the other tests, so testing it separately does not yield\nany benefit.\n\nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nSigned-off-by: Ghanshyam Thakkar \n---\nThis patch is depenedent on 'gt/unit-test-oidtree' for lib-oid.\n\n Makefile | 2 +-\n t/helper/test-oidmap.c | 123 ------------------------------\n t/helper/test-tool.c | 1 -\n t/helper/test-tool.h | 1 -\n t/t0016-oidmap.sh | 112 ---------------------------\n t/unit-tests/t-oidmap.c | 165 ++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 166 insertions(+), 238 deletions(-)\n delete mode 100644 t/helper/test-oidmap.c\n delete mode 100755 t/t0016-oidmap.sh\n create mode 100644 t/unit-tests/t-oidmap.c\n\ndiff --git a/Makefile b/Makefile\nindex 03751e0fc0..f7ed50f3a9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -810,7 +810,6 @@ 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 TEST_BUILTINS_OBJS += test-parse-options.o\n@@ -1334,6 +1333,7 @@ THIRD_PARTY_SOURCES += sha1dc/%\n \n UNIT_TEST_PROGRAMS += t-ctype\n UNIT_TEST_PROGRAMS += t-mem-pool\n+UNIT_TEST_PROGRAMS += t-oidmap\n UNIT_TEST_PROGRAMS += t-oidtree\n UNIT_TEST_PROGRAMS += t-prio-queue\n UNIT_TEST_PROGRAMS += t-strbuf\ndiff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c\ndeleted file mode 100644\nindex bd30244a54..0000000000\n--- a/t/helper/test-oidmap.c\n+++ /dev/null\n@@ -1,123 +0,0 @@\n-#include \"test-tool.h\"\n-#include \"hex.h\"\n-#include \"object-name.h\"\n-#include \"oidmap.h\"\n-#include \"repository.h\"\n-#include \"setup.h\"\n-#include \"strbuf.h\"\n-#include \"string-list.h\"\n-\n-/* key is an oid and value is a name (could be a refname for example) */\n-struct test_entry {\n-\tstruct oidmap_entry entry;\n-\tchar name[FLEX_ARRAY];\n-};\n-\n-#define DELIM \" \\t\\r\\n\"\n-\n-/*\n- * Read stdin line by line and print result of commands to stdout:\n- *\n- * hash oidkey -> sha1hash(oidkey)\n- * put oidkey namevalue -> NULL / old namevalue\n- * get oidkey -> NULL / namevalue\n- * remove oidkey -> NULL / old namevalue\n- * iterate -> oidkey1 namevalue1\\noidkey2 namevalue2\\n...\n- *\n- */\n-int cmd__oidmap(int argc UNUSED, const char **argv UNUSED)\n-{\n-\tstruct string_list parts = STRING_LIST_INIT_NODUP;\n-\tstruct strbuf line = STRBUF_INIT;\n-\tstruct oidmap map = OIDMAP_INIT;\n-\n-\tsetup_git_directory();\n-\n-\t/* init oidmap */\n-\toidmap_init(&map, 0);\n-\n-\t/* process commands from stdin */\n-\twhile (strbuf_getline(&line, stdin) != EOF) {\n-\t\tchar *cmd, *p1, *p2;\n-\t\tstruct test_entry *entry;\n-\t\tstruct object_id oid;\n-\n-\t\t/* break line into command and up to two parameters */\n-\t\tstring_list_setlen(&parts, 0);\n-\t\tstring_list_split_in_place(&parts, line.buf, DELIM, 2);\n-\t\tstring_list_remove_empty_items(&parts, 0);\n-\n-\t\t/* ignore empty lines */\n-\t\tif (!parts.nr)\n-\t\t\tcontinue;\n-\t\tif (!*parts.items[0].string || *parts.items[0].string == '#')\n-\t\t\tcontinue;\n-\n-\t\tcmd = parts.items[0].string;\n-\t\tp1 = parts.nr >= 1 ? parts.items[1].string : NULL;\n-\t\tp2 = parts.nr >= 2 ? parts.items[2].string : NULL;\n-\n-\t\tif (!strcmp(\"put\", cmd) && p1 && p2) {\n-\n-\t\t\tif (repo_get_oid(the_repository, p1, &oid)) {\n-\t\t\t\tprintf(\"Unknown oid: %s\\n\", p1);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\n-\t\t\t/* create entry with oid_key = p1, name_value = p2 */\n-\t\t\tFLEX_ALLOC_STR(entry, name, p2);\n-\t\t\toidcpy(&entry->entry.oid, &oid);\n-\n-\t\t\t/* add / replace entry */\n-\t\t\tentry = oidmap_put(&map, entry);\n-\n-\t\t\t/* print and free replaced entry, if any */\n-\t\t\tputs(entry ? entry->name : \"NULL\");\n-\t\t\tfree(entry);\n-\n-\t\t} else if (!strcmp(\"get\", cmd) && p1) {\n-\n-\t\t\tif (repo_get_oid(the_repository, p1, &oid)) {\n-\t\t\t\tprintf(\"Unknown oid: %s\\n\", p1);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\n-\t\t\t/* lookup entry in oidmap */\n-\t\t\tentry = oidmap_get(&map, &oid);\n-\n-\t\t\t/* print result */\n-\t\t\tputs(entry ? entry->name : \"NULL\");\n-\n-\t\t} else if (!strcmp(\"remove\", cmd) && p1) {\n-\n-\t\t\tif (repo_get_oid(the_repository, p1, &oid)) {\n-\t\t\t\tprintf(\"Unknown oid: %s\\n\", p1);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\n-\t\t\t/* remove entry from oidmap */\n-\t\t\tentry = oidmap_remove(&map, &oid);\n-\n-\t\t\t/* print result and free entry*/\n-\t\t\tputs(entry ? entry->name : \"NULL\");\n-\t\t\tfree(entry);\n-\n-\t\t} else if (!strcmp(\"iterate\", cmd)) {\n-\n-\t\t\tstruct oidmap_iter iter;\n-\t\t\toidmap_iter_init(&map, &iter);\n-\t\t\twhile ((entry = oidmap_iter_next(&iter)))\n-\t\t\t\tprintf(\"%s %s\\n\", oid_to_hex(&entry->entry.oid), entry->name);\n-\n-\t\t} else {\n-\n-\t\t\tprintf(\"Unknown command %s\\n\", cmd);\n-\n-\t\t}\n-\t}\n-\n-\tstring_list_clear(&parts, 0);\n-\tstrbuf_release(&line);\n-\toidmap_free(&map, 1);\n-\treturn 0;\n-}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 253324a06b..5f013d8b2b 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -45,7 +45,6 @@ static struct test_cmd cmds[] = {\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 },\n \t{ \"parse-options\", cmd__parse_options },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 460dd7d260..c7d3e43694 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -38,7 +38,6 @@ int cmd__lazy_init_name_hash(int argc, const char **argv);\n int cmd__match_trees(int argc, const char **argv);\n int cmd__mergesort(int argc, const char **argv);\n int cmd__mktemp(int argc, const char **argv);\n-int cmd__oidmap(int argc, const char **argv);\n int cmd__online_cpus(int argc, const char **argv);\n int cmd__pack_mtimes(int argc, const char **argv);\n int cmd__parse_options(int argc, const char **argv);\ndiff --git a/t/t0016-oidmap.sh b/t/t0016-oidmap.sh\ndeleted file mode 100755\nindex 0faef1f4f1..0000000000\n--- a/t/t0016-oidmap.sh\n+++ /dev/null\n@@ -1,112 +0,0 @@\n-#!/bin/sh\n-\n-test_description='test oidmap'\n-\n-TEST_PASSES_SANITIZE_LEAK=true\n-. ./test-lib.sh\n-\n-# This purposefully is very similar to t0011-hashmap.sh\n-\n-test_oidmap () {\n-\techo \"$1\" | test-tool oidmap $3 >actual &&\n-\techo \"$2\" >expect &&\n-\ttest_cmp expect actual\n-}\n-\n-\n-test_expect_success 'setup' '\n-\n-\ttest_commit one &&\n-\ttest_commit two &&\n-\ttest_commit three &&\n-\ttest_commit four\n-\n-'\n-\n-test_expect_success 'put' '\n-\n-test_oidmap \"put one 1\n-put two 2\n-put invalidOid 4\n-put three 3\" \"NULL\n-NULL\n-Unknown oid: invalidOid\n-NULL\"\n-\n-'\n-\n-test_expect_success 'replace' '\n-\n-test_oidmap \"put one 1\n-put two 2\n-put three 3\n-put invalidOid 4\n-put two deux\n-put one un\" \"NULL\n-NULL\n-NULL\n-Unknown oid: invalidOid\n-2\n-1\"\n-\n-'\n-\n-test_expect_success 'get' '\n-\n-test_oidmap \"put one 1\n-put two 2\n-put three 3\n-get two\n-get four\n-get invalidOid\n-get one\" \"NULL\n-NULL\n-NULL\n-2\n-NULL\n-Unknown oid: invalidOid\n-1\"\n-\n-'\n-\n-test_expect_success 'remove' '\n-\n-test_oidmap \"put one 1\n-put two 2\n-put three 3\n-remove one\n-remove two\n-remove invalidOid\n-remove four\" \"NULL\n-NULL\n-NULL\n-1\n-2\n-Unknown oid: invalidOid\n-NULL\"\n-\n-'\n-\n-test_expect_success 'iterate' '\n-\ttest-tool oidmap >actual.raw <<-\\EOF &&\n-\tput one 1\n-\tput two 2\n-\tput three 3\n-\titerate\n-\tEOF\n-\n-\t# sort \"expect\" too so we do not rely on the order of particular oids\n-\tsort >expect <<-EOF &&\n-\tNULL\n-\tNULL\n-\tNULL\n-\t$(git rev-parse one) 1\n-\t$(git rev-parse two) 2\n-\t$(git rev-parse three) 3\n-\tEOF\n-\n-\tsort actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_done\ndiff --git a/t/unit-tests/t-oidmap.c b/t/unit-tests/t-oidmap.c\nnew file mode 100644\nindex 0000000000..9b98a3ed09\n--- /dev/null\n+++ b/t/unit-tests/t-oidmap.c\n@@ -0,0 +1,165 @@\n+#include \"test-lib.h\"\n+#include \"lib-oid.h\"\n+#include \"oidmap.h\"\n+#include \"hash.h\"\n+#include \"hex.h\"\n+\n+/*\n+ * elements we will put in oidmap structs are made of a key: the entry.oid\n+ * field, which is of type struct object_id, and a value: the name field (could\n+ * be a refname for example)\n+ */\n+struct test_entry {\n+\tstruct oidmap_entry entry;\n+\tchar name[FLEX_ARRAY];\n+};\n+\n+static const char *key_val[][2] = { { \"11\", \"one\" },\n+\t\t\t\t { \"22\", \"two\" },\n+\t\t\t\t { \"33\", \"three\" } };\n+\n+static int put_and_check_null(struct oidmap *map, const char *hex,\n+\t\t\t const char *entry_name)\n+{\n+\tstruct test_entry *entry;\n+\n+\tFLEX_ALLOC_STR(entry, name, entry_name);\n+\tif (get_oid_arbitrary_hex(hex, &entry->entry.oid))\n+\t\treturn -1;\n+\tif (!check(oidmap_put(map, entry) == NULL))\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n+static void setup(void (*f)(struct oidmap *map))\n+{\n+\tstruct oidmap map = OIDMAP_INIT;\n+\tint ret = 0;\n+\n+\tfor (size_t i = 0; i < ARRAY_SIZE(key_val); i++)\n+\t\tif ((ret = put_and_check_null(&map, key_val[i][0],\n+\t\t\t\t\t key_val[i][1])))\n+\t\t\tbreak;\n+\n+\tif (!ret)\n+\t\tf(&map);\n+\toidmap_free(&map, 1);\n+}\n+\n+static void t_replace(struct oidmap *map)\n+{\n+\tstruct test_entry *entry, *prev;\n+\n+\tFLEX_ALLOC_STR(entry, name, \"un\");\n+\tif (get_oid_arbitrary_hex(\"11\", &entry->entry.oid))\n+\t\treturn;\n+\tprev = oidmap_put(map, entry);\n+\tif (!check(prev != NULL))\n+\t\treturn;\n+\tcheck_str(prev->name, \"one\");\n+\tfree(prev);\n+\n+\tFLEX_ALLOC_STR(entry, name, \"deux\");\n+\tif (get_oid_arbitrary_hex(\"22\", &entry->entry.oid))\n+\t\treturn;\n+\tprev = oidmap_put(map, entry);\n+\tif (!check(prev != NULL))\n+\t\treturn;\n+\tcheck_str(prev->name, \"two\");\n+\tfree(prev);\n+}\n+\n+static void t_get(struct oidmap *map)\n+{\n+\tstruct test_entry *entry;\n+\tstruct object_id oid;\n+\n+\tif (get_oid_arbitrary_hex(\"22\", &oid))\n+\t\treturn;\n+\tentry = oidmap_get(map, &oid);\n+\tif (!check(entry != NULL))\n+\t\treturn;\n+\tcheck_str(entry->name, \"two\");\n+\n+\tif (get_oid_arbitrary_hex(\"44\", &oid))\n+\t\treturn;\n+\tcheck(oidmap_get(map, &oid) == NULL);\n+\n+\tif (get_oid_arbitrary_hex(\"11\", &oid))\n+\t\treturn;\n+\tentry = oidmap_get(map, &oid);\n+\tif (!check(entry != NULL))\n+\t\treturn;\n+\tcheck_str(entry->name, \"one\");\n+}\n+\n+static void t_remove(struct oidmap *map)\n+{\n+\tstruct test_entry *entry;\n+\tstruct object_id oid;\n+\n+\tif (get_oid_arbitrary_hex(\"11\", &oid))\n+\t\treturn;\n+\tentry = oidmap_remove(map, &oid);\n+\tif (!check(entry != NULL))\n+\t\treturn;\n+\tcheck_str(entry->name, \"one\");\n+\tcheck(oidmap_get(map, &oid) == NULL);\n+\tfree(entry);\n+\n+\tif (get_oid_arbitrary_hex(\"22\", &oid))\n+\t\treturn;\n+\tentry = oidmap_remove(map, &oid);\n+\tif (!check(entry != NULL))\n+\t\treturn;\n+\tcheck_str(entry->name, \"two\");\n+\tcheck(oidmap_get(map, &oid) == NULL);\n+\tfree(entry);\n+\n+\tif (get_oid_arbitrary_hex(\"44\", &oid))\n+\t\treturn;\n+\tcheck(oidmap_remove(map, &oid) == NULL);\n+}\n+\n+static int key_val_contains(struct test_entry *entry)\n+{\n+\t/* the test is small enough to be able to bear O(n) */\n+\tfor (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {\n+\t\tif (!strcmp(key_val[i][1], entry->name)) {\n+\t\t\tstruct object_id oid;\n+\t\t\tif (get_oid_arbitrary_hex(key_val[i][0], &oid))\n+\t\t\t\treturn -1;\n+\t\t\tif (oideq(&entry->entry.oid, &oid))\n+\t\t\t\treturn 0;\n+\t\t}\n+\t}\n+\treturn 1;\n+}\n+\n+static void t_iterate(struct oidmap *map)\n+{\n+\tstruct oidmap_iter iter;\n+\tstruct test_entry *entry;\n+\tint ret;\n+\n+\toidmap_iter_init(map, &iter);\n+\twhile ((entry = oidmap_iter_next(&iter))) {\n+\t\tif (!check_int((ret = key_val_contains(entry)), ==, 0)) {\n+\t\t\tif (ret == -1)\n+\t\t\t\treturn;\n+\t\t\ttest_msg(\"obtained entry was not given in the input\\n\"\n+\t\t\t\t \" name: %s\\n oid: %s\\n\",\n+\t\t\t\t entry->name, oid_to_hex(&entry->entry.oid));\n+\t\t}\n+\t}\n+\tcheck_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val));\n+}\n+\n+int cmd_main(int argc UNUSED, const char **argv UNUSED)\n+{\n+\tTEST(setup(t_replace), \"replace works\");\n+\tTEST(setup(t_get), \"get works\");\n+\tTEST(setup(t_remove), \"remove works\");\n+\tTEST(setup(t_iterate), \"iterate works\");\n+\treturn test_done();\n+}\n-- \n2.45.2\n\n"},{"id":"497394","messageId":"ZnP6G6SSBynlBNUj@google.com","threadId":"61651","inReplyTo":"20240619175036.64291-1-shyamthakkar001@gmail.com","subject":"Re: [GSoC][PATCH] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2024-06-20T09:45:47Z","receivedAt":"2024-06-20T09:45:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ghanshyam Thakkar wrote:\n\n> helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h\n> library which is built on top of hashmap.h to store arbitrary\n> datastructure (which must contain oidmap_entry, which is a wrapper\n> around object_id). These entries can be accessed by querying their\n> associated object_id.\n>\n> Migrate them to the unit testing framework for better performance,\n> concise code and better debugging. Along with the migration also plug\n> memory leaks and make the test logic independent for all the tests.\n> The migration removes 'put' tests from t0016, because it is used as\n> setup to all the other tests, so testing it separately does not yield\n> any benefit.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Kaartic Sivaraam \n> Signed-off-by: Ghanshyam Thakkar \n> ---\n> This patch is depenedent on 'gt/unit-test-oidtree' for lib-oid.\n\nVery neat! I'm cc-ing Josh Steadmon for unit test framework expertise.\n\nPatch left unsnipped for reference.\n\n> Makefile | 2 +-\n> t/helper/test-oidmap.c | 123 ------------------------------\n> t/helper/test-tool.c | 1 -\n> t/helper/test-tool.h | 1 -\n> t/t0016-oidmap.sh | 112 ---------------------------\n> t/unit-tests/t-oidmap.c | 165 ++++++++++++++++++++++++++++++++++++++++\n> 6 files changed, 166 insertions(+), 238 deletions(-)\n> delete mode 100644 t/helper/test-oidmap.c\n> delete mode 100755 t/t0016-oidmap.sh\n> create mode 100644 t/unit-tests/t-oidmap.c\n> \n> diff --git a/Makefile b/Makefile\n> index 03751e0fc0..f7ed50f3a9 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -810,7 +810,6 @@ 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> TEST_BUILTINS_OBJS += test-parse-options.o\n> @@ -1334,6 +1333,7 @@ THIRD_PARTY_SOURCES += sha1dc/%\n> \n> UNIT_TEST_PROGRAMS += t-ctype\n> UNIT_TEST_PROGRAMS += t-mem-pool\n> +UNIT_TEST_PROGRAMS += t-oidmap\n> UNIT_TEST_PROGRAMS += t-oidtree\n> UNIT_TEST_PROGRAMS += t-prio-queue\n> UNIT_TEST_PROGRAMS += t-strbuf\n> diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c\n> deleted file mode 100644\n> index bd30244a54..0000000000\n> --- a/t/helper/test-oidmap.c\n> +++ /dev/null\n> @@ -1,123 +0,0 @@\n> -#include \"test-tool.h\"\n> -#include \"hex.h\"\n> -#include \"object-name.h\"\n> -#include \"oidmap.h\"\n> -#include \"repository.h\"\n> -#include \"setup.h\"\n> -#include \"strbuf.h\"\n> -#include \"string-list.h\"\n> -\n> -/* key is an oid and value is a name (could be a refname for example) */\n> -struct test_entry {\n> -\tstruct oidmap_entry entry;\n> -\tchar name[FLEX_ARRAY];\n> -};\n> -\n> -#define DELIM \" \\t\\r\\n\"\n> -\n> -/*\n> - * Read stdin line by line and print result of commands to stdout:\n> - *\n> - * hash oidkey -> sha1hash(oidkey)\n> - * put oidkey namevalue -> NULL / old namevalue\n> - * get oidkey -> NULL / namevalue\n> - * remove oidkey -> NULL / old namevalue\n> - * iterate -> oidkey1 namevalue1\\noidkey2 namevalue2\\n...\n> - *\n> - */\n> -int cmd__oidmap(int argc UNUSED, const char **argv UNUSED)\n> -{\n> -\tstruct string_list parts = STRING_LIST_INIT_NODUP;\n> -\tstruct strbuf line = STRBUF_INIT;\n> -\tstruct oidmap map = OIDMAP_INIT;\n> -\n> -\tsetup_git_directory();\n> -\n> -\t/* init oidmap */\n> -\toidmap_init(&map, 0);\n> -\n> -\t/* process commands from stdin */\n> -\twhile (strbuf_getline(&line, stdin) != EOF) {\n> -\t\tchar *cmd, *p1, *p2;\n> -\t\tstruct test_entry *entry;\n> -\t\tstruct object_id oid;\n> -\n> -\t\t/* break line into command and up to two parameters */\n> -\t\tstring_list_setlen(&parts, 0);\n> -\t\tstring_list_split_in_place(&parts, line.buf, DELIM, 2);\n> -\t\tstring_list_remove_empty_items(&parts, 0);\n> -\n> -\t\t/* ignore empty lines */\n> -\t\tif (!parts.nr)\n> -\t\t\tcontinue;\n> -\t\tif (!*parts.items[0].string || *parts.items[0].string == '#')\n> -\t\t\tcontinue;\n> -\n> -\t\tcmd = parts.items[0].string;\n> -\t\tp1 = parts.nr >= 1 ? parts.items[1].string : NULL;\n> -\t\tp2 = parts.nr >= 2 ? parts.items[2].string : NULL;\n> -\n> -\t\tif (!strcmp(\"put\", cmd) && p1 && p2) {\n> -\n> -\t\t\tif (repo_get_oid(the_repository, p1, &oid)) {\n> -\t\t\t\tprintf(\"Unknown oid: %s\\n\", p1);\n> -\t\t\t\tcontinue;\n> -\t\t\t}\n> -\n> -\t\t\t/* create entry with oid_key = p1, name_value = p2 */\n> -\t\t\tFLEX_ALLOC_STR(entry, name, p2);\n> -\t\t\toidcpy(&entry->entry.oid, &oid);\n> -\n> -\t\t\t/* add / replace entry */\n> -\t\t\tentry = oidmap_put(&map, entry);\n> -\n> -\t\t\t/* print and free replaced entry, if any */\n> -\t\t\tputs(entry ? entry->name : \"NULL\");\n> -\t\t\tfree(entry);\n> -\n> -\t\t} else if (!strcmp(\"get\", cmd) && p1) {\n> -\n> -\t\t\tif (repo_get_oid(the_repository, p1, &oid)) {\n> -\t\t\t\tprintf(\"Unknown oid: %s\\n\", p1);\n> -\t\t\t\tcontinue;\n> -\t\t\t}\n> -\n> -\t\t\t/* lookup entry in oidmap */\n> -\t\t\tentry = oidmap_get(&map, &oid);\n> -\n> -\t\t\t/* print result */\n> -\t\t\tputs(entry ? entry->name : \"NULL\");\n> -\n> -\t\t} else if (!strcmp(\"remove\", cmd) && p1) {\n> -\n> -\t\t\tif (repo_get_oid(the_repository, p1, &oid)) {\n> -\t\t\t\tprintf(\"Unknown oid: %s\\n\", p1);\n> -\t\t\t\tcontinue;\n> -\t\t\t}\n> -\n> -\t\t\t/* remove entry from oidmap */\n> -\t\t\tentry = oidmap_remove(&map, &oid);\n> -\n> -\t\t\t/* print result and free entry*/\n> -\t\t\tputs(entry ? entry->name : \"NULL\");\n> -\t\t\tfree(entry);\n> -\n> -\t\t} else if (!strcmp(\"iterate\", cmd)) {\n> -\n> -\t\t\tstruct oidmap_iter iter;\n> -\t\t\toidmap_iter_init(&map, &iter);\n> -\t\t\twhile ((entry = oidmap_iter_next(&iter)))\n> -\t\t\t\tprintf(\"%s %s\\n\", oid_to_hex(&entry->entry.oid), entry->name);\n> -\n> -\t\t} else {\n> -\n> -\t\t\tprintf(\"Unknown command %s\\n\", cmd);\n> -\n> -\t\t}\n> -\t}\n> -\n> -\tstring_list_clear(&parts, 0);\n> -\tstrbuf_release(&line);\n> -\toidmap_free(&map, 1);\n> -\treturn 0;\n> -}\n> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\n> index 253324a06b..5f013d8b2b 100644\n> --- a/t/helper/test-tool.c\n> +++ b/t/helper/test-tool.c\n> @@ -45,7 +45,6 @@ static struct test_cmd cmds[] = {\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 },\n> \t{ \"parse-options\", cmd__parse_options },\n> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\n> index 460dd7d260..c7d3e43694 100644\n> --- a/t/helper/test-tool.h\n> +++ b/t/helper/test-tool.h\n> @@ -38,7 +38,6 @@ int cmd__lazy_init_name_hash(int argc, const char **argv);\n> int cmd__match_trees(int argc, const char **argv);\n> int cmd__mergesort(int argc, const char **argv);\n> int cmd__mktemp(int argc, const char **argv);\n> -int cmd__oidmap(int argc, const char **argv);\n> int cmd__online_cpus(int argc, const char **argv);\n> int cmd__pack_mtimes(int argc, const char **argv);\n> int cmd__parse_options(int argc, const char **argv);\n> diff --git a/t/t0016-oidmap.sh b/t/t0016-oidmap.sh\n> deleted file mode 100755\n> index 0faef1f4f1..0000000000\n> --- a/t/t0016-oidmap.sh\n> +++ /dev/null\n> @@ -1,112 +0,0 @@\n> -#!/bin/sh\n> -\n> -test_description='test oidmap'\n> -\n> -TEST_PASSES_SANITIZE_LEAK=true\n> -. ./test-lib.sh\n> -\n> -# This purposefully is very similar to t0011-hashmap.sh\n> -\n> -test_oidmap () {\n> -\techo \"$1\" | test-tool oidmap $3 >actual &&\n> -\techo \"$2\" >expect &&\n> -\ttest_cmp expect actual\n> -}\n> -\n> -\n> -test_expect_success 'setup' '\n> -\n> -\ttest_commit one &&\n> -\ttest_commit two &&\n> -\ttest_commit three &&\n> -\ttest_commit four\n> -\n> -'\n> -\n> -test_expect_success 'put' '\n> -\n> -test_oidmap \"put one 1\n> -put two 2\n> -put invalidOid 4\n> -put three 3\" \"NULL\n> -NULL\n> -Unknown oid: invalidOid\n> -NULL\"\n> -\n> -'\n> -\n> -test_expect_success 'replace' '\n> -\n> -test_oidmap \"put one 1\n> -put two 2\n> -put three 3\n> -put invalidOid 4\n> -put two deux\n> -put one un\" \"NULL\n> -NULL\n> -NULL\n> -Unknown oid: invalidOid\n> -2\n> -1\"\n> -\n> -'\n> -\n> -test_expect_success 'get' '\n> -\n> -test_oidmap \"put one 1\n> -put two 2\n> -put three 3\n> -get two\n> -get four\n> -get invalidOid\n> -get one\" \"NULL\n> -NULL\n> -NULL\n> -2\n> -NULL\n> -Unknown oid: invalidOid\n> -1\"\n> -\n> -'\n> -\n> -test_expect_success 'remove' '\n> -\n> -test_oidmap \"put one 1\n> -put two 2\n> -put three 3\n> -remove one\n> -remove two\n> -remove invalidOid\n> -remove four\" \"NULL\n> -NULL\n> -NULL\n> -1\n> -2\n> -Unknown oid: invalidOid\n> -NULL\"\n> -\n> -'\n> -\n> -test_expect_success 'iterate' '\n> -\ttest-tool oidmap >actual.raw <<-\\EOF &&\n> -\tput one 1\n> -\tput two 2\n> -\tput three 3\n> -\titerate\n> -\tEOF\n> -\n> -\t# sort \"expect\" too so we do not rely on the order of particular oids\n> -\tsort >expect <<-EOF &&\n> -\tNULL\n> -\tNULL\n> -\tNULL\n> -\t$(git rev-parse one) 1\n> -\t$(git rev-parse two) 2\n> -\t$(git rev-parse three) 3\n> -\tEOF\n> -\n> -\tsort actual &&\n> -\ttest_cmp expect actual\n> -'\n> -\n> -test_done\n> diff --git a/t/unit-tests/t-oidmap.c b/t/unit-tests/t-oidmap.c\n> new file mode 100644\n> index 0000000000..9b98a3ed09\n> --- /dev/null\n> +++ b/t/unit-tests/t-oidmap.c\n> @@ -0,0 +1,165 @@\n> +#include \"test-lib.h\"\n> +#include \"lib-oid.h\"\n> +#include \"oidmap.h\"\n> +#include \"hash.h\"\n> +#include \"hex.h\"\n> +\n> +/*\n> + * elements we will put in oidmap structs are made of a key: the entry.oid\n> + * field, which is of type struct object_id, and a value: the name field (could\n> + * be a refname for example)\n> + */\n> +struct test_entry {\n> +\tstruct oidmap_entry entry;\n> +\tchar name[FLEX_ARRAY];\n> +};\n> +\n> +static const char *key_val[][2] = { { \"11\", \"one\" },\n> +\t\t\t\t { \"22\", \"two\" },\n> +\t\t\t\t { \"33\", \"three\" } };\n> +\n> +static int put_and_check_null(struct oidmap *map, const char *hex,\n> +\t\t\t const char *entry_name)\n> +{\n> +\tstruct test_entry *entry;\n> +\n> +\tFLEX_ALLOC_STR(entry, name, entry_name);\n> +\tif (get_oid_arbitrary_hex(hex, &entry->entry.oid))\n> +\t\treturn -1;\n> +\tif (!check(oidmap_put(map, entry) == NULL))\n> +\t\treturn -1;\n> +\treturn 0;\n> +}\n> +\n> +static void setup(void (*f)(struct oidmap *map))\n> +{\n> +\tstruct oidmap map = OIDMAP_INIT;\n> +\tint ret = 0;\n> +\n> +\tfor (size_t i = 0; i < ARRAY_SIZE(key_val); i++)\n> +\t\tif ((ret = put_and_check_null(&map, key_val[i][0],\n> +\t\t\t\t\t key_val[i][1])))\n> +\t\t\tbreak;\n> +\n> +\tif (!ret)\n> +\t\tf(&map);\n> +\toidmap_free(&map, 1);\n> +}\n> +\n> +static void t_replace(struct oidmap *map)\n> +{\n> +\tstruct test_entry *entry, *prev;\n> +\n> +\tFLEX_ALLOC_STR(entry, name, \"un\");\n> +\tif (get_oid_arbitrary_hex(\"11\", &entry->entry.oid))\n> +\t\treturn;\n> +\tprev = oidmap_put(map, entry);\n> +\tif (!check(prev != NULL))\n> +\t\treturn;\n> +\tcheck_str(prev->name, \"one\");\n> +\tfree(prev);\n> +\n> +\tFLEX_ALLOC_STR(entry, name, \"deux\");\n> +\tif (get_oid_arbitrary_hex(\"22\", &entry->entry.oid))\n> +\t\treturn;\n> +\tprev = oidmap_put(map, entry);\n> +\tif (!check(prev != NULL))\n> +\t\treturn;\n> +\tcheck_str(prev->name, \"two\");\n> +\tfree(prev);\n> +}\n> +\n> +static void t_get(struct oidmap *map)\n> +{\n> +\tstruct test_entry *entry;\n> +\tstruct object_id oid;\n> +\n> +\tif (get_oid_arbitrary_hex(\"22\", &oid))\n> +\t\treturn;\n> +\tentry = oidmap_get(map, &oid);\n> +\tif (!check(entry != NULL))\n> +\t\treturn;\n> +\tcheck_str(entry->name, \"two\");\n> +\n> +\tif (get_oid_arbitrary_hex(\"44\", &oid))\n> +\t\treturn;\n> +\tcheck(oidmap_get(map, &oid) == NULL);\n> +\n> +\tif (get_oid_arbitrary_hex(\"11\", &oid))\n> +\t\treturn;\n> +\tentry = oidmap_get(map, &oid);\n> +\tif (!check(entry != NULL))\n> +\t\treturn;\n> +\tcheck_str(entry->name, \"one\");\n> +}\n> +\n> +static void t_remove(struct oidmap *map)\n> +{\n> +\tstruct test_entry *entry;\n> +\tstruct object_id oid;\n> +\n> +\tif (get_oid_arbitrary_hex(\"11\", &oid))\n> +\t\treturn;\n> +\tentry = oidmap_remove(map, &oid);\n> +\tif (!check(entry != NULL))\n> +\t\treturn;\n> +\tcheck_str(entry->name, \"one\");\n> +\tcheck(oidmap_get(map, &oid) == NULL);\n> +\tfree(entry);\n> +\n> +\tif (get_oid_arbitrary_hex(\"22\", &oid))\n> +\t\treturn;\n> +\tentry = oidmap_remove(map, &oid);\n> +\tif (!check(entry != NULL))\n> +\t\treturn;\n> +\tcheck_str(entry->name, \"two\");\n> +\tcheck(oidmap_get(map, &oid) == NULL);\n> +\tfree(entry);\n> +\n> +\tif (get_oid_arbitrary_hex(\"44\", &oid))\n> +\t\treturn;\n> +\tcheck(oidmap_remove(map, &oid) == NULL);\n> +}\n> +\n> +static int key_val_contains(struct test_entry *entry)\n> +{\n> +\t/* the test is small enough to be able to bear O(n) */\n> +\tfor (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {\n> +\t\tif (!strcmp(key_val[i][1], entry->name)) {\n> +\t\t\tstruct object_id oid;\n> +\t\t\tif (get_oid_arbitrary_hex(key_val[i][0], &oid))\n> +\t\t\t\treturn -1;\n> +\t\t\tif (oideq(&entry->entry.oid, &oid))\n> +\t\t\t\treturn 0;\n> +\t\t}\n> +\t}\n> +\treturn 1;\n> +}\n> +\n> +static void t_iterate(struct oidmap *map)\n> +{\n> +\tstruct oidmap_iter iter;\n> +\tstruct test_entry *entry;\n> +\tint ret;\n> +\n> +\toidmap_iter_init(map, &iter);\n> +\twhile ((entry = oidmap_iter_next(&iter))) {\n> +\t\tif (!check_int((ret = key_val_contains(entry)), ==, 0)) {\n> +\t\t\tif (ret == -1)\n> +\t\t\t\treturn;\n> +\t\t\ttest_msg(\"obtained entry was not given in the input\\n\"\n> +\t\t\t\t \" name: %s\\n oid: %s\\n\",\n> +\t\t\t\t entry->name, oid_to_hex(&entry->entry.oid));\n> +\t\t}\n> +\t}\n> +\tcheck_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val));\n> +}\n> +\n> +int cmd_main(int argc UNUSED, const char **argv UNUSED)\n> +{\n> +\tTEST(setup(t_replace), \"replace works\");\n> +\tTEST(setup(t_get), \"get works\");\n> +\tTEST(setup(t_remove), \"remove works\");\n> +\tTEST(setup(t_iterate), \"iterate works\");\n> +\treturn test_done();\n> +}\n> -- \n> 2.45.2\n> \n> \n"},{"id":"497600","messageId":"D28PNKAL7263.TAZ31N4UDX5E@gmail.com","threadId":"61651","inReplyTo":"ZnP6G6SSBynlBNUj@google.com","subject":"Re: [GSoC][PATCH] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-06-25T01:35:20Z","receivedAt":"2024-06-25T01:35:27Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Jonathan Nieder wrote:\n> Ghanshyam Thakkar wrote:\n>\n> > helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h\n> > library which is built on top of hashmap.h to store arbitrary\n> > datastructure (which must contain oidmap_entry, which is a wrapper\n> > around object_id). These entries can be accessed by querying their\n> > associated object_id.\n> >\n> > Migrate them to the unit testing framework for better performance,\n> > concise code and better debugging. Along with the migration also plug\n> > memory leaks and make the test logic independent for all the tests.\n> > The migration removes 'put' tests from t0016, because it is used as\n> > setup to all the other tests, so testing it separately does not yield\n> > any benefit.\n> >\n> > Mentored-by: Christian Couder \n> > Mentored-by: Kaartic Sivaraam \n> > Signed-off-by: Ghanshyam Thakkar \n> > ---\n> > This patch is depenedent on 'gt/unit-test-oidtree' for lib-oid.\n>\n> Very neat! I'm cc-ing Josh Steadmon for unit test framework expertise.\n>\n> Patch left unsnipped for reference.\n\nFriendly reminder for reviews/acks. :)\n\nThanks.\n\n> > Makefile | 2 +-\n> > t/helper/test-oidmap.c | 123 ------------------------------\n> > t/helper/test-tool.c | 1 -\n> > t/helper/test-tool.h | 1 -\n> > t/t0016-oidmap.sh | 112 ---------------------------\n> > t/unit-tests/t-oidmap.c | 165 ++++++++++++++++++++++++++++++++++++++++\n> > 6 files changed, 166 insertions(+), 238 deletions(-)\n> > delete mode 100644 t/helper/test-oidmap.c\n> > delete mode 100755 t/t0016-oidmap.sh\n> > create mode 100644 t/unit-tests/t-oidmap.c\n> > \n> > diff --git a/Makefile b/Makefile\n> > index 03751e0fc0..f7ed50f3a9 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -810,7 +810,6 @@ 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> > TEST_BUILTINS_OBJS += test-parse-options.o\n> > @@ -1334,6 +1333,7 @@ THIRD_PARTY_SOURCES += sha1dc/%\n> > \n> > UNIT_TEST_PROGRAMS += t-ctype\n> > UNIT_TEST_PROGRAMS += t-mem-pool\n> > +UNIT_TEST_PROGRAMS += t-oidmap\n> > UNIT_TEST_PROGRAMS += t-oidtree\n> > UNIT_TEST_PROGRAMS += t-prio-queue\n> > UNIT_TEST_PROGRAMS += t-strbuf\n> > diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c\n> > deleted file mode 100644\n> > index bd30244a54..0000000000\n> > --- a/t/helper/test-oidmap.c\n> > +++ /dev/null\n> > @@ -1,123 +0,0 @@\n> > -#include \"test-tool.h\"\n> > -#include \"hex.h\"\n> > -#include \"object-name.h\"\n> > -#include \"oidmap.h\"\n> > -#include \"repository.h\"\n> > -#include \"setup.h\"\n> > -#include \"strbuf.h\"\n> > -#include \"string-list.h\"\n> > -\n> > -/* key is an oid and value is a name (could be a refname for example) */\n> > -struct test_entry {\n> > -\tstruct oidmap_entry entry;\n> > -\tchar name[FLEX_ARRAY];\n> > -};\n> > -\n> > -#define DELIM \" \\t\\r\\n\"\n> > -\n> > -/*\n> > - * Read stdin line by line and print result of commands to stdout:\n> > - *\n> > - * hash oidkey -> sha1hash(oidkey)\n> > - * put oidkey namevalue -> NULL / old namevalue\n> > - * get oidkey -> NULL / namevalue\n> > - * remove oidkey -> NULL / old namevalue\n> > - * iterate -> oidkey1 namevalue1\\noidkey2 namevalue2\\n...\n> > - *\n> > - */\n> > -int cmd__oidmap(int argc UNUSED, const char **argv UNUSED)\n> > -{\n> > -\tstruct string_list parts = STRING_LIST_INIT_NODUP;\n> > -\tstruct strbuf line = STRBUF_INIT;\n> > -\tstruct oidmap map = OIDMAP_INIT;\n> > -\n> > -\tsetup_git_directory();\n> > -\n> > -\t/* init oidmap */\n> > -\toidmap_init(&map, 0);\n> > -\n> > -\t/* process commands from stdin */\n> > -\twhile (strbuf_getline(&line, stdin) != EOF) {\n> > -\t\tchar *cmd, *p1, *p2;\n> > -\t\tstruct test_entry *entry;\n> > -\t\tstruct object_id oid;\n> > -\n> > -\t\t/* break line into command and up to two parameters */\n> > -\t\tstring_list_setlen(&parts, 0);\n> > -\t\tstring_list_split_in_place(&parts, line.buf, DELIM, 2);\n> > -\t\tstring_list_remove_empty_items(&parts, 0);\n> > -\n> > -\t\t/* ignore empty lines */\n> > -\t\tif (!parts.nr)\n> > -\t\t\tcontinue;\n> > -\t\tif (!*parts.items[0].string || *parts.items[0].string == '#')\n> > -\t\t\tcontinue;\n> > -\n> > -\t\tcmd = parts.items[0].string;\n> > -\t\tp1 = parts.nr >= 1 ? parts.items[1].string : NULL;\n> > -\t\tp2 = parts.nr >= 2 ? parts.items[2].string : NULL;\n> > -\n> > -\t\tif (!strcmp(\"put\", cmd) && p1 && p2) {\n> > -\n> > -\t\t\tif (repo_get_oid(the_repository, p1, &oid)) {\n> > -\t\t\t\tprintf(\"Unknown oid: %s\\n\", p1);\n> > -\t\t\t\tcontinue;\n> > -\t\t\t}\n> > -\n> > -\t\t\t/* create entry with oid_key = p1, name_value = p2 */\n> > -\t\t\tFLEX_ALLOC_STR(entry, name, p2);\n> > -\t\t\toidcpy(&entry->entry.oid, &oid);\n> > -\n> > -\t\t\t/* add / replace entry */\n> > -\t\t\tentry = oidmap_put(&map, entry);\n> > -\n> > -\t\t\t/* print and free replaced entry, if any */\n> > -\t\t\tputs(entry ? entry->name : \"NULL\");\n> > -\t\t\tfree(entry);\n> > -\n> > -\t\t} else if (!strcmp(\"get\", cmd) && p1) {\n> > -\n> > -\t\t\tif (repo_get_oid(the_repository, p1, &oid)) {\n> > -\t\t\t\tprintf(\"Unknown oid: %s\\n\", p1);\n> > -\t\t\t\tcontinue;\n> > -\t\t\t}\n> > -\n> > -\t\t\t/* lookup entry in oidmap */\n> > -\t\t\tentry = oidmap_get(&map, &oid);\n> > -\n> > -\t\t\t/* print result */\n> > -\t\t\tputs(entry ? entry->name : \"NULL\");\n> > -\n> > -\t\t} else if (!strcmp(\"remove\", cmd) && p1) {\n> > -\n> > -\t\t\tif (repo_get_oid(the_repository, p1, &oid)) {\n> > -\t\t\t\tprintf(\"Unknown oid: %s\\n\", p1);\n> > -\t\t\t\tcontinue;\n> > -\t\t\t}\n> > -\n> > -\t\t\t/* remove entry from oidmap */\n> > -\t\t\tentry = oidmap_remove(&map, &oid);\n> > -\n> > -\t\t\t/* print result and free entry*/\n> > -\t\t\tputs(entry ? entry->name : \"NULL\");\n> > -\t\t\tfree(entry);\n> > -\n> > -\t\t} else if (!strcmp(\"iterate\", cmd)) {\n> > -\n> > -\t\t\tstruct oidmap_iter iter;\n> > -\t\t\toidmap_iter_init(&map, &iter);\n> > -\t\t\twhile ((entry = oidmap_iter_next(&iter)))\n> > -\t\t\t\tprintf(\"%s %s\\n\", oid_to_hex(&entry->entry.oid), entry->name);\n> > -\n> > -\t\t} else {\n> > -\n> > -\t\t\tprintf(\"Unknown command %s\\n\", cmd);\n> > -\n> > -\t\t}\n> > -\t}\n> > -\n> > -\tstring_list_clear(&parts, 0);\n> > -\tstrbuf_release(&line);\n> > -\toidmap_free(&map, 1);\n> > -\treturn 0;\n> > -}\n> > diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\n> > index 253324a06b..5f013d8b2b 100644\n> > --- a/t/helper/test-tool.c\n> > +++ b/t/helper/test-tool.c\n> > @@ -45,7 +45,6 @@ static struct test_cmd cmds[] = {\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 },\n> > \t{ \"parse-options\", cmd__parse_options },\n> > diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\n> > index 460dd7d260..c7d3e43694 100644\n> > --- a/t/helper/test-tool.h\n> > +++ b/t/helper/test-tool.h\n> > @@ -38,7 +38,6 @@ int cmd__lazy_init_name_hash(int argc, const char **argv);\n> > int cmd__match_trees(int argc, const char **argv);\n> > int cmd__mergesort(int argc, const char **argv);\n> > int cmd__mktemp(int argc, const char **argv);\n> > -int cmd__oidmap(int argc, const char **argv);\n> > int cmd__online_cpus(int argc, const char **argv);\n> > int cmd__pack_mtimes(int argc, const char **argv);\n> > int cmd__parse_options(int argc, const char **argv);\n> > diff --git a/t/t0016-oidmap.sh b/t/t0016-oidmap.sh\n> > deleted file mode 100755\n> > index 0faef1f4f1..0000000000\n> > --- a/t/t0016-oidmap.sh\n> > +++ /dev/null\n> > @@ -1,112 +0,0 @@\n> > -#!/bin/sh\n> > -\n> > -test_description='test oidmap'\n> > -\n> > -TEST_PASSES_SANITIZE_LEAK=true\n> > -. ./test-lib.sh\n> > -\n> > -# This purposefully is very similar to t0011-hashmap.sh\n> > -\n> > -test_oidmap () {\n> > -\techo \"$1\" | test-tool oidmap $3 >actual &&\n> > -\techo \"$2\" >expect &&\n> > -\ttest_cmp expect actual\n> > -}\n> > -\n> > -\n> > -test_expect_success 'setup' '\n> > -\n> > -\ttest_commit one &&\n> > -\ttest_commit two &&\n> > -\ttest_commit three &&\n> > -\ttest_commit four\n> > -\n> > -'\n> > -\n> > -test_expect_success 'put' '\n> > -\n> > -test_oidmap \"put one 1\n> > -put two 2\n> > -put invalidOid 4\n> > -put three 3\" \"NULL\n> > -NULL\n> > -Unknown oid: invalidOid\n> > -NULL\"\n> > -\n> > -'\n> > -\n> > -test_expect_success 'replace' '\n> > -\n> > -test_oidmap \"put one 1\n> > -put two 2\n> > -put three 3\n> > -put invalidOid 4\n> > -put two deux\n> > -put one un\" \"NULL\n> > -NULL\n> > -NULL\n> > -Unknown oid: invalidOid\n> > -2\n> > -1\"\n> > -\n> > -'\n> > -\n> > -test_expect_success 'get' '\n> > -\n> > -test_oidmap \"put one 1\n> > -put two 2\n> > -put three 3\n> > -get two\n> > -get four\n> > -get invalidOid\n> > -get one\" \"NULL\n> > -NULL\n> > -NULL\n> > -2\n> > -NULL\n> > -Unknown oid: invalidOid\n> > -1\"\n> > -\n> > -'\n> > -\n> > -test_expect_success 'remove' '\n> > -\n> > -test_oidmap \"put one 1\n> > -put two 2\n> > -put three 3\n> > -remove one\n> > -remove two\n> > -remove invalidOid\n> > -remove four\" \"NULL\n> > -NULL\n> > -NULL\n> > -1\n> > -2\n> > -Unknown oid: invalidOid\n> > -NULL\"\n> > -\n> > -'\n> > -\n> > -test_expect_success 'iterate' '\n> > -\ttest-tool oidmap >actual.raw <<-\\EOF &&\n> > -\tput one 1\n> > -\tput two 2\n> > -\tput three 3\n> > -\titerate\n> > -\tEOF\n> > -\n> > -\t# sort \"expect\" too so we do not rely on the order of particular oids\n> > -\tsort >expect <<-EOF &&\n> > -\tNULL\n> > -\tNULL\n> > -\tNULL\n> > -\t$(git rev-parse one) 1\n> > -\t$(git rev-parse two) 2\n> > -\t$(git rev-parse three) 3\n> > -\tEOF\n> > -\n> > -\tsort actual &&\n> > -\ttest_cmp expect actual\n> > -'\n> > -\n> > -test_done\n> > diff --git a/t/unit-tests/t-oidmap.c b/t/unit-tests/t-oidmap.c\n> > new file mode 100644\n> > index 0000000000..9b98a3ed09\n> > --- /dev/null\n> > +++ b/t/unit-tests/t-oidmap.c\n> > @@ -0,0 +1,165 @@\n> > +#include \"test-lib.h\"\n> > +#include \"lib-oid.h\"\n> > +#include \"oidmap.h\"\n> > +#include \"hash.h\"\n> > +#include \"hex.h\"\n> > +\n> > +/*\n> > + * elements we will put in oidmap structs are made of a key: the entry.oid\n> > + * field, which is of type struct object_id, and a value: the name field (could\n> > + * be a refname for example)\n> > + */\n> > +struct test_entry {\n> > +\tstruct oidmap_entry entry;\n> > +\tchar name[FLEX_ARRAY];\n> > +};\n> > +\n> > +static const char *key_val[][2] = { { \"11\", \"one\" },\n> > +\t\t\t\t { \"22\", \"two\" },\n> > +\t\t\t\t { \"33\", \"three\" } };\n> > +\n> > +static int put_and_check_null(struct oidmap *map, const char *hex,\n> > +\t\t\t const char *entry_name)\n> > +{\n> > +\tstruct test_entry *entry;\n> > +\n> > +\tFLEX_ALLOC_STR(entry, name, entry_name);\n> > +\tif (get_oid_arbitrary_hex(hex, &entry->entry.oid))\n> > +\t\treturn -1;\n> > +\tif (!check(oidmap_put(map, entry) == NULL))\n> > +\t\treturn -1;\n> > +\treturn 0;\n> > +}\n> > +\n> > +static void setup(void (*f)(struct oidmap *map))\n> > +{\n> > +\tstruct oidmap map = OIDMAP_INIT;\n> > +\tint ret = 0;\n> > +\n> > +\tfor (size_t i = 0; i < ARRAY_SIZE(key_val); i++)\n> > +\t\tif ((ret = put_and_check_null(&map, key_val[i][0],\n> > +\t\t\t\t\t key_val[i][1])))\n> > +\t\t\tbreak;\n> > +\n> > +\tif (!ret)\n> > +\t\tf(&map);\n> > +\toidmap_free(&map, 1);\n> > +}\n> > +\n> > +static void t_replace(struct oidmap *map)\n> > +{\n> > +\tstruct test_entry *entry, *prev;\n> > +\n> > +\tFLEX_ALLOC_STR(entry, name, \"un\");\n> > +\tif (get_oid_arbitrary_hex(\"11\", &entry->entry.oid))\n> > +\t\treturn;\n> > +\tprev = oidmap_put(map, entry);\n> > +\tif (!check(prev != NULL))\n> > +\t\treturn;\n> > +\tcheck_str(prev->name, \"one\");\n> > +\tfree(prev);\n> > +\n> > +\tFLEX_ALLOC_STR(entry, name, \"deux\");\n> > +\tif (get_oid_arbitrary_hex(\"22\", &entry->entry.oid))\n> > +\t\treturn;\n> > +\tprev = oidmap_put(map, entry);\n> > +\tif (!check(prev != NULL))\n> > +\t\treturn;\n> > +\tcheck_str(prev->name, \"two\");\n> > +\tfree(prev);\n> > +}\n> > +\n> > +static void t_get(struct oidmap *map)\n> > +{\n> > +\tstruct test_entry *entry;\n> > +\tstruct object_id oid;\n> > +\n> > +\tif (get_oid_arbitrary_hex(\"22\", &oid))\n> > +\t\treturn;\n> > +\tentry = oidmap_get(map, &oid);\n> > +\tif (!check(entry != NULL))\n> > +\t\treturn;\n> > +\tcheck_str(entry->name, \"two\");\n> > +\n> > +\tif (get_oid_arbitrary_hex(\"44\", &oid))\n> > +\t\treturn;\n> > +\tcheck(oidmap_get(map, &oid) == NULL);\n> > +\n> > +\tif (get_oid_arbitrary_hex(\"11\", &oid))\n> > +\t\treturn;\n> > +\tentry = oidmap_get(map, &oid);\n> > +\tif (!check(entry != NULL))\n> > +\t\treturn;\n> > +\tcheck_str(entry->name, \"one\");\n> > +}\n> > +\n> > +static void t_remove(struct oidmap *map)\n> > +{\n> > +\tstruct test_entry *entry;\n> > +\tstruct object_id oid;\n> > +\n> > +\tif (get_oid_arbitrary_hex(\"11\", &oid))\n> > +\t\treturn;\n> > +\tentry = oidmap_remove(map, &oid);\n> > +\tif (!check(entry != NULL))\n> > +\t\treturn;\n> > +\tcheck_str(entry->name, \"one\");\n> > +\tcheck(oidmap_get(map, &oid) == NULL);\n> > +\tfree(entry);\n> > +\n> > +\tif (get_oid_arbitrary_hex(\"22\", &oid))\n> > +\t\treturn;\n> > +\tentry = oidmap_remove(map, &oid);\n> > +\tif (!check(entry != NULL))\n> > +\t\treturn;\n> > +\tcheck_str(entry->name, \"two\");\n> > +\tcheck(oidmap_get(map, &oid) == NULL);\n> > +\tfree(entry);\n> > +\n> > +\tif (get_oid_arbitrary_hex(\"44\", &oid))\n> > +\t\treturn;\n> > +\tcheck(oidmap_remove(map, &oid) == NULL);\n> > +}\n> > +\n> > +static int key_val_contains(struct test_entry *entry)\n> > +{\n> > +\t/* the test is small enough to be able to bear O(n) */\n> > +\tfor (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {\n> > +\t\tif (!strcmp(key_val[i][1], entry->name)) {\n> > +\t\t\tstruct object_id oid;\n> > +\t\t\tif (get_oid_arbitrary_hex(key_val[i][0], &oid))\n> > +\t\t\t\treturn -1;\n> > +\t\t\tif (oideq(&entry->entry.oid, &oid))\n> > +\t\t\t\treturn 0;\n> > +\t\t}\n> > +\t}\n> > +\treturn 1;\n> > +}\n> > +\n> > +static void t_iterate(struct oidmap *map)\n> > +{\n> > +\tstruct oidmap_iter iter;\n> > +\tstruct test_entry *entry;\n> > +\tint ret;\n> > +\n> > +\toidmap_iter_init(map, &iter);\n> > +\twhile ((entry = oidmap_iter_next(&iter))) {\n> > +\t\tif (!check_int((ret = key_val_contains(entry)), ==, 0)) {\n> > +\t\t\tif (ret == -1)\n> > +\t\t\t\treturn;\n> > +\t\t\ttest_msg(\"obtained entry was not given in the input\\n\"\n> > +\t\t\t\t \" name: %s\\n oid: %s\\n\",\n> > +\t\t\t\t entry->name, oid_to_hex(&entry->entry.oid));\n> > +\t\t}\n> > +\t}\n> > +\tcheck_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val));\n> > +}\n> > +\n> > +int cmd_main(int argc UNUSED, const char **argv UNUSED)\n> > +{\n> > +\tTEST(setup(t_replace), \"replace works\");\n> > +\tTEST(setup(t_get), \"get works\");\n> > +\tTEST(setup(t_remove), \"remove works\");\n> > +\tTEST(setup(t_iterate), \"iterate works\");\n> > +\treturn test_done();\n> > +}\n> > -- \n> > 2.45.2\n> > \n> > \n\n"},{"id":"497615","messageId":"827f6cea-2367-403f-ba8b-055c9c8a7259@gmail.com","threadId":"61651","inReplyTo":"ZnP6G6SSBynlBNUj@google.com","subject":"Re: [GSoC][PATCH] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-25T10:14:37Z","receivedAt":"2024-06-25T10:14:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ghanshyam\n\nOn 20/06/2024 10:45, Jonathan Nieder wrote:\n> Ghanshyam Thakkar wrote:\n> \n>> helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h\n>> library which is built on top of hashmap.h to store arbitrary\n>> datastructure (which must contain oidmap_entry, which is a wrapper\n>> around object_id).\n\nI'm not really sure what the sentence is trying to say. I think it would \nbe helpful to start the commit message with an introductory sentence \nexplaining that the oidmap is currently tested via `test-tool` and this \ncommit converts those tests to unit tests.\n\n These entries can be accessed by querying their\n>> associated object_id.\n>>\n>> Migrate them to the unit testing framework for better performance,\n>> concise code and better debugging. Along with the migration also plug\n>> memory leaks and make the test logic independent for all the tests.\n\n>> The migration removes 'put' tests from t0016, because it is used as\n>> setup to all the other tests, so testing it separately does not yield\n>> any benefit.\n\nThanks sounds sensible, thanks for explaining it in the commit message.\n\nOverall the patch looks pretty good, I've left a couple of comments below.\n\n>> Mentored-by: Christian Couder \n>> Mentored-by: Kaartic Sivaraam \n>> Signed-off-by: Ghanshyam Thakkar \n>> ---\n\n>> diff --git a/t/unit-tests/t-oidmap.c b/t/unit-tests/t-oidmap.c\n>> new file mode 100644\n>> index 0000000000..9b98a3ed09\n>> --- /dev/null\n>> +++ b/t/unit-tests/t-oidmap.c\n>> @@ -0,0 +1,165 @@\n>> +#include \"test-lib.h\"\n>> +#include \"lib-oid.h\"\n>> +#include \"oidmap.h\"\n>> +#include \"hash.h\"\n>> +#include \"hex.h\"\n>> +\n>> +/*\n>> + * elements we will put in oidmap structs are made of a key: the entry.oid\n>> + * field, which is of type struct object_id, and a value: the name field (could\n>> + * be a refname for example)\n>> + */\n>> +struct test_entry {\n>> +\tstruct oidmap_entry entry;\n>> +\tchar name[FLEX_ARRAY];\n>> +};\n>> +\n>> +static const char *key_val[][2] = { { \"11\", \"one\" },\n>> +\t\t\t\t { \"22\", \"two\" },\n>> +\t\t\t\t { \"33\", \"three\" } };\n>> +\n>> +static int put_and_check_null(struct oidmap *map, const char *hex,\n>> +\t\t\t const char *entry_name)\n>> +{\n>> +\tstruct test_entry *entry;\n>> +\n>> +\tFLEX_ALLOC_STR(entry, name, entry_name);\n>> +\tif (get_oid_arbitrary_hex(hex, &entry->entry.oid))\n>> +\t\treturn -1;\n\nWhen writing unit tests it is important to make sure that they fail, \nrather than just return early if there is an error. There are a number \nof places like this that return early without calling one of the check() \nmacros to make the test fail.\n\n>> +\tif (!check(oidmap_put(map, entry) == NULL))\n>> +\t\treturn -1;\n>> +\treturn 0;\n>> +}\n>> +\n>> +static void setup(void (*f)(struct oidmap *map))\n>> +{\n>> +\tstruct oidmap map = OIDMAP_INIT;\n>> +\tint ret = 0;\n>> +\n>> +\tfor (size_t i = 0; i < ARRAY_SIZE(key_val); i++)\n>> +\t\tif ((ret = put_and_check_null(&map, key_val[i][0],\n>> +\t\t\t\t\t key_val[i][1])))\n\nGiven there is only one caller I think it would be easier to see what is \ngoing on if the function body was just inlined into the loop here.\n\n>> +\t\t\tbreak;\n>> +\n>> +\tif (!ret)\n>> +\t\tf(&map);\n>> +\toidmap_free(&map, 1);\n>> +}\n\nThe tests for replace, get, remove all look like faithful translations \nof the old script and are fine apart from some missing check() calls \nwhen get_oid_arbitrary_hex() fails.\n\n>> +static int key_val_contains(struct test_entry *entry)\n>> +{\n>> +\t/* the test is small enough to be able to bear O(n) */\n\nIt is good to think about that but I'm not sure we need a comment about \nit in a small test like this.\n\n>> +\tfor (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {\n>> +\t\tif (!strcmp(key_val[i][1], entry->name)) {\n>> +\t\t\tstruct object_id oid;\n>> +\t\t\tif (get_oid_arbitrary_hex(key_val[i][0], &oid))\n>> +\t\t\t\treturn -1;\n>> +\t\t\tif (oideq(&entry->entry.oid, &oid))\n>> +\t\t\t\treturn 0;\n>> +\t\t}\n>> +\t}\n>> +\treturn 1;\n>> +}\n\nSo if we cannot construct the oid we return -1, if the oid matches we \nreturn 0 and if the oid does not match we return 1\n\n>> +static void t_iterate(struct oidmap *map)\n>> +{\n>> +\tstruct oidmap_iter iter;\n>> +\tstruct test_entry *entry;\n>> +\tint ret;\n>> +\n>> +\toidmap_iter_init(map, &iter);\n>> +\twhile ((entry = oidmap_iter_next(&iter))) {\n>> +\t\tif (!check_int((ret = key_val_contains(entry)), ==, 0)) {\n>> +\t\t\tif (ret == -1)\n>> +\t\t\t\treturn;\n>> +\t\t\ttest_msg(\"obtained entry was not given in the input\\n\"\n>> +\t\t\t\t \" name: %s\\n oid: %s\\n\",\n>> +\t\t\t\t entry->name, oid_to_hex(&entry->entry.oid));\n\nThis checks that all of the expect objects are present, but does not \ncheck for duplicate objects. An alternative would be to build an array \nof all the entries, then sort it by oid and compare that to a sorted \nversion of `key_val`. That is what the scripted version does. We don't \nhave any helpers for comparing arrays so you'd need to do that by \ncomparing each element in a loop.\n\n>> +\t\t}\n>> +\t}\n>> +\tcheck_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val));\n\nOne could argue that this helps guard against duplicate entries but \nthat's only true if we trust hashmap_get_size() so I think keeping this \nto check that hashmap_get_size() gives the correct size and changing the \nloop above would be better.\n\nThanks\n\nPhillip\n\n>> +}\n>> +\n>> +int cmd_main(int argc UNUSED, const char **argv UNUSED)\n>> +{\n>> +\tTEST(setup(t_replace), \"replace works\");\n>> +\tTEST(setup(t_get), \"get works\");\n>> +\tTEST(setup(t_remove), \"remove works\");\n>> +\tTEST(setup(t_iterate), \"iterate works\");\n>> +\treturn test_done();\n>> +}\n>> -- \n>> 2.45.2\n>>\n>>\n> \n\n"},{"id":"497656","messageId":"D29C89BS8UEJ.14F33FD8XJATD@gmail.com","threadId":"61651","inReplyTo":"827f6cea-2367-403f-ba8b-055c9c8a7259@gmail.com","subject":"Re: [GSoC][PATCH] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-06-25T19:16:46Z","receivedAt":"2024-06-25T19:16:53Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Phillip Wood wrote:\n> Hi Ghanshyam\n>\n> On 20/06/2024 10:45, Jonathan Nieder wrote:\n> > Ghanshyam Thakkar wrote:\n> > \n> >> helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h\n> >> library which is built on top of hashmap.h to store arbitrary\n> >> datastructure (which must contain oidmap_entry, which is a wrapper\n> >> around object_id).\n>\n> I'm not really sure what the sentence is trying to say. I think it would\n> be helpful to start the commit message with an introductory sentence\n> explaining that the oidmap is currently tested via `test-tool` and this\n> commit converts those tests to unit tests.\n\nGot it. Will improve. I just wanted to explain the basics of oidmap to\nhelp ease the review process.\n\n> These entries can be accessed by querying their\n> >> associated object_id.\n> >>\n> >> Migrate them to the unit testing framework for better performance,\n> >> concise code and better debugging. Along with the migration also plug\n> >> memory leaks and make the test logic independent for all the tests.\n>\n> >> The migration removes 'put' tests from t0016, because it is used as\n> >> setup to all the other tests, so testing it separately does not yield\n> >> any benefit.\n>\n> Thanks sounds sensible, thanks for explaining it in the commit message.\n>\n> Overall the patch looks pretty good, I've left a couple of comments\n> below.\n>\n> >> Mentored-by: Christian Couder \n> >> Mentored-by: Kaartic Sivaraam \n> >> Signed-off-by: Ghanshyam Thakkar \n> >> ---\n>\n> >> diff --git a/t/unit-tests/t-oidmap.c b/t/unit-tests/t-oidmap.c\n> >> new file mode 100644\n> >> index 0000000000..9b98a3ed09\n> >> --- /dev/null\n> >> +++ b/t/unit-tests/t-oidmap.c\n> >> @@ -0,0 +1,165 @@\n> >> +#include \"test-lib.h\"\n> >> +#include \"lib-oid.h\"\n> >> +#include \"oidmap.h\"\n> >> +#include \"hash.h\"\n> >> +#include \"hex.h\"\n> >> +\n> >> +/*\n> >> + * elements we will put in oidmap structs are made of a key: the entry.oid\n> >> + * field, which is of type struct object_id, and a value: the name field (could\n> >> + * be a refname for example)\n> >> + */\n> >> +struct test_entry {\n> >> +\tstruct oidmap_entry entry;\n> >> +\tchar name[FLEX_ARRAY];\n> >> +};\n> >> +\n> >> +static const char *key_val[][2] = { { \"11\", \"one\" },\n> >> +\t\t\t\t { \"22\", \"two\" },\n> >> +\t\t\t\t { \"33\", \"three\" } };\n> >> +\n> >> +static int put_and_check_null(struct oidmap *map, const char *hex,\n> >> +\t\t\t const char *entry_name)\n> >> +{\n> >> +\tstruct test_entry *entry;\n> >> +\n> >> +\tFLEX_ALLOC_STR(entry, name, entry_name);\n> >> +\tif (get_oid_arbitrary_hex(hex, &entry->entry.oid))\n> >> +\t\treturn -1;\n>\n> When writing unit tests it is important to make sure that they fail,\n> rather than just return early if there is an error. There are a number\n> of places like this that return early without calling one of the check()\n> macros to make the test fail.\n\nThey do fail. `get_oid_arbitrary_hex()` from 'unit-tests/lib-oid.h' is\na function specifically built for the use in unit tests. And it\ncontains in built `check_*` to ensure that the tests fails if something\ngoes wrong and also prints diagnostic info. Maybe we can add a check here\nas well to know the line number at which the call failed, but since we\nalready print queried hex value and other diagnostic info from\n`get_oid_arbitrary_hex()`, I thought it would be enough.\n\n> >> +\tif (!check(oidmap_put(map, entry) == NULL))\n> >> +\t\treturn -1;\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +static void setup(void (*f)(struct oidmap *map))\n> >> +{\n> >> +\tstruct oidmap map = OIDMAP_INIT;\n> >> +\tint ret = 0;\n> >> +\n> >> +\tfor (size_t i = 0; i < ARRAY_SIZE(key_val); i++)\n> >> +\t\tif ((ret = put_and_check_null(&map, key_val[i][0],\n> >> +\t\t\t\t\t key_val[i][1])))\n>\n> Given there is only one caller I think it would be easier to see what is\n> going on if the function body was just inlined into the loop here.\n\nYeah, will do.\n\n> >> +\t\t\tbreak;\n> >> +\n> >> +\tif (!ret)\n> >> +\t\tf(&map);\n> >> +\toidmap_free(&map, 1);\n> >> +}\n>\n> The tests for replace, get, remove all look like faithful translations\n> of the old script and are fine apart from some missing check() calls\n> when get_oid_arbitrary_hex() fails.\n>\n> >> +static int key_val_contains(struct test_entry *entry)\n> >> +{\n> >> +\t/* the test is small enough to be able to bear O(n) */\n>\n> It is good to think about that but I'm not sure we need a comment about\n> it in a small test like this.\n\nGot it. Will remove.\n\n> >> +\tfor (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {\n> >> +\t\tif (!strcmp(key_val[i][1], entry->name)) {\n> >> +\t\t\tstruct object_id oid;\n> >> +\t\t\tif (get_oid_arbitrary_hex(key_val[i][0], &oid))\n> >> +\t\t\t\treturn -1;\n> >> +\t\t\tif (oideq(&entry->entry.oid, &oid))\n> >> +\t\t\t\treturn 0;\n> >> +\t\t}\n> >> +\t}\n> >> +\treturn 1;\n> >> +}\n>\n> So if we cannot construct the oid we return -1, if the oid matches we\n> return 0 and if the oid does not match we return 1\n>\n> >> +static void t_iterate(struct oidmap *map)\n> >> +{\n> >> +\tstruct oidmap_iter iter;\n> >> +\tstruct test_entry *entry;\n> >> +\tint ret;\n> >> +\n> >> +\toidmap_iter_init(map, &iter);\n> >> +\twhile ((entry = oidmap_iter_next(&iter))) {\n> >> +\t\tif (!check_int((ret = key_val_contains(entry)), ==, 0)) {\n> >> +\t\t\tif (ret == -1)\n> >> +\t\t\t\treturn;\n> >> +\t\t\ttest_msg(\"obtained entry was not given in the input\\n\"\n> >> +\t\t\t\t \" name: %s\\n oid: %s\\n\",\n> >> +\t\t\t\t entry->name, oid_to_hex(&entry->entry.oid));\n>\n> This checks that all of the expect objects are present, but does not\n> check for duplicate objects. An alternative would be to build an array\n> of all the entries, then sort it by oid and compare that to a sorted\n> version of `key_val`. That is what the scripted version does. We don't\n> have any helpers for comparing arrays so you'd need to do that by\n> comparing each element in a loop.\n>\n> >> +\t\t}\n> >> +\t}\n> >> +\tcheck_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val));\n>\n> One could argue that this helps guard against duplicate entries but\n> that's only true if we trust hashmap_get_size() so I think keeping this\n> to check that hashmap_get_size() gives the correct size and changing the\n> loop above would be better.\n\nYeah, since I was not sure if hashmap's order is predictable, I first\nchecked if the entry exists and later checked if the size matches. I'll\ntry to do the array approach you mentioned.\n\nThank you for the review.\n"},{"id":"497676","messageId":"360290b2-f9eb-4a12-9832-1bb53ff455ef@gmail.com","threadId":"61651","inReplyTo":"D29C89BS8UEJ.14F33FD8XJATD@gmail.com","subject":"Re: [GSoC][PATCH] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-26T08:59:49Z","receivedAt":"2024-06-26T08:59:52Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ghanshyam\n\nOn 25/06/2024 20:16, Ghanshyam Thakkar wrote:\n> Phillip Wood wrote:\n>> When writing unit tests it is important to make sure that they fail,\n>> rather than just return early if there is an error. There are a number\n>> of places like this that return early without calling one of the check()\n>> macros to make the test fail.\n> \n> They do fail. `get_oid_arbitrary_hex()` from 'unit-tests/lib-oid.h' is\n> a function specifically built for the use in unit tests. And it\n> contains in built `check_*` to ensure that the tests fails if something\n> goes wrong and also prints diagnostic info. Maybe we can add a check here\n> as well to know the line number at which the call failed, but since we\n> already print queried hex value and other diagnostic info from\n> `get_oid_arbitrary_hex()`, I thought it would be enough.\n\nOh, sorry I didn't realize that. I agree that the check in \nget_oid_arbitary_hex() should be sufficient.\n\nBest Wishes\n\nPhillip\n"},{"id":"497803","messageId":"20240628122030.41554-1-shyamthakkar001@gmail.com","threadId":"61651","inReplyTo":"20240619175036.64291-1-shyamthakkar001@gmail.com","subject":"[GSoC][PATCH v2] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-06-28T12:20:21Z","receivedAt":"2024-06-28T12:21:09Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h\nlibrary which is built on top of hashmap.h.\n\nMigrate them to the unit testing framework for better performance,\nconcise code and better debugging. Along with the migration also plug\nmemory leaks and make the test logic independent for all the tests.\nThe migration removes 'put' tests from t0016, because it is used as\nsetup to all the other tests, so testing it separately does not yield\nany benefit.\n\nHelped-by: Phillip Wood \nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nSigned-off-by: Ghanshyam Thakkar \n---\nThis version addresses Phillip's review about detecting duplicates in\noidmap when iterating over it and removing put_and_check_null() to move\nthe relevant code to setup() instead. And contains some grammer fixes\nin the comment.\n\nRange-diff against v1:\n1: 45fa96550f ! 1: aabec4cd4d t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c\n @@ Commit message\n t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c\n \n helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h\n - library which is built on top of hashmap.h to store arbitrary\n - datastructure (which must contain oidmap_entry, which is a wrapper\n - around object_id). These entries can be accessed by querying their\n - associated object_id.\n + library which is built on top of hashmap.h.\n \n Migrate them to the unit testing framework for better performance,\n concise code and better debugging. Along with the migration also plug\n @@ Commit message\n setup to all the other tests, so testing it separately does not yield\n any benefit.\n \n + Helped-by: Phillip Wood \n Mentored-by: Christian Couder \n Mentored-by: Kaartic Sivaraam \n Signed-off-by: Ghanshyam Thakkar \n @@ t/unit-tests/t-oidmap.c (new)\n +#include \"hex.h\"\n +\n +/*\n -+ * elements we will put in oidmap structs are made of a key: the entry.oid\n ++ * Elements we will put in oidmap structs are made of a key: the entry.oid\n + * field, which is of type struct object_id, and a value: the name field (could\n -+ * be a refname for example)\n ++ * be a refname for example).\n + */\n +struct test_entry {\n +\tstruct oidmap_entry entry;\n @@ t/unit-tests/t-oidmap.c (new)\n +\t\t\t\t { \"22\", \"two\" },\n +\t\t\t\t { \"33\", \"three\" } };\n +\n -+static int put_and_check_null(struct oidmap *map, const char *hex,\n -+\t\t\t const char *entry_name)\n -+{\n -+\tstruct test_entry *entry;\n -+\n -+\tFLEX_ALLOC_STR(entry, name, entry_name);\n -+\tif (get_oid_arbitrary_hex(hex, &entry->entry.oid))\n -+\t\treturn -1;\n -+\tif (!check(oidmap_put(map, entry) == NULL))\n -+\t\treturn -1;\n -+\treturn 0;\n -+}\n -+\n +static void setup(void (*f)(struct oidmap *map))\n +{\n +\tstruct oidmap map = OIDMAP_INIT;\n +\tint ret = 0;\n +\n -+\tfor (size_t i = 0; i < ARRAY_SIZE(key_val); i++)\n -+\t\tif ((ret = put_and_check_null(&map, key_val[i][0],\n -+\t\t\t\t\t key_val[i][1])))\n ++\tfor (size_t i = 0; i < ARRAY_SIZE(key_val); i++){\n ++\t\tstruct test_entry *entry;\n ++\n ++\t\tFLEX_ALLOC_STR(entry, name, key_val[i][1]);\n ++\t\tif ((ret = get_oid_arbitrary_hex(key_val[i][0], &entry->entry.oid))) {\n ++\t\t\tfree(entry);\n +\t\t\tbreak;\n ++\t\t}\n ++\t\tentry = oidmap_put(&map, entry);\n ++\t\tif (!check(entry == NULL))\n ++\t\t\tfree(entry);\n ++\t}\n +\n +\tif (!ret)\n +\t\tf(&map);\n @@ t/unit-tests/t-oidmap.c (new)\n +\n +static int key_val_contains(struct test_entry *entry)\n +{\n -+\t/* the test is small enough to be able to bear O(n) */\n +\tfor (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {\n -+\t\tif (!strcmp(key_val[i][1], entry->name)) {\n -+\t\t\tstruct object_id oid;\n -+\t\t\tif (get_oid_arbitrary_hex(key_val[i][0], &oid))\n -+\t\t\t\treturn -1;\n -+\t\t\tif (oideq(&entry->entry.oid, &oid))\n -+\t\t\t\treturn 0;\n ++\t\tstruct object_id oid;\n ++\n ++\t\tif (get_oid_arbitrary_hex(key_val[i][0], &oid))\n ++\t\t\treturn -1;\n ++\n ++\t\tif (oideq(&entry->entry.oid, &oid)) {\n ++\t\t\tif (!strcmp(key_val[i][1], \"USED\"))\n ++\t\t\t\treturn 2;\n ++\t\t\tkey_val[i][1] = \"USED\";\n ++\t\t\treturn 0;\n +\t\t}\n +\t}\n +\treturn 1;\n @@ t/unit-tests/t-oidmap.c (new)\n +{\n +\tstruct oidmap_iter iter;\n +\tstruct test_entry *entry;\n -+\tint ret;\n +\n +\toidmap_iter_init(map, &iter);\n +\twhile ((entry = oidmap_iter_next(&iter))) {\n ++\t\tint ret;\n +\t\tif (!check_int((ret = key_val_contains(entry)), ==, 0)) {\n -+\t\t\tif (ret == -1)\n -+\t\t\t\treturn;\n -+\t\t\ttest_msg(\"obtained entry was not given in the input\\n\"\n -+\t \t\t\t \" name: %s\\n oid: %s\\n\",\n -+\t\t\t\t entry->name, oid_to_hex(&entry->entry.oid));\n ++\t\t\tswitch (ret) {\n ++\t\t\tcase -1:\n ++\t\t\t\tbreak; /* error message handled by get_oid_arbitrary_hex() */\n ++\t\t\tcase 1:\n ++\t\t\t\ttest_msg(\"obtained entry was not given in the input\\n\"\n ++\t\t\t\t\t \" name: %s\\n oid: %s\\n\",\n ++\t\t\t\t\t entry->name, oid_to_hex(&entry->entry.oid));\n ++\t\t\t\tbreak;\n ++\t\t\tcase 2:\n ++\t\t\t\ttest_msg(\"duplicate entry detected\\n\"\n ++\t\t\t\t\t \" name: %s\\n oid: %s\\n\",\n ++\t\t\t\t\t entry->name, oid_to_hex(&entry->entry.oid));\n ++\t\t\t\tbreak;\n ++\t\t\tdefault:\n ++\t\t\t\ttest_msg(\"BUG: invalid return value (%d) from key_val_contains()\",\n ++\t\t\t\t\t ret);\n ++\t\t\t\tbreak;\n ++\t\t\t}\n +\t\t}\n +\t}\n +\tcheck_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val));\n\n Makefile | 2 +-\n t/helper/test-oidmap.c | 123 ----------------------------\n t/helper/test-tool.c | 1 -\n t/helper/test-tool.h | 1 -\n t/t0016-oidmap.sh | 112 -------------------------\n t/unit-tests/t-oidmap.c | 176 ++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 177 insertions(+), 238 deletions(-)\n delete mode 100644 t/helper/test-oidmap.c\n delete mode 100755 t/t0016-oidmap.sh\n create mode 100644 t/unit-tests/t-oidmap.c\n\ndiff --git a/Makefile b/Makefile\nindex 3eab701b10..2a5c70d218 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -809,7 +809,6 @@ 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 TEST_BUILTINS_OBJS += test-parse-options.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-oidmap\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-oidmap.c b/t/helper/test-oidmap.c\ndeleted file mode 100644\nindex bd30244a54..0000000000\n--- a/t/helper/test-oidmap.c\n+++ /dev/null\n@@ -1,123 +0,0 @@\n-#include \"test-tool.h\"\n-#include \"hex.h\"\n-#include \"object-name.h\"\n-#include \"oidmap.h\"\n-#include \"repository.h\"\n-#include \"setup.h\"\n-#include \"strbuf.h\"\n-#include \"string-list.h\"\n-\n-/* key is an oid and value is a name (could be a refname for example) */\n-struct test_entry {\n-\tstruct oidmap_entry entry;\n-\tchar name[FLEX_ARRAY];\n-};\n-\n-#define DELIM \" \\t\\r\\n\"\n-\n-/*\n- * Read stdin line by line and print result of commands to stdout:\n- *\n- * hash oidkey -> sha1hash(oidkey)\n- * put oidkey namevalue -> NULL / old namevalue\n- * get oidkey -> NULL / namevalue\n- * remove oidkey -> NULL / old namevalue\n- * iterate -> oidkey1 namevalue1\\noidkey2 namevalue2\\n...\n- *\n- */\n-int cmd__oidmap(int argc UNUSED, const char **argv UNUSED)\n-{\n-\tstruct string_list parts = STRING_LIST_INIT_NODUP;\n-\tstruct strbuf line = STRBUF_INIT;\n-\tstruct oidmap map = OIDMAP_INIT;\n-\n-\tsetup_git_directory();\n-\n-\t/* init oidmap */\n-\toidmap_init(&map, 0);\n-\n-\t/* process commands from stdin */\n-\twhile (strbuf_getline(&line, stdin) != EOF) {\n-\t\tchar *cmd, *p1, *p2;\n-\t\tstruct test_entry *entry;\n-\t\tstruct object_id oid;\n-\n-\t\t/* break line into command and up to two parameters */\n-\t\tstring_list_setlen(&parts, 0);\n-\t\tstring_list_split_in_place(&parts, line.buf, DELIM, 2);\n-\t\tstring_list_remove_empty_items(&parts, 0);\n-\n-\t\t/* ignore empty lines */\n-\t\tif (!parts.nr)\n-\t\t\tcontinue;\n-\t\tif (!*parts.items[0].string || *parts.items[0].string == '#')\n-\t\t\tcontinue;\n-\n-\t\tcmd = parts.items[0].string;\n-\t\tp1 = parts.nr >= 1 ? parts.items[1].string : NULL;\n-\t\tp2 = parts.nr >= 2 ? parts.items[2].string : NULL;\n-\n-\t\tif (!strcmp(\"put\", cmd) && p1 && p2) {\n-\n-\t\t\tif (repo_get_oid(the_repository, p1, &oid)) {\n-\t\t\t\tprintf(\"Unknown oid: %s\\n\", p1);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\n-\t\t\t/* create entry with oid_key = p1, name_value = p2 */\n-\t\t\tFLEX_ALLOC_STR(entry, name, p2);\n-\t\t\toidcpy(&entry->entry.oid, &oid);\n-\n-\t\t\t/* add / replace entry */\n-\t\t\tentry = oidmap_put(&map, entry);\n-\n-\t\t\t/* print and free replaced entry, if any */\n-\t\t\tputs(entry ? entry->name : \"NULL\");\n-\t\t\tfree(entry);\n-\n-\t\t} else if (!strcmp(\"get\", cmd) && p1) {\n-\n-\t\t\tif (repo_get_oid(the_repository, p1, &oid)) {\n-\t\t\t\tprintf(\"Unknown oid: %s\\n\", p1);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\n-\t\t\t/* lookup entry in oidmap */\n-\t\t\tentry = oidmap_get(&map, &oid);\n-\n-\t\t\t/* print result */\n-\t\t\tputs(entry ? entry->name : \"NULL\");\n-\n-\t\t} else if (!strcmp(\"remove\", cmd) && p1) {\n-\n-\t\t\tif (repo_get_oid(the_repository, p1, &oid)) {\n-\t\t\t\tprintf(\"Unknown oid: %s\\n\", p1);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\n-\t\t\t/* remove entry from oidmap */\n-\t\t\tentry = oidmap_remove(&map, &oid);\n-\n-\t\t\t/* print result and free entry*/\n-\t\t\tputs(entry ? entry->name : \"NULL\");\n-\t\t\tfree(entry);\n-\n-\t\t} else if (!strcmp(\"iterate\", cmd)) {\n-\n-\t\t\tstruct oidmap_iter iter;\n-\t\t\toidmap_iter_init(&map, &iter);\n-\t\t\twhile ((entry = oidmap_iter_next(&iter)))\n-\t\t\t\tprintf(\"%s %s\\n\", oid_to_hex(&entry->entry.oid), entry->name);\n-\n-\t\t} else {\n-\n-\t\t\tprintf(\"Unknown command %s\\n\", cmd);\n-\n-\t\t}\n-\t}\n-\n-\tstring_list_clear(&parts, 0);\n-\tstrbuf_release(&line);\n-\toidmap_free(&map, 1);\n-\treturn 0;\n-}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 93436a82ae..da3e69128a 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -44,7 +44,6 @@ static struct test_cmd cmds[] = {\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 },\n \t{ \"parse-options\", cmd__parse_options },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex d9033d14e1..642a34578c 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -37,7 +37,6 @@ int cmd__lazy_init_name_hash(int argc, const char **argv);\n int cmd__match_trees(int argc, const char **argv);\n int cmd__mergesort(int argc, const char **argv);\n int cmd__mktemp(int argc, const char **argv);\n-int cmd__oidmap(int argc, const char **argv);\n int cmd__online_cpus(int argc, const char **argv);\n int cmd__pack_mtimes(int argc, const char **argv);\n int cmd__parse_options(int argc, const char **argv);\ndiff --git a/t/t0016-oidmap.sh b/t/t0016-oidmap.sh\ndeleted file mode 100755\nindex 0faef1f4f1..0000000000\n--- a/t/t0016-oidmap.sh\n+++ /dev/null\n@@ -1,112 +0,0 @@\n-#!/bin/sh\n-\n-test_description='test oidmap'\n-\n-TEST_PASSES_SANITIZE_LEAK=true\n-. ./test-lib.sh\n-\n-# This purposefully is very similar to t0011-hashmap.sh\n-\n-test_oidmap () {\n-\techo \"$1\" | test-tool oidmap $3 >actual &&\n-\techo \"$2\" >expect &&\n-\ttest_cmp expect actual\n-}\n-\n-\n-test_expect_success 'setup' '\n-\n-\ttest_commit one &&\n-\ttest_commit two &&\n-\ttest_commit three &&\n-\ttest_commit four\n-\n-'\n-\n-test_expect_success 'put' '\n-\n-test_oidmap \"put one 1\n-put two 2\n-put invalidOid 4\n-put three 3\" \"NULL\n-NULL\n-Unknown oid: invalidOid\n-NULL\"\n-\n-'\n-\n-test_expect_success 'replace' '\n-\n-test_oidmap \"put one 1\n-put two 2\n-put three 3\n-put invalidOid 4\n-put two deux\n-put one un\" \"NULL\n-NULL\n-NULL\n-Unknown oid: invalidOid\n-2\n-1\"\n-\n-'\n-\n-test_expect_success 'get' '\n-\n-test_oidmap \"put one 1\n-put two 2\n-put three 3\n-get two\n-get four\n-get invalidOid\n-get one\" \"NULL\n-NULL\n-NULL\n-2\n-NULL\n-Unknown oid: invalidOid\n-1\"\n-\n-'\n-\n-test_expect_success 'remove' '\n-\n-test_oidmap \"put one 1\n-put two 2\n-put three 3\n-remove one\n-remove two\n-remove invalidOid\n-remove four\" \"NULL\n-NULL\n-NULL\n-1\n-2\n-Unknown oid: invalidOid\n-NULL\"\n-\n-'\n-\n-test_expect_success 'iterate' '\n-\ttest-tool oidmap >actual.raw <<-\\EOF &&\n-\tput one 1\n-\tput two 2\n-\tput three 3\n-\titerate\n-\tEOF\n-\n-\t# sort \"expect\" too so we do not rely on the order of particular oids\n-\tsort >expect <<-EOF &&\n-\tNULL\n-\tNULL\n-\tNULL\n-\t$(git rev-parse one) 1\n-\t$(git rev-parse two) 2\n-\t$(git rev-parse three) 3\n-\tEOF\n-\n-\tsort actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_done\ndiff --git a/t/unit-tests/t-oidmap.c b/t/unit-tests/t-oidmap.c\nnew file mode 100644\nindex 0000000000..13532aa98b\n--- /dev/null\n+++ b/t/unit-tests/t-oidmap.c\n@@ -0,0 +1,176 @@\n+#include \"test-lib.h\"\n+#include \"lib-oid.h\"\n+#include \"oidmap.h\"\n+#include \"hash.h\"\n+#include \"hex.h\"\n+\n+/*\n+ * Elements we will put in oidmap structs are made of a key: the entry.oid\n+ * field, which is of type struct object_id, and a value: the name field (could\n+ * be a refname for example).\n+ */\n+struct test_entry {\n+\tstruct oidmap_entry entry;\n+\tchar name[FLEX_ARRAY];\n+};\n+\n+static const char *key_val[][2] = { { \"11\", \"one\" },\n+\t\t\t\t { \"22\", \"two\" },\n+\t\t\t\t { \"33\", \"three\" } };\n+\n+static void setup(void (*f)(struct oidmap *map))\n+{\n+\tstruct oidmap map = OIDMAP_INIT;\n+\tint ret = 0;\n+\n+\tfor (size_t i = 0; i < ARRAY_SIZE(key_val); i++){\n+\t\tstruct test_entry *entry;\n+\n+\t\tFLEX_ALLOC_STR(entry, name, key_val[i][1]);\n+\t\tif ((ret = get_oid_arbitrary_hex(key_val[i][0], &entry->entry.oid))) {\n+\t\t\tfree(entry);\n+\t\t\tbreak;\n+\t\t}\n+\t\tentry = oidmap_put(&map, entry);\n+\t\tif (!check(entry == NULL))\n+\t\t\tfree(entry);\n+\t}\n+\n+\tif (!ret)\n+\t\tf(&map);\n+\toidmap_free(&map, 1);\n+}\n+\n+static void t_replace(struct oidmap *map)\n+{\n+\tstruct test_entry *entry, *prev;\n+\n+\tFLEX_ALLOC_STR(entry, name, \"un\");\n+\tif (get_oid_arbitrary_hex(\"11\", &entry->entry.oid))\n+\t\treturn;\n+\tprev = oidmap_put(map, entry);\n+\tif (!check(prev != NULL))\n+\t\treturn;\n+\tcheck_str(prev->name, \"one\");\n+\tfree(prev);\n+\n+\tFLEX_ALLOC_STR(entry, name, \"deux\");\n+\tif (get_oid_arbitrary_hex(\"22\", &entry->entry.oid))\n+\t\treturn;\n+\tprev = oidmap_put(map, entry);\n+\tif (!check(prev != NULL))\n+\t\treturn;\n+\tcheck_str(prev->name, \"two\");\n+\tfree(prev);\n+}\n+\n+static void t_get(struct oidmap *map)\n+{\n+\tstruct test_entry *entry;\n+\tstruct object_id oid;\n+\n+\tif (get_oid_arbitrary_hex(\"22\", &oid))\n+\t\treturn;\n+\tentry = oidmap_get(map, &oid);\n+\tif (!check(entry != NULL))\n+\t\treturn;\n+\tcheck_str(entry->name, \"two\");\n+\n+\tif (get_oid_arbitrary_hex(\"44\", &oid))\n+\t\treturn;\n+\tcheck(oidmap_get(map, &oid) == NULL);\n+\n+\tif (get_oid_arbitrary_hex(\"11\", &oid))\n+\t\treturn;\n+\tentry = oidmap_get(map, &oid);\n+\tif (!check(entry != NULL))\n+\t\treturn;\n+\tcheck_str(entry->name, \"one\");\n+}\n+\n+static void t_remove(struct oidmap *map)\n+{\n+\tstruct test_entry *entry;\n+\tstruct object_id oid;\n+\n+\tif (get_oid_arbitrary_hex(\"11\", &oid))\n+\t\treturn;\n+\tentry = oidmap_remove(map, &oid);\n+\tif (!check(entry != NULL))\n+\t\treturn;\n+\tcheck_str(entry->name, \"one\");\n+\tcheck(oidmap_get(map, &oid) == NULL);\n+\tfree(entry);\n+\n+\tif (get_oid_arbitrary_hex(\"22\", &oid))\n+\t\treturn;\n+\tentry = oidmap_remove(map, &oid);\n+\tif (!check(entry != NULL))\n+\t\treturn;\n+\tcheck_str(entry->name, \"two\");\n+\tcheck(oidmap_get(map, &oid) == NULL);\n+\tfree(entry);\n+\n+\tif (get_oid_arbitrary_hex(\"44\", &oid))\n+\t\treturn;\n+\tcheck(oidmap_remove(map, &oid) == NULL);\n+}\n+\n+static int key_val_contains(struct test_entry *entry)\n+{\n+\tfor (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {\n+\t\tstruct object_id oid;\n+\n+\t\tif (get_oid_arbitrary_hex(key_val[i][0], &oid))\n+\t\t\treturn -1;\n+\n+\t\tif (oideq(&entry->entry.oid, &oid)) {\n+\t\t\tif (!strcmp(key_val[i][1], \"USED\"))\n+\t\t\t\treturn 2;\n+\t\t\tkey_val[i][1] = \"USED\";\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\treturn 1;\n+}\n+\n+static void t_iterate(struct oidmap *map)\n+{\n+\tstruct oidmap_iter iter;\n+\tstruct test_entry *entry;\n+\n+\toidmap_iter_init(map, &iter);\n+\twhile ((entry = oidmap_iter_next(&iter))) {\n+\t\tint ret;\n+\t\tif (!check_int((ret = key_val_contains(entry)), ==, 0)) {\n+\t\t\tswitch (ret) {\n+\t\t\tcase -1:\n+\t\t\t\tbreak; /* error message handled by get_oid_arbitrary_hex() */\n+\t\t\tcase 1:\n+\t\t\t\ttest_msg(\"obtained entry was not given in the input\\n\"\n+\t\t\t\t\t \" name: %s\\n oid: %s\\n\",\n+\t\t\t\t\t entry->name, oid_to_hex(&entry->entry.oid));\n+\t\t\t\tbreak;\n+\t\t\tcase 2:\n+\t\t\t\ttest_msg(\"duplicate entry detected\\n\"\n+\t\t\t\t\t \" name: %s\\n oid: %s\\n\",\n+\t\t\t\t\t entry->name, oid_to_hex(&entry->entry.oid));\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\ttest_msg(\"BUG: invalid return value (%d) from key_val_contains()\",\n+\t\t\t\t\t ret);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t}\n+\tcheck_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val));\n+}\n+\n+int cmd_main(int argc UNUSED, const char **argv UNUSED)\n+{\n+\tTEST(setup(t_replace), \"replace works\");\n+\tTEST(setup(t_get), \"get works\");\n+\tTEST(setup(t_remove), \"remove works\");\n+\tTEST(setup(t_iterate), \"iterate works\");\n+\treturn test_done();\n+}\n-- \n2.45.2\n\n"},{"id":"497931","messageId":"hxld3ldxomitv6hjuxq7munhppzie2nm3eworng6jnhf3suikx@rh6cunbz4vcz","threadId":"61651","inReplyTo":"20240628122030.41554-1-shyamthakkar001@gmail.com","subject":"Re: [GSoC][PATCH v2] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-07-01T20:32:30Z","receivedAt":"2024-07-01T20:32:36Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.06.28 17:50, Ghanshyam Thakkar wrote:\n> helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h\n> library which is built on top of hashmap.h.\n> \n> Migrate them to the unit testing framework for better performance,\n> concise code and better debugging. Along with the migration also plug\n> memory leaks and make the test logic independent for all the tests.\n> The migration removes 'put' tests from t0016, because it is used as\n> setup to all the other tests, so testing it separately does not yield\n> any benefit.\n> \n> Helped-by: Phillip Wood \n> Mentored-by: Christian Couder \n> Mentored-by: Kaartic Sivaraam \n> Signed-off-by: Ghanshyam Thakkar \n> ---\n> This version addresses Phillip's review about detecting duplicates in\n> oidmap when iterating over it and removing put_and_check_null() to move\n> the relevant code to setup() instead. And contains some grammer fixes\n> in the comment.\n\nIIUC this corrects all of the issues that Phillip noted in his earlier\nreview, except for checking for duplicates, is that right?\n\nPersonally I think this version is OK even without that check, and I'll\nbe away from email for the rest of this week, so I'll go ahead and sign\noff:\n\nReviewed-by: Josh Steadmon \n"},{"id":"497935","messageId":"xmqq4j98vmpw.fsf@gitster.g","threadId":"61651","inReplyTo":"20240628122030.41554-1-shyamthakkar001@gmail.com","subject":"Re: [GSoC][PATCH v2] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-01T21:14:35Z","receivedAt":"2024-07-01T21:14:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h\n> library which is built on top of hashmap.h.\n>\n> Migrate them to the unit testing framework for better performance,\n> concise code and better debugging. Along with the migration also plug\n> memory leaks and make the test logic independent for all the tests.\n> The migration removes 'put' tests from t0016, because it is used as\n> setup to all the other tests, so testing it separately does not yield\n> any benefit.\n>\n> Helped-by: Phillip Wood \n> Mentored-by: Christian Couder \n> Mentored-by: Kaartic Sivaraam \n> Signed-off-by: Ghanshyam Thakkar \n> ---\n> This version addresses Phillip's review about detecting duplicates in\n> oidmap when iterating over it ...\n\nHmph. You seem to overwrite key_val[i][1] ...\n\n> +/*\n> + * Elements we will put in oidmap structs are made of a key: the entry.oid\n> + * field, which is of type struct object_id, and a value: the name field (could\n> + * be a refname for example).\n> + */\n> +struct test_entry {\n> +\tstruct oidmap_entry entry;\n> +\tchar name[FLEX_ARRAY];\n> +};\n> +\n> +static const char *key_val[][2] = { { \"11\", \"one\" },\n> +\t\t\t\t { \"22\", \"two\" },\n> +\t\t\t\t { \"33\", \"three\" } };\n\n... in this test, rendering the key_val[] array unusuable for\nfurther tests. Is that intended and desirable?\n\nAs long as t_iterate() stays to be the last test, that would be OK,\nbut once somebody wants to add a new test after it, i.e.\n\n\tTEST(setup(t_iterate), \"iterate works\");\n+\tTEST(setup(t_frotz), \"frotz works\");\n\treturn test_done();\n\nthe setup() for that new test depends on the key_val[] array, whose\nvalue fields key_val[i][1] have been modified.\n\nThe TEST(setup(t_foo)) pattern is done so nicely to make sure that\neverybody is independent from everybody else, preparing the oidmap\nused for each specific test from scratch. It is a bit disappointing\nthat we are now invalidating this nice property.\n\n> +static int key_val_contains(struct test_entry *entry)\n> +{\n> +\tfor (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {\n> +\t\tstruct object_id oid;\n> +\n> +\t\tif (get_oid_arbitrary_hex(key_val[i][0], &oid))\n> +\t\t\treturn -1;\n> +\n> +\t\tif (oideq(&entry->entry.oid, &oid)) {\n> +\t\t\tif (!strcmp(key_val[i][1], \"USED\"))\n> +\t\t\t\treturn 2;\n> +\t\t\tkey_val[i][1] = \"USED\";\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t}\n> +\treturn 1;\n> +}\n\nOtherwise, the other tests looked reasonable. Looking really nice.\n\nAs I expect things to be slow this week, being a big vacation week\nin the US, we may not see much review activities this week. I'll\nqueue this version in the meantime, not merging it down to 'next',\nin case people start comment on it next week.\n\nThanks.\n\n> +int cmd_main(int argc UNUSED, const char **argv UNUSED)\n> +{\n> +\tTEST(setup(t_replace), \"replace works\");\n> +\tTEST(setup(t_get), \"get works\");\n> +\tTEST(setup(t_remove), \"remove works\");\n> +\tTEST(setup(t_iterate), \"iterate works\");\n> +\treturn test_done();\n> +}\n"},{"id":"497936","messageId":"xmqqzfr0u7x6.fsf@gitster.g","threadId":"61651","inReplyTo":"hxld3ldxomitv6hjuxq7munhppzie2nm3eworng6jnhf3suikx@rh6cunbz4vcz","subject":"Re: [GSoC][PATCH v2] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-01T21:19:33Z","receivedAt":"2024-07-01T21:19:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n>> This version addresses Phillip's review about detecting duplicates in\n>> oidmap when iterating over it and removing put_and_check_null() to move\n>> the relevant code to setup() instead. And contains some grammer fixes\n>> in the comment.\n>\n> IIUC this corrects all of the issues that Phillip noted in his earlier\n> review, except for checking for duplicates, is that right?\n\nThere is an attempted duplicate checking during iteration; the test\ndata source key_val[] array is (ab)used to record the already seen\nkeys during the iteration, which would work but is a hacky and\nunmaintainable way to do so.\n\nThanks for reviewing.\n"},{"id":"497942","messageId":"xmqqjzi4u52u.fsf@gitster.g","threadId":"61651","inReplyTo":"xmqq4j98vmpw.fsf@gitster.g","subject":"Re: [GSoC][PATCH v2] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-01T22:20:57Z","receivedAt":"2024-07-01T22:21:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Hmph. You seem to overwrite key_val[i][1] ...\n> ...\n> ... in this test, rendering the key_val[] array unusuable for\n> further tests. Is that intended and desirable?\n> ...\n> The TEST(setup(t_foo)) pattern is done so nicely to make sure that\n> everybody is independent from everybody else, preparing the oidmap\n> used for each specific test from scratch. It is a bit disappointing\n> that we are now invalidating this nice property.\n\nIt may be just the matter of doing something silly like this to\nrestore the \"different tests are independent and the source of truth\narray is intact\" property.\n\nThe first hunk should be reindented properly, if you are going to\ntake this and squash into your patch, by the way.\n\nThanks.\n\n t/unit-tests/t-oidmap.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git c/t/unit-tests/t-oidmap.c w/t/unit-tests/t-oidmap.c\nindex 13532aa98b..be2741c6c7 100644\n--- c/t/unit-tests/t-oidmap.c\n+++ w/t/unit-tests/t-oidmap.c\n@@ -14,7 +14,7 @@ struct test_entry {\n \tchar name[FLEX_ARRAY];\n };\n \n-static const char *key_val[][2] = { { \"11\", \"one\" },\n+static const char * const key_val[][2] = { { \"11\", \"one\" },\n \t\t\t\t { \"22\", \"two\" },\n \t\t\t\t { \"33\", \"three\" } };\n \n@@ -116,7 +116,7 @@ static void t_remove(struct oidmap *map)\n \tcheck(oidmap_remove(map, &oid) == NULL);\n }\n \n-static int key_val_contains(struct test_entry *entry)\n+static int key_val_contains(struct test_entry *entry, char seen[])\n {\n \tfor (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {\n \t\tstruct object_id oid;\n@@ -125,9 +125,9 @@ static int key_val_contains(struct test_entry *entry)\n \t\t\treturn -1;\n \n \t\tif (oideq(&entry->entry.oid, &oid)) {\n-\t\t\tif (!strcmp(key_val[i][1], \"USED\"))\n+\t\t\tif (seen[i])\n \t\t\t\treturn 2;\n-\t\t\tkey_val[i][1] = \"USED\";\n+\t\t\tseen[i] = 1;\n \t\t\treturn 0;\n \t\t}\n \t}\n@@ -138,11 +138,12 @@ static void t_iterate(struct oidmap *map)\n {\n \tstruct oidmap_iter iter;\n \tstruct test_entry *entry;\n+\tchar seen[ARRAY_SIZE(key_val)] = { 0 };\n \n \toidmap_iter_init(map, &iter);\n \twhile ((entry = oidmap_iter_next(&iter))) {\n \t\tint ret;\n-\t\tif (!check_int((ret = key_val_contains(entry)), ==, 0)) {\n+\t\tif (!check_int((ret = key_val_contains(entry, seen)), ==, 0)) {\n \t\t\tswitch (ret) {\n \t\t\tcase -1:\n \t\t\t\tbreak; /* error message handled by get_oid_arbitrary_hex() */\n\n\n\n"},{"id":"497953","messageId":"D2EQV4ZV3VEW.2CV0OLF3T4HVA@gmail.com","threadId":"61651","inReplyTo":"xmqqjzi4u52u.fsf@gitster.g","subject":"Re: [GSoC][PATCH v2] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-07-02T03:48:15Z","receivedAt":"2024-07-02T03:48:22Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n> > Hmph. You seem to overwrite key_val[i][1] ...\n> > ...\n> > ... in this test, rendering the key_val[] array unusuable for\n> > further tests. Is that intended and desirable?\n> > ...\n> > The TEST(setup(t_foo)) pattern is done so nicely to make sure that\n> > everybody is independent from everybody else, preparing the oidmap\n> > used for each specific test from scratch. It is a bit disappointing\n> > that we are now invalidating this nice property.\n>\n> It may be just the matter of doing something silly like this to\n> restore the \"different tests are independent and the source of truth\n> array is intact\" property.\n>\n> The first hunk should be reindented properly, if you are going to\n> take this and squash into your patch, by the way.\n>\n> Thanks.\n\nI think this is very reasonable. I'll squash this into my patch and wait\nfor any other comments from reviewers before sending another version.\n\nThanks.\n\n> t/unit-tests/t-oidmap.c | 11 ++++++-----\n> 1 file changed, 6 insertions(+), 5 deletions(-)\n>\n> diff --git c/t/unit-tests/t-oidmap.c w/t/unit-tests/t-oidmap.c\n> index 13532aa98b..be2741c6c7 100644\n> --- c/t/unit-tests/t-oidmap.c\n> +++ w/t/unit-tests/t-oidmap.c\n> @@ -14,7 +14,7 @@ struct test_entry {\n> char name[FLEX_ARRAY];\n> };\n> \n> -static const char *key_val[][2] = { { \"11\", \"one\" },\n> +static const char * const key_val[][2] = { { \"11\", \"one\" },\n> { \"22\", \"two\" },\n> { \"33\", \"three\" } };\n> \n> @@ -116,7 +116,7 @@ static void t_remove(struct oidmap *map)\n> check(oidmap_remove(map, &oid) == NULL);\n> }\n> \n> -static int key_val_contains(struct test_entry *entry)\n> +static int key_val_contains(struct test_entry *entry, char seen[])\n> {\n> for (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {\n> struct object_id oid;\n> @@ -125,9 +125,9 @@ static int key_val_contains(struct test_entry\n> *entry)\n> return -1;\n> \n> if (oideq(&entry->entry.oid, &oid)) {\n> - if (!strcmp(key_val[i][1], \"USED\"))\n> + if (seen[i])\n> return 2;\n> - key_val[i][1] = \"USED\";\n> + seen[i] = 1;\n> return 0;\n> }\n> }\n> @@ -138,11 +138,12 @@ static void t_iterate(struct oidmap *map)\n> {\n> struct oidmap_iter iter;\n> struct test_entry *entry;\n> + char seen[ARRAY_SIZE(key_val)] = { 0 };\n> \n> oidmap_iter_init(map, &iter);\n> while ((entry = oidmap_iter_next(&iter))) {\n> int ret;\n> - if (!check_int((ret = key_val_contains(entry)), ==, 0)) {\n> + if (!check_int((ret = key_val_contains(entry, seen)), ==, 0)) {\n> switch (ret) {\n> case -1:\n> break; /* error message handled by get_oid_arbitrary_hex() */\n\n"},{"id":"497978","messageId":"add972f8-7f9f-4bb5-b053-be135a66b024@gmail.com","threadId":"61651","inReplyTo":"xmqqjzi4u52u.fsf@gitster.g","subject":"Re: [GSoC][PATCH v2] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-02T15:17:54Z","receivedAt":"2024-07-02T15:17:59Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 01/07/2024 23:20, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> Hmph. You seem to overwrite key_val[i][1] ...\n>> ...\n>> ... in this test, rendering the key_val[] array unusuable for\n>> further tests. Is that intended and desirable?\n>> ...\n>> The TEST(setup(t_foo)) pattern is done so nicely to make sure that\n>> everybody is independent from everybody else, preparing the oidmap\n>> used for each specific test from scratch. It is a bit disappointing\n>> that we are now invalidating this nice property.\n> \n> It may be just the matter of doing something silly like this to\n> restore the \"different tests are independent and the source of truth\n> array is intact\" property.\n> \n> The first hunk should be reindented properly, if you are going to\n> take this and squash into your patch, by the way.\n\nThis looks good - we should definitely avoid overwriting key_val.\n\nBest Wishes\n\nPhillip\n\n> Thanks.\n> \n> t/unit-tests/t-oidmap.c | 11 ++++++-----\n> 1 file changed, 6 insertions(+), 5 deletions(-)\n> \n> diff --git c/t/unit-tests/t-oidmap.c w/t/unit-tests/t-oidmap.c\n> index 13532aa98b..be2741c6c7 100644\n> --- c/t/unit-tests/t-oidmap.c\n> +++ w/t/unit-tests/t-oidmap.c\n> @@ -14,7 +14,7 @@ struct test_entry {\n> \tchar name[FLEX_ARRAY];\n> };\n> \n> -static const char *key_val[][2] = { { \"11\", \"one\" },\n> +static const char * const key_val[][2] = { { \"11\", \"one\" },\n> \t\t\t\t { \"22\", \"two\" },\n> \t\t\t\t { \"33\", \"three\" } };\n> \n> @@ -116,7 +116,7 @@ static void t_remove(struct oidmap *map)\n> \tcheck(oidmap_remove(map, &oid) == NULL);\n> }\n> \n> -static int key_val_contains(struct test_entry *entry)\n> +static int key_val_contains(struct test_entry *entry, char seen[])\n> {\n> \tfor (size_t i = 0; i < ARRAY_SIZE(key_val); i++) {\n> \t\tstruct object_id oid;\n> @@ -125,9 +125,9 @@ static int key_val_contains(struct test_entry *entry)\n> \t\t\treturn -1;\n> \n> \t\tif (oideq(&entry->entry.oid, &oid)) {\n> -\t\t\tif (!strcmp(key_val[i][1], \"USED\"))\n> +\t\t\tif (seen[i])\n> \t\t\t\treturn 2;\n> -\t\t\tkey_val[i][1] = \"USED\";\n> +\t\t\tseen[i] = 1;\n> \t\t\treturn 0;\n> \t\t}\n> \t}\n> @@ -138,11 +138,12 @@ static void t_iterate(struct oidmap *map)\n> {\n> \tstruct oidmap_iter iter;\n> \tstruct test_entry *entry;\n> +\tchar seen[ARRAY_SIZE(key_val)] = { 0 };\n> \n> \toidmap_iter_init(map, &iter);\n> \twhile ((entry = oidmap_iter_next(&iter))) {\n> \t\tint ret;\n> -\t\tif (!check_int((ret = key_val_contains(entry)), ==, 0)) {\n> +\t\tif (!check_int((ret = key_val_contains(entry, seen)), ==, 0)) {\n> \t\t\tswitch (ret) {\n> \t\t\tcase -1:\n> \t\t\t\tbreak; /* error message handled by get_oid_arbitrary_hex() */\n> \n> \n> \n"},{"id":"497979","messageId":"16e06a6d-5fd0-4132-9d82-5c6f13b7f9ed@gmail.com","threadId":"61651","inReplyTo":"20240628122030.41554-1-shyamthakkar001@gmail.com","subject":"Re: [GSoC][PATCH v2] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-02T15:24:54Z","receivedAt":"2024-07-02T15:24:56Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ghanshyam\n\nOn 28/06/2024 13:20, Ghanshyam Thakkar wrote:\n> helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h\n> library which is built on top of hashmap.h.\n> \n> Migrate them to the unit testing framework for better performance,\n> concise code and better debugging. Along with the migration also plug\n> memory leaks and make the test logic independent for all the tests.\n> The migration removes 'put' tests from t0016, because it is used as\n> setup to all the other tests, so testing it separately does not yield\n> any benefit.\n> \n> Helped-by: Phillip Wood \n> Mentored-by: Christian Couder \n> Mentored-by: Kaartic Sivaraam \n> Signed-off-by: Ghanshyam Thakkar \n> ---\n> This version addresses Phillip's review about detecting duplicates in\n> oidmap when iterating over it and removing put_and_check_null() to move\n> the relevant code to setup() instead. And contains some grammer fixes\n> in the comment.\n\nThis version with Junio's fixup addresses my previous comments. One more \nthing occurred to me as I was reading it again\n\n> +static void t_iterate(struct oidmap *map)\n> +{\n> +\tstruct oidmap_iter iter;\n> +\tstruct test_entry *entry;\n\nI wonder if we want to add a bit of paranoia with\n\n\tint count = 0;\n\n> +\toidmap_iter_init(map, &iter);\n> +\twhile ((entry = oidmap_iter_next(&iter))) {\n> +\t\tint ret;\n> +\t\tif (!check_int((ret = key_val_contains(entry)), ==, 0)) {\n> +\t\t\tswitch (ret) {\n> +\t\t\tcase -1:\n> +\t\t\t\tbreak; /* error message handled by get_oid_arbitrary_hex() */\n> +\t\t\tcase 1:\n> +\t\t\t\ttest_msg(\"obtained entry was not given in the input\\n\"\n> +\t\t\t\t\t \" name: %s\\n oid: %s\\n\",\n> +\t\t\t\t\t entry->name, oid_to_hex(&entry->entry.oid));\n> +\t\t\t\tbreak;\n> +\t\t\tcase 2:\n> +\t\t\t\ttest_msg(\"duplicate entry detected\\n\"\n> +\t\t\t\t\t \" name: %s\\n oid: %s\\n\",\n> +\t\t\t\t\t entry->name, oid_to_hex(&entry->entry.oid));\n> +\t\t\t\tbreak;\n> +\t\t\tdefault:\n> +\t\t\t\ttest_msg(\"BUG: invalid return value (%d) from key_val_contains()\",\n> +\t\t\t\t\t ret);\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t} \n\t\t} else {\n\t\t\tcount++;\n\t\t}\n> +\t}\n\tcheck_int(count, ARRAY_SIZE(key_val));\n\nto check that we iterate over all the entries as well as checking the \nsize of the hashmap here.\n\n > +\tcheck_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val));\n\nBest Wishes\n\nPhillip\n\n> +}\n> +\n> +int cmd_main(int argc UNUSED, const char **argv UNUSED)\n> +{\n> +\tTEST(setup(t_replace), \"replace works\");\n> +\tTEST(setup(t_get), \"get works\");\n> +\tTEST(setup(t_remove), \"remove works\");\n> +\tTEST(setup(t_iterate), \"iterate works\");\n> +\treturn test_done();\n> +}\n"},{"id":"497982","messageId":"D2F6Z5WUBKKQ.2A00IY9IF37SI@gmail.com","threadId":"61651","inReplyTo":"16e06a6d-5fd0-4132-9d82-5c6f13b7f9ed@gmail.com","subject":"Re: [GSoC][PATCH v2] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-07-02T16:25:48Z","receivedAt":"2024-07-02T16:25:54Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Phillip Wood wrote:\n> Hi Ghanshyam\n>\n> On 28/06/2024 13:20, Ghanshyam Thakkar wrote:\n> > helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h\n> > library which is built on top of hashmap.h.\n> > \n> > Migrate them to the unit testing framework for better performance,\n> > concise code and better debugging. Along with the migration also plug\n> > memory leaks and make the test logic independent for all the tests.\n> > The migration removes 'put' tests from t0016, because it is used as\n> > setup to all the other tests, so testing it separately does not yield\n> > any benefit.\n> > \n> > Helped-by: Phillip Wood \n> > Mentored-by: Christian Couder \n> > Mentored-by: Kaartic Sivaraam \n> > Signed-off-by: Ghanshyam Thakkar \n> > ---\n> > This version addresses Phillip's review about detecting duplicates in\n> > oidmap when iterating over it and removing put_and_check_null() to move\n> > the relevant code to setup() instead. And contains some grammer fixes\n> > in the comment.\n>\n> This version with Junio's fixup addresses my previous comments. One more\n> thing occurred to me as I was reading it again\n>\n> > +static void t_iterate(struct oidmap *map)\n> > +{\n> > +\tstruct oidmap_iter iter;\n> > +\tstruct test_entry *entry;\n>\n> I wonder if we want to add a bit of paranoia with\n>\n> int count = 0;\n>\n> > +\toidmap_iter_init(map, &iter);\n> > +\twhile ((entry = oidmap_iter_next(&iter))) {\n> > +\t\tint ret;\n> > +\t\tif (!check_int((ret = key_val_contains(entry)), ==, 0)) {\n> > +\t\t\tswitch (ret) {\n> > +\t\t\tcase -1:\n> > +\t\t\t\tbreak; /* error message handled by get_oid_arbitrary_hex() */\n> > +\t\t\tcase 1:\n> > +\t\t\t\ttest_msg(\"obtained entry was not given in the input\\n\"\n> > +\t\t\t\t\t \" name: %s\\n oid: %s\\n\",\n> > +\t\t\t\t\t entry->name, oid_to_hex(&entry->entry.oid));\n> > +\t\t\t\tbreak;\n> > +\t\t\tcase 2:\n> > +\t\t\t\ttest_msg(\"duplicate entry detected\\n\"\n> > +\t\t\t\t\t \" name: %s\\n oid: %s\\n\",\n> > +\t\t\t\t\t entry->name, oid_to_hex(&entry->entry.oid));\n> > +\t\t\t\tbreak;\n> > +\t\t\tdefault:\n> > +\t\t\t\ttest_msg(\"BUG: invalid return value (%d) from key_val_contains()\",\n> > +\t\t\t\t\t ret);\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> > +\t\t} \n> } else {\n> count++;\n> }\n> > +\t}\n> check_int(count, ARRAY_SIZE(key_val));\n>\n> to check that we iterate over all the entries as well as checking the\n> size of the hashmap here.\n>\n> > + check_int(hashmap_get_size(&map->map), ==, ARRAY_SIZE(key_val));\n\nYeah, good idea. I'll include it in v3.\n\nThanks.\n"},{"id":"497984","messageId":"xmqq5xtnsjxp.fsf@gitster.g","threadId":"61651","inReplyTo":"16e06a6d-5fd0-4132-9d82-5c6f13b7f9ed@gmail.com","subject":"Re: [GSoC][PATCH v2] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-02T18:55:14Z","receivedAt":"2024-07-02T18:55:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> +\t}\n> \tcheck_int(count, ARRAY_SIZE(key_val));\n>\n> to check that we iterate over all the entries as well as checking the\n> size of the hashmap here.\n\nI think check_int() macro wants the comparison operator in the\nmiddle, but other than that small typo, the suggestion sounds quite\nsensible. If the iterator does not yield anything, the current test\nwould still pass.\n\nThanks.\n"}]}