| Message ID | 20201222000220.1491091-10-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | propose config-based hooks (part I) | expand |
> 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.
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
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. 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. > > > +/* > > + * 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.
diff --git a/builtin/bugreport.c b/builtin/bugreport.c index ad3cc9c02f..2fe65d8f1e 100644 --- a/builtin/bugreport.c +++ b/builtin/bugreport.c @@ -3,7 +3,7 @@ #include "strbuf.h" #include "help.h" #include "compat/compiler.h" -#include "run-command.h" +#include "hook.h" static void get_system_info(struct strbuf *sys_info) @@ -82,7 +82,7 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit) } for (i = 0; i < ARRAY_SIZE(hook); i++) - if (find_hook(hook[i])) + if (hook_exists(hook[i], configured_hookdir_opt())) strbuf_addf(hook_info, "%s\n", hook[i]); } diff --git a/hook.c b/hook.c index 5836bbb739..fbb69706d8 100644 --- a/hook.c +++ b/hook.c @@ -225,6 +225,21 @@ void run_hooks_opt_init(struct run_hooks_opt *o) o->run_hookdir = configured_hookdir_opt(); } +int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir) +{ + const char *value = NULL; /* throwaway */ + struct strbuf hook_key = STRBUF_INIT; + + int could_run_hookdir = (should_run_hookdir == HOOKDIR_INTERACTIVE || + should_run_hookdir == HOOKDIR_WARN || + should_run_hookdir == HOOKDIR_YES) + && !!find_hook(hookname); + + strbuf_addf(&hook_key, "hook.%s.command", hookname); + + return (!git_config_get_value(hook_key.buf, &value)) || could_run_hookdir; +} + void run_hooks_opt_clear(struct run_hooks_opt *o) { strvec_clear(&o->env); diff --git a/hook.h b/hook.h index 259662968f..762b6fadad 100644 --- a/hook.h +++ b/hook.h @@ -62,6 +62,15 @@ struct run_hooks_opt void run_hooks_opt_init(struct run_hooks_opt *o); void run_hooks_opt_clear(struct run_hooks_opt *o); +/* + * 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); + /* * Runs all hooks associated to the 'hookname' event in order. Each hook will be * passed 'env' and 'args'.
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'. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- builtin/bugreport.c | 4 ++-- hook.c | 15 +++++++++++++++ hook.h | 9 +++++++++ 3 files changed, 26 insertions(+), 2 deletions(-)