Subject: Re: [PATCH v7 14/17] run-command: add stdin callback for parallelization

From: Emily Shaffer <[email-masked]>
Date: Mon, 22 Feb 2021 15:38:21 -0800

On Sun, Jan 31, 2021 at 10:51:53PM -0800, Jonathan Tan wrote:
> 
> > If a user of the run_processes_parallel() API wants to pipe a large
> > amount of information to stdin of each parallel command, that
> > information could exceed the buffer of the pipe allocated for that
> > process's stdin.  Generally this is solved by repeatedly writing to
> > child_process.in between calls to start_command() and finish_command();
> > run_processes_parallel() did not provide users an opportunity to access
> > child_process at that time.
> 
> [snip]
> 
> > diff --git a/run-command.h b/run-command.h
> > index 6472b38bde..e058c0e2c8 100644
> > --- a/run-command.h
> > +++ b/run-command.h
> > @@ -436,6 +436,20 @@ typedef int (*start_failure_fn)(struct strbuf *out,
> >  				void *pp_cb,
> >  				void *pp_task_cb);
> >  
> > +/**
> > + * This callback is called repeatedly on every child process who requests
> > + * start_command() to create a pipe by setting child_process.in < 0.
> > + *
> > + * pp_cb is the callback cookie as passed into run_processes_parallel, and
> > + * pp_task_cb is the callback cookie as passed into get_next_task_fn.
> > + * The contents of 'send' will be read into the pipe and passed to the pipe.
> > + *
> > + * Return nonzero to close the pipe.
> > + */
> > +typedef int (*feed_pipe_fn)(struct strbuf *pipe,
> > +			    void *pp_cb,
> > +			    void *pp_task_cb);
> > +
> 
> As you mention above in the commit message, I think the clearest API to
> support what we need is to just have a callback (that has access to
> child_process) that is executed between process start and finish.
> 
> As it is, I think this callback is too specific in that it takes a
> struct strbuf. I think that this struct strbuf will just end up being
> unnecessary copying much of the time, when the user could have just
> written to the fd directly.

Since the rest of the run_processes_parallel() API passes strings around
with strbufs, I'd prefer to leave it as-is to match the general API
expectations and style.

 - Emily