On Thu, Aug 12, 2021 at 10:25:03AM -0700, Junio C Hamano wrote:
>
> Emily Shaffer <emilyshaffer@xxxxxxxxxx> 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 <emilyshaffer@xxxxxxxxxx>
> > ---
> > 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
