From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 20 Feb 2026 12:45:59 GMT
Subject: Re: [PATCH v2 1/8] hook: add internal state alloc/free callbacks
Message-ID: <aZhXh6aqlY0VMgEG@pks.im>
In-Reply-To: <20260218222352.55393-2-adrian.ratiu@collabora.com>

On Thu, Feb 19, 2026 at 12:23:45AM +0200, Adrian Ratiu wrote:
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 2d2b33d73d..f23772bc56 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -901,6 +901,26 @@ 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 *receive_hook_feed_state_alloc(void *feed_pipe_ctx)
> +{
> +	struct receive_hook_feed_state *init_state = feed_pipe_ctx;
> +	struct receive_hook_feed_state *data = xcalloc(1, sizeof(*data));

Tiny nit, not worth addressing: we often use `CALLOC_ARRAY(data, 1)`
nowadays.

> +	data->report = init_state->report;
> +	data->cmd = init_state->cmd;
> +	data->skip_broken = init_state->skip_broken;
> +	strbuf_init(&data->buf, 0);
> +	return data;
> +}

Okay, this basically creates the new instance by creating a deep copy of
the "template" structure.

One could split this up so that we have a "configuration" struct and a
"data" struct, where we then provide a pointer to the configuration into
the data structure, as only the buffer needs to change between the
individual hook invocations. That would avoid some copying around, but
it feels a bit unnecessary.

> +static void receive_hook_feed_state_free(void *data)
> +{
> +	struct receive_hook_feed_state *d = data;
> +	if (!d)
> +		return;
> +	strbuf_release(&d->buf);
> +	free(d);
> +}

I would expect that the hook interfaces know to not call `free()` in
case `alloc()` wasn't called, but I guess it doesn't hurt to be
defensive here anyway.

> @@ -908,7 +928,7 @@ static int run_receive_hook(struct command *commands,
>  {
>  	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
>  	struct command *iter = commands;
> -	struct receive_hook_feed_state feed_state;
> +	struct receive_hook_feed_state feed_init_state = { 0 };
>  	struct async sideband_async;
>  	int sideband_async_started = 0;
>  	int saved_stderr = -1;
> @@ -938,16 +958,15 @@ static int run_receive_hook(struct command *commands,
>  	prepare_sideband_async(&sideband_async, &saved_stderr, &sideband_async_started);
>  
>  	/* set up stdin callback */
> -	feed_state.cmd = commands;
> -	feed_state.skip_broken = skip_broken;
> -	feed_state.report = NULL;
> -	strbuf_init(&feed_state.buf, 0);
> -	opt.feed_pipe_cb_data = &feed_state;
> +	feed_init_state.cmd = commands;
> +	feed_init_state.skip_broken = skip_broken;

As far as I can see all of the data that we pass to the state struct is
static, so we might just as well initialize it right away, right?

    struct receive_hook_feed_state feed_init_state = {
        .cmd = commands,
        .skip_broken = skip_broken,
        .buf = STRBUF_INIT,
    };

> diff --git a/hook.c b/hook.c
> index cde7198412..83ff658866 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -133,6 +133,8 @@ static int notify_hook_finished(int result,
>  
>  static void run_hooks_opt_clear(struct run_hooks_opt *options)
>  {
> +	if (options->feed_pipe_cb_data_free)
> +		options->feed_pipe_cb_data_free(options->feed_pipe_cb_data);
>  	strvec_clear(&options->env);
>  	strvec_clear(&options->args);
>  }

I guess this here would be where we could skip `free` in case the data
wasn't even allocated. But as I said further up, I don't care all that
much.

> diff --git a/hook.h b/hook.h
> index 20eb56fd63..a6bdc6f90f 100644
> --- a/hook.h
> +++ b/hook.h
> @@ -5,6 +5,9 @@
>  
>  struct repository;
>  
> +typedef void (*cb_data_free_fn)(void *data);
> +typedef void *(*cb_data_alloc_fn)(void *init_ctx);
> +
>  struct run_hooks_opt
>  {
>  	/* Environment vars to be set for each hook */

Do we maybe want to scope these function typedefs to the hooks subsystem
by calling the `hook_data_free_fn` and `hook_data_alloc_fn`, or
something like that?

Patrick