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

From: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Date: Sep 13, 2023, 16:40 UTC
Message-ID: <ZQHmEKQ1q1lciUqE@ugly>
In-Reply-To: <51ad269b-d8df-4424-8127-293b2971d65d@gmail.com>

On Wed, Sep 13, 2023 at 04:33:06PM +0100, Phillip Wood wrote:

Show 15 quoted lines

>On 11/09/2023 11:32, Oswald Buddenhagen wrote:
>> On Mon, Sep 11, 2023 at 11:14:31AM +0100, Phillip Wood wrote:
>>> On 11/09/2023 11:00, Phillip Wood wrote:
>>>> There is an inevitable race between wait() returning and calling 
>>>> signal() to restore the handlers for SIGINT and SIGQUIT,
>>>
>>> In principle if we installed a signal handler to set a flag if a 
>>> signal is received while calling wait() and then once wait() returns 
>>> successfully see if the child was killed we can tell if the signal was 
>>> received while the child was alive.
>>>
>> yes, this is what i was already writing:
>
>I'm afraid that was not clear to me from your message.
>

i meant, this is what i already wrote before i read your reply-to-self. i just pasted it into the new reply i sent instead without adjusting for the new context. the sentence was meant to explain the slight "impedance mismatch".

Show 5 quoted lines

>> install the handlers when the sequencer is entered and leave them 
>> there. the handlers need to set (volatile) flag variables, which are 
>> checked by the sequencer on a regular basis.
>
>I did consider doing that before I submitted this patch

>but it is a much more invasive and substantial change.
>

yes

>The patch here makes it safe for the user to interrupt a subprocess 
>started by the sequencer.
>

for the exec case, i don't see how this actually improves anything. whether git gets killed along with the child, or catches the child's abnormal exit and immediately exits, makes no difference. arguably, it's even counter-productive, because from the outside it's random whether git will just exit on sigint or report that its child exited on sigint and exit with some other status.

actual value would come from doing something before exiting, but the commit message is pretty much saying that this is not the case.

the commit/edit case is more complicated, but arguably the problem is the (hypothetical?) editor that just ignores sigint rather than reprogramming the terminal appropriately for full-screen use. git-commit ignoring sigint seems like a somewhat misguided workaround, and piling on top of that won't really improve things.

>If I understand correctly your suggestion implies that the user could 
>interrupt the sequencer at any point and we'd need to exit and ensure 
>that they could safely continue the rebase afterwards.
>

yes

>That is not the case at the moment

>and I'm concerned making that promise could turn into a maintenance 
>burden in the future.
>

of course it would. the question is whether it would be worth it. with delayed state commits, some extra trasactionality might well be required.

regards