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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Calvin

On 07/02/2023 18:17, Calvin Wan wrote:
> 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.
> 
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> ---
>   run-command.c               | 16 ++++++++++++---
>   run-command.h               | 27 +++++++++++++++++++++++++
>   t/helper/test-run-command.c | 21 ++++++++++++++++++++
>   t/t0061-run-command.sh      | 39 +++++++++++++++++++++++++++++++++++++
>   4 files changed, 100 insertions(+), 3 deletions(-)
> 
> 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);
> @@ -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);
>   			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) {
>   				if (errno != EAGAIN)
>   					die_errno("read");
> +			} else {
> +				if (opts->duplicate_output)
> +					opts->duplicate_output(&pp->children[i].err,
> +					       strlen(pp->children[i].err.buf) - n,

Looking at how this is used in patch 7 I think it would be better to 
pass a const char*, length pair rather than a struct strbuf*, offset pair.
i.e.
	opts->duplicate_output(pp->children[i].err.buf + 
pp->children[i].err.len - n, n, ...)

That would make it clear that we do not expect duplicate_output() to 
alter the buffer and would avoid the duplicate_output() having to add 
the offset to the start of the buffer to find the new data.

Best Wishes

Phillip


> +					       opts->data,
> +					       pp->children[i].data);
> +			}
>   		}
>   	}
>   }
> 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);
> +
>   /**
>    * 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
> +	 */
> +	duplicate_output_fn duplicate_output;
> +
>   	/**
>   	 * task_finished: See task_finished_fn() above. This can be
>   	 * NULL to omit any special handling.
> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
> index 3ecb830f4a..ffd3cd0045 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 *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)
> +			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;
> +	}
> +
>   	jobs = atoi(argv[2]);
>   	strvec_clear(&proc.args);
>   	strvec_pushv(&proc.args, (const char **)argv + 3);
> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index e2411f6a9b..879e536638 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -135,6 +135,15 @@ test_expect_success 'run_command runs in parallel with more jobs available than
>   	test_cmp expect actual
>   '
>   
> +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 &&
> +	test_cmp expect err1
> +'
> +
>   test_expect_success 'run_command runs ungrouped in parallel with more jobs available than tasks' '
>   	test-tool run-command --ungroup run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
>   	test_line_count = 8 out &&
> @@ -147,6 +156,15 @@ test_expect_success 'run_command runs in parallel with as many jobs as tasks' '
>   	test_cmp expect actual
>   '
>   
> +test_expect_success 'run_command runs in parallel with as many jobs as tasks --duplicate-output' '
> +	test-tool run-command --duplicate-output run-command-parallel 4 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 &&
> +	test_cmp expect err1
> +'
> +
>   test_expect_success 'run_command runs ungrouped in parallel with as many jobs as tasks' '
>   	test-tool run-command --ungroup run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
>   	test_line_count = 8 out &&
> @@ -159,6 +177,15 @@ test_expect_success 'run_command runs in parallel with more tasks than jobs avai
>   	test_cmp expect actual
>   '
>   
> +test_expect_success 'run_command runs in parallel with more tasks than jobs available --duplicate-output' '
> +	test-tool run-command --duplicate-output run-command-parallel 3 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 &&
> +	test_cmp expect err1
> +'
> +
>   test_expect_success 'run_command runs ungrouped in parallel with more tasks than jobs available' '
>   	test-tool run-command --ungroup run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
>   	test_line_count = 8 out &&
> @@ -180,6 +207,12 @@ test_expect_success 'run_command is asked to abort gracefully' '
>   	test_cmp expect actual
>   '
>   
> +test_expect_success 'run_command is asked to abort gracefully --duplicate-output' '
> +	test-tool run-command --duplicate-output run-command-abort 3 false >out 2>err &&
> +	test_must_be_empty out &&
> +	test_cmp expect err
> +'
> +
>   test_expect_success 'run_command is asked to abort gracefully (ungroup)' '
>   	test-tool run-command --ungroup run-command-abort 3 false >out 2>err &&
>   	test_must_be_empty out &&
> @@ -196,6 +229,12 @@ test_expect_success 'run_command outputs ' '
>   	test_cmp expect actual
>   '
>   
> +test_expect_success 'run_command outputs --duplicate-output' '
> +	test-tool run-command --duplicate-output run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
> +	test_must_be_empty out &&
> +	test_cmp expect err
> +'
> +
>   test_expect_success 'run_command outputs (ungroup) ' '
>   	test-tool run-command --ungroup run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
>   	test_must_be_empty out &&