Re: [PATCH 1/4] hook: run a list of hooks
On Mon, Feb 09, 2026 at 08:16:35PM +0200, Adrian Ratiu wrote:
Show 56 quoted lines
> On Mon, 09 Feb 2026, Patrick Steinhardt <ps@pks.im> wrote:
> > On Wed, Feb 04, 2026 at 06:51:23PM +0200, Adrian Ratiu wrote:
> >> diff --git a/hook.c b/hook.c
> >> index cde7198412..fb90f91f3b 100644
> >> --- a/hook.c
> >> +++ b/hook.c
> >> @@ -47,9 +47,49 @@ const char *find_hook(struct repository *r, const char *name)
> >> return path.buf;
> >> }
> >>
> >> +/*
> >> + * Provides a list of hook commands to run for the 'hookname' event.
> >> + *
> >> + * This function consolidates hooks from two sources:
> >> + * 1. The config-based hooks (not yet implemented).
> >> + * 2. The "traditional" hook found in the repository hooks directory
> >> + * (e.g., .git/hooks/pre-commit).
> >> + *
> >> + * The list is ordered by execution priority.
> >> + *
> >> + * The caller is responsible for freeing the memory of the returned list
> >> + * using string_list_clear() and free().
> >> + */
> >> +static struct string_list *list_hooks(struct repository *r, const char *hookname)
> >> +{
> >> + struct string_list *hook_head;
> >> +
> >> + if (!hookname)
> >> + BUG("null hookname was provided to hook_list()!");
> >> +
> >> + hook_head = xmalloc(sizeof(struct string_list));
> >> + string_list_init_dup(hook_head);
> >> +
> >> + /*
> >> + * Add the default hook from hookdir. It does not have a friendly name
> >> + * like the hooks specified via configs, so add it with an empty name.
> >> + */
> >> + if (r->gitdir && find_hook(r, hookname))
> >> + string_list_append(hook_head, "");
> >
> > Why is there a check for `r->gitdir` here? Do we ever execute hooks
> > outside of a fully-initialized repository?
>
> Nice find. The answer is yes, see the last commit in this series.
>
> I've been cleaning up and untangling this series for quite a while now:
> - It used to be combined with the other parallel hooks series.
> - It used to implement its own linked list abstraction.
> - It used the hook.h string-list APIs AEvar didn't like.
> - and so on.
>
> This cheeck is just a leftover bit I missed during my cleanups and I
> really should be moved this to the last commit. I've been moving a lot
> of code around to make it as clear as possible.
>
> I'll do it / test in v2. Many thanks!That should indeed lead to less puzzlement, great!
Show 15 quoted lines
> > Other than that, we now insert hooks into the list. It's somewhat
> > surprising that we insert hook "names" here, instead of for example
> > adding the full hook path to the list. Is there any specific reason for
> > this decision?
>
> I'm also not a fan of this design, as I mentioned in the reply to Junio.
>
> We just need to differentiate between "new" hooks (from config) and
> "default/legacy" hooks (from the hookdir) and we kind-of abused the fact
> that the default hooks have no friendly name, so the name is empty, to
> differentiate them. :)
>
> Junio suggested we use a struct/union and a specific type to
> differentiate between the "default/legacy" hooks (from the hookdir) and
> the new config-based hooks.
Yeah, that sounds like a reasonable thing to do.
> That way we don't have to rely on the name anymore and we can also use
> the full path here, as you suggested.
>
> I'll do all this in v2. Thank you!
Patrick Steinhardt <ps@pks.im>