Subject: Re: [PATCH v2 1/6] hook: run a list of hooks instead

From: Emily Shaffer <[email-masked]>
Date: Mon, 16 Aug 2021 16:38:08 -0700

On Thu, Aug 12, 2021 at 10:25:03AM -0700, Junio C Hamano wrote:
> 
> Emily Shaffer <[email-masked]> writes:
> 
> > To prepare for multihook support, teach hook.[hc] to take a list of
> > hooks at run_hooks and run_found_hooks. Right now the list is always one
> > entry, but in the future we will allow users to supply more than one
> > executable for a single hook event.
> >
> > Signed-off-by: Emily Shaffer <[email-masked]>
> > ---
> >  builtin/hook.c |  14 ++++---
> >  hook.c         | 103 +++++++++++++++++++++++++++++++++++--------------
> >  hook.h         |  16 +++++++-
> >  3 files changed, 96 insertions(+), 37 deletions(-)
> >
> > diff --git a/builtin/hook.c b/builtin/hook.c
> > index 5eb7cf73a4..4d39c9e75e 100644
> > --- a/builtin/hook.c
> > +++ b/builtin/hook.c
> > @@ -25,7 +25,8 @@ static int run(int argc, const char **argv, const char *prefix)
> >  	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
> >  	int ignore_missing = 0;
> >  	const char *hook_name;
> > -	const char *hook_path;
> > +	struct list_head *hooks;
> > +
> 
> Natural.  We used to use the path to the hook because we were
> expecting only one. We now use the name to find a list of hooks.
> 
> All the caller sees is just list_head without any direct visibility
> into it, which feels like a great abstraction.  Presumably everything
> will go through the API functions taking this opaque "list of hooks"
> thing (or "the first one in the list" if the API function does not
> iterate over it, perhaps?).

Hum, I guess that in a later patch builtin/hook.c does learn to take
apart the list_head into a 'struct hook' to print the output of 'git
hook list'. I haven't read your review of that patch yet though.

> > diff --git a/hook.c b/hook.c
> > index ee20b2e365..80e150548c 100644
> > --- a/hook.c
> > +++ b/hook.c
> > @@ -4,6 +4,28 @@
> >  #include "hook-list.h"
> >  #include "config.h"
> >  
> > +static void free_hook(struct hook *ptr)
> > +{
> > +	if (ptr) {
> > +		free(ptr->feed_pipe_cb_data);
> > +	}
> 
> Lose the extra {}, as we do not do more than the above free() even
> at the end of the series?

ACK

> 
> > +struct list_head* hook_list(const char* hookname)
> 
> Shift both of the asterisks; our asterisks do not stick to the type
> but to the identifier.

ACK

> 
> > +{
> > +	struct list_head *hook_head = xmalloc(sizeof(struct list_head));
> > +
> > +	INIT_LIST_HEAD(hook_head);
> > +
> > +	if (!hookname)
> > +		return NULL;
> 
> Checking for invalid hookname first would avoid leaking hook_head,
> no?  The caller of hook_list() we saw earlier immediately calls
> list_empty() which will segfault.  The caller need to be tightened,
> but I wonder if it would be a programmer's error to pass a NULL
> hookname to this function.  If so, this can simply be a BUG() and
> the earlier allocation and initialization of hook_head can stay
> where they are.  Otherwise, the caller should see if this returns a
> NULL.

Ah, good point. I think this makes sense to BUG().

> 
> > +	if (have_git_dir()) {
> > +		const char *hook_path = find_hook(hookname);
> > +
> > +		/* Add the hook from the hookdir */
> > +		if (hook_path) {
> > +			struct hook *to_add = xmalloc(sizeof(*to_add));
> > +			to_add->hook_path = hook_path;
> > +			to_add->feed_pipe_cb_data = NULL;
> > +			list_add_tail(&to_add->list, hook_head);
> > +		}
> > +	}
> 
> Calling this function to grab a list of hooks when we are not in any
> repository is not an error but just we get "there is nothing to
> run".  Does the design give us a more useful behaviour, compared to
> alternatives like "you have to be in a repository or calling this
> function is an error"?

Later we enable calling hook_list without a gitdir, in patch 6
(https://lore.kernel.org/git/20210812004258.74318-7-emilyshaffer%40google.com).
So maybe the behavior as it is now is premature?

But leaving the hook list empty means we can behave gracefully if
anybody is calling a hook without necessarily being sure they have
gitdir. I would need to audit callsites to check if they are checking
whether they have one or not. I think in most cases they probably are
checking that.

> 
> Not an objection wrapped in a rhetorical question, but a genuine
> question.  "It would help this and that usecase" would be an ideal
> answer, "We could do either way, but we happened to have written the
> code this way first, and at the end of the series we did not see any
> practical downsides" would also be a great answer.

The main use case, today, for letting this work nicely even outside of
a gitdir, is sendemail-validate hook. But mostly, I was thinking that
when we're freed from dependence on .git/hooks/, there's no reason to
disallow out-of-repo hooks in case someone wants to add a new one in the
future - for example to 'git maintenance' daemon runs.

> 
> > +	return hook_head;
> > +}
> > +
> 
> > +/*
> > + * Provides a linked list of 'struct hook' detailing commands which should run
> > + * in response to the 'hookname' event, in execution order.
> > + */
> > +struct list_head* hook_list(const char *hookname);
> 
> struct list_head *hook_list(const char *hookname);
ACK

Thanks for the thoughts.
 - Emily