From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 20 Feb 2026 12:46:08 GMT
Subject: Re: [PATCH v2 2/8] hook: run a list of hooks to prepare for multihook support
Message-ID: <aZhXkDtNvcNpXgpJ@pks.im>
In-Reply-To: <20260218222352.55393-3-adrian.ratiu@collabora.com>

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.

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

> +}
> +
> +/* 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);

> +	/*
> +	 * 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);

> @@ -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()`?

> diff --git a/hook.h b/hook.h
> index a6bdc6f90f..3256d2dddb 100644
> --- a/hook.h
> +++ b/hook.h
> @@ -2,9 +2,41 @@
>  #define HOOK_H
>  #include "strvec.h"
>  #include "run-command.h"
> +#include "string-list.h"
>  
>  struct repository;
>  
> +/**
> + * Represents a hook command to be run.
> + * Hooks can be:
> + * 1. "traditional" (found in the hooks directory)
> + * 2. "configured" (defined in Git's configuration, not yet implemented).
> + * The 'kind' field determines which part of the union 'u' is valid.
> + */
> +struct hook {
> +	enum {
> +		HOOK_TRADITIONAL,
> +	} kind;
> +	union {
> +		struct {
> +			const char *path;
> +		} traditional;
> +	} u;
> +
> +	/**
> +	 * Opaque data pointer used to keep internal state across callback calls.
> +	 *
> +	 * It can be accessed directly via the third hook callback arg:
> +	 * struct ... *state = pp_task_cb;
> +	 *
> +	 * The caller is responsible for managing the memory for this data by
> +	 * providing alloc/free callbacks to `run_hooks_opt`.
> +	 *
> +	 * Only useful when using `run_hooks_opt.feed_pipe`, otherwise ignore it.
> +	 */
> +	void *feed_pipe_cb_data;
> +};

Makes sense to wrap all of this in a specific data structure.

Patrick