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

[Jonathan Tan <jonathantanmy@google.com>]

> > > +/**
> > > + * 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

By the rest of the run_processes_parallel() API, do you mean
get_next_task_fn, start_failure_fn, and task_finished_fn? If yes, I
think that it makes sense for them to be strbuf, because buffering is
needed to avoid outputs from individual child processes interleaving,
but that's not true here.

Having said that, this is an internal API so we could just leave it
as-is and then refactor it if we ever need something more flexible.