Calvin Wan <calvinwan@xxxxxxxxxx> writes:
> @@ -1645,14 +1650,19 @@ 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);
> if (n == 0) {
> close(pp->children[i].process.err);
> pp->children[i].state = GIT_CP_WAIT_CLEANUP;
> - } else if (n < 0)
> + } else if (n < 0) {
> if (errno != EAGAIN)
> die_errno("read");
> + } else if (opts->duplicate_output) {
> + opts->duplicate_output(&pp->children[i].err,
> + pp->children[i].err.len - n,
> + opts->data, pp->children[i].data);
> + }
> }
> }
> }
What do we think of the name "duplicate_output"? IMO it made sense in
earlier versions when we were copying the output to a separate buffer (I
believe it was renamed in response to [1]), but now that we're just
calling a callback on the main buffer, it seems misleading. Maybe
"output_buffered" would be better?
Sidenote: One convention from JS that I like is to name such event
listeners as "on_<event_name>", e.g. "on_output_buffered". This makes
naming a lot easier sometimes because you don't have to worry about
having your event listener being mistaken for something else. It
wouldn't be idiomatic for Git today, but I wonder what others think
about adopting this.
[1] https://lore.kernel.org/git/xmqq4jvxpw46.fsf@gitster.g/
> +/**
> + * This callback is called whenever output from a child process is buffered
> + *
> + * See run_processes_parallel() below for a discussion of the "struct
> + * strbuf *out" parameter.
> + *
> + * The offset refers to the number of bytes originally in "out" before
> + * the output from the child process was buffered. Therefore, the buffer
> + * range, "out + buf" to the end of "out", would contain the buffer of
> + * the child process output.
Looks like there's extra whitespace on the 'blank' lines.
