From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 26 Sep 2025 14:11:41 GMT
Subject: Re: [PATCH 04/10] transport: convert pre-push hook to hook.h
Message-ID: <1f942894-9393-4b5c-8d7f-2d0aaad594f1@gmail.com>
In-Reply-To: <20250925125352.1728840-5-adrian.ratiu@collabora.com>

Hi Adrian

On 25/09/2025 13:53, Adrian Ratiu wrote:
>   
> -static int run_pre_push_hook(struct transport *transport,
> -			     struct ref *remote_refs)
> +static int pre_push_hook_feed_stdin(int hook_stdin_fd, void *pp_cb, void *pp_task_cb UNUSED)
>   {
> -	int ret = 0, x;
> -	struct ref *r;
> -	struct child_process proc = CHILD_PROCESS_INIT;
> -	struct strbuf buf;
> -	const char *hook_path = find_hook(the_repository, "pre-push");
> -
> -	if (!hook_path)
> -		return 0;
> +	struct hook_cb_data *hook_cb = pp_cb;
> +	struct ref *r = hook_cb->options->feed_pipe_ctx;
>   
> -	strvec_push(&proc.args, hook_path);
> -	strvec_push(&proc.args, transport->remote->name);
> -	strvec_push(&proc.args, transport->url);
> +	if (r) {
> +		struct strbuf buf = STRBUF_INIT;

If we passed the strbuf in as part of the context and called 
strbuf_reset() before using it each time we'd avoid allocating a new 
buffer for each ref just as the current code does.

Thanks

Phillip

> +		int ret = 0;
> +		hook_cb->options->feed_pipe_ctx = r->next;
>   
> -	proc.in = -1;
> -	proc.trace2_hook_name = "pre-push";
> +		if (!r->peer_ref) return 0;
> +		if (r->status == REF_STATUS_REJECT_NONFASTFORWARD) return 0;
> +		if (r->status == REF_STATUS_REJECT_STALE) return 0;
> +		if (r->status == REF_STATUS_REJECT_REMOTE_UPDATED) return 0;
> +		if (r->status == REF_STATUS_UPTODATE) return 0;
>   
> -	if (start_command(&proc)) {
> -		finish_command(&proc);
> -		return -1;
> -	}
> +		strbuf_addf(&buf, "%s %s %s %s\n",
> +			    r->peer_ref->name, oid_to_hex(&r->new_oid),
> +			    r->name, oid_to_hex(&r->old_oid));
>   
> -	sigchain_push(SIGPIPE, SIG_IGN);
> +		ret = write_in_full(hook_stdin_fd, buf.buf, buf.len);
>   
> -	strbuf_init(&buf, 256);
> +		strbuf_release(&buf);
>   
> -	for (r = remote_refs; r; r = r->next) {
> -		if (!r->peer_ref) continue;
> -		if (r->status == REF_STATUS_REJECT_NONFASTFORWARD) continue;
> -		if (r->status == REF_STATUS_REJECT_STALE) continue;
> -		if (r->status == REF_STATUS_REJECT_REMOTE_UPDATED) continue;
> -		if (r->status == REF_STATUS_UPTODATE) continue;
> +		/* We do not mind if a hook does not read all refs. */
> +		if (ret < 0 && errno != EPIPE)
> +			return ret;
>   
> -		strbuf_reset(&buf);
> -		strbuf_addf( &buf, "%s %s %s %s\n",
> -			 r->peer_ref->name, oid_to_hex(&r->new_oid),
> -			 r->name, oid_to_hex(&r->old_oid));
> -
> -		if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
> -			/* We do not mind if a hook does not read all refs. */
> -			if (errno != EPIPE)
> -				ret = -1;
> -			break;
> -		}
> +		return 0;
>   	}
>   
> -	strbuf_release(&buf);
> +	return 1; /* we ran out of refs: no more input to feed */
> +}
>   
> -	x = close(proc.in);
> -	if (!ret)
> -		ret = x;
> +static int run_pre_push_hook(struct transport *transport,
> +			     struct ref *remote_refs)
> +{
> +	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
>   
> -	sigchain_pop(SIGPIPE);
> +	strvec_push(&opt.args, transport->remote->name);
> +	strvec_push(&opt.args, transport->url);
>   
> -	x = finish_command(&proc);
> -	if (!ret)
> -		ret = x;
> +	opt.feed_pipe = pre_push_hook_feed_stdin;
> +	opt.feed_pipe_ctx = remote_refs;
>   
> -	return ret;
> +	return run_hooks_opt(the_repository, "pre-push", &opt);
>   }
>   
>   int transport_push(struct repository *r,