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

[Adrian Ratiu <adrian.ratiu@collabora.com>]

On Tue, 10 Feb 2026, Patrick Steinhardt <ps@pks.im> wrote:
> 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.

Ok, I'm convinced. :) Will do it in v2.