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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 5, 2026, 21:59 UTC
Message-ID: <xmqqqzqykg1c.fsf@gitster.g>
In-Reply-To: <20260204165126.1548805-2-adrian.ratiu@collabora.com>

Adrian Ratiu <adrian.ratiu@collabora.com> writes:

Show 12 quoted lines

> 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;

So the new calling convention is to give a string_list_item to the callback function, whose .util member has the historical "state". OK.

Show 49 quoted lines

> @@ -944,6 +963,8 @@ static int run_receive_hook(struct command *commands,
>  	strbuf_init(&feed_state.buf, 0);
>  	opt.feed_pipe_cb_data = &feed_state;
>  	opt.feed_pipe = feed_receive_hook_cb;
> +	opt.copy_feed_pipe_cb_data = copy_receive_hook_feed_state;
> +	opt.free_feed_pipe_cb_data = free_receive_hook_feed_state;
>  
>  	ret = run_hooks_opt(the_repository, hook_name, &opt);
>  
> 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, "");
> +
> +	return hook_head;
> +}

OK. I would have expected the caller to supply a string_list (presumably an empty one would be the most common usage) and the only responsibility for this function would be to stuff found hooks to the given string_list. But this arrangement to give a newly created string list would work just fine as well.

Show 12 quoted lines

>  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;
>  }

OK. It is not just "git hook list", but the internal function list_hooks() that is the topic of this step of the series. So anywhere that uses find_hook() in the current codebase is a good candidate to migrate to use the new function when appropriate, and this is one of such places. I presume that find_hook() will stay to be a "find the program in $GIT_DIR/hooks/ directory" function that has a single answer (i.e. pathnmame), but the list_hooks() may return more than one string_list_item, one of them might be the traditional hook and others may come from somewhere else. Do they need to be differentiated in some way (i.e., Is "where did the come from?" a legitimate thing for a caller of list_hook() to ask)? There are many unknowns while reading this step because we still haven't seen the other sources of the hooks.

Show 23 quoted lines

> @@ -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 */
>  
>  	cp->no_stdin = 1;
>  	strvec_pushv(&cp->env, hook_cb->options->env.v);
> @@ -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 */

Make a macro or a small helper function that takes a single string_list_item that is an element in the return value of list_hooks(), and tells if it is a program in $GIT_DIR/hooks/ directory. Writing code that depends on this "we did not bother to invent a way to allow users to give friendly names to traditional on-disk hooks, so it is registered with an empty string as its name" convention and having to document it is inefficient, both for writers of the code and for readers of the code.

Or perhaps the .util member of each of these can be a pointer to something like:

	struct {
		enum {
                	HOOK_TRADITIONAL, HOOK_CONFIGURED,
		} kind;
		union {
			struct {
				const char *path;
			} traditional;
			struct {
				const char *friendly_name;
				const char *path;
				... maybe others ...
			} configured;
		} u;
	}

so that this codepath does not even have to ...

> +		const char *hook_path = find_hook(hook_cb->repository,
> +						  hook_cb->hook_name);

... run the same find_hook() again here?

Show 13 quoted lines

> -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)

Ditto.

Show 7 quoted lines

> +			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;
>  }