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

Hi Johannes

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

Hi Phillip,

On Thu, 7 Sep 2023, Phillip Wood via GitGitGadget wrote:

quoted

From: Phillip Wood <redacted>

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

quoted

Signed-off-by: Phillip Wood <redacted>
---
     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