From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 10 Feb 2026 13:43:58 GMT
Subject: Re: [PATCH 1/4] hook: run a list of hooks
Message-ID: <aYs2HiFkEciZkydr@pks.im>
In-Reply-To: <878qd1235o.fsf@gentoo.mail-host-address-is-not-set>

On Mon, Feb 09, 2026 at 08:16:35PM +0200, Adrian Ratiu wrote:
> 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!

> > 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!

Perfect, thanks!

Patrick