# [GSoC][PATCH] unit-tests: add tests for oidset.h 10 messages from 2024-08-24 to 2024-09-30. Participants: Ghanshyam Thakkar, Patrick Steinhardt, Christian Couder, Junio C Hamano. Thread: https://gitlist.dev/t/62000 ## Ghanshyam Thakkar, 2024-08-24 17:20 Subject: [GSoC][PATCH] unit-tests: add tests for oidset.h Message-ID: <20240824172028.39419-1-shyamthakkar001@gmail.com> URL: https://gitlist.dev/e/20240824172028.39419-1-shyamthakkar001%40gmail.com ``` Add tests for oidset.h library, which were not previously present using the unit testing framework. This imposes a new restriction of running the test from the 't/' and 't/unit-tests/bin' for constructing the path to the test files which are used by t_parse_file(), which tests the parsing of object_ids from a file. This restriction is similar to the one we already have for end-to-end tests, wherein, we can only run those tests from 't/'. The addition of allowing 't/unit-tests/bin' for allowing to run tests from is for running individual unit tests, which is not currently possible via any 'make' target. And 'make unit-tests-test-tool' also runs from 't/unit-tests/bin' Mentored-by: Christian Couder Mentored-by: Kaartic Sivaraam Signed-off-by: Ghanshyam Thakkar --- I know there is some hesitance from the community in imposing the restriction of running the unit tests from certain directories, so if this case does not justify imposing such a restriction, I am fine with removing t_parse_file() in the next version. Thanks. Makefile | 1 + t/unit-tests/lib-oid.c | 2 +- t/unit-tests/lib-oid.h | 1 + t/unit-tests/t-oidset.c | 222 ++++++++++++++++++++++++++++++ t/unit-tests/t-oidset/sha1-oids | 10 ++ t/unit-tests/t-oidset/sha256-oids | 10 ++ 6 files changed, 245 insertions(+), 1 deletion(-) create mode 100644 t/unit-tests/t-oidset.c create mode 100644 t/unit-tests/t-oidset/sha1-oids create mode 100644 t/unit-tests/t-oidset/sha256-oids diff --git a/Makefile b/Makefile index e298c8b55e..5c1762fa1b 100644 --- a/Makefile +++ b/Makefile @@ -1338,6 +1338,7 @@ UNIT_TEST_PROGRAMS += t-hash UNIT_TEST_PROGRAMS += t-hashmap UNIT_TEST_PROGRAMS += t-mem-pool UNIT_TEST_PROGRAMS += t-oidmap +UNIT_TEST_PROGRAMS += t-oidset UNIT_TEST_PROGRAMS += t-oidtree UNIT_TEST_PROGRAMS += t-prio-queue UNIT_TEST_PROGRAMS += t-reftable-basics diff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c index 37105f0a8f..8f0ccac532 100644 --- a/t/unit-tests/lib-oid.c +++ b/t/unit-tests/lib-oid.c @@ -3,7 +3,7 @@ #include "strbuf.h" #include "hex.h" -static int init_hash_algo(void) +int init_hash_algo(void) { static int algo = -1; diff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h index 8d2acca768..fc3e7aa376 100644 --- a/t/unit-tests/lib-oid.h +++ b/t/unit-tests/lib-oid.h @@ -14,4 +14,5 @@ */ int get_oid_arbitrary_hex(const char *s, struct object_id *oid); +int init_hash_algo(void); #endif /* LIB_OID_H */ diff --git a/t/unit-tests/t-oidset.c b/t/unit-tests/t-oidset.c new file mode 100644 index 0000000000..4a63f9ea94 --- /dev/null +++ b/t/unit-tests/t-oidset.c @@ -0,0 +1,222 @@ +#include "test-lib.h" +#include "oidset.h" +#include "lib-oid.h" +#include "hex.h" +#include "strbuf.h" + +static const char *const hex_input[] = { "00", "11", "22", "33", "aa", "cc" }; + +static void strbuf_test_data_path(struct strbuf *buf, int hash_algo) +{ + strbuf_getcwd(buf); + strbuf_strip_suffix(buf, "/unit-tests/bin"); + strbuf_addf(buf, "/unit-tests/t-oidset/%s", + hash_algo == GIT_HASH_SHA1 ? "sha1-oids" : "sha256-oids"); +} + +static void setup(void (*f)(struct oidset *st)) +{ + struct oidset st = OIDSET_INIT; + struct object_id oid; + int ret = 0; + + if (!check_int(oidset_size(&st), ==, 0)) { + test_skip_all("OIDSET_INIT is broken"); + return; + } + + for (size_t i = 0; i < ARRAY_SIZE(hex_input); i++) { + if ((ret = get_oid_arbitrary_hex(hex_input[i], &oid))) + break; + if (!check_int((ret = oidset_insert(&st, &oid)), ==, 0)) + break; + } + + if (!ret && check_int(oidset_size(&st), ==, ARRAY_SIZE(hex_input))) + f(&st); + + oidset_clear(&st); +} + +static void t_contains(struct oidset *st) +{ + struct object_id oid; + + for (size_t i = 0; i < ARRAY_SIZE(hex_input); i++) { + if (!get_oid_arbitrary_hex(hex_input[i], &oid)) { + if (!check_int(oidset_contains(st, &oid), ==, 1)) + test_msg("oid: %s", oid_to_hex(&oid)); + } + } + + if (!get_oid_arbitrary_hex("55", &oid)) + check_int(oidset_contains(st, &oid), ==, 0); +} + +static void t_insert_dup(struct oidset *st) +{ + struct object_id oid; + + if (!get_oid_arbitrary_hex("11", &oid)) + check_int(oidset_insert(st, &oid), ==, 1); + + if (!get_oid_arbitrary_hex("aa", &oid)) + check_int(oidset_insert(st, &oid), ==, 1); + + check_int(oidset_size(st), ==, ARRAY_SIZE(hex_input)); +} + +static void t_insert_from_set(struct oidset *st_src) +{ + struct oidset st_dest = OIDSET_INIT; + struct oidset_iter iter_src, iter_dest; + struct object_id *oid_src, *oid_dest; + struct object_id oid; + size_t count = 0; + + oidset_insert_from_set(&st_dest, st_src); + check_int(oidset_size(st_src), ==, ARRAY_SIZE(hex_input)); + check_int(oidset_size(&st_dest), ==, oidset_size(st_src)); + + oidset_iter_init(st_src, &iter_src); + oidset_iter_init(&st_dest, &iter_dest); + + /* check that oidset_insert_from_set() makes a copy of the object_ids */ + while ((oid_src = oidset_iter_next(&iter_src)) && + (oid_dest = oidset_iter_next(&iter_dest))) { + check(oid_src != oid_dest); + count++; + } + check_int(count, ==, ARRAY_SIZE(hex_input)); + + for (size_t i = 0; i < ARRAY_SIZE(hex_input); i++) { + if (!get_oid_arbitrary_hex(hex_input[i], &oid)) { + if (!check_int(oidset_contains(&st_dest, &oid), ==, 1)) + test_msg("oid: %s", oid_to_hex(&oid)); + } + } + + if (!get_oid_arbitrary_hex("55", &oid)) + check_int(oidset_contains(&st_dest, &oid), ==, 0); + oidset_clear(&st_dest); +} + +static void t_remove(struct oidset *st) +{ + struct object_id oid; + + if (!get_oid_arbitrary_hex("55", &oid)) { + check_int(oidset_remove(st, &oid), ==, 0); + check_int(oidset_size(st), ==, ARRAY_SIZE(hex_input)); + } + + if (!get_oid_arbitrary_hex("22", &oid)) { + check_int(oidset_remove(st, &oid), ==, 1); + check_int(oidset_size(st), ==, ARRAY_SIZE(hex_input) - 1); + check_int(oidset_contains(st, &oid), ==, 0); + } + + if (!get_oid_arbitrary_hex("cc", &oid)) { + check_int(oidset_remove(st, &oid), ==, 1); + check_int(oidset_size(st), ==, ARRAY_SIZE(hex_input) - 2); + check_int(oidset_contains(st, &oid), ==, 0); + } + + if (!get_oid_arbitrary_hex("00", &oid)) + { + /* remove a value inserted more than once */ + check_int(oidset_insert(st, &oid), ==, 1); + check_int(oidset_remove(st, &oid), ==, 1); + check_int(oidset_size(st), ==, ARRAY_SIZE(hex_input) - 3); + check_int(oidset_contains(st, &oid), ==, 0); + } + + if (!get_oid_arbitrary_hex("22", &oid)) + check_int(oidset_remove(st, &oid), ==, 0); +} + +static int input_contains(struct object_id *oid, char *seen) +{ + for (size_t i = 0; i < ARRAY_SIZE(hex_input); i++) { + struct object_id oid_input; + if (get_oid_arbitrary_hex(hex_input[i], &oid_input)) + return -1; + if (oideq(&oid_input, oid)) { + if (seen[i]) + return 2; + seen[i] = 1; + return 0; + } + } + return 1; +} + +static void t_iterate(struct oidset *st) +{ + struct oidset_iter iter; + struct object_id *oid; + char seen[ARRAY_SIZE(hex_input)] = { 0 }; + int count = 0; + + oidset_iter_init(st, &iter); + while ((oid = oidset_iter_next(&iter))) { + int ret; + if (!check_int((ret = input_contains(oid, seen)), ==, 0)) { + switch (ret) { + case -1: + break; /* handled by get_oid_arbitrary_hex() */ + case 1: + test_msg("obtained object_id was not given in the input\n" + " object_id: %s", oid_to_hex(oid)); + break; + case 2: + test_msg("duplicate object_id detected\n" + " object_id: %s", oid_to_hex(oid)); + break; + } + } else { + count++; + } + } + check_int(count, ==, ARRAY_SIZE(hex_input)); + check_int(oidset_size(st), ==, ARRAY_SIZE(hex_input)); +} + +static void t_parse_file(void) +{ + struct strbuf path = STRBUF_INIT; + struct oidset st = OIDSET_INIT; + struct object_id oid; + int hash_algo = init_hash_algo(); + + if (!check_int(hash_algo, !=, GIT_HASH_UNKNOWN)) + return; + + strbuf_test_data_path(&path, hash_algo); + oidset_parse_file(&st, path.buf, &hash_algos[hash_algo]); + check_int(oidset_size(&st), ==, 6); + + if (!get_oid_arbitrary_hex("00", &oid)) + check_int(oidset_contains(&st, &oid), ==, 1); + if (!get_oid_arbitrary_hex("44", &oid)) + check_int(oidset_contains(&st, &oid), ==, 1); + if (!get_oid_arbitrary_hex("cc", &oid)) + check_int(oidset_contains(&st, &oid), ==, 1); + + if (!get_oid_arbitrary_hex("11", &oid)) + check_int(oidset_contains(&st, &oid), ==, 0); + + oidset_clear(&st); + strbuf_release(&path); +} + +int cmd_main(int argc UNUSED, const char **argv UNUSED) +{ + TEST(setup(t_contains), "contains works"); + TEST(setup(t_insert_dup), "insert an already inserted value works"); + TEST(setup(t_insert_from_set), "insert from one set to another works"); + TEST(setup(t_remove), "remove works"); + TEST(setup(t_iterate), "iteration works"); + TEST(t_parse_file(), "parsing from file works"); + return test_done(); +} diff --git a/t/unit-tests/t-oidset/sha1-oids b/t/unit-tests/t-oidset/sha1-oids new file mode 100644 index 0000000000..881f45e661 --- /dev/null +++ b/t/unit-tests/t-oidset/sha1-oids @@ -0,0 +1,10 @@ +# comments are ignored +0000000000000000000000000000000000000000 +9900000000000000000000000000000000000000 +dd00000000000000000000000000000000000000 + + 4400000000000000000000000000000000000000 + +bb00000000000000000000000000000000000000 # test comment +cc00000000000000000000000000000000000000 +# 1100000000000000000000000000000000000000 diff --git a/t/unit-tests/t-oidset/sha256-oids b/t/unit-tests/t-oidset/sha256-oids new file mode 100644 index 0000000000..3c1c687812 --- /dev/null +++ b/t/unit-tests/t-oidset/sha256-oids @@ -0,0 +1,10 @@ +# comments are ignored +0000000000000000000000000000000000000000000000000000000000000000 +9900000000000000000000000000000000000000000000000000000000000000 +dd00000000000000000000000000000000000000000000000000000000000000 + + 4400000000000000000000000000000000000000000000000000000000000000 + +bb00000000000000000000000000000000000000000000000000000000000000 # test comment +cc00000000000000000000000000000000000000000000000000000000000000 +# 1100000000000000000000000000000000000000000000000000000000000000 -- 2.46.0 ``` ## Patrick Steinhardt, 2024-08-26 07:02 Subject: Re: [GSoC][PATCH] unit-tests: add tests for oidset.h Message-ID: URL: https://gitlist.dev/e/Zswok6P5dYf7ob5P%40tanuki In-Reply-To: <20240824172028.39419-1-shyamthakkar001@gmail.com> ``` On Sat, Aug 24, 2024 at 10:50:23PM +0530, Ghanshyam Thakkar wrote: > Add tests for oidset.h library, which were not previously present using > the unit testing framework. > > This imposes a new restriction of running the test from the 't/' and > 't/unit-tests/bin' for constructing the path to the test files which > are used by t_parse_file(), which tests the parsing of object_ids from > a file. This restriction is similar to the one we already have for > end-to-end tests, wherein, we can only run those tests from 't/'. The > addition of allowing 't/unit-tests/bin' for allowing to run tests from > is for running individual unit tests, which is not currently possible > via any 'make' target. And 'make unit-tests-test-tool' also runs from > 't/unit-tests/bin' > > Mentored-by: Christian Couder > Mentored-by: Kaartic Sivaraam > Signed-off-by: Ghanshyam Thakkar > --- > I know there is some hesitance from the community in imposing the > restriction of running the unit tests from certain directories, so > if this case does not justify imposing such a restriction, I am fine > with removing t_parse_file() in the next version. Another option would be to set up a preprocessor define that gives us the path to the unit test root directory. But I'm also okayish with the current version. If it turns out to be annoying we can still iterate. > diff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c > index 37105f0a8f..8f0ccac532 100644 > --- a/t/unit-tests/lib-oid.c > +++ b/t/unit-tests/lib-oid.c > @@ -3,7 +3,7 @@ > #include "strbuf.h" > #include "hex.h" > > -static int init_hash_algo(void) > +int init_hash_algo(void) > { > static int algo = -1; > > diff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h > index 8d2acca768..fc3e7aa376 100644 > --- a/t/unit-tests/lib-oid.h > +++ b/t/unit-tests/lib-oid.h > @@ -14,4 +14,5 @@ > */ > int get_oid_arbitrary_hex(const char *s, struct object_id *oid); > > +int init_hash_algo(void); > #endif /* LIB_OID_H */ Let's add a comment to explain what this does. Also, do we maybe want to give it a name that ties it to the unit tests? `init_hash_algo()` is quite generic and may easily lead to conflicting symbol names. Maybe something like `t_oid_init_hash_algo()`. `t_` to indicate that it is testing-related, `oid_` indicates that it's part of "lib-oid.h", and the remainder describes what it does. > diff --git a/t/unit-tests/t-oidset.c b/t/unit-tests/t-oidset.c > new file mode 100644 > index 0000000000..4a63f9ea94 > --- /dev/null > +++ b/t/unit-tests/t-oidset.c > @@ -0,0 +1,222 @@ > +#include "test-lib.h" > +#include "oidset.h" > +#include "lib-oid.h" > +#include "hex.h" > +#include "strbuf.h" > + > +static const char *const hex_input[] = { "00", "11", "22", "33", "aa", "cc" }; I think we typically write this `const char * const`, with another space between `*` and `const`. > +static void strbuf_test_data_path(struct strbuf *buf, int hash_algo) > +{ > + strbuf_getcwd(buf); > + strbuf_strip_suffix(buf, "/unit-tests/bin"); > + strbuf_addf(buf, "/unit-tests/t-oidset/%s", > + hash_algo == GIT_HASH_SHA1 ? "sha1-oids" : "sha256-oids"); > +} I wouldn't prefix this with `strbuf_`, as it is not part of the strbuf subsystem. The function just happens to use a strbuf. > +static void setup(void (*f)(struct oidset *st)) I was wondering what `st` stands for. I'd either call it just `s` or `set`. > +{ > + struct oidset st = OIDSET_INIT; > + struct object_id oid; > + int ret = 0; > + > + if (!check_int(oidset_size(&st), ==, 0)) { > + test_skip_all("OIDSET_INIT is broken"); > + return; > + } > + > + for (size_t i = 0; i < ARRAY_SIZE(hex_input); i++) { > + if ((ret = get_oid_arbitrary_hex(hex_input[i], &oid))) > + break; > + if (!check_int((ret = oidset_insert(&st, &oid)), ==, 0)) > + break; > + } In both of these cases I'd split out the assignment into a separate line. While the first instance is likely fine, the second instance makes me a bit uneasy as it is a macro. I generally do not trust macros to do the correct thing when being passed a statement with side effects. [snip] > +static int input_contains(struct object_id *oid, char *seen) > +{ > + for (size_t i = 0; i < ARRAY_SIZE(hex_input); i++) { > + struct object_id oid_input; > + if (get_oid_arbitrary_hex(hex_input[i], &oid_input)) > + return -1; > + if (oideq(&oid_input, oid)) { > + if (seen[i]) > + return 2; > + seen[i] = 1; > + return 0; > + } > + } > + return 1; > +} This function is somewhat confusing. Contains what? What are the parameters? Is one of them the expectation, the other one the actual state? What do we compare against? I think it would help to get it a more descriptive name that says what we check for and remove the dependence on global state. Also, the `seen` array does not seem to be used by any caller. So maybe we should allocate it ourselves in this function such that it is self-contained. It requires more allocations, sure, but I highly doubt that this is going to be important in this test. Patrick ``` ## Christian Couder, 2024-08-26 09:31 Subject: Re: [GSoC][PATCH] unit-tests: add tests for oidset.h Message-ID: URL: https://gitlist.dev/e/CAP8UFD2yTMNmx0n1jhOu7dz_4XeOyTy1iLmRWYmuf9QJf75hsQ%40mail.gmail.com In-Reply-To: <20240824172028.39419-1-shyamthakkar001@gmail.com> ``` On Sat, Aug 24, 2024 at 7:20 PM Ghanshyam Thakkar wrote: > > Add tests for oidset.h library, which were not previously present using > the unit testing framework. It might be interesting to also say if there are tests for oidset in the end-to-end tests, not just in the unit test framework. Also I think oidset.h is more an API than a library. > This imposes a new restriction of running the test from the 't/' and > 't/unit-tests/bin' Either "from 't/' and 't/unit-tests/bin'" or "from the 't/' and 't/unit-tests/bin' directory" would be a bit better. > for constructing the path to the test files which > are used by t_parse_file(), which tests the parsing of object_ids from > a file. This might be clearer if it mentioned that t_parse_file() actually tests oidset_parse_file() which is part of the oidset.h API. > This restriction is similar to the one we already have for > end-to-end tests, wherein, we can only run those tests from 't/'. Ok. > The > addition of allowing 't/unit-tests/bin' for allowing to run tests from > is for running individual unit tests, Maybe: "Allowing to run tests from 't/unit-tests/bin', in addition to 't/', makes it possible to run individual unit tests," > which is not currently possible > via any 'make' target. And 'make unit-tests-test-tool' also runs from > 't/unit-tests/bin' It would be nice if you gave a few examples of commands that can be run after this patch while they didn't work before it. > Mentored-by: Christian Couder > Mentored-by: Kaartic Sivaraam > Signed-off-by: Ghanshyam Thakkar > --- > I know there is some hesitance from the community in imposing the > restriction of running the unit tests from certain directories, so > if this case does not justify imposing such a restriction, I am fine > with removing t_parse_file() in the next version. My opinion is that it might be good to remove t_parse_file() for now, as testing oidset_parse_file() might not be so important. Later an iteration in a separate patch could then perhap add it while better discussing if the restrictions that come with adding it are worth it. > diff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c > index 37105f0a8f..8f0ccac532 100644 > --- a/t/unit-tests/lib-oid.c > +++ b/t/unit-tests/lib-oid.c > @@ -3,7 +3,7 @@ > #include "strbuf.h" > #include "hex.h" > > -static int init_hash_algo(void) > +int init_hash_algo(void) > { > static int algo = -1; > > diff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h > index 8d2acca768..fc3e7aa376 100644 > --- a/t/unit-tests/lib-oid.h > +++ b/t/unit-tests/lib-oid.h > @@ -14,4 +14,5 @@ > */ > int get_oid_arbitrary_hex(const char *s, struct object_id *oid); > > +int init_hash_algo(void); > #endif /* LIB_OID_H */ It seems that the changes above will go away when some patches you already sent will be merged, which should happen soon. It might have been nice to say this in the section after the "---" line. > +static void t_parse_file(void) > +{ > + struct strbuf path = STRBUF_INIT; > + struct oidset st = OIDSET_INIT; > + struct object_id oid; > + int hash_algo = init_hash_algo(); > + > + if (!check_int(hash_algo, !=, GIT_HASH_UNKNOWN)) > + return; If initializing the hash algo fails here, it is likely because it already failed when get_oid_arbitrary_hex() (which initializes it) was called in the tests before this one. So I think it might be even better to move the above hash algo initialization code to setup() and make setup() error out in case the initialization fails. Then setup() could pass 'hash_algo' to all the functions it calls, even if some of them don't use it. > + strbuf_test_data_path(&path, hash_algo); > + oidset_parse_file(&st, path.buf, &hash_algos[hash_algo]); > + check_int(oidset_size(&st), ==, 6); > + > + if (!get_oid_arbitrary_hex("00", &oid)) > + check_int(oidset_contains(&st, &oid), ==, 1); > + if (!get_oid_arbitrary_hex("44", &oid)) > + check_int(oidset_contains(&st, &oid), ==, 1); > + if (!get_oid_arbitrary_hex("cc", &oid)) > + check_int(oidset_contains(&st, &oid), ==, 1); > + > + if (!get_oid_arbitrary_hex("11", &oid)) > + check_int(oidset_contains(&st, &oid), ==, 0); > + > + oidset_clear(&st); > + strbuf_release(&path); > +} Thanks. ``` ## Junio C Hamano, 2024-08-26 15:46 Subject: Re: [GSoC][PATCH] unit-tests: add tests for oidset.h Message-ID: URL: https://gitlist.dev/e/xmqqttf7mgmm.fsf%40gitster.g In-Reply-To: ``` Christian Couder writes: >> Add tests for oidset.h library, which were not previously present using >> the unit testing framework. > > It might be interesting to also say if there are tests for oidset in > the end-to-end tests, not just in the unit test framework. Also I > think oidset.h is more an API than a library. Thanks for pointing these out; 100% agreed. >> This imposes a new restriction of running the test from the 't/' and >> 't/unit-tests/bin' I thought we just got rid of such an restriction during the review of another unit-test topic? If this is a recurring theme, perhaps we should teach t/unit-test/test-lib.c a few ways to specify where the auxiliary files for unit-tests are (e.g. "-d " command line option, or $GIT_UNIT_TEST_DATA_DIR environment variable). Even though the end-to-end tests do not allow you to start them from an arbitrary directory (it shouldn't be a rocket science to teach them to do so, though), they can run in an arbitrary place with the "--root" option without hindering its ability to read its auxiliary data files, because they can learn where the t/ directory is by looking at $TEST_DIRECTORY and a few other variables. A similar idea should be applicable to the unit-tests framework. Thanks. ``` ## Junio C Hamano, 2024-09-26 18:28 Subject: Re: [GSoC][PATCH] unit-tests: add tests for oidset.h Message-ID: URL: https://gitlist.dev/e/xmqqy13ei819.fsf%40gitster.g In-Reply-To: ``` Christian Couder writes: > On Sat, Aug 24, 2024 at 7:20 PM Ghanshyam Thakkar > wrote: >> >> Add tests for oidset.h library, which were not previously present using >> the unit testing framework. > > It might be interesting to also say if there are tests for oidset in > the end-to-end tests, not just in the unit test framework. Also I > think oidset.h is more an API than a library. > ... > If initializing the hash algo fails here, it is likely because it > already failed when get_oid_arbitrary_hex() (which initializes it) was > called in the tests before this one. So I think it might be even > better to move the above hash algo initialization code to setup() and > make setup() error out in case the initialization fails. Then setup() > could pass 'hash_algo' to all the functions it calls, even if some of > them don't use it. > ... > > Thanks. While reviewing the "What's cooking" list of topics after tagging -rc0 of this development cycle, I noticed that this topic from late August has been expecting but not yet seeing an update. As discussed elsewhere on the "Project Tracking" thread, I am in favor of formally adopting a policy to discard a topic from 'seen' after being inactive for 3 weeks, without having seen a clear consensus that it is good enough to be moved to 'next'. Interested parties are still free to revive the topic even after such a discard event. Side note: The definition of being "inactive" for the purpose of the policy is that nobody has discussed the topic, no new iteration of the topic was posted, and no responses to the review comments were given. I'll discard this one by the end of this week unless the topic sees any activity. It looks to me that the project decided that a longer term direction to adopt "clar" as the unit-tests framework, so this patch would need to be written even if it were perfect in the old world order anyway. Thanks. ``` ## Junio C Hamano, 2024-09-26 19:12 Subject: [PATCH] howto-maintain-git: discarding inactive topics Message-ID: URL: https://gitlist.dev/e/xmqqikuii60q.fsf%40gitster.g In-Reply-To: ``` When a patch series happened to look interesting to the maintainer but is not ready for 'next', it is applied on a topic branch and merged to the 'seen' branch to keep an eye on it. In an ideal world, the participants give reviews and the original author responds to the reviews, and such iterations may produce newer versions of the patch series, and at some point, a concensus is formed that the latest round is good enough for 'next'. Then the topic is merged to 'next' for inclusion in a future release. In a much less ideal world we live in, however, a topic sometimes get stalled. The original author may not respond to hanging review comments, may promise an update will be sent but does not manage to do so, nobody talks about the topic on the list and nobody builds upon it, etc. Following the recent trend to document and give more transparency to the decision making process, let's set a deadline to keep a topic still alive, and actively discard those that are inactive for a long period of time. Signed-off-by: Junio C Hamano --- Documentation/howto/maintain-git.txt | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git i/Documentation/howto/maintain-git.txt w/Documentation/howto/maintain-git.txt index da31332f11..e797b32522 100644 --- i/Documentation/howto/maintain-git.txt +++ w/Documentation/howto/maintain-git.txt @@ -67,7 +67,22 @@ the mailing list after each feature release is made: before getting merged to 'master'. - 'seen' branch is used to publish other proposed changes that do - not yet pass the criteria set for 'next' (see above). + not yet pass the criteria set for 'next' (see above), but there + is no promise that 'seen' will contain everything. A topic that + had no reviewer reaction may not be picked up. + + - A new topic will first get merged to 'seen', unless it is + trivially correct and clearly urgent, in which case it may be + directly merged to 'next' or even to 'master'. + + - If a topic that was picked up to 'seen' becomes and stays + inactive for 3 calendar weeks without having seen a clear + consensus that it is good enough to be moved to 'next', the + topic may be discarded from 'seen'. Interested parties are + still free to revive the topic. For the purpose of this + guideline, the definition of being "inactive" is that nobody + has discussed the topic, no new iteration of the topic was + posted, and no responses to the review comments were given. - The tips of 'master' and 'maint' branches will not be rewound to allow people to build their own customization on top of them. ``` ## Christian Couder, 2024-09-27 08:57 Subject: Re: [GSoC][PATCH] unit-tests: add tests for oidset.h Message-ID: URL: https://gitlist.dev/e/CAP8UFD3JzYCJf4%2BJLvfW_8m6kp%3DO0NMKi1dF1Fof9%3DDmvZ4u2w%40mail.gmail.com In-Reply-To: ``` On Thu, Sep 26, 2024 at 8:28 PM Junio C Hamano wrote: > I'll discard this one by the end of this week unless the topic sees > any activity. It looks to me that the project decided that a longer > term direction to adopt "clar" as the unit-tests framework, so this > patch would need to be written even if it were perfect in the old > world order anyway. Yeah, unless Ghanshyam or someone else wants to continue working on it, I think finishing this work should be part of the "Convert unit tests to use the clar testing framework" Outreachy project that Patrick and Phillip agreed to co-mentor. This project will only start next December though (supposing a good Outreachy intern is selected), so it's fine to discard it in the meantime. Thanks. ``` ## Junio C Hamano, 2024-09-27 18:47 Subject: Re: [GSoC][PATCH] unit-tests: add tests for oidset.h Message-ID: URL: https://gitlist.dev/e/xmqqcykpgchf.fsf%40gitster.g In-Reply-To: ``` Christian Couder writes: > On Thu, Sep 26, 2024 at 8:28 PM Junio C Hamano wrote: > >> I'll discard this one by the end of this week unless the topic sees >> any activity. It looks to me that the project decided that a longer >> term direction to adopt "clar" as the unit-tests framework, so this >> patch would need to be written even if it were perfect in the old >> world order anyway. > > Yeah, unless Ghanshyam or someone else wants to continue working on > it, I think finishing this work should be part of the "Convert unit > tests to use the clar testing framework" Outreachy project that > Patrick and Phillip agreed to co-mentor. This project will only start > next December though (supposing a good Outreachy intern is selected), > so it's fine to discard it in the meantime. And of course it does not have to wait until December. If anybody wants to work on adding a unit test for oidset, they can do so immediately. A new unit-test, including the oidset one, should be written using clar framework. Thanks. ``` ## Patrick Steinhardt, 2024-09-28 07:02 Subject: Re: [GSoC][PATCH] unit-tests: add tests for oidset.h Message-ID: URL: https://gitlist.dev/e/ZveqArC9NNs44Fjc%40pks.im In-Reply-To: ``` On Fri, Sep 27, 2024 at 11:47:40AM -0700, Junio C Hamano wrote: > Christian Couder writes: > > > On Thu, Sep 26, 2024 at 8:28 PM Junio C Hamano wrote: > > > >> I'll discard this one by the end of this week unless the topic sees > >> any activity. It looks to me that the project decided that a longer > >> term direction to adopt "clar" as the unit-tests framework, so this > >> patch would need to be written even if it were perfect in the old > >> world order anyway. > > > > Yeah, unless Ghanshyam or someone else wants to continue working on > > it, I think finishing this work should be part of the "Convert unit > > tests to use the clar testing framework" Outreachy project that > > Patrick and Phillip agreed to co-mentor. This project will only start > > next December though (supposing a good Outreachy intern is selected), > > so it's fine to discard it in the meantime. > > And of course it does not have to wait until December. > > If anybody wants to work on adding a unit test for oidset, they can > do so immediately. A new unit-test, including the oidset one, > should be written using clar framework. I'm also happy to help anybody who wants write such a new unit test suite. Let me use this to give a quick status update regarding my upstream quest to address the feedback I got during reviews on the clar itself: - There is a .editorconfig file now. - All the cross-platform compatibility fixes have been merged. - We have Win32 wired up in CI. Doing so via Makefiles was too much of a hassle, so I converted the project to use CMake for easier cross platform testability. The fact that the project uses CMake does not impact us though, as we wire it up ourselves anyway. - All memory allocation errors are now handled consistently. Currently in review is: - Self-tests for the clar, where we use clar to assert that clar works. - A small memory leak fix, as well as wiring up leak sanitizers in CI. I've also got a patch series sitting locally that introduces type-safe wrappers for the assertions that I'll move into review once self-tests have landed. That would then address the last bit of feedback I got, if I remember correctly. Just to let you folks know that I didn't just do nothing after this has landed in Git. Patrick ``` ## Junio C Hamano, 2024-09-30 18:48 Subject: Re: [GSoC][PATCH] unit-tests: add tests for oidset.h Message-ID: URL: https://gitlist.dev/e/xmqqmsjp6kqb.fsf%40gitster.g In-Reply-To: ``` Patrick Steinhardt writes: > Let me use this to give a quick status update regarding my upstream > quest to address the feedback I got during reviews on the clar itself: > > - There is a .editorconfig file now. > > - All the cross-platform compatibility fixes have been merged. > > - We have Win32 wired up in CI. Doing so via Makefiles was too much of > a hassle, so I converted the project to use CMake for easier cross > platform testability. The fact that the project uses CMake does not > impact us though, as we wire it up ourselves anyway. > > - All memory allocation errors are now handled consistently. > > Currently in review is: > > - Self-tests for the clar, where we use clar to assert that clar > works. > > - A small memory leak fix, as well as wiring up leak sanitizers in CI. > > I've also got a patch series sitting locally that introduces type-safe > wrappers for the assertions that I'll move into review once self-tests > have landed. That would then address the last bit of feedback I got, if > I remember correctly. > > Just to let you folks know that I didn't just do nothing after this has > landed in Git. ;-) Nice to see that the code is improved not just for us but for other consumers. Thanks. ```