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

From: Emily Shaffer <[email-masked]>
Date: Wed, 10 Mar 2021 10:24:20 -0800

On Tue, Feb 23, 2021 at 11:33:24AM -0800, Jonathan Tan wrote:
> 
> > > > +/**
> > > > + * 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.

Yeah, with that in mind I'll leave it as it is. I don't like the idea of
directly exposing the child's pipe via callback; to me it feels like bad
object-oriented design, but that maybe doesn't apply here :)

Anyway, like you say, we can change it later if someone doesn't like
this, no problem.