Re: [PATCH v5 0/2] hook API: connect hooks to the TTY again, fixes a v2.36.0 regression

Ævar Arnfjörð Bjarmason  [off-list ref] 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.