Re: [PATCH] rebase -i: ignore signals when forking subprocesses

From: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Date: Sep 10, 2023, 10:05 UTC
Message-ID: <ZP2U8TBNjKs5ebky@ugly>
In-Reply-To: <159c16ae-4dbf-4669-bd9d-2f7c52107a68@gmail.com>

On Fri, Sep 08, 2023 at 10:59:06AM +0100, Phillip Wood wrote:

>Ah, I hadn't thought about "gc --auto". I was assuming that the calling 
>code would see the child had been killed and exit but that's not always 
>the case.
>

that's a quite reasonable assumption. ignoring gc's exit status is ok-ish, but ignoring its termination signal is absolutely not.

>On 07/09/2023 22:06, Jeff King wrote:
>> I think this really comes down to: does the user perceive the child
>> process as the current "main" process running in the foreground?
>>

that is indeed a key point here. note that the shell doesn't enable job control in scripts, either.

Show 5 quoted lines

>The child not dying is tricky, if it is in the same process group as 
>git then even if git dies the I think the shell will wait for the child 
>to exit before showing the prompt again so it is not clear to me that 
>the user is disadvantaged by git ignoring SIGINT in that case.
>

there is no such thing as waiting for grandchildren. the grandchild is reparented to init when the child exits.

there is a situation were one can be deadlocked by a non-exiting grandchild: when doing a blocking read of the child's output past its exit, when the grandchild has inherited stdout. but that's a implementation bug in the parent. and not relevant here.

On Fri, Sep 08, 2023 at 02:11:51PM +0100, Phillip Wood wrote:

Show 13 quoted lines

>On 08/09/2023 10:59, Phillip Wood wrote:
>>> I've never done it before, but from my reading we basically want to 
>>> do
>>> (in the forked process before we exec):
>>>
>>>    setsid();
>>>    open("/dev/tty");
>> 
>> Do we want a whole new session? As I understand it to launch a 
>> foreground job shells put the child in its own process group and then 
>> call tcsetpgrp() to change the foreground process group of the 
>> controlling terminal to that of the child.
>

this would indeed be the right way if we wanted to isolate the children more, but ...

>It is better for handling SIGINT and SIGQUIT when we don't want git to 
>be killed but in complicates the handling of SIGTSTP and friends. [...]
>

... this shows that we really don't want that; we don't want to replicate interactive shell behavior. that is even before the divergence on windows.

so i think your patch is approaching things the right way. though blocking signals doesn't appear right - to ensure git's own clean exit while it has no children, it must catch sigint anyway, and temporarily ignoring it around spawning children sounds racy.

regards