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

From: Emily Shaffer <[email-masked]>
Date: Fri, 12 Feb 2021 14:15:17 -0800

On Sat, Jan 30, 2021 at 08:39:28PM -0800, Jonathan Tan wrote:
> 
> > 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.
> 
> 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().
> 
> > +/*
> > + * 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.

No, I don't think it has been. So you mean something like:

  enum hookdir_opt
  {
  	HOOKDIR_NO,
  	HOOKDIR_WARN,
  	HOOKDIR_INTERACTIVE,
  	HOOKDIR_YES,
  	HOOKDIR_USE_CFG,
  	HOOKDIR_UNKNOWN,
  };

(name subject to quibbling) and then reimagining
configured_hookdir_opt() to something like:

  enum hookdir_opt resolve_hookdir_opt(enum hookdir_opt o)
  {
  	if (o != HOOKDIR_USE_CFG)
		return o;
	/* former contents of configured_hookdir_opt here */
  }

I like that, if nobody has complaints.

 - Emily