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

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

[Phillip Wood via GitGitGadget]

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.

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?

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1581%2Fphillipwood%2Fsequencer-subprocesses-ignore-sigint-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1581/phillipwood/sequencer-subprocesses-ignore-sigint-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1581

 sequencer.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index a66dcf8ab26..26d70f68454 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1059,6 +1059,7 @@ static int run_git_commit(const char *defmsg,
 			  unsigned int flags)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
+	int res;
 
 	if ((flags & CLEANUP_MSG) && (flags & VERBATIM_MSG))
 		BUG("CLEANUP_MSG and VERBATIM_MSG are mutually exclusive");
@@ -1116,10 +1117,16 @@ static int run_git_commit(const char *defmsg,
 	if (!(flags & EDIT_MSG))
 		strvec_push(&cmd.args, "--allow-empty-message");
 
+	sigchain_push(SIGINT, SIG_IGN);
+	sigchain_push(SIGQUIT, SIG_IGN);
 	if (is_rebase_i(opts) && !(flags & EDIT_MSG))
-		return run_command_silent_on_success(&cmd);
+		res = run_command_silent_on_success(&cmd);
 	else
-		return run_command(&cmd);
+		res = run_command(&cmd);
+	sigchain_pop(SIGINT);
+	sigchain_pop(SIGQUIT);
+
+	return res;
 }
 
 static int rest_is_empty(const struct strbuf *sb, int start)
@@ -3628,10 +3635,14 @@ static int do_exec(struct repository *r, const char *command_line)
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	int dirty, status;
 
+	sigchain_push(SIGINT, SIG_IGN);
+	sigchain_push(SIGQUIT, SIG_IGN);
 	fprintf(stderr, _("Executing: %s\n"), command_line);
 	cmd.use_shell = 1;
 	strvec_push(&cmd.args, command_line);
 	status = run_command(&cmd);
+	sigchain_pop(SIGINT);
+	sigchain_pop(SIGQUIT);
 
 	/* force re-reading of the cache */
 	discard_index(r->index);
@@ -4111,7 +4122,11 @@ static int do_merge(struct repository *r,
 				NULL, 0);
 		rollback_lock_file(&lock);
 
+		sigchain_push(SIGINT, SIG_IGN);
+		sigchain_push(SIGQUIT, SIG_IGN);
 		ret = run_command(&cmd);
