Re: [PATCH v2 1/8] hook: add internal state alloc/free callbacks
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Feb 20, 2026, 12:45 UTC
- 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:
Show 12 quoted lines
> 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.
Show 6 quoted lines
> + 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.
Show 8 quoted lines
> +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.
Show 20 quoted lines
> @@ -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,
};Show 13 quoted lines
> 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.
Show 14 quoted lines
> 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