From: Adrian Ratiu Date: Fri, 20 Feb 2026 14:31:10 GMT Subject: Re: [PATCH v2 4/8] hook: include hooks from the config Message-ID: <87tsvbwkpd.fsf@collabora.com> In-Reply-To: On Fri, 20 Feb 2026, Patrick Steinhardt wrote: > On Thu, Feb 19, 2026 at 12:23:48AM +0200, Adrian Ratiu wrote: >> diff --git a/builtin/hook.c b/builtin/hook.c >> index 51660c4941..e151bb2cd1 100644 >> --- a/builtin/hook.c >> +++ b/builtin/hook.c >> @@ -54,6 +56,10 @@ static void hook_clear(struct hook *h, cb_data_free_fn cb_data_free) >> >> if (h->kind == HOOK_TRADITIONAL) >> free((void *)h->u.traditional.path); >> + else if (h->kind == HOOK_CONFIGURED) { >> + free((void *)h->u.configured.friendly_name); >> + free((void *)h->u.configured.command); >> + } > > The `if` branch also needs curly braces now. Ack, will fix. >> @@ -101,6 +107,187 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, >> string_list_append(hook_list, hook_path)->util = h; >> } >> >> +static void unsorted_string_list_remove(struct string_list *list, >> + const char *str) >> +{ >> + struct string_list_item *item = unsorted_string_list_lookup(list, str); >> + if (item) >> + unsorted_string_list_delete_item(list, item - list->items, 0); >> +} > > This looks like a function that could reasonably be added to > "string-list.{c,h}". Yes, I'll move the helper there. >> +/* >> + * Callback struct to collect all hook.* keys in a single config pass. >> + * commands: friendly-name to command map. >> + * event_hooks: event-name to list of friendly-names map. >> + * disabled_hooks: set of friendly-names with hook.name.enabled = false. >> + */ >> +struct hook_all_config_cb { >> + struct strmap commands; >> + struct strmap event_hooks; > > Hm, curious that we've got two maps. I'd have expected to have a single > map from "friendly name" or "hook name" to `struct hook`. But maybe > we'll assemble these structs for those maps later on. > Exactly. I'll explain below. >> + struct string_list disabled_hooks; >> +}; > > We don't have support for disabled hooks yet. I assume this'll be added > by a later commit, only. Good catch. I need to move this definition to the later commit. > > [snip] >> +/* Populate `cache` with the complete hook configuration */ >> +static void build_hook_config_map(struct repository *r, struct strmap *cache) >> +{ >> + struct hook_all_config_cb cb_data; >> + struct hashmap_iter iter; >> + struct strmap_entry *e; >> + >> + strmap_init(&cb_data.commands); >> + strmap_init(&cb_data.event_hooks); >> + string_list_init_dup(&cb_data.disabled_hooks); >> + >> + /* Parse all configs in one run. */ >> + repo_config(r, hook_config_lookup_all, &cb_data); >> + >> + /* Construct the cache from parsed configs. */ >> + strmap_for_each_entry(&cb_data.event_hooks, &iter, e) { >> + struct string_list *hook_names = e->value; >> + struct string_list *hooks = xcalloc(1, sizeof(*hooks)); >> + >> + string_list_init_dup(hooks); >> + >> + for (size_t i = 0; i < hook_names->nr; i++) { >> + const char *hname = hook_names->items[i].string; >> + char *command; >> + >> + command = strmap_get(&cb_data.commands, hname); >> + if (!command) >> + die(_("'hook.%s.command' must be configured or " >> + "'hook.%s.event' must be removed;" >> + " aborting."), hname, hname); >> + >> + /* util stores the command; owned by the cache. */ >> + string_list_append(hooks, hname)->util = >> + xstrdup(command); >> + } >> + >> + strmap_put(cache, e->key, hooks); >> + } >> + >> + strmap_clear(&cb_data.commands, 1); >> + string_list_clear(&cb_data.disabled_hooks, 0); >> + strmap_for_each_entry(&cb_data.event_hooks, &iter, e) { >> + string_list_clear(e->value, 0); >> + free(e->value); >> + } >> + strmap_clear(&cb_data.event_hooks, 0); >> +} > > Okay, this is where we assemble the hooks. Still curious that the result > isn't a `struct hook` for each configured hook. > > [snip] >> +static void list_hooks_add_configured(struct repository *r, >> + const char *hookname, >> + struct string_list *list, >> + struct run_hooks_opt *options) >> +{ >> + struct strmap *cache = get_hook_config_cache(r); >> + struct string_list *configured_hooks = strmap_get(cache, hookname); >> + >> + /* Iterate through configured hooks and initialize internal states */ >> + for (size_t i = 0; configured_hooks && i < configured_hooks->nr; i++) { >> + const char *friendly_name = configured_hooks->items[i].string; >> + const char *command = configured_hooks->items[i].util; >> + struct hook *hook = xcalloc(1, sizeof(struct hook)); >> + >> + if (options && options->feed_pipe_cb_data_alloc) >> + hook->feed_pipe_cb_data = >> + options->feed_pipe_cb_data_alloc( >> + options->feed_pipe_ctx); >> + >> + hook->kind = HOOK_CONFIGURED; >> + hook->u.configured.friendly_name = xstrdup(friendly_name); >> + hook->u.configured.command = xstrdup(command); >> + >> + string_list_append(list, friendly_name)->util = hook; >> + } >> +} > > Okay, here we finally create the hook structures. Is there any specific > reason why we don't cache these structures directly? That is exactly what my first local cache implementation attempt did. I doesn't work because: 1. struct hook contains the internal hook state which cannot be reused betwee hook calls (nor can it be cached). 2. struct hook requires the initialization ctx passed to feed_pipe_cb_data_alloc() which is different depending on the hook. So 1 and 2 above make struct hook non-cacheable. Maybe the following explains it better. :) We have 3 different operations: 1. Parsing the config file once via hook_config_lookup_all() 2. Storing/reusing the config cache via build_hook_config_map() 3. Creating the struct hooks for execution via list_hooks_add_configured(). Caching can only work on the parsed config data, at step 2. Hope this all makes sense. Maybe we could rename these steps/functions to better reflect their purpouse, or document this better somewhere in the source code?