From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 10 Feb 2026 13:43:52 GMT
Subject: Re: [PATCH 3/4] hook: include hooks from the config
Message-ID: <aYs2GGTU6UJWJFOf@pks.im>
In-Reply-To: <87343920ny.fsf@collabora.com>

On Mon, Feb 09, 2026 at 09:10:25PM +0200, Adrian Ratiu wrote:
> On Mon, 09 Feb 2026, Patrick Steinhardt <ps@pks.im> wrote:
> > On Wed, Feb 04, 2026 at 06:51:25PM +0200, Adrian Ratiu wrote:
> >> +	/* Extract the hook name */
> >> +	hook_name = xmemdupz(name, name_len);
> >> +
> >> +	/* Remove the hook if already in the list, so we append in config order. */
> >> +	if ((item = unsorted_string_list_lookup(data->list, hook_name)))
> >> +		unsorted_string_list_delete_item(data->list, item - data->list->items, 0);
> >> +
> >> +	/* The list takes ownership of hook_name, so append with nodup */
> >> +	string_list_append_nodup(data->list, hook_name);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  struct string_list *list_hooks(struct repository *r, const char *hookname)
> >>  {
> >> -	struct string_list *hook_head;
> >> +	struct hook_config_cb cb_data;
> >>  
> >>  	if (!hookname)
> >>  		BUG("null hookname was provided to hook_list()!");
> >>  
> >> -	hook_head = xmalloc(sizeof(struct string_list));
> >> -	string_list_init_dup(hook_head);
> >> +	cb_data.hook_event = hookname;
> >> +	cb_data.list = xmalloc(sizeof(struct string_list));
> >> +	string_list_init_dup(cb_data.list);
> >> +
> >> +	/* Add the hooks from the config, e.g. hook.myhook.event = pre-commit */
> >> +	repo_config(r, hook_config_lookup, &cb_data);
> >
> > There are cases where we invoke the same hook multiple times per
> > process. One such candidate is for example the "reference-transaction"
> > hook, which is executed thrice per ref transaction. Would it make sense
> > if we cached the value in `r` so that we can avoid repeatedly parsing
> > the configuration for this hook?
> >
> > We could even go one step further: instead of caching the value for
> > _one_ hook, we could parse _all_ hooks lazily and store them in a map.
> 
> I think this can be done, yes, though I'm unsure of the performance gain
> vs the added complexity. Since after the first lookup the config files
> are hot in the OS page cache, the next lookups are cheap/fast and we'd
> be shaving off only a few microseconds of parsing.
> 
> So I'm unsure if this added complexity is worth it or not.

Maybe I'm naive, but I don't really expect there to be much complexity.
Quite on the contrary: if we unify all configuration parsing into
`hook_config_lookup` I would even claim that this cache would almost
come as a side effect. We would simply iterate through all hook-related
configuration and build the map of all hooks as we go.

Whether it'd matter performance-wise... I think in certain cases it
actually would. The mentioned reference-transaction hook for example had
some worst cases in the past where it would execute thrice per reference
update in certain code paths. In the worst cases I've seen it execute
tens of thousands of times in a single process.

Sure, these are outliers. But such cases do exist, and I expect that the
performance impact could be non-negligible here if combined with
non-trivial configuration.

Patrick