From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 19 Dec 2025 12:38:44 GMT
Subject: Re: [PATCH v5 11/11] receive-pack: convert receive hooks to hook API
Message-ID: <aUVHVMNTFWWn2xjZ@pks.im>
In-Reply-To: <20251218171126.588066-12-adrian.ratiu@collabora.com>

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!

Patrick