Re: [PATCH 1/2] Wait for child on signal death for aliases to builtins

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 7, 2020, 01:50 UTC
Message-ID: <xmqqwo3g6qlu.fsf@gitster.c.googlers.com>
In-Reply-To: <20200706204106.GA85133@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 16 quoted lines

> On Sun, Jul 05, 2020 at 12:18:37AM +0200, trygveaa@gmail.com wrote:
>
>> From: Trygve Aaberge <trygveaa@gmail.com>
>> 
>> When you hit ^C all the processes in the tree receives it. When a git
>> command uses a pager, git ignores this and waits until the pager quits.
>> However, when using an alias there is an additional process in the tree
>> which didn't ignore the signal. That caused it to exit which in turn
>> caused the pager to exit. This fixes that for aliases to builtins.
>> 
>> This was originally fixed in 46df6906f3 (see that for a more in
>> explanation), but broke by a regression in b914084007.
>
> Good catch. The regression is technically in b914084007, but the real
> culprit is the extra (commented out) code path added in ee4512ed48
> (trace2: create new combined trace facility, 2019-02-22).

True, as Dscho also mentioned. I'll just do "b914084007" => "ee4512ed48 and b914084007" while queueing.

Show 5 quoted lines

> Your fix here looks good, but it does make me wonder if we could avoid
> or shrink this duplicate code path. I.e., could it just do the logging
> necessary but still leave the actual process spawn to the
> execv_dashed_external() below. It may be hard to untangle, though, so
> certainly this makes sense in the meantime.

It probably is a bit more involved than a typical low hanging fruit. I am OK to leave this for later clean-up.

> A test would be nice, but I don't think it's very feasible for the same
> reason mentioned in 46df6906f3.

True.  Thanks.