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?
+ 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.
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.
Best Wishes
Phillip
