Re: [GSoC][PATCH v3] t: port helper/test-oid-array.c to unit-tests/t-oid-array.c

From: Christian Couder <christian.couder@gmail.com>
Date: Aug 25, 2024, 06:38 UTC
Message-ID: <CAP8UFD3mq+k8QXDrFAp5bfoCN+sNgm3vJvuhryxVYDaj-SZU0g@mail.gmail.com>
In-Reply-To: <20240824170223.36080-1-shyamthakkar001@gmail.com>

On Sat, Aug 24, 2024 at 7:02 PM Ghanshyam Thakkar <shyamthakkar001@gmail.com> wrote:

Show 9 quoted lines

>
> 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."

Show 15 quoted lines

> And
> init_hash_algo():lib-oid.c can aid in this process, so make it public.
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> ---
> 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.

Show 12 quoted lines

> 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);

[...]

Show 14 quoted lines

> +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.