Re: [PATCH v7 09/17] hook: replace find_hook() with hook_exists()
- From
Emily Shaffer <emilyshaffer@google.com>
- Date
- Feb 12, 2021, 22:15 UTC
- Message-ID
- <YCb99egCYoTJUU8B@google.com>
- In-Reply-To
- <20210131043928.1036246-1-jonathantanmy@google.com>
On Sat, Jan 30, 2021 at 08:39:28PM -0800, Jonathan Tan wrote:
Show 31 quoted lines
> > > 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