Re: [PATCH v4 1/5] run-command: add duplicate_output_fn to run_processes_parallel_opts

From: Jonathan Tan <jonathantanmy@google.com>
Date: Nov 28, 2022, 20:45 UTC
Message-ID: <20221128204538.2748977-1-jonathantanmy@google.com>
In-Reply-To: <20221108184200.2813458-2-calvinwan@google.com>

Calvin Wan <calvinwan@google.com> writes:

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

Thanks for spotting "ungroup" - that helps me to focus my review.

Show 14 quoted lines

> @@ -1680,8 +1683,14 @@ 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);
> +			struct strbuf buf = STRBUF_INIT;
> +			int n = strbuf_read_once(&buf, pp->children[i].process.err, 0);
> +			strbuf_addbuf(&pp->children[i].err, &buf);
> +			if (opts->duplicate_output)
> +				opts->duplicate_output(&buf, &pp->children[i].err,
> +					  opts->data,
> +					  pp->children[i].data);
> +			strbuf_release(&buf);

[snip]

> Add duplicate_output_fn as an optionally set function in
> run_process_parallel_opts. If set, output from each child process is
> copied and passed to the callback function whenever output from the
> child process is buffered to allow for separate parsing.

Looking at this patch, since this new option is incompatible with "ungroup", I would have expected that the new functionality be in a place that already contains an "if (ungroup)", and thus would go into the "else" block. Looking at the code, it seems like a reasonable place would be in pp_collect_finished().

Is the reason this is not there because we only want the output of the child process, not anything that the callback functions might write to the out strbuf? If yes, is there a reason for that? If not, I think the code would be simpler if we did what I suggested. (Maybe this has already been discussed previously - if that is the case, the reason for doing it this way should be in the commit message.)

Show 35 quoted lines

> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
> index 3ecb830f4a..40dd329e02 100644
> --- a/t/helper/test-run-command.c
> +++ b/t/helper/test-run-command.c
> @@ -52,6 +52,21 @@ static int no_job(struct child_process *cp,
>  	return 0;
>  }
>  
> +static void duplicate_output(struct strbuf *process_out,
> +			struct strbuf *out,
> +			void *pp_cb,
> +			void *pp_task_cb)
> +{
> +	struct string_list list = STRING_LIST_INIT_DUP;
> +
> +	string_list_split(&list, process_out->buf, '\n', -1);
> +	for (size_t i = 0; i < list.nr; i++) {
> +		if (strlen(list.items[i].string) > 0)
> +			fprintf(stderr, "duplicate_output: %s\n", list.items[i].string);
> +	}
> +	string_list_clear(&list, 0);
> +}
> +
>  static int task_finished(int result,
>  			 struct strbuf *err,
>  			 void *pp_cb,
> @@ -439,6 +454,12 @@ int cmd__run_command(int argc, const char **argv)
>  		opts.ungroup = 1;
>  	}
>  
> +	if (!strcmp(argv[1], "--duplicate-output")) {
> +		argv += 1;
> +		argc -= 1;
> +		opts.duplicate_output = duplicate_output;
> +	}

In the tests, can we also write things from the callback functions? Whether we think that callback output should be duplicated or not, we should test what happens to them.