Re: [PATCH 02/10] hook: provide stdin via callback

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 25, 2025, 20:05 UTC
Message-ID: <xmqqh5wqfgbs.fsf@gitster.g>
In-Reply-To: <20250925125352.1728840-3-adrian.ratiu@collabora.com>

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

Show 11 quoted lines

> @@ -69,6 +69,10 @@ static int pick_next_hook(struct child_process *cp,
>  	if (hook_cb->options->path_to_stdin) {
>  		cp->no_stdin = 0;
>  		cp->in = xopen(hook_cb->options->path_to_stdin, O_RDONLY);
> +	} else if (hook_cb->options->feed_pipe) {
> +		cp->no_stdin = 0;
> +		/* start_command() will allocate a pipe / stdin fd for us */
> +		cp->in = -1;
>  	}
>  	cp->stdout_to_stderr = 1;
>  	cp->trace2_hook_name = hook_cb->hook_name;

OK, so when feed_pipe is defined, just like when path_to_stdin is specified, we stop saying there is nothing coming from the standard input, and intead set cp->in so that the child process would read from there. Unlike path_to_stdin case it is not pointing at a file descriptor that is opened for a filesystem entity. ".in = -1" is a standard signal to run-command.[ch] machinery that a pipe to that child is to be prepared.

Show 16 quoted lines

> @@ -37,6 +38,24 @@ struct run_hooks_opt
>  	 * Path to file which should be piped to stdin for each hook.
>  	 */
>  	const char *path_to_stdin;
> +
> +	/**
> +	 * Callback to ask for more content to pipe to each hook stdin.
> +	 *
> +	 * If a hook needs to consume large quantities of data (e.g. a list of all refs received in a
> +	 * client push), feeding data via in-memory strings or slurping to/from files via path_to_stdin
> +	 * will not be efficient, so this callback allows for piecemeal reading and writing.
> +	 *
> +	 * Add initalization context to hook.feed_pipe_ctx.
> +	 */
> +	feed_pipe_fn feed_pipe;
> +	void *feed_pipe_ctx;

The comment for the member is a bit too wide. More importantly, this does not seem to capture the fact that this is completely ignored when path_to_stdin is already in effect. We should at least document it if we wanted to leave the behaviour as is, but I wonder if we want to detect and flag it as BUG() if both feed_pipe and path_to_stdin are not NULL. There is no inherent reason why the data prepared in a file must take precedence over data coming over a pipe.

Thanks.