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

[Jonathan Tan <jonathantanmy@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.

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

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