Re: [PATCH 3/4] hook: include hooks from the config

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 10, 2026, 13:43 UTC
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:

Show 47 quoted lines

> 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