Re: [PATCH v2 04/10] transport: convert pre-push to hook API

[Patrick Steinhardt <ps@pks.im>]

On Fri, Oct 17, 2025 at 05:15:38PM +0300, Adrian Ratiu wrote:
> diff --git a/transport.c b/transport.c
> index c7f06a7382..67368754bf 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1316,65 +1316,56 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing)
>  	die(_("Aborting."));
>  }
>  
> -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");
> +	struct hook_cb_data *hook_cb = pp_cb;
> +	struct ref *r = hook_cb->options->feed_pipe_ctx;
> +	struct strbuf *buf = hook_cb->options->feed_pipe_cb_data;

Same question here, isn't `feed_pipe_cb_data` accessible via
`pp_task_cb`? May very well be that I misunderstand the two callback
context and data, I found that part to be a bit hard to follow.

> +	int ret = 0;
>  
> -	if (!hook_path)
> -		return 0;
> +	if (!r)
> +		return 1; /* no more refs */
>  
> -	strvec_push(&proc.args, hook_path);
> -	strvec_push(&proc.args, transport->remote->name);
> -	strvec_push(&proc.args, transport->url);
> +	if (!buf)
> +		BUG("pipe_task_cb must contain a valid strbuf");
>  
> -	proc.in = -1;
> -	proc.trace2_hook_name = "pre-push";
> +	hook_cb->options->feed_pipe_ctx = r->next;

I think that the lines between the "task data" and "task context" are
being blurred here. I understood it so that the task data is what is
specific to the callback, and that data may be changed to keep track of
the state. Subsequent commits do it that way, so shouldn't we also treat
the context as immutable here and instead handle iteration via the data?

> +	strbuf_reset(buf);

Nit: it would make sense to move this reset down a bit close to the
first call that writes to it.

> -	if (start_command(&proc)) {
> -		finish_command(&proc);
> -		return -1;
> -	}
> -
> -	sigchain_push(SIGPIPE, SIG_IGN);
> +	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;

Nit, feel free to ignore: this might read a tiny bit nicer with a switch
statement.

Patrick