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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 13, 2023, 17:52 UTC
Message-ID: <xmqqbklxcv1v.fsf@gitster.g>
In-Reply-To: <kl6lo7pyax9q.fsf@chooglen-macbookpro.roam.corp.google.com>

Glen Choo <chooglen@google.com> writes:

Show 5 quoted lines

> 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?

Yeah, we do not even know what the callback does to the data we are giving it. The only thing we know is that we have output from the child, and in addition to the usual buffering we do ourselves, we are allowing the callback to peek into the buffered data in advance.

If the callback does consume it *and* remove the buffered data it consumed right away, then as you say, "duplicate" becomes a word that totally misses the point. There is no duplication, as the callback consumed and we no longer has our own copy, either.

If the callback consumes it but leaves the buffered data as-is, and we would show that once the child finishes anyway, you can say that we are feeding a duplicate of buffered data to the callback. The mechanism could be used merely to count how much output we have accumulated so far to update the progress-bar, for example, and the output may be given after the process is done. But note that we are not doing an "output" of "buffered" data in such a case.

To me, both "duplicate_output" and "output_buffered" sound like they are names that are quite specific to the expected use case the person who proposed the names had in mind, yet it is a bit hard to guess exactly what the expected use cases they had in mind were, because the names are not quite specific enough.

> Sidenote: One convention from JS that I like is to name such event
> listeners as "on_<event_name>", e.g. "on_output_buffered".

Thanks for bringing this up. I agree that "Upon X happening, do this" is a very good convention to follow. I think the callback is made whenever the child emits to the standard error stream, so "on_error_output" (if we are worried that "error" has a too strong "something bad happend" connotation, then perhaps "on_stderr_output" may dampen it) perhaps?