{"thread":{"id":"60980","subject":"[PATCH] unit-tests: convert t/helper/test-oid-array.c to unit-tests","startedAt":"2024-02-23T19:33:29Z","lastAt":"2024-02-27T09:59:21Z","messageCount":5,"participants":["Ghanshyam Thakkar","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"489253","messageId":"20240223193257.9222-1-shyamthakkar001@gmail.com","threadId":"60980","inReplyTo":null,"subject":"[PATCH] unit-tests: convert t/helper/test-oid-array.c to unit-tests","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-23T19:32:48Z","receivedAt":"2024-02-23T19:33:29Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Migrate t/helper/test-oid-array.c and t/t0064-oid-array.sh to the\nrecently added unit testing framework. This would improve runtime\nperformance and provide better debugging via displaying array content,\nindex at which the test failed etc. directly to stdout.\n\nThere is only one change in the new testing approach. In the previous\ntesting method, a new repo gets initialized for the test according to\nGIT_TEST_DEFAULT_HASH algorithm. In unit testing however, we do not\nneed to initialize the repo. We can set the length of the hexadecimal\nstrbuf according to the algorithm used directly.\n\nSigned-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>\n---\n[RFC]: I recently saw a series by Eric W. Biederman [1] which enables\nthe use of oid's with different hash algorithms into the same\noid_array safely. However, there were no tests added for this. So, I\nam wondering if we should have a input format which allows us to\nspecify hash algo for each oid with its hex value. i.e. \"sha1:55\" or\n\"sha256:55\", instead of just \"55\" and relying on GIT_TEST_DEFAULT_HASH\nfor algo. So far, I tried to imitate the existing tests but I suppose\nthis may be useful in the future if that series gets merged.\n\n Makefile                   |   2 +-\n t/helper/test-oid-array.c  |  45 --------\n t/helper/test-tool.c       |   1 -\n t/helper/test-tool.h       |   1 -\n t/t0064-oid-array.sh       | 104 -----------------\n t/unit-tests/t-oid-array.c | 222 +++++++++++++++++++++++++++++++++++++\n 6 files changed, 223 insertions(+), 152 deletions(-)\n delete mode 100644 t/helper/test-oid-array.c\n delete mode 100755 t/t0064-oid-array.sh\n create mode 100644 t/unit-tests/t-oid-array.c\n\ndiff --git a/Makefile b/Makefile\nindex 78e874099d..5060d7eff3 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -820,7 +820,6 @@ TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o\n TEST_BUILTINS_OBJS += test-match-trees.o\n TEST_BUILTINS_OBJS += test-mergesort.o\n TEST_BUILTINS_OBJS += test-mktemp.o\n-TEST_BUILTINS_OBJS += test-oid-array.o\n TEST_BUILTINS_OBJS += test-oidmap.o\n TEST_BUILTINS_OBJS += test-oidtree.o\n TEST_BUILTINS_OBJS += test-online-cpus.o\n@@ -1346,6 +1345,7 @@ UNIT_TEST_PROGRAMS += t-mem-pool\n UNIT_TEST_PROGRAMS += t-strbuf\n UNIT_TEST_PROGRAMS += t-ctype\n UNIT_TEST_PROGRAMS += t-prio-queue\n+UNIT_TEST_PROGRAMS += t-oid-array\n UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o\ndiff --git a/t/helper/test-oid-array.c b/t/helper/test-oid-array.c\ndeleted file mode 100644\nindex aafe398ef0..0000000000\n--- a/t/helper/test-oid-array.c\n+++ /dev/null\n@@ -1,45 +0,0 @@\n-#include \"test-tool.h\"\n-#include \"hex.h\"\n-#include \"oid-array.h\"\n-#include \"setup.h\"\n-#include \"strbuf.h\"\n-\n-static int print_oid(const struct object_id *oid, void *data UNUSED)\n-{\n-\tputs(oid_to_hex(oid));\n-\treturn 0;\n-}\n-\n-int cmd__oid_array(int argc UNUSED, const char **argv UNUSED)\n-{\n-\tstruct oid_array array = OID_ARRAY_INIT;\n-\tstruct strbuf line = STRBUF_INIT;\n-\tint nongit_ok;\n-\n-\tsetup_git_directory_gently(&nongit_ok);\n-\n-\twhile (strbuf_getline(&line, stdin) != EOF) {\n-\t\tconst char *arg;\n-\t\tstruct object_id oid;\n-\n-\t\tif (skip_prefix(line.buf, \"append \", &arg)) {\n-\t\t\tif (get_oid_hex(arg, &oid))\n-\t\t\t\tdie(\"not a hexadecimal oid: %s\", arg);\n-\t\t\toid_array_append(&array, &oid);\n-\t\t} else if (skip_prefix(line.buf, \"lookup \", &arg)) {\n-\t\t\tif (get_oid_hex(arg, &oid))\n-\t\t\t\tdie(\"not a hexadecimal oid: %s\", arg);\n-\t\t\tprintf(\"%d\\n\", oid_array_lookup(&array, &oid));\n-\t\t} else if (!strcmp(line.buf, \"clear\"))\n-\t\t\toid_array_clear(&array);\n-\t\telse if (!strcmp(line.buf, \"for_each_unique\"))\n-\t\t\toid_array_for_each_unique(&array, print_oid, NULL);\n-\t\telse\n-\t\t\tdie(\"unknown command: %s\", line.buf);\n-\t}\n-\n-\tstrbuf_release(&line);\n-\toid_array_clear(&array);\n-\n-\treturn 0;\n-}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 482a1e58a4..ad74bfffbe 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -42,7 +42,6 @@ static struct test_cmd cmds[] = {\n \t{ \"match-trees\", cmd__match_trees },\n \t{ \"mergesort\", cmd__mergesort },\n \t{ \"mktemp\", cmd__mktemp },\n-\t{ \"oid-array\", cmd__oid_array },\n \t{ \"oidmap\", cmd__oidmap },\n \t{ \"oidtree\", cmd__oidtree },\n \t{ \"online-cpus\", cmd__online_cpus },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex b1be7cfcf5..4f961a38c0 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -65,7 +65,6 @@ int cmd__scrap_cache_tree(int argc, const char **argv);\n int cmd__serve_v2(int argc, const char **argv);\n int cmd__sha1(int argc, const char **argv);\n int cmd__sha1_is_sha1dc(int argc, const char **argv);\n-int cmd__oid_array(int argc, const char **argv);\n int cmd__sha256(int argc, const char **argv);\n int cmd__sigchain(int argc, const char **argv);\n int cmd__simple_ipc(int argc, const char **argv);\ndiff --git a/t/t0064-oid-array.sh b/t/t0064-oid-array.sh\ndeleted file mode 100755\nindex 88c89e8f48..0000000000\n--- a/t/t0064-oid-array.sh\n+++ /dev/null\n@@ -1,104 +0,0 @@\n-#!/bin/sh\n-\n-test_description='basic tests for the oid array implementation'\n-\n-TEST_PASSES_SANITIZE_LEAK=true\n-. ./test-lib.sh\n-\n-echoid () {\n-\tprefix=\"${1:+$1 }\"\n-\tshift\n-\twhile test $# -gt 0\n-\tdo\n-\t\techo \"$prefix$ZERO_OID\" | sed -e \"s/00/$1/g\"\n-\t\tshift\n-\tdone\n-}\n-\n-test_expect_success 'ordered enumeration' '\n-\techoid \"\" 44 55 88 aa >expect &&\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techo for_each_unique\n-\t} | test-tool oid-array >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'ordered enumeration with duplicate suppression' '\n-\techoid \"\" 44 55 88 aa >expect &&\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techo for_each_unique\n-\t} | test-tool oid-array >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'lookup' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -eq 1\n-'\n-\n-test_expect_success 'lookup non-existing entry' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 33\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -lt 0\n-'\n-\n-test_expect_success 'lookup with duplicates' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -ge 3 &&\n-\ttest \"$n\" -le 5\n-'\n-\n-test_expect_success 'lookup non-existing entry with duplicates' '\n-\t{\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid append 88 44 aa 55 &&\n-\t\techoid lookup 66\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -lt 0\n-'\n-\n-test_expect_success 'lookup with almost duplicate values' '\n-\t# n-1 5s\n-\troot=$(echoid \"\" 55) &&\n-\troot=${root%5} &&\n-\t{\n-\t\tid1=\"${root}5\" &&\n-\t\tid2=\"${root}f\" &&\n-\t\techo \"append $id1\" &&\n-\t\techo \"append $id2\" &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -eq 0\n-'\n-\n-test_expect_success 'lookup with single duplicate value' '\n-\t{\n-\t\techoid append 55 55 &&\n-\t\techoid lookup 55\n-\t} | test-tool oid-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -ge 0 &&\n-\ttest \"$n\" -le 1\n-'\n-\n-test_done\ndiff --git a/t/unit-tests/t-oid-array.c b/t/unit-tests/t-oid-array.c\nnew file mode 100644\nindex 0000000000..b4f43c025d\n--- /dev/null\n+++ b/t/unit-tests/t-oid-array.c\n@@ -0,0 +1,222 @@\n+#include \"test-lib.h\"\n+#include \"hex.h\"\n+#include \"oid-array.h\"\n+#include \"strbuf.h\"\n+\n+#define INPUT \"88\", \"44\", \"aa\", \"55\"\n+#define INPUT_DUP \\\n+\t\"88\", \"44\", \"aa\", \"55\", \"88\", \"44\", \"aa\", \"55\", \"88\", \"44\", \"aa\", \"55\"\n+#define INPUT_ONLY_DUP \"55\", \"55\"\n+#define ENUMERATION_RESULT_SORTED \"44\", \"55\", \"88\", \"aa\"\n+\n+/*\n+ * allocates the memory based on the hash algorithm used and sets the length to\n+ * it.\n+ */\n+static void hex_strbuf_init(struct strbuf *hex)\n+{\n+\tstatic int sz = -1;\n+\n+\tif (sz == -1) {\n+\t\tchar *algo_env = getenv(\"GIT_TEST_DEFAULT_HASH\");\n+\t\tif (algo_env && !strcmp(algo_env, \"sha256\"))\n+\t\t\tsz = GIT_SHA256_HEXSZ;\n+\t\telse\n+\t\t\tsz = GIT_SHA1_HEXSZ;\n+\t}\n+\n+\tstrbuf_init(hex, sz);\n+\tstrbuf_setlen(hex, sz);\n+}\n+\n+/* callback function for for_each used for printing */\n+static int print_cb(const struct object_id *oid, void *data)\n+{\n+\tint *i = data;\n+\ttest_msg(\"%d. %s\", *i, oid_to_hex(oid));\n+\t*i += 1;\n+\treturn 0;\n+}\n+\n+/* prints the oid_array with a message title */\n+static void print_oid_array(struct oid_array *array, char *msg)\n+{\n+\tint i = 0;\n+\ttest_msg(\"%s\", msg);\n+\toid_array_for_each(array, print_cb, &i);\n+}\n+\n+/* fills the hex strbuf with alternating characters from 'c' */\n+static void fill_hex_strbuf(struct strbuf *hex, char *c)\n+{\n+\tsize_t i;\n+\tfor (i = 0; i < hex->len; i++)\n+\t\thex->buf[i] = (i & 1) ? c[1] : c[0];\n+}\n+\n+/* populates object_id with hexadecimal representation generated from 'c' */\n+static int get_oid_hex_input(struct object_id *oid, char *c)\n+{\n+\tint ret;\n+\tstruct strbuf hex;\n+\n+\thex_strbuf_init(&hex);\n+\tfill_hex_strbuf(&hex, c);\n+\tret = get_oid_hex_any(hex.buf, oid);\n+\tif (ret == GIT_HASH_UNKNOWN)\n+\t\ttest_msg(\"not a valid hexadecimal oid: %s\", hex.buf);\n+\tstrbuf_release(&hex);\n+\treturn ret;\n+}\n+\n+/* populates the oid_array with input from entries array */\n+static int populate_oid_array(struct oid_array *oidarray, char **entries,\n+\t\t\t      size_t len)\n+{\n+\tsize_t i;\n+\tstruct object_id oid;\n+\n+\tfor (i = 0; i < len; i++) {\n+\t\tif (!check_int(get_oid_hex_input(&oid, entries[i]), !=,\n+\t\t\t       GIT_HASH_UNKNOWN))\n+\t\t\treturn -1;\n+\t\toid_array_append(oidarray, &oid);\n+\t}\n+\treturn 0;\n+}\n+\n+/* callback function for enumeration test */\n+static int add_to_oid_array(const struct object_id *oid, void *data)\n+{\n+\tstruct oid_array *array = data;\n+\toid_array_append(array, oid);\n+\treturn 0;\n+}\n+\n+static void test_enumeration(char *input_entries[], size_t input_size,\n+\t\t\t     char *expected_entries[], size_t expected_size)\n+{\n+\tint i;\n+\tstruct oid_array input = OID_ARRAY_INIT;\n+\tstruct oid_array actual = OID_ARRAY_INIT;\n+\tstruct oid_array expect = OID_ARRAY_INIT;\n+\n+\tif (populate_oid_array(&input, input_entries, input_size) == -1)\n+\t\tgoto cleanup;\n+\n+\tif (populate_oid_array(&expect, expected_entries, expected_size) == -1)\n+\t\tgoto cleanup;\n+\n+\toid_array_for_each_unique(&input, add_to_oid_array, &actual);\n+\tif (!check_int(expect.nr, ==, expected_size) ||\n+\t    !check_int(actual.nr, ==, expected_size))\n+\t\tgoto cleanup;\n+\n+\tfor (i = 0; i < expected_size; i++) {\n+\t\tif (!check_int(oideq(&actual.oid[i], &expect.oid[i]), ==, 1)) {\n+\t\t\ttest_msg(\"failed at index %d\", i);\n+\t\t\tprint_oid_array(&expect, \"expected array content:\");\n+\t\t\tprint_oid_array(&actual, \"actual array content:\");\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t}\n+\n+cleanup:\n+\toid_array_clear(&input);\n+\toid_array_clear(&actual);\n+\toid_array_clear(&expect);\n+}\n+\n+#define ENUMERATION_INPUT(INPUT, RESULT, NAME)                                \\\n+\tstatic void t_ordered_enumeration_##NAME(void)                        \\\n+\t{                                                                     \\\n+\t\tchar *input_entries[] = { INPUT };                            \\\n+\t\tchar *expected_entries[] = { RESULT };                        \\\n+\t\tsize_t input_size = ARRAY_SIZE(input_entries);                \\\n+\t\tsize_t expected_size = ARRAY_SIZE(expected_entries);          \\\n+\t\ttest_enumeration(input_entries, input_size, expected_entries, \\\n+\t\t\t\t expected_size);                              \\\n+\t}\n+\n+ENUMERATION_INPUT(INPUT, ENUMERATION_RESULT_SORTED, non_duplicate)\n+ENUMERATION_INPUT(INPUT_DUP, ENUMERATION_RESULT_SORTED, duplicate)\n+\n+static void lookup_setup(void (*f)(struct strbuf *buf, struct oid_array *array))\n+{\n+\tstruct oid_array array = OID_ARRAY_INIT;\n+\tstruct strbuf buf;\n+\n+\thex_strbuf_init(&buf);\n+\tf(&buf, &array);\n+\toid_array_clear(&array);\n+\tstrbuf_release(&buf);\n+}\n+\n+#define LOOKUP_INPUT(INPUT, QUERY, NAME, CONDITION)                           \\\n+\tstatic void t_##NAME(struct strbuf *buf UNUSED,                       \\\n+\t\t\t     struct oid_array *array)                         \\\n+\t{                                                                     \\\n+\t\tstruct object_id oid_query;                                   \\\n+\t\tchar *input_entries[] = { INPUT };                            \\\n+\t\tsize_t input_size = ARRAY_SIZE(input_entries);                \\\n+\t\tint ret;                                                      \\\n+\t\tif (!check_int(get_oid_hex_input(&oid_query, QUERY), !=,      \\\n+\t\t\t       GIT_HASH_UNKNOWN))                             \\\n+\t\t\treturn;                                               \\\n+\t\tif (!check_int(populate_oid_array(array, input_entries,       \\\n+\t\t\t\t\t\t  input_size),                \\\n+\t\t\t       !=, -1))                                       \\\n+\t\t\treturn;                                               \\\n+\t\tret = oid_array_lookup(array, &oid_query);                    \\\n+\t\tif (!check(CONDITION)) {                                      \\\n+\t\t\tprint_oid_array(array, \"array content:\");             \\\n+\t\t\ttest_msg(\"oid query for lookup: %s\", oid_query.hash); \\\n+\t\t}                                                             \\\n+\t}\n+\n+/* ret is return value of oid_array_lookup() */\n+LOOKUP_INPUT(INPUT, \"55\", lookup, ret == 1)\n+LOOKUP_INPUT(INPUT, \"33\", lookup_nonexist, ret < 1)\n+LOOKUP_INPUT(INPUT_DUP, \"66\", lookup_nonexist_dup, ret < 0)\n+LOOKUP_INPUT(INPUT_DUP, \"55\", lookup_dup, ret >= 3 && ret <= 5)\n+LOOKUP_INPUT(INPUT_ONLY_DUP, \"55\", lookup_only_dup, ret >= 0 && ret <= 1)\n+\n+static void t_lookup_almost_dup(struct strbuf *hex, struct oid_array *array)\n+{\n+\tstruct object_id oid;\n+\n+\tfill_hex_strbuf(hex, \"55\");\n+\tif (!check_int(get_oid_hex_any(hex->buf, &oid), !=, GIT_HASH_UNKNOWN))\n+\t\treturn;\n+\n+\toid_array_append(array, &oid);\n+\t/* last character different */\n+\thex->buf[hex->len - 1] = 'f';\n+\tif (!check_int(get_oid_hex_any(hex->buf, &oid), !=, GIT_HASH_UNKNOWN))\n+\t\treturn;\n+\n+\toid_array_append(array, &oid);\n+\tif (!check_int(oid_array_lookup(array, &oid), ==, 1)) {\n+\t\tprint_oid_array(array, \"array content:\");\n+\t\ttest_msg(\"oid query for lookup: %s\", hex->buf);\n+\t}\n+}\n+\n+int cmd_main(int argc, const char **argv)\n+{\n+\tTEST(t_ordered_enumeration_non_duplicate(),\n+\t     \"ordered enumeration works\");\n+\tTEST(t_ordered_enumeration_duplicate(),\n+\t     \"ordered enumeration with duplicate suppresion works\");\n+\tTEST(lookup_setup(t_lookup), \"lookup works\");\n+\tTEST(lookup_setup(t_lookup_nonexist), \"lookup non-existant entry\");\n+\tTEST(lookup_setup(t_lookup_dup), \"lookup with duplicates works\");\n+\tTEST(lookup_setup(t_lookup_nonexist_dup),\n+\t     \"lookup non-existant entry with duplicates\");\n+\tTEST(lookup_setup(t_lookup_almost_dup),\n+\t     \"lookup with almost duplicate values works\");\n+\tTEST(lookup_setup(t_lookup_only_dup),\n+\t     \"lookup with single duplicate value works\");\n+\n+\treturn test_done();\n+}\n-- \n2.43.2\n\n"},{"id":"489254","messageId":"CZCPMSA5S0Q8.1M7XX3A8302KR@gmail.com","threadId":"60980","inReplyTo":"20240223193257.9222-1-shyamthakkar001@gmail.com","subject":"Re: [PATCH] unit-tests: convert t/helper/test-oid-array.c to unit-tests","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-23T19:37:04Z","receivedAt":"2024-02-23T19:37:08Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Sat Feb 24, 2024 at 1:02 AM IST, Ghanshyam Thakkar wrote:\n> [RFC]: I recently saw a series by Eric W. Biederman [1] which enables\n> the use of oid's with different hash algorithms into the same\n> oid_array safely. However, there were no tests added for this. So, I\n> am wondering if we should have a input format which allows us to\n> specify hash algo for each oid with its hex value. i.e. \"sha1:55\" or\n> \"sha256:55\", instead of just \"55\" and relying on GIT_TEST_DEFAULT_HASH\n> for algo. So far, I tried to imitate the existing tests but I suppose\n> this may be useful in the future if that series gets merged.\n\n[1]:\nhttps://lore.kernel.org/git/20231002024034.2611-2-ebiederm@gmail.com/\n\napologies for the noise.\n"},{"id":"489377","messageId":"CAP8UFD088GRkVQWjrBFk04_HFfiEk64Saxm2toYsci36oHgkdA@mail.gmail.com","threadId":"60980","inReplyTo":"20240223193257.9222-1-shyamthakkar001@gmail.com","subject":"Re: [PATCH] unit-tests: convert t/helper/test-oid-array.c to unit-tests","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-02-26T15:11:32Z","receivedAt":"2024-02-26T15:11:46Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Feb 23, 2024 at 8:33 PM Ghanshyam Thakkar\n<shyamthakkar001@gmail.com> wrote:\n>\n> Migrate t/helper/test-oid-array.c and t/t0064-oid-array.sh to the\n> recently added unit testing framework. This would improve runtime\n> performance and provide better debugging via displaying array content,\n> index at which the test failed etc. directly to stdout.\n\nIt might not be a good idea to start working on a GSoC project we\npropose (Move existing tests to a unit testing framework) right now.\nYou can work on it as part of your GSoC application, to show an\nexample of how you would do it, and we might review that as part of\nreviewing your application. But for such a project if many candidates\nstarted working on it and sent patches to the mailing list before they\nget selected, then the project might be nearly finished before the\nGSoC even starts.\n\nSo I think it would be better to work on other things instead, like\nperhaps reviewing other people's work or working on other bug fixes or\nfeatures. Anyway now that this is on the mailing list, I might as well\nreview it as it could help with your application. But please consider\nworking on other things.\n\n> There is only one change in the new testing approach. In the previous\n> testing method, a new repo gets initialized for the test according to\n> GIT_TEST_DEFAULT_HASH algorithm.\n\nIt looks like this happens in \"t/test-lib-functions.sh\", right?\nTelling a bit more about how and where that happens might help\nreviewers who would like to take a look.\n\n> In unit testing however, we do not\n> need to initialize the repo. We can set the length of the hexadecimal\n> strbuf according to the algorithm used directly.\n\nSo is your patch doing that or not? It might be better to be explicit.\nAlso if 'strbuf's are used, then is it really worth it to set their\nlength in advance, instead of just letting them grow to the right\nlength as we add hex to them?\n\n> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>\n> ---\n> [RFC]: I recently saw a series by Eric W. Biederman [1] which enables\n> the use of oid's with different hash algorithms into the same\n> oid_array safely. However, there were no tests added for this. So, I\n> am wondering if we should have a input format which allows us to\n> specify hash algo for each oid with its hex value. i.e. \"sha1:55\" or\n> \"sha256:55\", instead of just \"55\" and relying on GIT_TEST_DEFAULT_HASH\n> for algo. So far, I tried to imitate the existing tests but I suppose\n> this may be useful in the future if that series gets merged.\n\nThe fact that there is a series touching the same area might also hint\nthat it might not be the right time to work on this.\n\n> diff --git a/t/unit-tests/t-oid-array.c b/t/unit-tests/t-oid-array.c\n> new file mode 100644\n> index 0000000000..b4f43c025d\n> --- /dev/null\n> +++ b/t/unit-tests/t-oid-array.c\n> @@ -0,0 +1,222 @@\n> +#include \"test-lib.h\"\n> +#include \"hex.h\"\n> +#include \"oid-array.h\"\n> +#include \"strbuf.h\"\n> +\n> +#define INPUT \"88\", \"44\", \"aa\", \"55\"\n> +#define INPUT_DUP \\\n> +       \"88\", \"44\", \"aa\", \"55\", \"88\", \"44\", \"aa\", \"55\", \"88\", \"44\", \"aa\", \"55\"\n\nCan you reuse INPUT in INPUT_DUP?\n\n> +#define INPUT_ONLY_DUP \"55\", \"55\"\n> +#define ENUMERATION_RESULT_SORTED \"44\", \"55\", \"88\", \"aa\"\n> +\n> +/*\n> + * allocates the memory based on the hash algorithm used and sets the length to\n> + * it.\n> + */\n> +static void hex_strbuf_init(struct strbuf *hex)\n> +{\n> +       static int sz = -1;\n> +\n> +       if (sz == -1) {\n> +               char *algo_env = getenv(\"GIT_TEST_DEFAULT_HASH\");\n> +               if (algo_env && !strcmp(algo_env, \"sha256\"))\n> +                       sz = GIT_SHA256_HEXSZ;\n> +               else\n> +                       sz = GIT_SHA1_HEXSZ;\n> +       }\n> +\n> +       strbuf_init(hex, sz);\n> +       strbuf_setlen(hex, sz);\n> +}\n\nA strbuf can grow when we add stuff to it. We don't need to know its\nsize in advance. So I am not sure this function is actually useful.\n\n> +/* fills the hex strbuf with alternating characters from 'c' */\n> +static void fill_hex_strbuf(struct strbuf *hex, char *c)\n> +{\n> +       size_t i;\n> +       for (i = 0; i < hex->len; i++)\n> +               hex->buf[i] = (i & 1) ? c[1] : c[0];\n\nThere is strbuf_addch() to add a single char to a strbuf, or\nstrbuf_add() and strbuf_addstr() to add many chars at once.\n\n> +}\n"},{"id":"489412","messageId":"CZF8YROS9RVC.9H2EKYCF08VK@gmail.com","threadId":"60980","inReplyTo":"CAP8UFD088GRkVQWjrBFk04_HFfiEk64Saxm2toYsci36oHgkdA@mail.gmail.com","subject":"Re: [PATCH] unit-tests: convert t/helper/test-oid-array.c to unit-tests","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-26T19:11:24Z","receivedAt":"2024-02-26T19:11:29Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Mon Feb 26, 2024 at 8:41 PM IST, Christian Couder wrote:\n> It might not be a good idea to start working on a GSoC project we\n> propose (Move existing tests to a unit testing framework) right now.\n> You can work on it as part of your GSoC application, to show an\n> example of how you would do it, and we might review that as part of\n> reviewing your application. But for such a project if many candidates\n> started working on it and sent patches to the mailing list before they\n> get selected, then the project might be nearly finished before the\n> GSoC even starts.\n>\n> So I think it would be better to work on other things instead, like\n> perhaps reviewing other people's work or working on other bug fixes or\n> features. Anyway now that this is on the mailing list, I might as well\n> review it as it could help with your application. But please consider\n> working on other things.\n\nI understand and will work on other things.\n\n> > There is only one change in the new testing approach. In the previous\n> > testing method, a new repo gets initialized for the test according to\n> > GIT_TEST_DEFAULT_HASH algorithm.\n>\n> It looks like this happens in \"t/test-lib-functions.sh\", right?\n> Telling a bit more about how and where that happens might help\n> reviewers who would like to take a look.\n\nYeah, that is happens in \"t/test-lib-functions.sh\". I will update the\ncommit message to describe this better.\n>\n> > In unit testing however, we do not\n> > need to initialize the repo. We can set the length of the hexadecimal\n> > strbuf according to the algorithm used directly.\n>\n> So is your patch doing that or not? It might be better to be explicit.\n> Also if 'strbuf's are used, then is it really worth it to set their\n> length in advance, instead of just letting them grow to the right\n> length as we add hex to them?\n\nI thought of it like this: If we were to just let them grow, then we\nwould need separate logic for reusing that strbuf or use a different\none everytime since it always grows. By separating allocation\n(hex_strbuf_init) and manipulation (fill_hex_strbuf), that same strbuf\ncan be reused for different hex values.\n\nBut, none of the test currently need to reuse the same strbuf, so I\nsuppose it is better to just let it grow and even if the need arises we\ncan use strbuf_splice().\n\n\n> > Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>\n> > ---\n> > [RFC]: I recently saw a series by Eric W. Biederman [1] which enables\n> > the use of oid's with different hash algorithms into the same\n> > oid_array safely. However, there were no tests added for this. So, I\n> > am wondering if we should have a input format which allows us to\n> > specify hash algo for each oid with its hex value. i.e. \"sha1:55\" or\n> > \"sha256:55\", instead of just \"55\" and relying on GIT_TEST_DEFAULT_HASH\n> > for algo. So far, I tried to imitate the existing tests but I suppose\n> > this may be useful in the future if that series gets merged.\n>\n> The fact that there is a series touching the same area might also hint\n> that it might not be the right time to work on this.\n\nI understand.\n\n> > diff --git a/t/unit-tests/t-oid-array.c b/t/unit-tests/t-oid-array.c\n> > new file mode 100644\n> > index 0000000000..b4f43c025d\n> > --- /dev/null\n> > +++ b/t/unit-tests/t-oid-array.c\n> > @@ -0,0 +1,222 @@\n> > +#include \"test-lib.h\"\n> > +#include \"hex.h\"\n> > +#include \"oid-array.h\"\n> > +#include \"strbuf.h\"\n> > +\n> > +#define INPUT \"88\", \"44\", \"aa\", \"55\"\n> > +#define INPUT_DUP \\\n> > +       \"88\", \"44\", \"aa\", \"55\", \"88\", \"44\", \"aa\", \"55\", \"88\", \"44\", \"aa\", \"55\"\n>\n> Can you reuse INPUT in INPUT_DUP?\n\nYeah, that would be more clearer.\n\n> > +#define INPUT_ONLY_DUP \"55\", \"55\"\n> > +#define ENUMERATION_RESULT_SORTED \"44\", \"55\", \"88\", \"aa\"\n> > +\n> > +/*\n> > + * allocates the memory based on the hash algorithm used and sets the length to\n> > + * it.\n> > + */\n> > +static void hex_strbuf_init(struct strbuf *hex)\n> > +{\n> > +       static int sz = -1;\n> > +\n> > +       if (sz == -1) {\n> > +               char *algo_env = getenv(\"GIT_TEST_DEFAULT_HASH\");\n> > +               if (algo_env && !strcmp(algo_env, \"sha256\"))\n> > +                       sz = GIT_SHA256_HEXSZ;\n> > +               else\n> > +                       sz = GIT_SHA1_HEXSZ;\n> > +       }\n> > +\n> > +       strbuf_init(hex, sz);\n> > +       strbuf_setlen(hex, sz);\n> > +}\n>\n> A strbuf can grow when we add stuff to it. We don't need to know its\n> size in advance. So I am not sure this function is actually useful.\n\nYeah, this was mainly for deciding the hash algorithm but that logic can\nbe moved to fill_hex_strbuf.\n\n> > +/* fills the hex strbuf with alternating characters from 'c' */\n> > +static void fill_hex_strbuf(struct strbuf *hex, char *c)\n> > +{\n> > +       size_t i;\n> > +       for (i = 0; i < hex->len; i++)\n> > +               hex->buf[i] = (i & 1) ? c[1] : c[0];\n>\n> There is strbuf_addch() to add a single char to a strbuf, or\n> strbuf_add() and strbuf_addstr() to add many chars at once.\nWill update it.\n\nThank you for the feedback and review.\n\n"},{"id":"489479","messageId":"CAP8UFD0yOXPyTvRCXxhoWXASW+HP230jVMCDzipg5PLAyVXJUA@mail.gmail.com","threadId":"60980","inReplyTo":"CZF8YROS9RVC.9H2EKYCF08VK@gmail.com","subject":"Re: [PATCH] unit-tests: convert t/helper/test-oid-array.c to unit-tests","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-02-27T09:59:06Z","receivedAt":"2024-02-27T09:59:21Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Feb 26, 2024 at 8:11 PM Ghanshyam Thakkar\n<shyamthakkar001@gmail.com> wrote:\n>\n> On Mon Feb 26, 2024 at 8:41 PM IST, Christian Couder wrote:\n\n> > So I think it would be better to work on other things instead, like\n> > perhaps reviewing other people's work or working on other bug fixes or\n> > features. Anyway now that this is on the mailing list, I might as well\n> > review it as it could help with your application. But please consider\n> > working on other things.\n>\n> I understand and will work on other things.\n\nThanks!\n\n> > > In unit testing however, we do not\n> > > need to initialize the repo. We can set the length of the hexadecimal\n> > > strbuf according to the algorithm used directly.\n> >\n> > So is your patch doing that or not? It might be better to be explicit.\n> > Also if 'strbuf's are used, then is it really worth it to set their\n> > length in advance, instead of just letting them grow to the right\n> > length as we add hex to them?\n>\n> I thought of it like this: If we were to just let them grow, then we\n> would need separate logic for reusing that strbuf or use a different\n> one everytime since it always grows. By separating allocation\n> (hex_strbuf_init) and manipulation (fill_hex_strbuf), that same strbuf\n> can be reused for different hex values.\n>\n> But, none of the test currently need to reuse the same strbuf, so I\n> suppose it is better to just let it grow and even if the need arises we\n> can use strbuf_splice().\n\nIt's not a problem to use a new strbuf for each different hex value.\nTests don't need a lot of performance as they are used mostly by\ndevelopers, not by everyone using Git. Also if you want to reuse a\nstrbuf, you can just use strbuf_reset() on it and then reuse it.\n"}]}