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;
