Re: [PATCH 1/4] hook: run a list of hooks

[Patrick Steinhardt <ps@pks.im>]

On Wed, Feb 04, 2026 at 06:51:23PM +0200, Adrian Ratiu wrote:
> From: Emily Shaffer <emilyshaffer@google.com>
> 
> Teach hook.[hc] to run lists of hooks to prepare for multihook support.
> 
> Currently, the hook list contains only one entry representing the
> "legacy" hook from the hookdir, but next commits will allow users
> to supply more than one executable/command for a single hook event
> in addition to these default "legacy" hooks.
> 
> All hook commands still run sequentially. A further patch series will
> enable running them in parallel by increasing .jobs > 1 where possible.
> 
> Each hook command requires its own internal state copy, even when
> running sequentially, so add an API to allow hooks to duplicate/free
> their internal void *pp_task_cb state.

I think the order in this commit message is a bit off. We typically
first state the problem that we aim to solve before presenting the
solution.

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index b5379a4895..72fde2207c 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -849,7 +849,8 @@ struct receive_hook_feed_state {
>  
>  static int feed_receive_hook_cb(int hook_stdin_fd, void *pp_cb UNUSED, void *pp_task_cb)
>  {
> -	struct receive_hook_feed_state *state = pp_task_cb;
> +	struct string_list_item *h = pp_task_cb;
> +	struct receive_hook_feed_state *state = h->util;
>  	struct command *cmd = state->cmd;
>  
>  	strbuf_reset(&state->buf);
> @@ -901,6 +902,24 @@ static int feed_receive_hook_cb(int hook_stdin_fd, void *pp_cb UNUSED, void *pp_
>  	return state->cmd ? 0 : 1;  /* 0 = more to come, 1 = EOF */
>  }
>  
> +static void *copy_receive_hook_feed_state(const void *data)

Nit: our coding guidelines say that the name of the struct should come
first. So this would be `receive_hook_feed_state_copy()`.

> +{
> +	const struct receive_hook_feed_state *orig = data;
> +	struct receive_hook_feed_state *new_data = xmalloc(sizeof(*new_data));
> +	memcpy(new_data, orig, sizeof(*new_data));
> +	strbuf_init(&new_data->buf, 0);
> +	return new_data;
> +}
> +
> +static void free_receive_hook_feed_state(void *data)

Likewise, this would be `receive_hook_feed_state_free()`.

> diff --git a/hook.c b/hook.c
> index cde7198412..fb90f91f3b 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -47,9 +47,49 @@ const char *find_hook(struct repository *r, const char *name)
>  	return path.buf;
>  }
>  
> +/*
> + * 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 string_list *hook_head;
> +
> +	if (!hookname)
> +		BUG("null hookname was provided to hook_list()!");
> +
> +	hook_head = xmalloc(sizeof(struct string_list));
> +	string_list_init_dup(hook_head);
> +
> +	/*
> +	 * Add the default hook from hookdir. It does not have a friendly name
> +	 * like the hooks specified via configs, so add it with an empty name.
> +	 */
> +	if (r->gitdir && find_hook(r, hookname))
> +		string_list_append(hook_head, "");

Why is there a check for `r->gitdir` here? Do we ever execute hooks
outside of a fully-initialized repository?

Other than that, we now insert hooks into the list. It's somewhat
surprising that we insert hook "names" here, instead of for example
adding the full hook path to the list. Is there any specific reason for
this decision?

> +	return hook_head;
> +}
> +
>  int hook_exists(struct repository *r, const char *name)
>  {
> -	return !!find_hook(r, name);
> +	int exists = 0;
> +	struct string_list *hooks = list_hooks(r, name);
> +
> +	exists = hooks->nr > 0;
> +
> +	string_list_clear(hooks, 1);
> +	free(hooks);
> +	return exists;
>  }
>  
>  static int pick_next_hook(struct child_process *cp,

And here we verify that there's at least one such hook that we found.
Which would currently mean that the hook in ".git/hooks/" exists.

> @@ -58,10 +98,11 @@ static int pick_next_hook(struct child_process *cp,
>  			  void **pp_task_cb)
>  {
>  	struct hook_cb_data *hook_cb = pp_cb;
> -	const char *hook_path = hook_cb->hook_path;
> +	struct string_list *hook_list = hook_cb->hook_command_list;
> +	struct string_list_item *to_run = hook_cb->next_hook++;
>  
> -	if (!hook_path)
> -		return 0;
> +	if (!to_run || to_run >= hook_list->items + hook_list->nr)
> +		return 0; /* no hook left to run */

Hm, okay. Wouldn't it be a bit more ergonomic to instead store the index
of the current hook instead of storing the string list item itself? In
that case we could verify whether `hook_cb->hook_cur < hook_list.nr` or
something like this.

> @@ -85,33 +126,50 @@ 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);
> +	/* find hook commands */
> +	if (!*to_run->string) {
> +		/* ...from hookdir signified by empty name */
> +		const char *hook_path = find_hook(hook_cb->repository,
> +						  hook_cb->hook_name);
> +		if (!hook_path)
> +			BUG("hookdir in hook list but no hook present in filesystem");
> +
> +		if (hook_cb->options->dir)
> +			hook_path = absolute_path(hook_path);
> +
> +		strvec_push(&cp->args, hook_path);
> +	}

We could avoid this special casing if we stored the absolute paths in
the hook list right away. But maybe there is a good reason you don't.

> +	if (!cp->args.nr)
> +		BUG("configured hook must have at least one command");
> +
>  	strvec_pushv(&cp->args, hook_cb->options->args.v);
>  
>  	/*
>  	 * Provide per-hook internal state via task_cb for easy access, so
>  	 * hook callbacks don't have to go through hook_cb->options.
>  	 */
> -	*pp_task_cb = hook_cb->options->feed_pipe_cb_data;
> -
> -	/*
> -	 * This pick_next_hook() will be called again, we're only
> -	 * running one hook, so indicate that no more work will be
> -	 * done.
> -	 */
> -	hook_cb->hook_path = NULL;
> +	*pp_task_cb = to_run;
>  
>  	return 1;
>  }
>  
> -static int notify_start_failure(struct strbuf *out UNUSED,
> +static int notify_start_failure(struct strbuf *out,
>  				void *pp_cb,
> -				void *pp_task_cp UNUSED)
> +				void *pp_task_cb)
>  {
>  	struct hook_cb_data *hook_cb = pp_cb;
> +	struct string_list_item *hook = pp_task_cb;
>  
>  	hook_cb->rc |= 1;
>  
> +	if (out && hook) {
> +		if (*hook->string)
> +			strbuf_addf(out, _("Couldn't start hook '%s'\n"), hook->string);
> +		else
> +			strbuf_addstr(out, _("Couldn't start hook from hooks directory\n"));
> +	}
> +
>  	return 1;
>  }

