Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > This series fixes a v2.36.0 regression[1]. See [2] for the v4. The > reasons for why a regression needs this relatively large change to > move forward is discussed in past rounds, e.g. around [3]. CI at > https://github.com/avar/git/actions/runs/2428475773 > > Changes since v4, mainly to address comments by Johannes (thanks for > the review!): This version looks good to me. > @@ run-command.c: static void pp_init(struct parallel_processes *pp, > for (i = 0; i < n; i++) { > strbuf_init(&pp->children[i].err, 0); > child_process_init(&pp->children[i].process); > -+ if (!pp->pfd) > -+ continue; > - pp->pfd[i].events = POLLIN | POLLHUP; > - pp->pfd[i].fd = -1; > +- pp->pfd[i].events = POLLIN | POLLHUP; > +- pp->pfd[i].fd = -1; > ++ if (pp->pfd) { > ++ pp->pfd[i].events = POLLIN | POLLHUP; > ++ pp->pfd[i].fd = -1; > ++ } > } This change is merely a personal taste---it does not match mine but that is Meh ;-) > -@@ run-command.c: static void pp_cleanup(struct parallel_processes *pp) > - */ > - static int pp_start_one(struct parallel_processes *pp) > - { > -+ const int ungroup = pp->ungroup; It may have made the resulting code easier to read if the local variable was kept as a synonym as "pp->" is short enough but is repeated often, but what is written is good enough and I do not see a need to flip-flop. > -+static void pp_mark_ungrouped_for_cleanup(struct parallel_processes *pp) > -+{ > -+ int i; > -+ > -+ if (!pp->ungroup) > -+ BUG("only reachable if 'ungrouped'"); > -+ > -+ for (i = 0; i < pp->max_processes; i++) > -+ pp->children[i].state = GIT_CP_WAIT_CLEANUP; > -+} Good to see this inlined. I find the caller easier to follow without it. Thanks for a quick succession of rerolling. Will queue.
