From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Fri, 20 Feb 2026 13:40:30 GMT
Subject: Re: [PATCH v2 1/8] hook: add internal state alloc/free callbacks
Message-ID: <87342vy1m9.fsf@gentoo.mail-host-address-is-not-set>
In-Reply-To: <aZhXh6aqlY0VMgEG@pks.im>

On Fri, 20 Feb 2026, Patrick Steinhardt <ps@pks.im> wrote:
> 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.

I'll fix this since I'm planning to do a v3 anyway. Thanks for the
pointer.

>
>> +	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.

Yes, I also thought about this and reached the same conclusion.

>
>> +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.

Agreed.

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

I missed this. Thanks. Will do in v3.

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

That is correct. I think I'll just drop that pointer check anyway in v3.

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

Good idea. I'll rename them in v3.