Re: [PATCH v2 2/8] hook: run a list of hooks to prepare for multihook support
On Thu, Feb 19, 2026 at 12:23:46AM +0200, Adrian Ratiu wrote:
Show 9 quoted lines
> 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.
Show 21 quoted lines
> +{
> + 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.
Show 14 quoted lines
> +}
> +
> +/* 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);
Show 39 quoted lines
> + /*
> + * 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);
Show 8 quoted lines
> @@ -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()`?
Show 42 quoted lines
> 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