Re: [PATCH 04/36] t/helper: merge test-lazy-init-name-hash into test-tool

[Jeff Hostetler <git@jeffhostetler.com>]

On 3/18/2018 4:47 AM, Eric Sunshine wrote:
> On Sun, Mar 18, 2018 at 4:25 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Sun, Mar 18, 2018 at 3:11 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Sat, Mar 17, 2018 at 3:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>>>> -extern int test_lazy_init_name_hash(struct index_state *istate, int try_threaded);
>>>> +extern int lazy_init_name_hash_for_testing(struct index_state *istate, int try_threaded);
>>>
>>> I get why you renamed this since the "main" function in the test
>>> program wants to be called 'test_lazy_init_name_hash'...
>>>
>>>> diff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper/test-lazy-init-name-hash.c
>>>> @@ -9,6 +10,9 @@ static int perf;
>>>> +static int (*init_name_hash)(struct index_state *istate, int try_threaded) =
>>>> +       lazy_init_name_hash_for_testing;
>>>> +
>>>> @@ -33,9 +37,9 @@ static void dump_run(void)
>>>>          if (single) {
>>>> -               test_lazy_init_name_hash(&the_index, 0);
>>>> +               init_name_hash(&the_index, 0);
>>>
>>> ... but I'm having trouble understanding why this indirection through
>>> 'init_name_hash' is used rather than just calling
>>> lazy_init_name_hash_for_testing() directly. Am I missing something
>>> obvious or is 'init_name_hash' just an unneeded artifact of an earlier
>>> iteration before the rename in cache.{c,h}?
>>
>> Nope. It just feels too long to me and because we're already in the
>> test I don't feel the need to repeat _for_testing everywhere. If it's
>> confusing, I'll remove init_name_hash.
> 
> Without an explanatory in-code comment, I'd guess that someone coming
> across this in the future will also stumble over it just like I did in
> the review.

I agree with Eric, this indirection seems unnecessary and confusing.
Generally, when I see function pointers initialized like that, I
think that there are plans for additional providers/drivers for that
functionality, but I don't see that here.  And it seems odd.

> 
> What if, instead, you rename test_lazy_init_name_hash() to
> lazy_init_name_hash_test(), shifting 'test' from the front to the back
> of the name? That way, the name remains the same length at the call
> sites in the test helper, and you don't have to introduce the
> confusing, otherwise unneeded 'init_name_hash'.
> 

I see 2 problems.
1. The test function in name-hash.c that needs access to private data.
     I'm not a fan of the "_for_testing" suffix, but I'm open.  I might
     either leave it as is, or make it a "TEST_" or "test__" prefix (as
     a guide for people used to languages with namespaces.

2. The new name for cmd_main().  There's no real need why it needs to
    be "test_*", right?   Your cmds[] maps the command line string to a
    function, but it could be anything.  That is, "cmd_main()" could be
    renamed "cmd__lazy_init_name_hash()".  Then you can conceptually
    reserve the "cmd__" prefix throughout t/helper for each test handler.

Just a thought,
Thanks,
Jeff