From: Junio C Hamano Date: Tue, 07 Jul 2020 01:50:05 GMT Subject: Re: [PATCH 1/2] Wait for child on signal death for aliases to builtins Message-ID: In-Reply-To: <20200706204106.GA85133@coredump.intra.peff.net> Jeff King writes: > On Sun, Jul 05, 2020 at 12:18:37AM +0200, trygveaa@gmail.com wrote: > >> From: Trygve Aaberge >> >> 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. > 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.