From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Fri, 20 Feb 2026 13:51:40 GMT
Subject: Re: [PATCH v2 2/8] hook: run a list of hooks to prepare for multihook support
Message-ID: <87zf53wmj7.fsf@collabora.com>
In-Reply-To: <aZhXkDtNvcNpXgpJ@pks.im>

On Fri, 20 Feb 2026, Patrick Steinhardt <ps@pks.im> wrote:
> On Thu, Feb 19, 2026 at 12:23:46AM +0200, Adrian Ratiu wrote:
>> diff --git a/hook.c b/hook.c
>> index 83ff658866..c008a7232d 100644
>> --- a/hook.c
>> +++ b/hook.c
>> @@ -47,9 +47,97 @@ const char *find_hook(struct repository *r, const char *name)
>>  	return path.buf;
>>  }
>>  
>> +static void hook_clear(struct hook *h, cb_data_free_fn cb_data_free)
>
> Nit: this should be called `hook_free()` as it also frees the struct
> itself.

Thanks. Junio also told me to prefer the _free() variants for functions
instead of _clear(), so I'll do that in v3.

I'll also carefully look so names match the semantics of the functions
I'm adding.

>> +{
>> +	if (!h)
>> +		return;
>> +
>> +	if (h->kind == HOOK_TRADITIONAL)
>> +		free((void *)h->u.traditional.path);
>> +
>> +	if (cb_data_free)
>> +		cb_data_free(h->feed_pipe_cb_data);
>> +
>> +	free(h);
>> +}
>> +
>> +static void hook_list_clear(struct string_list *hooks, cb_data_free_fn cb_data_free)
>> +{
>> +	struct string_list_item *item;
>> +
>> +	for_each_string_list_item(item, hooks)
>> +		hook_clear(item->util, cb_data_free);
>> +
>> +	string_list_clear(hooks, 0);
>
> We have `string_list_clear_func()` to perform a deep free for you.

Ack, will use it in v3.

>> +}
>> +
>> +/* Helper to detect and add default "traditional" hooks from the hookdir. */
>> +static void list_hooks_add_default(struct repository *r, const char *hookname,
>> +				   struct string_list *hook_list,
>> +				   struct run_hooks_opt *options)
>> +{
>> +	const char *hook_path = find_hook(r, hookname);
>> +	struct hook *h;
>> +
>> +	if (!hook_path)
>> +		return;
>> +
>> +	h = xcalloc(1, sizeof(struct hook));
>
> Better: CALLOC_ARRAY(h, 1);

Ack, will use it in v3.

>> +	/*
>> +	 * If the hook is to run in a specific dir, a relative path can
>> +	 * become invalid in that dir, so convert to an absolute path.
>> +	 */
>> +	if (options && options->dir)
>> +		hook_path = absolute_path(hook_path);
>> +
>> +	/* Setup per-hook internal state cb data */
>> +	if (options && options->feed_pipe_cb_data_alloc)
>> +		h->feed_pipe_cb_data = options->feed_pipe_cb_data_alloc(options->feed_pipe_ctx);
>> +
>> +	h->kind = HOOK_TRADITIONAL;
>> +	h->u.traditional.path = xstrdup(hook_path);
>> +
>> +	string_list_append(hook_list, hook_path)->util = h;
>> +}
>> +
>> +/*
>> + * Provides a list of hook commands to run for the 'hookname' event.
>> + *
>> + * This function consolidates hooks from two sources:
>> + * 1. The config-based hooks (not yet implemented).
>> + * 2. The "traditional" hook found in the repository hooks directory
>> + *    (e.g., .git/hooks/pre-commit).
>> + *
>> + * The list is ordered by execution priority.
>> + *
>> + * The caller is responsible for freeing the memory of the returned list
>> + * using string_list_clear() and free().
>> + */
>> +static struct string_list *list_hooks(struct repository *r, const char *hookname,
>> +			       struct run_hooks_opt *options)
>> +{
>> +	struct string_list *hook_head;
>> +
>> +	if (!hookname)
>> +		BUG("null hookname was provided to hook_list()!");
>> +
>> +	hook_head = xmalloc(sizeof(struct string_list));
>
> CALLOC_ARRAY(hook_head, 1);

Ack, will use it in v3.

>> @@ -85,21 +176,20 @@ static int pick_next_hook(struct child_process *cp,
>>  	cp->trace2_hook_name = hook_cb->hook_name;
>>  	cp->dir = hook_cb->options->dir;
>>  
>> -	strvec_push(&cp->args, hook_path);
>> +	/* Add hook exec paths or commands */
>> +	if (h->kind == HOOK_TRADITIONAL)
>> +		strvec_push(&cp->args, h->u.traditional.path);
>
> Should we be defensive here and have an `else BUG()`?

Yes, good catch. I think in another place where I verified this I
actually added a BUG.

Will add in v3.