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

Hi Peff

Thanks for your thoughts, I hoped I get a interesting response if I cc'd 
you and I wasn't disappointed!

On 07/09/2023 22:06, Jeff King wrote:

On Thu, Sep 07, 2023 at 10:03:02AM +0000, Phillip Wood via GitGitGadget wrote:

quoted

     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 think that would be quite surprising in most cases, as the user may
not be aware when or if a sub-program is in use.

Imagine that you're running a script which runs git-commit in a loop,
which in turn runs "gc --auto", which in turn runs a pre-auto-gc hook.
You want to stop it, so you hit ^C, which sends SIGINT to all of the
processes.

I think most people would expect the whole process chain to stop
immediately. But in your proposal, we'd kill the hook only. That kind-of
propagates up to "gc --auto" (which says "OK, the hook says don't run").
And then that doesn't really propagate to git-commit, which ignores the
exit code of gc and continues on, running the post-commit hook and so
on. And then outer loop of the script continues, invoking the next
commit, and so on. To actually quit you have to hit ^C several times
with the right timing (the exact number of which is unknown to you, as
it depends on the depth of the process chain).

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.

I think this really comes down to: does the user perceive the child
process as the current "main" process running in the foreground? For
most run-command invocations, I would say no. For something like running
an editor, the answer is more clearly yes.

For something like sequencer "exec" commands, I think it really depends
on what the command is doing. If it is "git commit --amend", then that
is going to open an editor and take over the terminal. If it is "make",
then probably not. But it may be OK to do here, just because we know
that a signal exit from the child will be immediately read and
propagated by the sequencer machinery (assuming the child dies; if it
blocks SIGINT, too, then now you cannot interrupt it at all!).

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. Part of the 
motivation for this patch is that I'd like to move the sequencer to a 
model where it only saves its state to disk when it is stopping for the 
user to fix conflicts. To do that safely it cannot die if the user 
interprets a child with SIGINT.

In the classic Unix world, I think the solution here is setsid(),
process groups, and controlling terminals. One example in your commit
message is the sequencer kicking off git-commit, which kicks off an
editor. The user hits ^C then, and the sequencer is killed. But I think
your patch is papering over the deeper bug. In 913ef36093
(launch_editor: ignore terminal signals while editor has control,
2012-11-30), we did this same "ignore INT" trick.

Yes, that was the inspiration for this patch

But I think the right
solution is actually to start the editor in its own process group, and
let it be the foreground of the terminal. And then a ^C while the editor
is running would not only not hit git-commit, but it would not hit any
sequencer or other intermediate processes above it.

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. I agree that would be a 
better way of doing things on unix.

But of course none of that probably has any meaning on Windows. I'm not
sure if there are analogous concepts there we could access with
alternate code, or if it would need to be a totally different strategy.

Lets see if Johannes has any comments about that.

Best Wishes

Phillip