On Sat, Aug 24, 2024 at 7:02 PM Ghanshyam Thakkar
<shyamthakkar001@xxxxxxxxx> wrote:
>
> helper/test-oid-array.c along with t0064-oid-array.sh test the
> oid-array.h API, which provides storage and processing
> efficiency over large lists of object identifiers.
>
> Migrate them to the unit testing framework for better runtime
> performance and efficiency. Also 'the_hash_algo' is used internally in
> oid_array_lookup(), but we do not initialize a repository directory,
> therefore initialize the_hash_algo manually.
Even if 'the_hash_algo' is used internally in oid_array_lookup()
through oid_pos(), this patch initializes the hash algo for the repo
using repo_set_hash_algo(), which contains the following single
instruction:
repo->hash_algo = &hash_algos[hash_algo];
So "therefore initialize the_hash_algo manually" is not quite true, as
it doesn't look like 'the_hash_algo' is even used.
Also I think it's not clear how initializing a repository directory is
related to the hash algo.
So maybe something like the following would be better:
"As we don't initialize a repository in these tests, the hash algo
that functions like oid_array_lookup() use is not initialized,
therefore call repo_set_hash_algo() to initialize it."
> And
> init_hash_algo():lib-oid.c can aid in this process, so make it public.
>
> Mentored-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx>
> Helped-by: Phillip Wood <phillip.wood123@xxxxxxxxx>
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@xxxxxxxxx>
> ---
> Changes in v3:
> - changed commmit message and comments for more accurate description
> - removed test_min() and return early when actual.nr and expect.nr
> don't match
> - rename result to expect for more accurate description
> - removed a redundant check in t_enumeration()
> - add check_int() around one of calls of get_oid_arbitrary_hex()
This looks good.
> - rebased to latest master
It's nice to say it was rebased, but it's better to tell the reason
why it was rebased.
> diff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h
> index 8d2acca768..c949af082c 100644
> --- a/t/unit-tests/lib-oid.h
> +++ b/t/unit-tests/lib-oid.h
> @@ -13,5 +13,13 @@
> * environment variable.
> */
> int get_oid_arbitrary_hex(const char *s, struct object_id *oid);
> +/*
> + * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of
> + * GIT_TEST_DEFAULT_HASH environment variable. The fallback value in case
> + * of absence of GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1. It also uses
Nit: maybe: s/in case of absence/in the absence/
> + * check(algo != GIT_HASH_UNKNOWN) before returning to verify if the
> + * GIT_TEST_DEFAULT_HASH's value is valid or not.
> + */
> +int init_hash_algo(void);
[...]
> +static void t_enumeration(const char **input_args, size_t input_sz,
> + const char **expect_args, size_t expect_sz)
> +{
> + struct oid_array input = OID_ARRAY_INIT, expect = OID_ARRAY_INIT,
> + actual = OID_ARRAY_INIT;
> + size_t i;
> +
> + if (fill_array(&input, input_args, input_sz))
> + return;
> + if (fill_array(&expect, expect_args, expect_sz))
> + return;
> +
> + oid_array_for_each_unique(&input, add_to_oid_array, &actual);
> + if(!check_uint(actual.nr, ==, expect.nr))
Missing space between 'if' and '('.
> + return;
The rest of the patch looks good to me. Thanks.
