Re: [PATCH v4 1/5] run-command: add duplicate_output_fn to run_processes_parallel_opts
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Nov 30, 2022, 09:53 UTC
- Message-ID
- <221130.86lensiwy0.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <kl6lo7spqqzg.fsf@chooglen-macbookpro.roam.corp.google.com>
On Tue, Nov 29 2022, Glen Choo wrote:
Show 39 quoted lines
> Calvin Wan <calvinwan@google.com> 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)
>> + 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);
> + }
>
> which also has the nice benefit of not touching the strbuf_read_once()
> line.We should also use "size_t n" there, not "int n", which is what it returns. It won't matter for overflow in this case, but let's not truncate for no good reason, it makes spotting actual overflows in compiler output harder.
And why does "&buf" exist at all? 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...).
That would avoid the copy entirely.