From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Sat, 29 Nov 2025 13:03:52 GMT
Subject: Re: [PATCH v3 02/10] hook: provide stdin via callback
Message-ID: <87iketf0sn.fsf@gentoo.mail-host-address-is-not-set>
In-Reply-To: <20251124172043.1650014-3-adrian.ratiu@collabora.com>

On Mon, 24 Nov 2025, Adrian Ratiu <adrian.ratiu@collabora.com> 
wrote:
> From: Emily Shaffer <emilyshaffer@google.com> 
> 
> This adds a callback mechanism for feeding stdin to hooks 
> alongside the existing path_to_stdin (which slurps a file's 
> content to stdin). 
> 
> The advantage of this new callback is that it can feed stdin 
> without going through the FS layer. This helps when feeding 
> large amount of data and uses the run-command parallel stdin 
> callback introduced in the preceding commit. 
> 
> 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> --- 
>  hook.c | 15 +++++++++++++++ hook.h | 38 
>  ++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 
>  insertions(+) 
> 
> diff --git a/hook.c b/hook.c index b3de1048bf..cd2bb7418a 100644 
> --- a/hook.c +++ b/hook.c @@ -65,11 +65,22 @@ static int 
> pick_next_hook(struct child_process *cp, 
>   cp->no_stdin = 1; strvec_pushv(&cp->env, 
>  hook_cb->options->env.v); 
> + +	if (hook_cb->options->path_to_stdin && 
> hook_cb->options->feed_pipe) +		BUG("options 
> path_to_stdin and feed_pipe are mutually exclusive"); + 
>  	/* reopen the file for stdin; run_command closes it. */ if 
>  (hook_cb->options->path_to_stdin) { cp->no_stdin = 0; cp->in = 
>  xopen(hook_cb->options->path_to_stdin, O_RDONLY); } 
> + +	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; cp->dir = hook_cb->options->dir; 
> @@ -140,6 +151,7 @@ int run_hooks_opt(struct repository *r, 
> const char *hook_name, 
>   .get_next_task = pick_next_hook, .start_failure = 
>  notify_start_failure, 
> +		.feed_pipe = options->feed_pipe, 
>  		.task_finished = notify_hook_finished,  .data = 
>  &cb_data, 
> @@ -148,6 +160,9 @@ int run_hooks_opt(struct repository *r, 
> const char *hook_name, 
>  	if (!options) BUG("a struct run_hooks_opt must be provided 
>  to run_hooks");  
> +	if (options->path_to_stdin && options->feed_pipe) + 
> BUG("options path_to_stdin and feed_pipe are mutually 
> exclusive"); + 
>  	if (options->invoked_hook) *options->invoked_hook = 0;  
> diff --git a/hook.h b/hook.h index 11863fa734..dd87326a5a 100644 
> --- a/hook.h +++ b/hook.h @@ -1,6 +1,7 @@ 
>  #ifndef HOOK_H #define HOOK_H #include "strvec.h" 
> +#include "run-command.h" 
>   struct repository;  
> @@ -37,6 +38,43 @@ struct run_hooks_opt 
>  	 * Path to file which should be piped to stdin for each 
>  hook.  */ const char *path_to_stdin; 
> + +	/** +	 * Callback used to incrementally feed a child 
> hook stdin pipe.  +	 * +	 * Useful especially if a hook 
> consumes large quantities of data +	 * (e.g. a list of all 
> refs in a client push), so feeding it via +	 * in-memory 
> strings or slurping to/from files is inefficient.  +	 * While 
> the callback allows piecemeal writing, it can also be +	 * 
> used for smaller inputs, where it gets called only once.  +	 * 
> +	 * Add hook callback initalization context to 
> `feed_pipe_ctx`.  +	 * Add Hook callback internal state to 
> `feed_pipe_cb_data`.  +	 * +	 */ +	feed_pipe_fn 
> feed_pipe; + +	/** +	 * Opaque data pointer used to 
> pass context to `feed_pipe_fn`.  +	 * +	 * It can be 
> accessed via the second callback arg: +	 * ((struct 
> hook_cb_data *) pp_cb)->hook_cb->options->feed_pipe_ctx; +	 * 
> +	 * The caller is responsible for managing the memory for 
> this data.  +	 * Only useful when using 
> `run_hooks_opt.feed_pipe`, otherwise ignore it.  +	 */ + 
> void *feed_pipe_ctx; + +	/** +	 * Opaque data pointer 
> used to keep internal state across callback calls.  +	 * +	 * 
> It can be accessed via the second callback arg: +	 * 
> ((struct hook_cb_data *) 
> pp_cb)->hook_cb->options->feed_pipe_cb_data; 

I just noticed the small inconsistency in this comment (second cb 
arg vs the actual code example). Will fix in v4.

I also have an idea how to further simplify this API based on the 
parallel hook execution work I'm doing (that is a separate patch 
series built upon this one), so I'll see if I can make this 
simpler, to avoid going through hook_cb->options entirely, similar 
to what Patrick suggested in v2.