Re: [PATCH 02/10] hook: provide stdin via callback
On Thu, 25 Sep 2025, Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> 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. Yes, that is all correct.
Show 21 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.
Very good points. I agree we should flag a BUG() when both are
provided, because the stdin feed methods are mutually exclusive.
This is actually something I thought about while coding/testing v1
and just forgot to add in.
Will do in v2 and also make the comment less wide.
Many thanks for spotting this!
Adrian Ratiu <adrian.ratiu@collabora.com>