Re: [PATCH v8 1/6] run-command: add duplicate_output_fn to run_processes_parallel_opts

From: Glen Choo <chooglen@google.com>
To: Calvin Wan <calvinwan@google.com>, git@vger.kernel.org
Cc: Calvin Wan <calvinwan@google.com>,
	avarab@gmail.com, newren@gmail.com, jonathantanmy@google.com,
	phillip.wood123@gmail.com
Subject: Re: [PATCH v8 1/6] run-command: add duplicate_output_fn to run_processes_parallel_opts
Date: Mon, 13 Feb 2023 14:34:57 +0800	[thread overview]
Message-ID: <kl6lo7pyax9q.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
In-Reply-To: <20230209000212.1892457-2-calvinwan@google.com>

Calvin Wan <calvinwan@google.com> 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.