Re: [PATCH v5 11/11] receive-pack: convert receive hooks to hook API

From: Patrick Steinhardt <ps@pks.im>
Date: Dec 19, 2025, 12:38 UTC
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:

Show 34 quoted lines

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

Show 7 quoted lines

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

Show 11 quoted lines

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

Show 14 quoted lines

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