Re: [PATCH v6 1/2] run-command: add an "ungroup" option to run_process_parallel()
From: Emily Shaffer <hidden>
Date: 2022-06-17 00:07:11
On Tue, Jun 07, 2022 at 10:48:19AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted hunk ↗ jump to hunk
@@ -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;