Christian Couder <christian.couder@xxxxxxxxx> wrote:
> 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.
the_hash_algo is just:
define the_hash_algo the_repository->hash_algo
and we do initialize the_repository->hash_algo manually.
>
> Also I think it's not clear how initializing a repository directory is
> related to the hash algo.
That is mentioned because the old code in helper/test-oid-array.c used
to call setup_git_directory_gently() to setup a git directory, which
would also initialize the_repository->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."
Will change.
>
> > 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.
No particular reason other than the fact that v2 was posted 20 days ago.
And it is not merged into 'seen' or 'next', so it shouldn't be a
problem.
Thanks.
