From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 20 Feb 2026 12:46:19 GMT
Subject: Re: [PATCH v2 4/8] hook: include hooks from the config
Message-ID: <aZhXm1-jw_Mi8-vL@pks.im>
In-Reply-To: <20260218222352.55393-5-adrian.ratiu@collabora.com>

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.

> @@ -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}".

> +/*
> + * 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.

> +	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.

[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?

Patrick