From: Adrian Ratiu Date: Fri, 26 Sep 2025 12:03:45 GMT Subject: Re: [PATCH 02/10] hook: provide stdin via callback Message-ID: <87ecrto1y6.fsf@collabora.com> In-Reply-To: On Thu, 25 Sep 2025, Junio C Hamano wrote: > Adrian Ratiu 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. >> @@ -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!