Re: [PATCH v4 1/5] run-command: add duplicate_output_fn to run_processes_parallel_opts
- From
Glen Choo <chooglen@google.com>
- Date
- Nov 29, 2022, 23:29 UTC
- Message-ID
- <kl6lo7spqqzg.fsf@chooglen-macbookpro.roam.corp.google.com>
- In-Reply-To
- <20221108184200.2813458-2-calvinwan@google.com>
Calvin Wan <calvinwan@google.com> writes:
Show 17 quoted lines
> @@ -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.