From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Sat, 20 Dec 2025 10:40:19 GMT
Subject: Re: [PATCH v5 11/11] receive-pack: convert receive hooks to hook API
Message-ID: <87h5tl4ess.fsf@gentoo.mail-host-address-is-not-set>
In-Reply-To: <aUVHVMNTFWWn2xjZ@pks.im>

On Fri, 19 Dec 2025, Patrick Steinhardt <ps@pks.im> wrote:
> On Thu, Dec 18, 2025 at 07:11:25PM +0200, Adrian Ratiu wrote:
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index d1c40a768d..f22d975879 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -933,20 +878,51 @@ static int run_receive_hook(struct command *commands,
>>  			    int skip_broken,
>>  			    const struct string_list *push_options)
>>  {
>> -	struct receive_hook_feed_state state;
>> -	int status;
>> -
>> -	strbuf_init(&state.buf, 0);
>> -	state.cmd = commands;
>> -	state.skip_broken = skip_broken;
>> -	state.report = NULL;
>> -	if (feed_receive_hook(&state, NULL, NULL))
>> +	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
>> +	struct command *iter = commands;
>> +	struct receive_hook_feed_state *feed_state;
>> +	int ret;
>> +
>> +	/* if there are no valid commands, don't invoke the hook at all. */
>> +	while (iter && skip_broken && (iter->error_string || iter->did_not_exist))
>> +		iter = iter->next;
>> +	if (!iter)
>>  		return 0;
>> -	state.cmd = commands;
>> -	state.push_options = push_options;
>> -	status = run_and_feed_hook(hook_name, feed_receive_hook, &state);
>> -	strbuf_release(&state.buf);
>> -	return status;
>> +
>> +	if (push_options) {
>> +		int i;
>
> Nit: this variable could be declared in the loop.
>
>> +		for (i = 0; i < push_options->nr; i++)
>> +			strvec_pushf(&opt.env, "GIT_PUSH_OPTION_%d=%s", i,
>> +				     push_options->items[i].string);
>> +		strvec_pushf(&opt.env, "GIT_PUSH_OPTION_COUNT=%"PRIuMAX"",
>> +					     (uintmax_t)push_options->nr);
>> +	} else
>> +		strvec_push(&opt.env, "GIT_PUSH_OPTION_COUNT");
>
> Nit: this should also use curly braces according to our modern coding
> guidelines:
>
> 	- When there are multiple arms to a conditional and some of them
> 	  require braces, enclose even a single line block in braces for
> 	  consistency.
>
>> +	if (tmp_objdir)
>> +		strvec_pushv(&opt.env, tmp_objdir_env(tmp_objdir));
>> +
>> +	prepare_push_cert_sha1(&opt);
>> +
>> +	/* set up sideband printer */
>> +	if (use_sideband)
>> +		opt.consume_output = hook_output_to_sideband;
>> +
>> +	/* set up stdin callback */
>> +	feed_state = xmalloc(sizeof(struct receive_hook_feed_state));
>
> It feels somewhat unnecessary to allocate this structure as it could
> have just as well be allocated on the stack.
>
>> +	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;
>> +	opt.feed_pipe = feed_receive_hook_cb;
>> +
>> +	ret = run_hooks_opt(the_repository, hook_name, &opt);
>> +
>> +	strbuf_release(&feed_state->buf);
>> +	FREE_AND_NULL(opt.feed_pipe_cb_data);
>> +
>> +	return ret;
>>  }
>
> All of these are nits, and the remaining patches all look good to me.
> I'll leave it to you to decide whether you want to do one more (and
> hopefully last) reroll.

Thanks for the review, much appreciated as always.

Sure, I can do one more reroll to fix these latest nits.

Will give it about a week in case it gathers more feedback.