Re: [PATCH 02/10] hook: provide stdin via callback

[Adrian Ratiu <adrian.ratiu@collabora.com>]

On Thu, 25 Sep 2025, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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