Re: [PATCH v7 1/7] run-command: add duplicate_output_fn to run_processes_parallel_opts

From: Calvin Wan <calvinwan@google.com>
Date: Feb 8, 2023, 22:50 UTC
Message-ID: <CAFySSZCX=tySZf1xSd59GH0qS9PWtUC6ekLB1bWtZR3c09+ttw@mail.gmail.com>
In-Reply-To: <230207.86y1p914ci.gmgdl@evledraar.gmail.com>

Show 6 quoted lines

> But would ou mind if this addition of yours were instead:
>
>         if (opts->ungroup) {
>                 if (opts->duplicate_output)
>                         BUG("duplicate_output and ungroup are incompatible with each other")
>         }

I don't see why not -- will change.

Show 11 quoted lines

> > @@ -1645,14 +1648,21 @@ static void pp_buffer_stderr(struct parallel_processes *pp,
> >       for (size_t i = 0; i < opts->processes; i++) {
> >               if (pp->children[i].state == GIT_CP_WORKING &&
> >                   pp->pfd[i].revents & (POLLIN | POLLHUP)) {
> > -                     int n = strbuf_read_once(&pp->children[i].err,
> > -                                              pp->children[i].process.err, 0);
> > +                     ssize_t n = strbuf_read_once(&pp->children[i].err,
> > +                                                  pp->children[i].process.err, 0);
>
> This s/int/ssize_t/ change is a good on, but not mentioned in the commit
> message. Maybe worth splitting out?

I'll call this and the style change out in the commit message instead of splitting it out.

> And why is this thing that wants to prove to us that we're capturing the
> output wanting to strip successive newlines?

I added it as a sanity check originally, but you're right that this is unnecessary. Thanks for your comments on the other stylistic nits. I've gone ahead and fixed them all.