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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 7, 2023, 22:16 UTC
Message-ID: <230207.86y1p914ci.gmgdl@evledraar.gmail.com>
In-Reply-To: <20230207181706.363453-2-calvinwan@google.com>

On Tue, Feb 07 2023, Calvin Wan wrote:

Show 14 quoted lines

> 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

Show 8 quoted lines

> @@ -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.

Show 5 quoted lines

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

Show 32 quoted lines

> +			}
>  		}
>  	}
>  }
> 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.

Show 12 quoted 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?

Show 10 quoted lines

> +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.

Show 6 quoted lines

> +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".