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

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

On Fri, 20 Feb 2026, Patrick Steinhardt <ps@pks.im> 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?