From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 24 Nov 2025 22:55:07 GMT
Subject: Re: [PATCH v3 04/10] transport: convert pre-push to hook API
Message-ID: <xmqqy0nvujlg.fsf@gitster.g>
In-Reply-To: <20251124172043.1650014-5-adrian.ratiu@collabora.com>

Adrian Ratiu <adrian.ratiu@collabora.com> writes:

> From: Emily Shaffer <emilyshaffer@google.com>
>
> Move the pre-push hook from custom run-command invocations to
> the new hook API which doesn't require a custom child_process
> structure and signal toggling.
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
>  transport.c | 95 ++++++++++++++++++++++++++++-------------------------
>  1 file changed, 51 insertions(+), 44 deletions(-)

So, this completes what 01/10 hinted at when it created a
generalized interface modelled after how pre-push hook was run.  We
used to spawn the pre-push hook and fed its standard input by
calling write_in_full().  Now that is largely encapsulated in
run_hooks_opt(), but the application specific processing (namely,
what we write to the pre-push hook, i.e. the list of ref update
status) is given in pre_push_hook_feed_stdin() callback defined here
and given to the run_hooks_opt() call.

In other words, the mechanisms are very cleanly separated between
generic machinery and the client specific processing.  Nice.

How and where does the pipe we are writing into (i.e. hook_stdin_fd)
gets closed when we are done with the child?

> +static int pre_push_hook_feed_stdin(int hook_stdin_fd, void *pp_cb, void *pp_task_cb UNUSED)
> +{
> +	struct hook_cb_data *hook_cb = pp_cb;
> +	struct feed_pre_push_hook_data *data = hook_cb->options->feed_pipe_cb_data;
> +	const struct ref *r = data->refs;
> +	int ret = 0;
>  
> -	strvec_push(&proc.args, hook_path);
> -	strvec_push(&proc.args, transport->remote->name);
> -	strvec_push(&proc.args, transport->url);
> +	if (!r)
> +		return 1; /* no more refs */
>  
> -	proc.in = -1;
> -	proc.trace2_hook_name = "pre-push";
> +	data->refs = r->next;
>  
> -	if (start_command(&proc)) {
> -		finish_command(&proc);
> -		return -1;
> +	switch (r->status) {
> +	case REF_STATUS_REJECT_ALREADY_EXISTS:
> +	case REF_STATUS_REJECT_FETCH_FIRST:
> +	case REF_STATUS_REJECT_NEEDS_FORCE:
> +	case REF_STATUS_REJECT_NODELETE:
> +	case REF_STATUS_REJECT_NONFASTFORWARD:
> +	case REF_STATUS_REJECT_REMOTE_UPDATED:
> +	case REF_STATUS_REJECT_SHALLOW:
> +	case REF_STATUS_REJECT_STALE:
> +	case REF_STATUS_UPTODATE:
> +		return 0; /* skip refs which won't be pushed */
> +	default:
> +		break;
>  	}
>  
> -	sigchain_push(SIGPIPE, SIG_IGN);
> +	if (!r->peer_ref)
> +		return 0;
>  
> -	strbuf_init(&buf, 256);
> +	strbuf_reset(&data->buf);
> +	strbuf_addf(&data->buf, "%s %s %s %s\n",
> +		    r->peer_ref->name, oid_to_hex(&r->new_oid),
> +		    r->name, oid_to_hex(&r->old_oid));
>  
> -	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;
> +	ret = write_in_full(hook_stdin_fd, data->buf.buf, data->buf.len);
> +	if (ret < 0 && errno != EPIPE)
> +		return ret; /* We do not mind if a hook does not read all refs. */
>  
> -		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));
> +	return 0;
> +}
>  
> -		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;
> -		}
> -	}
> +static int run_pre_push_hook(struct transport *transport,
> +			     struct ref *remote_refs)
> +{
> +	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
> +	struct feed_pre_push_hook_data data;
> +	int ret = 0;
> +
> +	strvec_push(&opt.args, transport->remote->name);
> +	strvec_push(&opt.args, transport->url);
>  
> -	strbuf_release(&buf);
> +	strbuf_init(&data.buf, 0);
> +	data.refs = remote_refs;
>  
> -	x = close(proc.in);
> -	if (!ret)
> -		ret = x;
> +	opt.feed_pipe = pre_push_hook_feed_stdin;
> +	opt.feed_pipe_cb_data = &data;
>  
> -	sigchain_pop(SIGPIPE);
> +	ret = run_hooks_opt(the_repository, "pre-push", &opt);
>  
> -	x = finish_command(&proc);
> -	if (!ret)
> -		ret = x;
> +	strbuf_release(&data.buf);
>  
>  	return ret;
>  }