Re: [PATCH v7 1/7] run-command: add duplicate_output_fn to run_processes_parallel_opts

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Feb 07 2023, Calvin Wan wrote:

> diff --git a/run-command.c b/run-command.c
> index 756f1839aa..cad88befe0 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1526,6 +1526,9 @@ static void pp_init(struct parallel_processes *pp,
>  	if (!opts->get_next_task)
>  		BUG("you need to specify a get_next_task function");
>  
> +	if (opts->duplicate_output && opts->ungroup)
> +		BUG("duplicate_output and ungroup are incompatible with each other");
> +
>  	CALLOC_ARRAY(pp->children, n);
>  	if (!opts->ungroup)
>  		CALLOC_ARRAY(pp->pfd, n);

A trivial request, not worth a re-roll in itself: The "prep" topic[1] I
have for Emily's eventual config-based hooks doesn't need to add new
run-command.c modes that are incompatible with ungroup, but that happens
in the next stage of that saga.

When I merge your topic here with that, the end result here is:

	if (opts->ungroup) {
		if (opts->feed_pipe)
			BUG(".ungroup=1 is incompatible with .feed_pipe != NULL");
		if (opts->consume_sideband)
			BUG(".ungroup=1 is incompatible with .consume_sideband != NULL");
	}

	if (!opts->get_next_task)
		BUG("you need to specify a get_next_task function");

	if (opts->duplicate_output && opts->ungroup)
		BUG("duplicate_output and ungroup are incompatible with each other");

So, whether do the incompatibility check before or after
"get_next_task" is arbitrary. If I had to pick, I think doing it after as you're
doing here probably makes more sense.

But would ou mind if this addition of yours were instead:

	if (opts->ungroup) {
		if (opts->duplicate_output)
			BUG("duplicate_output and ungroup are incompatible with each other")
	}

Like I said, a trivial request.

But it will save us the eventual refactoring of that into nested checks
as we add more of these options.

To the extent that we need to mention the seemingly odd looking pattern
we could just say that we're future-proofing this for future
incompatible modes.

1. https://lore.kernel.org/git/cover-0.5-00000000000-20230123T170550Z-avarab@gmail.com/#t

> @@ -1645,14 +1648,21 @@ 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);

This s/int/ssize_t/ change is a good on, but not mentioned in the commit
message. Maybe worth splitting out?

If I revert that back to "int" on top of this entire topic our tests
still pass, so while it's a good change it seems entirely unrelated to
the "duplicate_output" subject of this patch.

>  			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) {

Here you're adding braces, which is an otherwise good change (but maybe
worth splitting up, I haven't read the rest of this topic to see if
there's even more style changes).

In this case we should/could have done this change with the pre-image,
before "duplicate_output".

>  				if (errno != EAGAIN)
>  					die_errno("read");
> +			} else {
> +				if (opts->duplicate_output)

I've read ahead and this topic adds nothing new to this "else" block, so
why the extra indentation instead of:

	} else if (opts->duplicate_output) {
		[...];

> +					opts->duplicate_output(&pp->children[i].err,
> +					       strlen(pp->children[i].err.buf) - n,

Uh, why are we getting the length of strbuf with strlen()? Am I missing
something obvious here, or should this be:

	pp->children[i].err.len - n

?

> +					       opts->data,
> +					       pp->children[i].data);

Especially with how otherwise painful the wrapping is here (well, not
very, but we can easily save a \t-indent here).

> +			}
>  		}
>  	}
>  }
> diff --git a/run-command.h b/run-command.h
> index 072db56a4d..6dcf999f6c 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -408,6 +408,27 @@ typedef int (*start_failure_fn)(struct strbuf *out,
>  				void *pp_cb,
>  				void *pp_task_cb);
>  
> +/**
> + * 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.
> + *
> + * pp_cb is the callback cookie as passed into run_processes_parallel,
> + * pp_task_cb is the callback cookie as passed into get_next_task_fn.
> + *
> + * This function is incompatible with "ungroup"
> + */
> +typedef void (*duplicate_output_fn)(struct strbuf *out,
> +				    size_t offset,
> +				    void *pp_cb,
> +				    void *pp_task_cb);

There's some over-wrapping here, I see some existing code does it, but
for new code we could follow our usual style, which would put this on
two lines.

> +
>  /**
>   * This callback is called on every child process that finished processing.
>   *
> @@ -461,6 +482,12 @@ struct run_process_parallel_opts
>  	 */
>  	start_failure_fn start_failure;
>  
> +	/**
> +	 * duplicate_output: See duplicate_output_fn() above. This should be
> +	 * NULL unless process specific output is needed
> +	 */

Here we mostly refer to the previous docs, but the "unless process
specific output is neeed" is very confusing. Without seeing the name or
having read the above I'd think this were some "do_not_pipe_to_dev_null"
feature.

Shouldn't we say "Unless you need to capture the output... leave this at
NULL" or something?

> +static void duplicate_output(struct strbuf *out,
> +			size_t offset,
> +			void *pp_cb UNUSED,
> +			void *pp_task_cb UNUSED)
> +{
> +	struct string_list list = STRING_LIST_INIT_DUP;
> +
> +	string_list_split(&list, out->buf + offset, '\n', -1);
> +	for (size_t i = 0; i < list.nr; i++) {
> +		if (strlen(list.items[i].string) > 0)

First, you can use for_each_string_list_item() here to make this look
much nicer/simpler.

Second, don't use strlen(s) > 0, just use strlen(s).

Third, you can git rid of the {} braces for the "for" here.

But just getting rid of that strlen() check and printing makes all your
tests pass.

And why is this thing that wants to prove to us that we're capturing the
output wanting to strip successive newlines?

Using a struct string_list for this is also pretty wasteful, we could
just make this a while-loop that printed this string when it sees "\n".

But it's just test code, so we don't care, I think it's fine for it to
be wastful, I just don't see why it's doing what it's doing, and what
it's going out of its way to do isn't tested for here.

> +test_expect_success 'run_command runs in parallel with more jobs available than tasks --duplicate-output' '
> +	test-tool run-command --duplicate-output run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
> +	test_must_be_empty out &&
> +	test 4 = $(grep -c "duplicate_output: Hello" err) &&
> +	test 4 = $(grep -c "duplicate_output: World" err) &&
> +	sed "/duplicate_output/d" err > err1 &&

Style: ">f" not "> f".