From: Adrian Ratiu Date: Tue, 10 Feb 2026 13:56:28 GMT Subject: Re: [PATCH 3/4] hook: include hooks from the config Message-ID: <87zf5gzoqb.fsf@collabora.com> In-Reply-To: On Tue, 10 Feb 2026, Patrick Steinhardt wrote: > On Mon, Feb 09, 2026 at 09:10:25PM +0200, Adrian Ratiu wrote: >> On Mon, 09 Feb 2026, Patrick Steinhardt 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.