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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sep 8, 2023, 10:02 UTC
Message-ID: <4fecbf96-aa04-41bd-a589-bf7c368d586c@gmail.com>
In-Reply-To: <912205bc-37c5-edbf-2f85-f26991ad65eb@gmx.de>

Hi Johannes

On 07/09/2023 13:57, Johannes Schindelin wrote:

Show 23 quoted lines

> Hi Phillip,
> 
> On Thu, 7 Sep 2023, Phillip Wood via GitGitGadget wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> If the user presses Ctrl-C to interrupt a program run by a rebase "exec"
>> command then SIGINT will also be sent to the git process running the
>> rebase resulting in it being killed. Fortunately the consequences of
>> this are not severe as all the state necessary to continue the rebase is
>> saved to disc but it would be better to avoid killing git and instead
>> report that the command failed. A similar situation occurs when the
>> sequencer runs "git commit" or "git merge". If the user generates SIGINT
>> while editing the commit message then the git processes creating the
>> commit will ignore it but the git process running the rebase will be
>> killed.
>>
>> Fix this by ignoring SIGINT and SIGQUIT when forking "exec" commands,
>> "git commit" and "git merge". This matches what git already does when
>> running the user's editor and matches the behavior of the standard
>> library's system() function.
> 
> ACK

Thanks

Show 13 quoted lines

>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>      rebase -i: ignore signals when forking subprocesses
>>
>>      Having written this I started thinking about what happens when we fork
>>      hooks, merge strategies and merge drivers. I now wonder if it would be
>>      better to change run_command() instead - are there any cases where we
>>      actually want git to be killed when the user interrupts a child process?
> 
> I am not sure that we can rely on arbitrary hooks to do the right thing
> upon Ctrl+C, which is to wrap up and leave. So I _guess_ that we will have
> to leave it an opt-in.

Peff pointed out it doesn't play well with "gc --auto" either. Do you have any thoughts (particularly about the implications for Windows) on his suggestion to put the child in it's own session, or putting the child in its own process group and making that the foreground process group of the controlling terminal?

> However, we could easily make it an option that `run_command()` handles,
> much like `no_stdin`.

That's an interesting idea.

Best Wishes

Phillip