Re: [PATCH v7 09/17] hook: replace find_hook() with hook_exists()

On Sat, Jan 30, 2021 at 08:39:28PM -0800, Jonathan Tan wrote:

quoted

Add a helper to easily determine whether any hooks exist for a given
hook event.

Many callers want to check whether some state could be modified by a
hook; that check should include the config-based hooks as well. Optimize
by checking the config directly. Since commands which execute hooks
might want to take args to replace 'hook.runHookDir', let
'hook_exists()' mirror the behavior of 'hook.runHookDir'.

The text makes sense, but the title might better be "introduce
hook_exists()" instead of "replace", since find_hook() is still around.

Yep. Also, removed the one instance I did replace in this commit for
some reason (builtin/bugreport.c). Not sure why that's here and not
later on in part II.

Also maybe briefly mention the future plans - e.g. in the future, no
code will use find_hook() except <whatever the hook-internal functions
are>, because all of them will use hook_exists() and run_hook().

Nice, good point - I'll do that.

quoted

+/*
+ * Returns 1 if any hooks are specified in the config or if a hook exists in the
+ * hookdir. Typically, invoke hook_exsts() like:
+ *   hook_exists(hookname, configured_hookdir_opt());
+ * Like with run_hooks, if you take a --run-hookdir flag, reflect that
+ * user-specified behavior here instead.
+ */
+int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir);

I wonder if enum hookdir_opt should support a "unspecified" instead, in
which case hook_exists() will automatically read the config (instead of
relying on the caller to call configured_hookdir_opt()), but I see that
this patch set is version 7 and perhaps this design point has already
been discussed.

Nope, and I think that's a pretty neat idea. I've done so.