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

On Sun, Mar 18, 2018 at 4:25 AM, Duy Nguyen [off-list ref] wrote:

quoted

On Sun, Mar 18, 2018 at 3:11 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Sat, Mar 17, 2018 at 3:53 AM, Nguyễn Thái Ngọc Duy [off-list ref] wrote:

quoted

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

quoted

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