Re: [PATCH 1/4] run-command: add pipe_output to run_processes_parallel

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

On Thu, Sep 22 2022, Calvin Wan wrote:

> diff --git a/run-command.c b/run-command.c
> index 14f17830f5..893bc1d294 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1474,6 +1474,7 @@ enum child_state {
>  };
>  
>  int run_processes_parallel_ungroup;
> +int run_processes_parallel_pipe_output;
>  struct parallel_processes {
>  	void *data;
>  
> @@ -1770,10 +1771,12 @@ int run_processes_parallel(int n,
>  	int output_timeout = 100;
>  	int spawn_cap = 4;
>  	int ungroup = run_processes_parallel_ungroup;
> +	int pipe_output = run_processes_parallel_pipe_output;
>  	struct parallel_processes pp;
>  
>  	/* unset for the next API user */
>  	run_processes_parallel_ungroup = 0;
> +	run_processes_parallel_pipe_output = 0;
>  
>  	pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb,
>  		ungroup);

I guess we could live with this, but this passing a function argument as
a global variable interface is something that came out of a topic to fix
a release regression:
https://lore.kernel.org/git/cover-0.6-00000000000-20220421T122108Z-avarab@gmail.com/

An earlier version of that series simply changed the API to pass an
"opts" struct instead:
https://lore.kernel.org/git/patch-v2-2.8-5f0a6e9925f-20220518T195858Z-avarab@gmail.com/

I really should have submitted those post-release cleanup patches
already, and I'm not sure whether the right thing at this point is to
take this & do the cleanup for "ungroup" *and* this new argument later.

But maybe you're interested in cherry-picking & adjusting the relevant
part of that series for this one? I.e. we're not in some post-release
regression hurry, so rather than extending the use of this odd interface
we could (and maybe should) just fix how we're doing it first.

On the implementation:

> + * If the "pipe_output" option is specified, the output will be piped
> + * to task_finished_fn in the "struct strbuf *out" variable. The output
> + * will still be printed unless the callback resets the strbuf. The
> + * "pipe_output" option can be enabled by setting the global
> + * "run_processes_parallel_pipe_output" to "1" before invoking
> + * run_processes_parallel(), it will be set back to "0" as soon as the
> + * API reads that setting.

...okey, but...

> +static int task_finished_pipe_output(int result,
> +			 struct strbuf *err,
> +			 void *pp_cb,
> +			 void *pp_task_cb)
> +{
> +	if (err && pipe_output) {
> +		fprintf(stderr, "%s", err->buf);
> +		strbuf_reset(err);

...my memory's hazy, and I haven't re-logged in any detail, but is it
really the API interface here that the "output" callback function is
responsible for resetting the strbuf that the API gives to it?

That seems backwards to me, and e.g. a look at "start_failure" shows
that we strbuf_reset() the "err".

What's the point of doing it in the API consumer? If it doesn't do it
we'll presumably keep accumulating output. Is there a use-case for that?

Or perhaps it's not needed & this is really just misleading boilerplate?

> @@ -140,6 +140,11 @@ test_expect_success 'run_command runs ungrouped in parallel with more jobs avail
>  	test_line_count = 4 err
>  '
>  
> +test_expect_success 'run_command runs pipe_output in parallel with more jobs available than tasks' '
> +	test-tool run-command --pipe-output run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
> +	test_cmp expect actual
> +'
> +

Like the global argument, the copy/pasting for "ungroup" was mostly a
matter of expediency.

But at least in that case we have a different assertion (test_cmp
v.s. test_line_count).

But here this test case seems to be exactly the same as for the
"vanilla" version.

So can't we make this some:

	for opt in '' '--pipe-output'
	do
		test_expect_success ...
	done

?