+		sigchain_pop(SIGINT);
+		sigchain_pop(SIGQUIT);
 
 		/* force re-reading of the cache */
 		if (!ret) {

base-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3
-- 
gitgitgadget

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

[Johannes Schindelin]

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

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

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

Ciao,
Johannes

>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1581%2Fphillipwood%2Fsequencer-subprocesses-ignore-sigint-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1581/phillipwood/sequencer-subprocesses-ignore-sigint-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1581
>
>  sequencer.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index a66dcf8ab26..26d70f68454 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1059,6 +1059,7 @@ static int run_git_commit(const char *defmsg,
>  			  unsigned int flags)
>  {
>  	struct child_process cmd = CHILD_PROCESS_INIT;
> +	int res;
>
>  	if ((flags & CLEANUP_MSG) && (flags & VERBATIM_MSG))
>  		BUG("CLEANUP_MSG and VERBATIM_MSG are mutually exclusive");
> @@ -1116,10 +1117,16 @@ static int run_git_commit(const char *defmsg,
>  	if (!(flags & EDIT_MSG))
>  		strvec_push(&cmd.args, "--allow-empty-message");
>
> +	sigchain_push(SIGINT, SIG_IGN);
> +	sigchain_push(SIGQUIT, SIG_IGN);
>  	if (is_rebase_i(opts) && !(flags & EDIT_MSG))
> -		return run_command_silent_on_success(&cmd);
> +		res = run_command_silent_on_success(&cmd);
>  	else
> -		return run_command(&cmd);
> +		res = run_command(&cmd);
> +	sigchain_pop(SIGINT);
> +	sigchain_pop(SIGQUIT);
> +
> +	return res;
>  }
>
>  static int rest_is_empty(const struct strbuf *sb, int start)
> @@ -3628,10 +3635,14 @@ static int do_exec(struct repository *r, const char *command_line)
>  	struct child_process cmd = CHILD_PROCESS_INIT;
>  	int dirty, status;
>
> +	sigchain_push(SIGINT, SIG_IGN);
> +	sigchain_push(SIGQUIT, SIG_IGN);
>  	fprintf(stderr, _("Executing: %s\n"), command_line);
>  	cmd.use_shell = 1;
>  	strvec_push(&cmd.args, command_line);
>  	status = run_command(&cmd);
> +	sigchain_pop(SIGINT);
> +	sigchain_pop(SIGQUIT);
>
>  	/* force re-reading of the cache */
>  	discard_index(r->index);
> @@ -4111,7 +4122,11 @@ static int do_merge(struct repository *r,
>  				NULL, 0);
>  		rollback_lock_file(&lock);
>
> +		sigchain_push(SIGINT, SIG_IGN);
> +		sigchain_push(SIGQUIT, SIG_IGN);
>  		ret = run_command(&cmd);
> +		sigchain_pop(SIGINT);
> +		sigchain_pop(SIGQUIT);
>
>  		/* force re-reading of the cache */
>  		if (!ret) {
>
> base-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3
> --
> gitgitgadget
>

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

[Phillip Wood]

Hi Johannes

On 07/09/2023 13:57, Johannes Schindelin wrote:
> 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

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

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

[Johannes Schindelin]

Hi Phillip,

On Fri, 8 Sep 2023, Phillip Wood wrote:

> On 07/09/2023 13:57, Johannes Schindelin wrote:
> >
> > On Thu, 7 Sep 2023, Phillip Wood via GitGitGadget wrote:
> >
> > >      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?

The concept of "sessions" does not really translate well into the Windows
world. Neither does the concept of a "process group".

Ciao,
Johannes

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

[Jeff King]

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).

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!).

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. 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");

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.

-Peff

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

[Phillip Wood]

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

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

[Phillip Wood]

On 08/09/2023 10:59, Phillip Wood wrote:
>> 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.

It is better for handling SIGINT and SIGQUIT when we don't want git to 
be killed but in complicates the handling of SIGTSTP and friends. We'd 
need to pass WUNTRACED to waitpid() and then do 
"raise(WSTOPSIG(wstatus))" to propagate the signal up to the shell. When 
resuming we'd need to call tcsetpgrp() again if git is resumed in the 
foreground before sending SIGCONT to the child.

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

I had a quick google and it looks like cygwin somehow manages to 
implement tcsetpgrp() but the windows terminal does not have any concept 
of a foreground process group

Best Wishes

Phillip

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

[Oswald Buddenhagen]

On Fri, Sep 08, 2023 at 10:59:06AM +0100, Phillip Wood wrote:
>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.
>
that's a quite reasonable assumption.
ignoring gc's exit status is ok-ish, but ignoring its termination signal 
is absolutely not.

>On 07/09/2023 22:06, Jeff King wrote:
>> I think this really comes down to: does the user perceive the child
>> process as the current "main" process running in the foreground?
>> 
that is indeed a key point here.
note that the shell doesn't enable job control in scripts, either.

>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.
>
there is no such thing as waiting for grandchildren. the grandchild is 
reparented to init when the child exits.

there is a situation were one can be deadlocked by a non-exiting 
grandchild: when doing a blocking read of the child's output past its 
exit, when the grandchild has inherited stdout. but that's a 
implementation bug in the parent. and not relevant here.

On Fri, Sep 08, 2023 at 02:11:51PM +0100, Phillip Wood wrote:
>On 08/09/2023 10:59, Phillip Wood wrote:
>>> 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.
>
this would indeed be the right way if we wanted to isolate the children 
more, but ...

>It is better for handling SIGINT and SIGQUIT when we don't want git to 
>be killed but in complicates the handling of SIGTSTP and friends. [...]
>
... this shows that we really don't want that; we don't want to 
replicate interactive shell behavior. that is even before the divergence 
on windows.

so i think your patch is approaching things the right way.
though blocking signals doesn't appear right - to ensure git's own clean 
exit while it has no children, it must catch sigint anyway, and 
temporarily ignoring it around spawning children sounds racy.

regards

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

[Phillip Wood]

On 10/09/2023 11:05, Oswald Buddenhagen wrote:
>> 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.
>>
> there is no such thing as waiting for grandchildren. the grandchild is 
> reparented to init when the child exits.
> 
> there is a situation were one can be deadlocked by a non-exiting 
> grandchild: when doing a blocking read of the child's output past its 
> exit, when the grandchild has inherited stdout. but that's a 
> implementation bug in the parent. and not relevant here.

Yes I got carried away and thought that the shell waited for all the 
processes in the foreground process group, but it can only wait on those 
processes that it created.

> On Fri, Sep 08, 2023 at 02:11:51PM +0100, Phillip Wood wrote:
>> On 08/09/2023 10:59, Phillip Wood wrote:
>>>> 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.
>>
> this would indeed be the right way if we wanted to isolate the children 
> more, but ...
> 
>> It is better for handling SIGINT and SIGQUIT when we don't want git to 
>> be killed but in complicates the handling of SIGTSTP and friends. [...]
>>
> ... this shows that we really don't want that; we don't want to 
> replicate interactive shell behavior. that is even before the divergence 
> on windows.

Yeah, I'm not enthusiastic about emulating the shell's job control in git.

> so i think your patch is approaching things the right way.
> though blocking signals doesn't appear right - to ensure git's own clean 
> exit while it has no children, it must catch sigint anyway, and 
> temporarily ignoring it around spawning children sounds racy.

There is an inevitable race between wait() returning and calling 
signal() to restore the handlers for SIGINT and SIGQUIT, it is such a 
small window I'm not sure it is a problem in practice. There is also a 
race when creating the child but if we block signals before calling 
fork, then ignore SIGINT and SIGQUIT in the parent before unblocking 
them we're OK because the child will be killed as soon as it unblocks 
signal by any signal received while the signals were blocks and we'll 
detect that in the parent and exit. Currently editor.c just ignores the 
signals after fork has returned in the parent which means it is 
theoretically  possible to kill git with SIGINT while the child is running.

Best Wishes

Phillip

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

[Phillip Wood]

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. In practice if the child is catching 
SIGINT or SIGQUIT we cannot rely on it re-raising the signal so that 
wont work.

Best Wishes

Phillip

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

[Oswald Buddenhagen]

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:

my point is that you shouldn't be doing that in the first place.
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.

a few notes on that:
- install without SA_RESTART, so syscalls can actually return with EINTR
   and give us the opportunity to check the flag.
- an alternative to setting flags is setjmp()/longjmp(), but you really
   don't want to go there.
- install with SA_RESETHAND, so the second ctrl-c will kill git
   regardless, providing an escape hatch.

>In practice if the child is catching SIGINT or SIGQUIT we cannot rely 
>on it re-raising the signal so that wont work.
>
yes, but that's a minor issue, i think.
by far most hooks and other things that might be invoked within 
sequencer context don't mess with signals in the first place.
the things that do should be presumed to do the right thing, which means 
at least a non-zero exit status in case of a premature termination, 
which will yield pretty much the same effect on our side anyway.
so the only actually problematic situation would be us completely 
ignoring the exit code (like the git-gc call, but that's clearly a bug 
in git, and we control both sides, so it's easily fixable).

regards

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

[Phillip Wood]

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.

> my point is that you shouldn't be doing that in the first place.
> 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. The patch here makes it safe for 
the user to interrupt a subprocess started by the sequencer. 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. That is not the 
case at the moment and I'm concerned making that promise could turn into 
a maintenance burden in the future.

Best Wishes

Phillip

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

[Oswald Buddenhagen]

On Wed, Sep 13, 2023 at 04:33:06PM +0100, Phillip Wood wrote:
>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".

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

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

[Jeff King]

On Fri, Sep 08, 2023 at 10:59:06AM +0100, Phillip Wood wrote:

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

One thing I am not clear on is the convention on who does the process
group and controlling terminal setup. Should Git do it to say "I am
handing off control of the terminal to the editor that I am spawning"?
Or should the editor say "I am an editor which has a user interface that
takes over the terminal; I will control it while I am running".

The latter makes much more sense to me, as Git cannot know how the
editor plans to behave. But as I understand it, this kind of job control
stuff is implemented by the calling shell, which does the tcsetpgrp()
call.

So I dunno. It sounds to me like the "right" thing here is making Git
more shell-like in handing control to a program (like the editor) that
we expect to be in the foreground of the terminal. As opposed to the
"ignore SIGINT temporarily" thing which feels more like band-aid. But
I'm wary of getting into a rabbit hole of portability headaches and
weird corners of Unix terminal-handling conventions.

-Peff

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

[Phillip Wood]

On 14/09/2023 10:56, Jeff King wrote:
> On Fri, Sep 08, 2023 at 10:59:06AM +0100, Phillip Wood wrote:
> 
>> 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.
> 
> One thing I am not clear on is the convention on who does the process
> group and controlling terminal setup. Should Git do it to say "I am
> handing off control of the terminal to the editor that I am spawning"?
> Or should the editor say "I am an editor which has a user interface that
> takes over the terminal; I will control it while I am running".

As I understand it the editor has a controlling terminal (assuming there 
is a controlling terminal associated with the editor's session id), not 
the other way round. If the editor is launched in the background then it 
will receive SIGTTIN when it tries to read from it's controlling 
terminal which stops the process unless the process installs a signal 
handler.

> The latter makes much more sense to me, as Git cannot know how the
> editor plans to behave. But as I understand it, this kind of job control
> stuff is implemented by the calling shell, which does the tcsetpgrp()
> call.

Yes, my understanding is that the shell puts all the processes in a 
pipeline in the same process group and calls tcsetpgrp() if it wants 
that job to be run in the foreground.

> So I dunno. It sounds to me like the "right" thing here is making Git
> more shell-like in handing control to a program (like the editor) that
> we expect to be in the foreground of the terminal. As opposed to the
> "ignore SIGINT temporarily" thing which feels more like band-aid. But
> I'm wary of getting into a rabbit hole of portability headaches and
> weird corners of Unix terminal-handling conventions.

I'm wary of that too, it has the potential to end up adding a lot of 
fiddly code checking if git is in the foreground or background and 
propagating SIGTSTP and friends up to the shell. In any case we'd need 
the "ignore SIGINT" thing for windows anyway. Ignoring SIGINT and 
SIGQUIT in the parent is what system(3) does. As long as git exits 
promptly when the interrupted child is killed I think that is 
reasonable. Would you be happier if we re-raised the signal once we have 
cleaned up any state that needs to be written before exiting?

Best Wishes

Phillip

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

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

The above made me wonder if we can guarantee that the intention of
the end-user is always to kill only the external program and not
"git".  But with or without this change, "git" will stop making
progress after such a signal (in other words, it is not like killing
"exec sleep 20" will make "git rebase -i -x 'sleep 20'" to move to
the next step without complaining), so "ignore signals" is not all
that harmful as the phrasing makes it sound like.  With the patch,
we just handle signals that will kill the external programs, and
their consequences, a bit more gracefully.

But that makes me wonder what happens if the external program has
good reasons to ignore the signal (that is, the program does not die
when signaled, without misbehaving).  If "git" dies in such a case,
would it help the overall end-user experience, or would it even
hurt?  If the latter, then "git" that ignores these interactive
interrupts would be improvement in both cases (i.e. external
programs that dies with signals, and those that ignores them).  If
the former, however, "git" that ignores the signals would be a
regression.

Other than that, the change is well reasoned, I would think.

Thanks.