Subject: Re: [PATCH v2 4/6] hook: allow running non-native hooks

From: Junio C Hamano <[email-masked]>
Date: Wed, 18 Aug 2021 14:24:23 -0700

Emily Shaffer <[email-masked]> writes:

>> Yes, I see what you mean. Ok. I have been wanting to change the naming
>> anyways - most functions in hook.h are verb-y ("find hook", "run hooks",
>> so on) but hook_list stands out as the only noun-y function.
>> 
>> So I considered changing it to "list_hooks" and "list_hooks_gently", to align
>> with find_hook(_gently)....

I do not claim I am goot at naming (or better than you at it
anyway), but list-hooks sounds to me like it is calling printf()
to show the hooks to the user, not computing a list of hooks and
returning it to the caller.

>> I also think that approach would make a callsite easier to understand
>> than checking for null from hook_list().
>> 
>>   const char *hookname = "my-new-hook";
>> 
>>   /* Here it's pretty clear what the reason for the error was... */
>>   if (!known_hook(hookname))
>>     BUG("is hook '%s' in Documentation/githooks.txt?", hookname);

Yes.  The callsite becomes easier to understand, and it separates
the responsibility between the helper to respond to "please give me
list of defined hooks" and its caller that may react to the returned
list with "ok, among these hooks, this does not look kosher for such
and such reason, so I'd die/warn/error" much cleaner.