{"thread":{"id":"62000","subject":"[GSoC][PATCH] unit-tests: add tests for oidset.h","startedAt":"2024-08-24T17:20:47Z","lastAt":"2024-09-30T18:48:47Z","messageCount":10,"participants":["Ghanshyam Thakkar","Patrick Steinhardt","Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"501628","messageId":"20240824172028.39419-1-shyamthakkar001@gmail.com","threadId":"62000","inReplyTo":null,"subject":"[GSoC][PATCH] unit-tests: add tests for oidset.h","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-08-24T17:20:23Z","receivedAt":"2024-08-24T17:20:47Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Add tests for oidset.h library, which were not previously present using\nthe unit testing framework.\n\nThis imposes a new restriction of running the test from the 't/' and\n't/unit-tests/bin' for constructing the path to the test files which\nare used by t_parse_file(), which tests the parsing of object_ids from\na file. This restriction is similar to the one we already have for\nend-to-end tests, wherein, we can only run those tests from 't/'. The\naddition of allowing 't/unit-tests/bin' for allowing to run tests from\nis for running individual unit tests, which is not currently possible\nvia any 'make' target. And 'make unit-tests-test-tool' also runs from\n't/unit-tests/bin'\n\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\nSigned-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>\n---\nI know there is some hesitance from the community in imposing the\nrestriction of running the unit tests from certain directories, so\nif this case does not justify imposing such a restriction, I am fine\nwith removing t_parse_file() in the next version.\n\nThanks.\n\n Makefile                          |   1 +\n t/unit-tests/lib-oid.c            |   2 +-\n t/unit-tests/lib-oid.h            |   1 +\n t/unit-tests/t-oidset.c           | 222 ++++++++++++++++++++++++++++++\n t/unit-tests/t-oidset/sha1-oids   |  10 ++\n t/unit-tests/t-oidset/sha256-oids |  10 ++\n 6 files changed, 245 insertions(+), 1 deletion(-)\n create mode 100644 t/unit-tests/t-oidset.c\n create mode 100644 t/unit-tests/t-oidset/sha1-oids\n create mode 100644 t/unit-tests/t-oidset/sha256-oids\n\ndiff --git a/Makefile b/Makefile\nindex e298c8b55e..5c1762fa1b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1338,6 +1338,7 @@ UNIT_TEST_PROGRAMS += t-hash\n UNIT_TEST_PROGRAMS += t-hashmap\n UNIT_TEST_PROGRAMS += t-mem-pool\n UNIT_TEST_PROGRAMS += t-oidmap\n+UNIT_TEST_PROGRAMS += t-oidset\n UNIT_TEST_PROGRAMS += t-oidtree\n UNIT_TEST_PROGRAMS += t-prio-queue\n UNIT_TEST_PROGRAMS += t-reftable-basics\ndiff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c\nindex 37105f0a8f..8f0ccac532 100644\n--- a/t/unit-tests/lib-oid.c\n+++ b/t/unit-tests/lib-oid.c\n@@ -3,7 +3,7 @@\n #include \"strbuf.h\"\n #include \"hex.h\"\n \n-static int init_hash_algo(void)\n+int init_hash_algo(void)\n {\n \tstatic int algo = -1;\n \ndiff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h\nindex 8d2acca768..fc3e7aa376 100644\n--- a/t/unit-tests/lib-oid.h\n+++ b/t/unit-tests/lib-oid.h\n@@ -14,4 +14,5 @@\n  */\n int get_oid_arbitrary_hex(const char *s, struct object_id *oid);\n \n+int init_hash_algo(void);\n #endif /* LIB_OID_H */\ndiff --git a/t/unit-tests/t-oidset.c b/t/unit-tests/t-oidset.c\nnew file mode 100644\nindex 0000000000..4a63f9ea94\n--- /dev/null\n+++ b/t/unit-tests/t-oidset.c\n@@ -0,0 +1,222 @@\n+#include \"test-lib.h\"\n+#include \"oidset.h\"\n+#include \"lib-oid.h\"\n+#include \"hex.h\"\n+#include \"strbuf.h\"\n+\n+static const char *const hex_input[] = { \"00\", \"11\", \"22\", \"33\", \"aa\", \"cc\" };\n+\n+static void strbuf_test_data_path(struct strbuf *buf, int hash_algo)\n+{\n+\tstrbuf_getcwd(buf);\n+\tstrbuf_strip_suffix(buf, \"/unit-tests/bin\");\n+\tstrbuf_addf(buf, \"/unit-tests/t-oidset/%s\",\n+\t\t    hash_algo == GIT_HASH_SHA1 ? \"sha1-oids\" : \"sha256-oids\");\n+}\n+\n+static void setup(void (*f)(struct oidset *st))\n+{\n+\tstruct oidset st = OIDSET_INIT;\n+\tstruct object_id oid;\n+\tint ret = 0;\n+\n+\tif (!check_int(oidset_size(&st), ==, 0)) {\n+\t\ttest_skip_all(\"OIDSET_INIT is broken\");\n+\t\treturn;\n+\t}\n+\n+\tfor (size_t i = 0; i < ARRAY_SIZE(hex_input); i++) {\n+\t\tif ((ret = get_oid_arbitrary_hex(hex_input[i], &oid)))\n+\t\t\tbreak;\n+\t\tif (!check_int((ret = oidset_insert(&st, &oid)), ==, 0))\n+\t\t\tbreak;\n+\t}\n+\n+\tif (!ret && check_int(oidset_size(&st), ==, ARRAY_SIZE(hex_input)))\n+\t\tf(&st);\n+\n+\toidset_clear(&st);\n+}\n+\n+static void t_contains(struct oidset *st)\n+{\n+\tstruct object_id oid;\n+\n+\tfor (size_t i = 0; i < ARRAY_SIZE(hex_input); i++) {\n+\t\tif (!get_oid_arbitrary_hex(hex_input[i], &oid)) {\n+\t\t\tif (!check_int(oidset_contains(st, &oid), ==, 1))\n+\t\t\t\ttest_msg(\"oid: %s\", oid_to_hex(&oid));\n+\t\t}\n+\t}\n+\n+\tif (!get_oid_arbitrary_hex(\"55\", &oid))\n+\t\tcheck_int(oidset_contains(st, &oid), ==, 0);\n+}\n+\n+static void t_insert_dup(struct oidset *st)\n+{\n+\tstruct object_id oid;\n+\n+\tif (!get_oid_arbitrary_hex(\"11\", &oid))\n+\t\tcheck_int(oidset_insert(st, &oid), ==, 1);\n+\n+\tif (!get_oid_arbitrary_hex(\"aa\", &oid))\n+\t\tcheck_int(oidset_insert(st, &oid), ==, 1);\n+\n+\tcheck_int(oidset_size(st), ==, ARRAY_SIZE(hex_input));\n+}\n+\n+static void t_insert_from_set(struct oidset *st_src)\n+{\n+\tstruct oidset st_dest = OIDSET_INIT;\n+\tstruct oidset_iter iter_src, iter_dest;\n+\tstruct object_id *oid_src, *oid_dest;\n+\tstruct object_id oid;\n+\tsize_t count = 0;\n+\n+\toidset_insert_from_set(&st_dest, st_src);\n+\tcheck_int(oidset_size(st_src), ==, ARRAY_SIZE(hex_input));\n+\tcheck_int(oidset_size(&st_dest), ==, oidset_size(st_src));\n+\t\n+\toidset_iter_init(st_src, &iter_src);\n+\toidset_iter_init(&st_dest, &iter_dest);\n+\n+\t/* check that oidset_insert_from_set() makes a copy of the object_ids */\n+\twhile ((oid_src = oidset_iter_next(&iter_src)) &&\n+\t       (oid_dest = oidset_iter_next(&iter_dest))) {\n+\t\tcheck(oid_src != oid_dest);\n+\t\tcount++;\n+\t}\n+\tcheck_int(count, ==, ARRAY_SIZE(hex_input));\n+\n+\tfor (size_t i = 0; i < ARRAY_SIZE(hex_input); i++) {\n+\t\tif (!get_oid_arbitrary_hex(hex_input[i], &oid)) {\n+\t\t\tif (!check_int(oidset_contains(&st_dest, &oid), ==, 1))\n+\t\t\t\ttest_msg(\"oid: %s\", oid_to_hex(&oid));\n+\t\t}\n+\t}\n+\n+\tif (!get_oid_arbitrary_hex(\"55\", &oid))\n+\t\tcheck_int(oidset_contains(&st_dest, &oid), ==, 0);\n+\toidset_clear(&st_dest);\n+}\n+\n+static void t_remove(struct oidset *st)\n+{\n+\tstruct object_id oid;\n+\n+\tif (!get_oid_arbitrary_hex(\"55\", &oid)) {\n+\t\tcheck_int(oidset_remove(st, &oid), ==, 0);\n+\t\tcheck_int(oidset_size(st), ==, ARRAY_SIZE(hex_input));\n+\t}\n+\n+\tif (!get_oid_arbitrary_hex(\"22\", &oid)) {\n+\t\tcheck_int(oidset_remove(st, &oid), ==, 1);\n+\t\tcheck_int(oidset_size(st), ==, ARRAY_SIZE(hex_input) - 1);\n+\t\tcheck_int(oidset_contains(st, &oid), ==, 0);\n+\t}\n+\n+\tif (!get_oid_arbitrary_hex(\"cc\", &oid)) {\n+\t\tcheck_int(oidset_remove(st, &oid), ==, 1);\n+\t\tcheck_int(oidset_size(st), ==, ARRAY_SIZE(hex_input) - 2);\n+\t\tcheck_int(oidset_contains(st, &oid), ==, 0);\n+\t}\n+\n+\tif (!get_oid_arbitrary_hex(\"00\", &oid))\n+\t{\n+\t\t/* remove a value inserted more than once */\n+\t\tcheck_int(oidset_insert(st, &oid), ==, 1);\n+\t\tcheck_int(oidset_remove(st, &oid), ==, 1);\n+\t\tcheck_int(oidset_size(st), ==, ARRAY_SIZE(hex_input) - 3);\n+\t\tcheck_int(oidset_contains(st, &oid), ==, 0);\n+\t}\n+\n+\tif (!get_oid_arbitrary_hex(\"22\", &oid))\n+\t\tcheck_int(oidset_remove(st, &oid), ==, 0);\n+}\n+\n+static int input_contains(struct object_id *oid, char *seen)\n+{\n+\tfor (size_t i = 0; i < ARRAY_SIZE(hex_input); i++) {\n+\t\tstruct object_id oid_input;\n+\t\tif (get_oid_arbitrary_hex(hex_input[i], &oid_input))\n+\t\t\treturn -1;\n+\t\tif (oideq(&oid_input, oid)) {\n+\t\t\tif (seen[i])\n+\t\t\t\treturn 2;\n+\t\t\tseen[i] = 1;\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\treturn 1;\n+}\n+\n+static void t_iterate(struct oidset *st)\n+{\n+\tstruct oidset_iter iter;\n+\tstruct object_id *oid;\n+\tchar seen[ARRAY_SIZE(hex_input)] = { 0 };\n+\tint count = 0;\n+\n+\toidset_iter_init(st, &iter);\n+\twhile ((oid = oidset_iter_next(&iter))) {\n+\t\tint ret;\n+\t\tif (!check_int((ret = input_contains(oid, seen)), ==, 0)) {\n+\t\t\tswitch (ret) {\n+\t\t\tcase -1:\n+\t\t\t\tbreak; /* handled by get_oid_arbitrary_hex() */\n+\t\t\tcase 1:\n+\t\t\t\ttest_msg(\"obtained object_id was not given in the input\\n\"\n+\t\t\t\t\t \"  object_id: %s\", oid_to_hex(oid));\n+\t\t\t\tbreak;\n+\t\t\tcase 2:\n+\t\t\t\ttest_msg(\"duplicate object_id detected\\n\"\n+\t\t\t\t\t \"  object_id: %s\", oid_to_hex(oid));\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t} else {\n+\t\t\tcount++;\n+\t\t}\n+\t}\n+\tcheck_int(count, ==, ARRAY_SIZE(hex_input));\n+\tcheck_int(oidset_size(st), ==, ARRAY_SIZE(hex_input));\n+}\n+\n+static void t_parse_file(void)\n+{\n+\tstruct strbuf path = STRBUF_INIT;\n+\tstruct oidset st = OIDSET_INIT;\n+\tstruct object_id oid;\n+\tint hash_algo = init_hash_algo();\n+\n+\tif (!check_int(hash_algo, !=, GIT_HASH_UNKNOWN))\n+\t\treturn;\n+\n+\tstrbuf_test_data_path(&path, hash_algo);\n+\toidset_parse_file(&st, path.buf, &hash_algos[hash_algo]);\n+\tcheck_int(oidset_size(&st), ==, 6);\n+\n+\tif (!get_oid_arbitrary_hex(\"00\", &oid))\n+\t\tcheck_int(oidset_contains(&st, &oid), ==, 1);\n+\tif (!get_oid_arbitrary_hex(\"44\", &oid))\n+\t\tcheck_int(oidset_contains(&st, &oid), ==, 1);\n+\tif (!get_oid_arbitrary_hex(\"cc\", &oid))\n+\t\tcheck_int(oidset_contains(&st, &oid), ==, 1);\n+\n+\tif (!get_oid_arbitrary_hex(\"11\", &oid))\n+\t\tcheck_int(oidset_contains(&st, &oid), ==, 0);\n+\n+\toidset_clear(&st);\n+\tstrbuf_release(&path);\n+}\n+\n+int cmd_main(int argc UNUSED, const char **argv UNUSED)\n+{\n+\tTEST(setup(t_contains), \"contains works\");\n+\tTEST(setup(t_insert_dup), \"insert an already inserted value works\");\n+\tTEST(setup(t_insert_from_set), \"insert from one set to another works\");\n+\tTEST(setup(t_remove), \"remove works\");\n+\tTEST(setup(t_iterate), \"iteration works\");\n+\tTEST(t_parse_file(), \"parsing from file works\");\n+\treturn test_done();\n+}\ndiff --git a/t/unit-tests/t-oidset/sha1-oids b/t/unit-tests/t-oidset/sha1-oids\nnew file mode 100644\nindex 0000000000..881f45e661\n--- /dev/null\n+++ b/t/unit-tests/t-oidset/sha1-oids\n@@ -0,0 +1,10 @@\n+# comments are ignored\n+0000000000000000000000000000000000000000\n+9900000000000000000000000000000000000000\n+dd00000000000000000000000000000000000000\n+\n+  4400000000000000000000000000000000000000\n+\n+bb00000000000000000000000000000000000000 # test comment\n+cc00000000000000000000000000000000000000\n+# 1100000000000000000000000000000000000000\ndiff --git a/t/unit-tests/t-oidset/sha256-oids b/t/unit-tests/t-oidset/sha256-oids\nnew file mode 100644\nindex 0000000000..3c1c687812\n--- /dev/null\n+++ b/t/unit-tests/t-oidset/sha256-oids\n@@ -0,0 +1,10 @@\n+# comments are ignored\n+0000000000000000000000000000000000000000000000000000000000000000\n+9900000000000000000000000000000000000000000000000000000000000000\n+dd00000000000000000000000000000000000000000000000000000000000000\n+\n+  4400000000000000000000000000000000000000000000000000000000000000\n+\n+bb00000000000000000000000000000000000000000000000000000000000000 # test comment\n+cc00000000000000000000000000000000000000000000000000000000000000\n+# 1100000000000000000000000000000000000000000000000000000000000000\n-- \n2.46.0\n\n"},{"id":"501641","messageId":"Zswok6P5dYf7ob5P@tanuki","threadId":"62000","inReplyTo":"20240824172028.39419-1-shyamthakkar001@gmail.com","subject":"Re: [GSoC][PATCH] unit-tests: add tests for oidset.h","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:02:49Z","receivedAt":"2024-08-26T07:02:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Aug 24, 2024 at 10:50:23PM +0530, Ghanshyam Thakkar wrote:\n> Add tests for oidset.h library, which were not previously present using\n> the unit testing framework.\n> \n> This imposes a new restriction of running the test from the 't/' and\n> 't/unit-tests/bin' for constructing the path to the test files which\n> are used by t_parse_file(), which tests the parsing of object_ids from\n> a file. This restriction is similar to the one we already have for\n> end-to-end tests, wherein, we can only run those tests from 't/'. The\n> addition of allowing 't/unit-tests/bin' for allowing to run tests from\n> is for running individual unit tests, which is not currently possible\n> via any 'make' target. And 'make unit-tests-test-tool' also runs from\n> 't/unit-tests/bin'\n> \n> Mentored-by: Christian Couder <chriscool@tuxfamily.org>\n> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>\n> ---\n> I know there is some hesitance from the community in imposing the\n> restriction of running the unit tests from certain directories, so\n> if this case does not justify imposing such a restriction, I am fine\n> with removing t_parse_file() in the next version.\n\nAnother option would be to set up a preprocessor define that gives us\nthe path to the unit test root directory. But I'm also okayish with the\ncurrent version. If it turns out to be annoying we can still iterate.\n\n> diff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c\n> index 37105f0a8f..8f0ccac532 100644\n> --- a/t/unit-tests/lib-oid.c\n> +++ b/t/unit-tests/lib-oid.c\n> @@ -3,7 +3,7 @@\n>  #include \"strbuf.h\"\n>  #include \"hex.h\"\n>  \n> -static int init_hash_algo(void)\n> +int init_hash_algo(void)\n>  {\n>  \tstatic int algo = -1;\n>  \n> diff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h\n> index 8d2acca768..fc3e7aa376 100644\n> --- a/t/unit-tests/lib-oid.h\n> +++ b/t/unit-tests/lib-oid.h\n> @@ -14,4 +14,5 @@\n>   */\n>  int get_oid_arbitrary_hex(const char *s, struct object_id *oid);\n>  \n> +int init_hash_algo(void);\n>  #endif /* LIB_OID_H */\n\nLet's add a comment to explain what this does. Also, do we maybe want to\ngive it a name that ties it to the unit tests? `init_hash_algo()` is\nquite generic and may easily lead to conflicting symbol names.\n\nMaybe something like `t_oid_init_hash_algo()`. `t_` to indicate that it\nis testing-related, `oid_` indicates that it's part of \"lib-oid.h\", and\nthe remainder describes what it does.\n\n> diff --git a/t/unit-tests/t-oidset.c b/t/unit-tests/t-oidset.c\n> new file mode 100644\n> index 0000000000..4a63f9ea94\n> --- /dev/null\n> +++ b/t/unit-tests/t-oidset.c\n> @@ -0,0 +1,222 @@\n> +#include \"test-lib.h\"\n> +#include \"oidset.h\"\n> +#include \"lib-oid.h\"\n> +#include \"hex.h\"\n> +#include \"strbuf.h\"\n> +\n> +static const char *const hex_input[] = { \"00\", \"11\", \"22\", \"33\", \"aa\", \"cc\" };\n\nI think we typically write this `const char * const`, with another space\nbetween `*` and `const`.\n\n> +static void strbuf_test_data_path(struct strbuf *buf, int hash_algo)\n> +{\n> +\tstrbuf_getcwd(buf);\n> +\tstrbuf_strip_suffix(buf, \"/unit-tests/bin\");\n> +\tstrbuf_addf(buf, \"/unit-tests/t-oidset/%s\",\n> +\t\t    hash_algo == GIT_HASH_SHA1 ? \"sha1-oids\" : \"sha256-oids\");\n> +}\n\nI wouldn't prefix this with `strbuf_`, as it is not part of the strbuf\nsubsystem. The function just happens to use a strbuf.\n\n> +static void setup(void (*f)(struct oidset *st))\n\nI was wondering what `st` stands for. I'd either call it just `s` or\n`set`.\n\n> +{\n> +\tstruct oidset st = OIDSET_INIT;\n> +\tstruct object_id oid;\n> +\tint ret = 0;\n> +\n> +\tif (!check_int(oidset_size(&st), ==, 0)) {\n> +\t\ttest_skip_all(\"OIDSET_INIT is broken\");\n> +\t\treturn;\n> +\t}\n> +\n> +\tfor (size_t i = 0; i < ARRAY_SIZE(hex_input); i++) {\n> +\t\tif ((ret = get_oid_arbitrary_hex(hex_input[i], &oid)))\n> +\t\t\tbreak;\n> +\t\tif (!check_int((ret = oidset_insert(&st, &oid)), ==, 0))\n> +\t\t\tbreak;\n> +\t}\n\nIn both of these cases I'd split out the assignment into a separate\nline. While the first instance is likely fine, the second instance makes\nme a bit uneasy as it is a macro. I generally do not trust macros to do\nthe correct thing when being passed a statement with side effects.\n\n[snip]\n> +static int input_contains(struct object_id *oid, char *seen)\n> +{\n> +\tfor (size_t i = 0; i < ARRAY_SIZE(hex_input); i++) {\n> +\t\tstruct object_id oid_input;\n> +\t\tif (get_oid_arbitrary_hex(hex_input[i], &oid_input))\n> +\t\t\treturn -1;\n> +\t\tif (oideq(&oid_input, oid)) {\n> +\t\t\tif (seen[i])\n> +\t\t\t\treturn 2;\n> +\t\t\tseen[i] = 1;\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t}\n> +\treturn 1;\n> +}\n\nThis function is somewhat confusing. Contains what? What are the\nparameters? Is one of them the expectation, the other one the actual\nstate? What do we compare against?\n\nI think it would help to get it a more descriptive name that says what\nwe check for and remove the dependence on global state. Also, the `seen`\narray does not seem to be used by any caller. So maybe we should\nallocate it ourselves in this function such that it is self-contained.\nIt requires more allocations, sure, but I highly doubt that this is\ngoing to be important in this test.\n\nPatrick\n"},{"id":"501666","messageId":"CAP8UFD2yTMNmx0n1jhOu7dz_4XeOyTy1iLmRWYmuf9QJf75hsQ@mail.gmail.com","threadId":"62000","inReplyTo":"20240824172028.39419-1-shyamthakkar001@gmail.com","subject":"Re: [GSoC][PATCH] unit-tests: add tests for oidset.h","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-08-26T09:31:22Z","receivedAt":"2024-08-26T09:31:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Aug 24, 2024 at 7:20 PM Ghanshyam Thakkar\n<shyamthakkar001@gmail.com> wrote:\n>\n> Add tests for oidset.h library, which were not previously present using\n> the unit testing framework.\n\nIt might be interesting to also say if there are tests for oidset in\nthe end-to-end tests, not just in the unit test framework. Also I\nthink oidset.h is more an API than a library.\n\n> This imposes a new restriction of running the test from the 't/' and\n> 't/unit-tests/bin'\n\nEither \"from 't/' and 't/unit-tests/bin'\" or \"from the 't/' and\n't/unit-tests/bin' directory\" would be a bit better.\n\n> for constructing the path to the test files which\n> are used by t_parse_file(), which tests the parsing of object_ids from\n> a file.\n\nThis might be clearer if it mentioned that t_parse_file() actually\ntests oidset_parse_file() which is part of the oidset.h API.\n\n> This restriction is similar to the one we already have for\n> end-to-end tests, wherein, we can only run those tests from 't/'.\n\nOk.\n\n> The\n> addition of allowing 't/unit-tests/bin' for allowing to run tests from\n> is for running individual unit tests,\n\nMaybe: \"Allowing to run tests from 't/unit-tests/bin', in addition to\n't/', makes it possible to run individual unit tests,\"\n\n> which is not currently possible\n> via any 'make' target. And 'make unit-tests-test-tool' also runs from\n> 't/unit-tests/bin'\n\nIt would be nice if you gave a few examples of commands that can be\nrun after this patch while they didn't work before it.\n\n> Mentored-by: Christian Couder <chriscool@tuxfamily.org>\n> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>\n> ---\n> I know there is some hesitance from the community in imposing the\n> restriction of running the unit tests from certain directories, so\n> if this case does not justify imposing such a restriction, I am fine\n> with removing t_parse_file() in the next version.\n\nMy opinion is that it might be good to remove t_parse_file() for now,\nas testing oidset_parse_file() might not be so important. Later an\niteration in a separate patch could then perhap add it while better\ndiscussing if the restrictions that come with adding it are worth it.\n\n> diff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c\n> index 37105f0a8f..8f0ccac532 100644\n> --- a/t/unit-tests/lib-oid.c\n> +++ b/t/unit-tests/lib-oid.c\n> @@ -3,7 +3,7 @@\n>  #include \"strbuf.h\"\n>  #include \"hex.h\"\n>\n> -static int init_hash_algo(void)\n> +int init_hash_algo(void)\n>  {\n>         static int algo = -1;\n>\n> diff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h\n> index 8d2acca768..fc3e7aa376 100644\n> --- a/t/unit-tests/lib-oid.h\n> +++ b/t/unit-tests/lib-oid.h\n> @@ -14,4 +14,5 @@\n>   */\n>  int get_oid_arbitrary_hex(const char *s, struct object_id *oid);\n>\n> +int init_hash_algo(void);\n>  #endif /* LIB_OID_H */\n\nIt seems that the changes above will go away when some patches you\nalready sent will be merged, which should happen soon. It might have\nbeen nice to say this in the section after the \"---\" line.\n\n> +static void t_parse_file(void)\n> +{\n> +       struct strbuf path = STRBUF_INIT;\n> +       struct oidset st = OIDSET_INIT;\n> +       struct object_id oid;\n> +       int hash_algo = init_hash_algo();\n> +\n> +       if (!check_int(hash_algo, !=, GIT_HASH_UNKNOWN))\n> +               return;\n\nIf initializing the hash algo fails here, it is likely because it\nalready failed when get_oid_arbitrary_hex() (which initializes it) was\ncalled in the tests before this one. So I think it might be even\nbetter to move the above hash algo initialization code to setup() and\nmake setup() error out in case the initialization fails. Then setup()\ncould pass 'hash_algo' to all the functions it calls, even if some of\nthem don't use it.\n\n> +       strbuf_test_data_path(&path, hash_algo);\n> +       oidset_parse_file(&st, path.buf, &hash_algos[hash_algo]);\n> +       check_int(oidset_size(&st), ==, 6);\n> +\n> +       if (!get_oid_arbitrary_hex(\"00\", &oid))\n> +               check_int(oidset_contains(&st, &oid), ==, 1);\n> +       if (!get_oid_arbitrary_hex(\"44\", &oid))\n> +               check_int(oidset_contains(&st, &oid), ==, 1);\n> +       if (!get_oid_arbitrary_hex(\"cc\", &oid))\n> +               check_int(oidset_contains(&st, &oid), ==, 1);\n> +\n> +       if (!get_oid_arbitrary_hex(\"11\", &oid))\n> +               check_int(oidset_contains(&st, &oid), ==, 0);\n> +\n> +       oidset_clear(&st);\n> +       strbuf_release(&path);\n> +}\n\nThanks.\n"},{"id":"501672","messageId":"xmqqttf7mgmm.fsf@gitster.g","threadId":"62000","inReplyTo":"CAP8UFD2yTMNmx0n1jhOu7dz_4XeOyTy1iLmRWYmuf9QJf75hsQ@mail.gmail.com","subject":"Re: [GSoC][PATCH] unit-tests: add tests for oidset.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-26T15:46:25Z","receivedAt":"2024-08-26T15:46:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n>> Add tests for oidset.h library, which were not previously present using\n>> the unit testing framework.\n>\n> It might be interesting to also say if there are tests for oidset in\n> the end-to-end tests, not just in the unit test framework. Also I\n> think oidset.h is more an API than a library.\n\nThanks for pointing these out; 100% agreed.\n\n>> This imposes a new restriction of running the test from the 't/' and\n>> 't/unit-tests/bin'\n\nI thought we just got rid of such an restriction during the review\nof another unit-test topic?  If this is a recurring theme, perhaps\nwe should teach t/unit-test/test-lib.c a few ways to specify where\nthe auxiliary files for unit-tests are (e.g. \"-d <datadir>\" command\nline option, or $GIT_UNIT_TEST_DATA_DIR environment variable).\n\nEven though the end-to-end tests do not allow you to start them from\nan arbitrary directory (it shouldn't be a rocket science to teach\nthem to do so, though), they can run in an arbitrary place with the\n\"--root\" option without hindering its ability to read its auxiliary\ndata files, because they can learn where the t/ directory is by\nlooking at $TEST_DIRECTORY and a few other variables.  A similar\nidea should be applicable to the unit-tests framework.\n\nThanks.\n\n"},{"id":"503582","messageId":"xmqqy13ei819.fsf@gitster.g","threadId":"62000","inReplyTo":"CAP8UFD2yTMNmx0n1jhOu7dz_4XeOyTy1iLmRWYmuf9QJf75hsQ@mail.gmail.com","subject":"Re: [GSoC][PATCH] unit-tests: add tests for oidset.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-26T18:28:34Z","receivedAt":"2024-09-26T18:28:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> On Sat, Aug 24, 2024 at 7:20 PM Ghanshyam Thakkar\n> <shyamthakkar001@gmail.com> wrote:\n>>\n>> Add tests for oidset.h library, which were not previously present using\n>> the unit testing framework.\n>\n> It might be interesting to also say if there are tests for oidset in\n> the end-to-end tests, not just in the unit test framework. Also I\n> think oidset.h is more an API than a library.\n> ...\n> If initializing the hash algo fails here, it is likely because it\n> already failed when get_oid_arbitrary_hex() (which initializes it) was\n> called in the tests before this one. So I think it might be even\n> better to move the above hash algo initialization code to setup() and\n> make setup() error out in case the initialization fails. Then setup()\n> could pass 'hash_algo' to all the functions it calls, even if some of\n> them don't use it.\n> ...\n>\n> Thanks.\n\nWhile reviewing the \"What's cooking\" list of topics after tagging\n-rc0 of this development cycle, I noticed that this topic from late\nAugust has been expecting but not yet seeing an update.\n\nAs discussed elsewhere on the \"Project Tracking\" thread, I am in\nfavor of formally adopting a policy to discard a topic from 'seen'\nafter being inactive for 3 weeks, without having seen a clear\nconsensus that it is good enough to be moved to 'next'.  Interested\nparties are still free to revive the topic even after such a discard\nevent.\n\n    Side note: The definition of being \"inactive\" for the purpose of\n    the policy is that nobody has discussed the topic, no new\n    iteration of the topic was posted, and no responses to the\n    review comments were given.\n\nI'll discard this one by the end of this week unless the topic sees\nany activity.  It looks to me that the project decided that a longer\nterm direction to adopt \"clar\" as the unit-tests framework, so this\npatch would need to be written even if it were perfect in the old\nworld order anyway.\n\nThanks.\n\n"},{"id":"503584","messageId":"xmqqikuii60q.fsf@gitster.g","threadId":"62000","inReplyTo":"xmqqy13ei819.fsf@gitster.g","subject":"[PATCH] howto-maintain-git: discarding inactive topics","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-26T19:12:05Z","receivedAt":"2024-09-26T19:12:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When a patch series happened to look interesting to the maintainer\nbut is not ready for 'next', it is applied on a topic branch and\nmerged to the 'seen' branch to keep an eye on it.  In an ideal\nworld, the participants give reviews and the original author\nresponds to the reviews, and such iterations may produce newer\nversions of the patch series, and at some point, a concensus is\nformed that the latest round is good enough for 'next'.  Then the\ntopic is merged to 'next' for inclusion in a future release.\n\nIn a much less ideal world we live in, however, a topic sometimes\nget stalled.  The original author may not respond to hanging review\ncomments, may promise an update will be sent but does not manage to\ndo so, nobody talks about the topic on the list and nobody builds\nupon it, etc.\n\nFollowing the recent trend to document and give more transparency to\nthe decision making process, let's set a deadline to keep a topic\nstill alive, and actively discard those that are inactive for a long\nperiod of time.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/howto/maintain-git.txt | 17 ++++++++++++++++-\n 1 file changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git i/Documentation/howto/maintain-git.txt w/Documentation/howto/maintain-git.txt\nindex da31332f11..e797b32522 100644\n--- i/Documentation/howto/maintain-git.txt\n+++ w/Documentation/howto/maintain-git.txt\n@@ -67,7 +67,22 @@ the mailing list after each feature release is made:\n    before getting merged to 'master'.\n \n  - 'seen' branch is used to publish other proposed changes that do\n-   not yet pass the criteria set for 'next' (see above).\n+   not yet pass the criteria set for 'next' (see above), but there\n+   is no promise that 'seen' will contain everything.  A topic that\n+   had no reviewer reaction may not be picked up.\n+\n+   - A new topic will first get merged to 'seen', unless it is\n+     trivially correct and clearly urgent, in which case it may be\n+     directly merged to 'next' or even to 'master'.\n+\n+   - If a topic that was picked up to 'seen' becomes and stays\n+     inactive for 3 calendar weeks without having seen a clear\n+     consensus that it is good enough to be moved to 'next', the\n+     topic may be discarded from 'seen'.  Interested parties are\n+     still free to revive the topic.  For the purpose of this\n+     guideline, the definition of being \"inactive\" is that nobody\n+     has discussed the topic, no new iteration of the topic was\n+     posted, and no responses to the review comments were given.\n \n  - The tips of 'master' and 'maint' branches will not be rewound to\n    allow people to build their own customization on top of them.\n"},{"id":"503606","messageId":"CAP8UFD3JzYCJf4+JLvfW_8m6kp=O0NMKi1dF1Fof9=DmvZ4u2w@mail.gmail.com","threadId":"62000","inReplyTo":"xmqqy13ei819.fsf@gitster.g","subject":"Re: [GSoC][PATCH] unit-tests: add tests for oidset.h","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-09-27T08:57:36Z","receivedAt":"2024-09-27T08:57:50Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Sep 26, 2024 at 8:28 PM Junio C Hamano <gitster@pobox.com> wrote:\n\n> I'll discard this one by the end of this week unless the topic sees\n> any activity.  It looks to me that the project decided that a longer\n> term direction to adopt \"clar\" as the unit-tests framework, so this\n> patch would need to be written even if it were perfect in the old\n> world order anyway.\n\nYeah, unless Ghanshyam or someone else wants to continue working on\nit, I think finishing this work should be part of the \"Convert unit\ntests to use the clar testing framework\" Outreachy project that\nPatrick and Phillip agreed to co-mentor. This project will only start\nnext December though (supposing a good Outreachy intern is selected),\nso it's fine to discard it in the meantime.\n\nThanks.\n"},{"id":"503618","messageId":"xmqqcykpgchf.fsf@gitster.g","threadId":"62000","inReplyTo":"CAP8UFD3JzYCJf4+JLvfW_8m6kp=O0NMKi1dF1Fof9=DmvZ4u2w@mail.gmail.com","subject":"Re: [GSoC][PATCH] unit-tests: add tests for oidset.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-27T18:47:40Z","receivedAt":"2024-09-27T18:47:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> On Thu, Sep 26, 2024 at 8:28 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> I'll discard this one by the end of this week unless the topic sees\n>> any activity.  It looks to me that the project decided that a longer\n>> term direction to adopt \"clar\" as the unit-tests framework, so this\n>> patch would need to be written even if it were perfect in the old\n>> world order anyway.\n>\n> Yeah, unless Ghanshyam or someone else wants to continue working on\n> it, I think finishing this work should be part of the \"Convert unit\n> tests to use the clar testing framework\" Outreachy project that\n> Patrick and Phillip agreed to co-mentor. This project will only start\n> next December though (supposing a good Outreachy intern is selected),\n> so it's fine to discard it in the meantime.\n\nAnd of course it does not have to wait until December.\n\nIf anybody wants to work on adding a unit test for oidset, they can\ndo so immediately.  A new unit-test, including the oidset one,\nshould be written using clar framework.\n\nThanks.\n"},{"id":"503626","messageId":"ZveqArC9NNs44Fjc@pks.im","threadId":"62000","inReplyTo":"xmqqcykpgchf.fsf@gitster.g","subject":"Re: [GSoC][PATCH] unit-tests: add tests for oidset.h","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-28T07:02:36Z","receivedAt":"2024-09-28T07:03:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Sep 27, 2024 at 11:47:40AM -0700, Junio C Hamano wrote:\n> Christian Couder <christian.couder@gmail.com> writes:\n> \n> > On Thu, Sep 26, 2024 at 8:28 PM Junio C Hamano <gitster@pobox.com> wrote:\n> >\n> >> I'll discard this one by the end of this week unless the topic sees\n> >> any activity.  It looks to me that the project decided that a longer\n> >> term direction to adopt \"clar\" as the unit-tests framework, so this\n> >> patch would need to be written even if it were perfect in the old\n> >> world order anyway.\n> >\n> > Yeah, unless Ghanshyam or someone else wants to continue working on\n> > it, I think finishing this work should be part of the \"Convert unit\n> > tests to use the clar testing framework\" Outreachy project that\n> > Patrick and Phillip agreed to co-mentor. This project will only start\n> > next December though (supposing a good Outreachy intern is selected),\n> > so it's fine to discard it in the meantime.\n> \n> And of course it does not have to wait until December.\n> \n> If anybody wants to work on adding a unit test for oidset, they can\n> do so immediately.  A new unit-test, including the oidset one,\n> should be written using clar framework.\n\nI'm also happy to help anybody who wants write such a new unit test\nsuite.\n\nLet me use this to give a quick status update regarding my upstream\nquest to address the feedback I got during reviews on the clar itself:\n\n  - There is a .editorconfig file now.\n\n  - All the cross-platform compatibility fixes have been merged.\n\n  - We have Win32 wired up in CI. Doing so via Makefiles was too much of\n    a hassle, so I converted the project to use CMake for easier cross\n    platform testability. The fact that the project uses CMake does not\n    impact us though, as we wire it up ourselves anyway.\n\n  - All memory allocation errors are now handled consistently.\n\nCurrently in review is:\n\n  - Self-tests for the clar, where we use clar to assert that clar\n    works.\n\n  - A small memory leak fix, as well as wiring up leak sanitizers in CI.\n\nI've also got a patch series sitting locally that introduces type-safe\nwrappers for the assertions that I'll move into review once self-tests\nhave landed. That would then address the last bit of feedback I got, if\nI remember correctly.\n\nJust to let you folks know that I didn't just do nothing after this has\nlanded in Git.\n\nPatrick\n"},{"id":"503758","messageId":"xmqqmsjp6kqb.fsf@gitster.g","threadId":"62000","inReplyTo":"ZveqArC9NNs44Fjc@pks.im","subject":"Re: [GSoC][PATCH] unit-tests: add tests for oidset.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-30T18:48:44Z","receivedAt":"2024-09-30T18:48:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Let me use this to give a quick status update regarding my upstream\n> quest to address the feedback I got during reviews on the clar itself:\n>\n>   - There is a .editorconfig file now.\n>\n>   - All the cross-platform compatibility fixes have been merged.\n>\n>   - We have Win32 wired up in CI. Doing so via Makefiles was too much of\n>     a hassle, so I converted the project to use CMake for easier cross\n>     platform testability. The fact that the project uses CMake does not\n>     impact us though, as we wire it up ourselves anyway.\n>\n>   - All memory allocation errors are now handled consistently.\n>\n> Currently in review is:\n>\n>   - Self-tests for the clar, where we use clar to assert that clar\n>     works.\n>\n>   - A small memory leak fix, as well as wiring up leak sanitizers in CI.\n>\n> I've also got a patch series sitting locally that introduces type-safe\n> wrappers for the assertions that I'll move into review once self-tests\n> have landed. That would then address the last bit of feedback I got, if\n> I remember correctly.\n>\n> Just to let you folks know that I didn't just do nothing after this has\n> landed in Git.\n\n;-)\n\nNice to see that the code is improved not just for us but for other\nconsumers.\n\nThanks.\n"}]}