diff mbox series

[v4,7/9] hook: replace run-command.h:find_hook

Message ID 20200909004939.1942347-8-emilyshaffer@google.com
State New
Headers show
Series propose config-based hooks | expand

Commit Message

Emily Shaffer Sept. 9, 2020, 12:49 a.m. UTC
Add a helper to easily determine whether any hooks exist for a given
hook event.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 hook.c | 9 +++++++++
 hook.h | 1 +
 2 files changed, 10 insertions(+)

Comments

Junio C Hamano Sept. 9, 2020, 8:32 p.m. UTC | #1
Emily Shaffer <emilyshaffer@google.com> writes:

> Add a helper to easily determine whether any hooks exist for a given
> hook event.
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  hook.c | 9 +++++++++
>  hook.h | 1 +
>  2 files changed, 10 insertions(+)

Should we consider the last three patches still work-in-progress
technology demonstration, or are these meant as a proposal for a new
API element as-is?

It is perfectly fine if it is the former.  I just want to make sure
we share a common understanding on the direction in which we want
these patches to take us.  Here is my take:

 - For now, a hook/event that is aware of the config-based hook
   system is supposed to use hook_exists(), while the traditional
   ones still use find_hook().  We expect more and more will be
   converted to the former over time.

 - Invoking hook scripts under the new world order is done by
   including hook.h and calling run_hooks(), not by driving the
   run-command API yourself (I count run_hook_ve() as part of the
   latter) like the traditional code did.  We expect more and more
   will be converted to the former over time.

 - From the point of view of the end users who have been happily
   using scripts in $GIT_DIR/hooks, everything will stay the same.
   hook_exists() will find them (by calling find_hook() as a
   fallback) and run_hooks() will run them (by relying on
   hook_list() to include them).

I am guessing that the above gives us a high-level description.

The new interface needs to be described in hook.h once the series
graduates from the technology demonstration state, in order to help
others who want to help updating the callsites of traditional hooks
to the new API.  And the above three-bullet point list is my attempt
to figure out what kind of things need to be documented to help
them.

I am not seeing anything in run_hooks() that consumes input from us
over pipe, by the way, without which we cannot do things like the
"pre-receive" hooks under the new world order.  Are they planned to
come in the future, after these "we feed anything they need from the
command line and from the enviornment" hooks are dealt with in this
first pass?

Thanks.

> diff --git a/hook.c b/hook.c
> index 0dab981681..7c7b922369 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -111,6 +111,15 @@ struct list_head* hook_list(const struct strbuf* hookname)
>  	return &hook_head;
>  }
>  
> +int hook_exists(const char *hookname)
> +{
> +	const char *value = NULL;
> +	struct strbuf hook_key = STRBUF_INIT;
> +	strbuf_addf(&hook_key, "hook.%s.command", hookname);
> +
> +	return (!git_config_get_value(hook_key.buf, &value)) || !!find_hook(hookname);
> +}
> +
>  int run_hooks(const char *const *env, const struct strbuf *hookname,
>  	      const struct strvec *args)
>  {
> diff --git a/hook.h b/hook.h
> index d020788a6b..d94511b609 100644
> --- a/hook.h
> +++ b/hook.h
> @@ -11,6 +11,7 @@ struct hook
>  };
>  
>  struct list_head* hook_list(const struct strbuf *hookname);
> +int hook_exists(const char *hookname);
>  int run_hooks(const char *const *env, const struct strbuf *hookname,
>  	      const struct strvec *args);
Emily Shaffer Sept. 10, 2020, 7:08 p.m. UTC | #2
On Wed, Sep 09, 2020 at 01:32:12PM -0700, Junio C Hamano wrote:
> 
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > Add a helper to easily determine whether any hooks exist for a given
> > hook event.
> >
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> >  hook.c | 9 +++++++++
> >  hook.h | 1 +
> >  2 files changed, 10 insertions(+)
> 
> Should we consider the last three patches still work-in-progress
> technology demonstration, or are these meant as a proposal for a new
> API element as-is?

The former. I'm irritated with myself for spending a long time fidgeting
with the wording on this reroll and still forgetting to mark the last
three "RFC" as I had planned to do.

> It is perfectly fine if it is the former.  I just want to make sure
> we share a common understanding on the direction in which we want
> these patches to take us.  Here is my take:
> 
>  - For now, a hook/event that is aware of the config-based hook
>    system is supposed to use hook_exists(), while the traditional
>    ones still use find_hook().  We expect more and more will be
>    converted to the former over time.
> 
>  - Invoking hook scripts under the new world order is done by
>    including hook.h and calling run_hooks(), not by driving the
>    run-command API yourself (I count run_hook_ve() as part of the
>    latter) like the traditional code did.  We expect more and more
>    will be converted to the former over time.
> 
>  - From the point of view of the end users who have been happily
>    using scripts in $GIT_DIR/hooks, everything will stay the same.
>    hook_exists() will find them (by calling find_hook() as a
>    fallback) and run_hooks() will run them (by relying on
>    hook_list() to include them).
> 
> I am guessing that the above gives us a high-level description.

