Re: [PATCH v6 1/2] run-command: add an "ungroup" option to run_process_parallel()

[Emily Shaffer <emilyshaffer@google.com>]

On Tue, Jun 07, 2022 at 10:48:19AM +0200, Ævar Arnfjörð Bjarmason wrote:
> @@ -1766,8 +1791,15 @@ int run_processes_parallel(int n,
>  		}
>  		if (!pp.nr_processes)
>  			break;
> -		pp_buffer_stderr(&pp, output_timeout);
> -		pp_output(&pp);
> +		if (ungroup) {
> +			int i;
> +
> +			for (i = 0; i < pp.max_processes; i++)
> +				pp.children[i].state = GIT_CP_WAIT_CLEANUP;

FYI, this broke for us downstream where we are carrying patches adding a
'pp_buffer_stdin()' and friends to enable stdin buffering to parallel
processes. It also appears to break when pp.max_processes exceeds the
number of actual tasks provided. I needed to add something like this

+    if (pp.children[i].state == GIT_CP_WORKING &&
+        !pp.children[i].process.in)
+            pp.children[i].state = GIT_CP_WAIT_CLEANUP;

That is, only set the WAIT_CLEANUP state if the task wasn't waiting to
be given work (GIT_CP_FREE -> GIT_CP_WAIT_CLEANUP leads to some "error:
waitpid is confused" errors) and if stdin is not currently being
buffered. In the case where .process.in is > 0, GIT_CP_WAIT_CLEANUP
causes pp_collect_finished() to stop spinning in this IO buffer loop,
but there is still stdin to pass along.

Anyway, I think the first part (GIT_CP_FREE -> GIT_CP_WAIT_CLEANUP) is a
bug that matters for the patch as it is now; the stdin bit will not
matter until the later config-based-hooks patches which introduce stdin
buffering anyway.

Thanks very much for this fix otherwise.

 - Emily

> +		} else {
> +			pp_buffer_stderr(&pp, output_timeout);
> +			pp_output(&pp);
> +		}
>  		code = pp_collect_finished(&pp);
>  		if (code) {
>  			pp.shutdown = 1;