On Wed, Nov 30, 2022 at 2:26 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
>
> On 30/11/2022 09:53, Ævar Arnfjörð Bjarmason wrote:
> >
> > On Tue, Nov 29 2022, Glen Choo wrote:
> >
> >> Calvin Wan <calvinwan@xxxxxxxxxx> writes:
> >>
> >>> @@ -1680,8 +1683,14 @@ 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);
> >>> + struct strbuf buf = STRBUF_INIT;
> >>> + int n = strbuf_read_once(&buf, pp->children[i].process.err, 0);
> >>> + strbuf_addbuf(&pp->children[i].err, &buf);
> >>> + if (opts->duplicate_output)
>
> Shouldn't we be checking if n < 0 before we do this?
Yes we should.
>
> >>> + opts->duplicate_output(&buf, &pp->children[i].err,
> >>> + opts->data,
> >>> + pp->children[i].data);
> >>> + strbuf_release(&buf);
> >>> if (n == 0) {
> >>> close(pp->children[i].process.err);
> >>> pp->children[i].state = GIT_CP_WAIT_CLEANUP;
> >>
> >> A common pattern is that optional behavior does not impose additional
> >> costs on the non-optional part. Here, we unconditionally do a
> >> strbuf_addbuf() even though we don't use the result in the "else" case.
> >>
> >> So this might be more idiomatically written as:
> >>
> >> int n = strbuf_read_once(&pp->children[i].err,
> >> pp->children[i].process.err, 0);
> >> + if (opts->duplicate_output) {
> >> + struct strbuf buf = STRBUF_INIT;
> >> + strbuf_addbuf(&buf, &pp->children[i].err);
> >> + opts->duplicate_output(&buf, &pp->children[i].err,
> >> + opts->data,
> >> + pp->children[i].data);
> >> + strbuf_release(&buf);
> >> + }
> >>
> > [...]
> > And why does "&buf" exist at all?
>
> I was wondering that as too
>
> >Why can't we just pass
> > &pp->children[i].err, and if this callback cares about the last thing we
> > read let's pass it an offset, so it can know what came from the
> > strbuf_read_once() (I don't know if it actually cares about that
> > either...).
>
> Or we could just pass a `const char*`, `size_t` pair.
Both you and Avar are correct that &buf isn't necessary. I think the
offset idea works better so the function calls stay similar.
>
> > That would avoid the copy entirely.
>
> Is the copying quadratic at the moment? - it looks like each call to
> strbuf_read_once() appends to the buffer and we copy the whole buffer
> each time.
>
Depends on how you're defining it, but doesn't really matter since it's
going away anyways ;)
