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

[Phillip Wood <phillip.wood123@gmail.com>]

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