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

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Mar 19, 2018, 09:40 UTC
Message-ID: <edbc7234-ee60-6f3e-ae09-94721e4b5faa@jeffhostetler.com>
In-Reply-To: <CAPig+cRGpD1_U5PgO=-1wrsiyWqY4GmE3tWNdgCnuAh7PUjvHg@mail.gmail.com>

On 3/18/2018 4:47 AM, Eric Sunshine wrote:

Show 32 quoted lines

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

Show 7 quoted lines

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