From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 25 Sep 2025 20:05:43 GMT
Subject: Re: [PATCH 02/10] hook: provide stdin via callback
Message-ID: <xmqqh5wqfgbs.fsf@gitster.g>
In-Reply-To: <20250925125352.1728840-3-adrian.ratiu@collabora.com>

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

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

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