patch, 5 partsignore SIG{INT,QUIT} when launching editor

This is a re-roll of the pf/editor-ignore-sigint series.

There are two changes from the original:

  1. We ignore both SIGINT and SIGQUIT for "least surprise" compared to
     system(3).

  2. We now use "code + 128" to look for signal death (instead of
     WTERMSIG), as per run-command's documentation on how it munges the
     code.

People mentioned some buggy editors which go into an infinite EIO loop when their parent dies due to SIGQUIT. That should be a non-issue now, as we will be ignoring SIGQUIT. And even if you could replicate it (e.g., with another signal) those programs should be (and reportedly have been) fixed. It is not git's job to babysit its child processes.

The patches are:

  [1/5]: run-command: drop silent_exec_failure arg from wait_or_whine
  [2/5]: launch_editor: refactor to use start/finish_command
  [3/5]: launch_editor: ignore terminal signals while editor has control
  [4/5]: run-command: do not warn about child death from terminal
  [5/5]: launch_editor: propagate signals from editor to git

Since this can be thought of as "act more like system(3)", I wondered whether the signal-ignore logic should be moved into run-command, or even used by default for blocking calls to run_command (which are basically our version of system(3)). But it is detrimental in the common case that the child is not taking control of the terminal, and is just an implementation detail (e.g., we call "git update-ref" behind the scenes, but the user does not know or care). If they hit ^C during such a run and we are ignoring SIGINT, then either:

  1. we will notice the child died by signal and report an
     error in the subprocess rather than just dying; the end result is
     similar, but the error is unnecessarily confusing

  2. we do not bother to check the child's return code (because we do
     not care whether the child succeeded or not, like a "gc --auto");
     we end up totally ignoring the user's request to abort the
     operation

So I do not think we care about this behavior except for launching the editor. And the signal-propagation behavior of 5/5 is really so weirdly editor-specific (because it is about behaving well whether the child blocks signals or not).

-Peff

We do not actually use this parameter; instead we complain from the child itself (for fork/exec) or from start_command (if we are using spawn on Windows).

Signed-off-by: Jeff King <peff@peff.net>
---
 run-command.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

The launch_editor function uses the convenient run_command_* interface. Let's use the more flexible start_command and finish_command functions, which will let us manipulate the parent state while we're waiting for the child to finish.

Signed-off-by: Jeff King <peff@peff.net>
---
 editor.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

From: Paul Fox <pgf@foxharp.boston.ma.us>

The user's editor likely catches SIGINT (ctrl-C). but if the user spawns a command from the editor and uses ctrl-C to kill that command, the SIGINT will likely also kill git itself (depending on the editor, this can leave the terminal in an unusable state).

Let's ignore it while the editor is running, and do the same for SIGQUIT, which many editors also ignore. This matches the behavior if we were to use system(3) instead of run-command.

Signed-off-by: Paul Fox <pgf@foxharp.boston.ma.us>
Signed-off-by: Jeff King <peff@peff.net>
---
 editor.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

SIGINT and SIGQUIT are not generally interesting signals to the user, since they are typically caused by them hitting "^C" or otherwise telling their terminal to send the signal.

Signed-off-by: Jeff King <peff@peff.net>
---
 run-command.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

We block SIGINT and SIGQUIT while the editor runs so that git is not killed accidentally by a stray "^C" meant for the editor or its subprocesses. This works because most editors ignore SIGINT.

However, some editor wrappers, like emacsclient, expect to die due to ^C. We detect the signal death in the editor and properly exit, but not before writing a useless error message to stderr. Instead, let's notice when the editor was killed by a terminal signal and just raise the signal on ourselves. This skips the message and looks to our parent like we received SIGINT ourselves.

The end effect is that if the user's editor ignores SIGINT, we will, too. And if it does not, then we will behave as if we did not ignore it. That should make all users happy.

Note that in the off chance that another part of git has ignored SIGINT while calling launch_editor, we will still properly detect and propagate the failed return code from the editor (i.e., the worst case is that we generate the useless error, not fail to notice the editor's death).

Signed-off-by: Jeff King <peff@peff.net>
---
 editor.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

On Fri, Nov 30, 2012 at 05:39:43PM -0500, Jeff King wrote:

Also some good editors printed error message after they got EIO, confusing the user.

Looks good to me. I've tested this with ed (always ignores SIGINT and SIGQUIT), vim (always ignores SIGINT, but dies after three SIGQUIT) and "sleep" (dies after SIGINT and SIGQUIT) and git works now as expected. Doing what editor does is probably the best thing to do.

Tested-by: Krzysztof Mazur <krzysiek@podlesie.net>

Thanks,

Krzysiek

jeff wrote:
 > This is a re-roll of the pf/editor-ignore-sigint series.
 > 
 > There are two changes from the original:
 > 
 >   1. We ignore both SIGINT and SIGQUIT for "least surprise" compared to
 >      system(3).
 > 
 >   2. We now use "code + 128" to look for signal death (instead of
 >      WTERMSIG), as per run-command's documentation on how it munges the
 >      code.

this series all looks good to me.  thanks for re- and re-re-rolling.

paul

 > 
 > People mentioned some buggy editors which go into an infinite EIO loop
 > when their parent dies due to SIGQUIT. That should be a non-issue now,
 > as we will be ignoring SIGQUIT. And even if you could replicate it
 > (e.g., with another signal) those programs should be (and reportedly
 > have been) fixed. It is not git's job to babysit its child processes.
 > 
 > The patches are:
 > 
 >   [1/5]: run-command: drop silent_exec_failure arg from wait_or_whine
 >   [2/5]: launch_editor: refactor to use start/finish_command
 >   [3/5]: launch_editor: ignore terminal signals while editor has control
 >   [4/5]: run-command: do not warn about child death from terminal
 >   [5/5]: launch_editor: propagate signals from editor to git
 > 
 > Since this can be thought of as "act more like system(3)", I wondered
 > whether the signal-ignore logic should be moved into run-command, or
 > even used by default for blocking calls to run_command (which are
 > basically our version of system(3)). But it is detrimental in the common
 > case that the child is not taking control of the terminal, and is just
 > an implementation detail (e.g., we call "git update-ref" behind the
 > scenes, but the user does not know or care). If they hit ^C during such
 > a run and we are ignoring SIGINT, then either:
 > 
 >   1. we will notice the child died by signal and report an
 >      error in the subprocess rather than just dying; the end result is
 >      similar, but the error is unnecessarily confusing
 > 
 >   2. we do not bother to check the child's return code (because we do
 >      not care whether the child succeeded or not, like a "gc --auto");
 >      we end up totally ignoring the user's request to abort the
 >      operation
 > 
 > So I do not think we care about this behavior except for launching the
 > editor. And the signal-propagation behavior of 5/5 is really so weirdly
 > editor-specific (because it is about behaving well whether the child
 > blocks signals or not).
 > 
 > -Peff

=---------------------
 paul fox, pgf@foxharp.boston.ma.us (arlington, ma, where it's 24.8 degrees)

Jeff King <peff@peff.net> writes:

Nicely explained.  Very much appreciated.