Yes. I am also working on a patch locally to include a config -
optionally users could shut off the $GIT_DIR/hooks, but I don't see us
making that the default behavior any time soon (or ever).

> 
> The new interface needs to be described in hook.h once the series
> graduates from the technology demonstration state, in order to help
> others who want to help updating the callsites of traditional hooks
> to the new API.  And the above three-bullet point list is my attempt
> to figure out what kind of things need to be documented to help
> them.

Sure. Agreed. Thanks for pointing it out - I had planned on updating the
`git help hook` manpage but adding API comments in hook.h had slipped my
mind, so the reminder is useful.

> 
> I am not seeing anything in run_hooks() that consumes input from us
> over pipe, by the way, without which we cannot do things like the
> "pre-receive" hooks under the new world order.  Are they planned to
> come in the future, after these "we feed anything they need from the
> command line and from the enviornment" hooks are dealt with in this
> first pass?

I included this conversion to demonstrate the tech and give people
something to look at (and shout to stop if so needed). I do plan to
include hooks which need piped input; in fact, I'm hoping to target one
such for the next conversion I do. The todo list looks like so:

 1. semantics for checking hook.runHookDir config
 2. convert all the hooks which take input in interesting ways (or, just
 all the hooks)
 3. add user friendliness via 'git hook add', 'git hook edit', etc

 The config semantics are in progress and I'm hoping to send this week.

 As for submission plan, I don't mind including new architecture (if
 unused) except for the code bloat; I'd rather push all the
 "conversions" simultaneously, so users don't have to wonder "is this
 hook a new and supported one, or not?".  I don't mind adding the
 niceties ('git hook add' etc) later as the config is a little annoying
 for a human to write themselves, but not impossible.

  - Emily
Jonathan Tan Sept. 23, 2020, 11:20 p.m. UTC | #3
> +int hook_exists(const char *hookname)
> +{
> +	const char *value = NULL;
> +	struct strbuf hook_key = STRBUF_INIT;
> +	strbuf_addf(&hook_key, "hook.%s.command", hookname);
> +
> +	return (!git_config_get_value(hook_key.buf, &value)) || !!find_hook(hookname);
> +}

I was surprised that this didn't share code with hook_list. Upon further
thought, hook_list might be expensive if hooks are present, but if we
can cache results, I think it's worth it. A caller that calls this
function usually will run hooks if they are present, so it's not wasted
work to construct the hook list.
Jonathan Nieder Oct. 5, 2020, 11:42 p.m. UTC | #4
Emily Shaffer wrote:

> Subject: hook: replace run-command.h:find_hook

tiny nit: This doesn't remove find_hook, so this may want to be
described as "add replacement for" instead of "replace".

[...]
> --- a/hook.c
> +++ b/hook.c
> @@ -111,6 +111,15 @@ struct list_head* hook_list(const struct strbuf* hookname)
>  	return &hook_head;
>  }
>  
> +int hook_exists(const char *hookname)
> +{
> +	const char *value = NULL;
> +	struct strbuf hook_key = STRBUF_INIT;
> +	strbuf_addf(&hook_key, "hook.%s.command", hookname);
> +
> +	return (!git_config_get_value(hook_key.buf, &value)) || !!find_hook(hookname);
> +}

This feels a bit fragile, since it can go out of sync with run_hooks.
I think I'd prefer if they shared code and this function either
returned a parsed structure that could be used later to run hooks or
cached the result keyed by hookname.

Thanks,
Jonathan
diff mbox series

Patch

diff --git a/hook.c b/hook.c
index 0dab981681..7c7b922369 100644
--- a/hook.c
+++ b/hook.c
@@ -111,6 +111,15 @@  struct list_head* hook_list(const struct strbuf* hookname)
 	return &hook_head;
 }
 
+int hook_exists(const char *hookname)
+{
+	const char *value = NULL;
+	struct strbuf hook_key = STRBUF_INIT;
+	strbuf_addf(&hook_key, "hook.%s.command", hookname);
+
+	return (!git_config_get_value(hook_key.buf, &value)) || !!find_hook(hookname);
+}
+
 int run_hooks(const char *const *env, const struct strbuf *hookname,
 	      const struct strvec *args)
 {
diff --git a/hook.h b/hook.h
index d020788a6b..d94511b609 100644
--- a/hook.h
+++ b/hook.h
@@ -11,6 +11,7 @@  struct hook
 };
 
 struct list_head* hook_list(const struct strbuf *hookname);
+int hook_exists(const char *hookname);
 int run_hooks(const char *const *env, const struct strbuf *hookname,
 	      const struct strvec *args);