From: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Date: Sun, 10 Sep 2023 10:05:37 GMT
Subject: Re: [PATCH] rebase -i: ignore signals when forking subprocesses
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.

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