From: Emily Shaffer <nasamuffin@google.com>
Date: Fri, 10 Oct 2025 19:57:13 GMT
Subject: Re: [PATCH 02/10] hook: provide stdin via callback
Message-ID: <CAJoAoZm6uNtEoo_tdbqjGMSj4OnQuFesxt_iyOTgNHA1LX3iwQ@mail.gmail.com>
In-Reply-To: <20250925125352.1728840-3-adrian.ratiu@collabora.com>

On Thu, Sep 25, 2025 at 5:54 AM 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 |  8 ++++++++
>  hook.h | 22 ++++++++++++++++++++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/hook.c b/hook.c
> index b3de1048bf..54568d5bc0 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -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;
> @@ -140,6 +144,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 +153,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("choose only one method to populate hook stdin");
> +
>         if (options->invoked_hook)
>                 *options->invoked_hook = 0;
>
> diff --git a/hook.h b/hook.h
> index 11863fa734..8fdbc8c673 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,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;
> +
> +       /**
> +        * Use this to keep internal state for your feed_pipe_fn callback.
> +        * Only useful if you are using run_hooks_opt.feed_pipe. Otherwise, ignore it.
> +        */
> +       void *feed_pipe_cb_data;
>  };
>
>  #define RUN_HOOKS_OPT_INIT { \
> @@ -44,6 +63,9 @@ struct run_hooks_opt
>         .args = STRVEC_INIT, \
>  }
>
> +/**
> + * Callback data provided to feed_pipe_fn.
> + */

It looks like this comment was maybe a note to yourself? (Or a note to
myself, eons ago?) But hook_cb_data is used in all the parallel hook
callbacks, not just feed_pipe_fn, so I don't think this is accurate.


>  struct hook_cb_data {
>         /* rc reflects the cumulative failure state */
>         int rc;
> --
> 2.49.1
>