Okay, here we can give a bit more detail in the error message. But that
alone doesn't quite feel worth the additional complexity.

> @@ -172,26 +231,50 @@ int run_hooks_opt(struct repository *r, const char *hook_name,
>  	if (!options->jobs)
>  		BUG("run_hooks_opt must be called with options.jobs >= 1");
>  
> +	/*
> +	 * Ensure cb_data copy and free functions are either provided together,
> +	 * or neither one is provided.
> +	 */
> +	if ((options->copy_feed_pipe_cb_data && !options->free_feed_pipe_cb_data) ||
> +	    (!options->copy_feed_pipe_cb_data && options->free_feed_pipe_cb_data))
> +		BUG("copy_feed_pipe_cb_data and free_feed_pipe_cb_data must be set together");

I wonder whether it would make the commit a bit easier to review if you
split it up into two:

  - One commit that introduces the copy/free callback functions.

  - One commit that introduces the hook list.

Then the changes would be a bit more focussed.

>  	if (options->invoked_hook)
>  		*options->invoked_hook = 0;
>  
> -	if (!hook_path && !options->error_if_missing)
> -		goto cleanup;
> -
> -	if (!hook_path) {
> -		ret = error("cannot find a hook named %s", hook_name);
> +	if (!cb_data.hook_command_list->nr) {
> +		if (options->error_if_missing)
> +			ret = error("cannot find a hook named %s", hook_name);
>  		goto cleanup;
>  	}
>  
> -	cb_data.hook_path = hook_path;
> -	if (options->dir) {
> -		strbuf_add_absolute_path(&abs_path, hook_path);
> -		cb_data.hook_path = abs_path.buf;
> +	/*
> +	 * Initialize the iterator/cursor which holds the next hook to run.
> +	 * run_process_parallel() calls pick_next_hook() which increments it for
> +	 * each hook command in the list until all hooks have been run.
> +	 */
> +	cb_data.next_hook = cb_data.hook_command_list->items;
> +
> +	/*
> +	 * Give each hook its own copy of the initial void *pp_task_cb state, if
> +	 * a copy callback was provided.
> +	 */
> +	if (options->copy_feed_pipe_cb_data) {
> +		struct string_list_item *item;
> +		for_each_string_list_item(item, cb_data.hook_command_list)
> +			item->util = options->copy_feed_pipe_cb_data(options->feed_pipe_cb_data);
>  	}

Makes sense. We cannot just peform a shallow copy of the callback data
as it may contain data itself that needs to be copied. So we have to
handle the copying ourselves.

One thing I wondered is whether it would lead to a cleaner design if
this was instead provided as a callback to allocate and initialize a
completely _new_ callback data. In that case the caller wouldn't have to
first initialize the data only to copy it in a different place, all the
logic would be self-contained in that one callback.

The downside would of course be that we cannot use for example the
stack, and it might make it harder to reuse state across multiple hook
invocations, if that's a feature we care about.

Patrick