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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 2, 2022, 20:05 UTC
Message-ID: <xmqq7d5yn85f.fsf@gitster.g>
In-Reply-To: <cover-v5-0.2-00000000000-20220602T131858Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 7 quoted lines

> 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.

Show 15 quoted lines

>     @@ 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 ;-)

Show 5 quoted lines

>     -@@ 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.

Show 10 quoted lines

>     -+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.