[PATCH 0/5] ignore SIG{INT,QUIT} when launching editor

[PATCH 0/5] ignore SIG{INT,QUIT} when launching editor

[Jeff King]

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

[PATCH 1/5] run-command: drop silent_exec_failure arg from wait_or_whine

[Jeff King]

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

diff --git a/run-command.c b/run-command.c
index 3b982e4..3aae270 100644
--- a/run-command.c
+++ b/run-command.c
@@ -226,7 +226,7 @@ static inline void set_cloexec(int fd)
 		fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
 }
 
-static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
+static int wait_or_whine(pid_t pid, const char *argv0)
 {
 	int status, code = -1;
 	pid_t waiting;
@@ -432,8 +432,7 @@ fail_pipe:
 		 * At this point we know that fork() succeeded, but execvp()
 		 * failed. Errors have been reported to our stderr.
 		 */
-		wait_or_whine(cmd->pid, cmd->argv[0],
-			      cmd->silent_exec_failure);
+		wait_or_whine(cmd->pid, cmd->argv[0]);
 		failed_errno = errno;
 		cmd->pid = -1;
 	}
@@ -538,7 +537,7 @@ int finish_command(struct child_process *cmd)
 
 int finish_command(struct child_process *cmd)
 {
-	return wait_or_whine(cmd->pid, cmd->argv[0], cmd->silent_exec_failure);
+	return wait_or_whine(cmd->pid, cmd->argv[0]);
 }
 
 int run_command(struct child_process *cmd)
-- 
1.8.0.1.620.g558b0aa

[PATCH 2/5] launch_editor: refactor to use start/finish_command

[Jeff King]

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

diff --git a/editor.c b/editor.c
index d834003..842f782 100644
--- a/editor.c
+++ b/editor.c
@@ -37,8 +37,16 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 
 	if (strcmp(editor, ":")) {
 		const char *args[] = { editor, path, NULL };
+		struct child_process p;
 
-		if (run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env))
+		memset(&p, 0, sizeof(p));
+		p.argv = args;
+		p.env = env;
+		p.use_shell = 1;
+		if (start_command(&p) < 0)
+			return error("unable to start editor '%s'", editor);
+
+		if (finish_command(&p))
 			return error("There was a problem with the editor '%s'.",
 					editor);
 	}
-- 
1.8.0.1.620.g558b0aa

[PATCH 3/5] launch_editor: ignore terminal signals while editor has control

[Jeff King]

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

diff --git a/editor.c b/editor.c
index 842f782..c892a81 100644
--- a/editor.c
+++ b/editor.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "strbuf.h"
 #include "run-command.h"
+#include "sigchain.h"
 
 #ifndef DEFAULT_EDITOR
 #define DEFAULT_EDITOR "vi"
@@ -38,6 +39,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 	if (strcmp(editor, ":")) {
 		const char *args[] = { editor, path, NULL };
 		struct child_process p;
+		int ret;
 
 		memset(&p, 0, sizeof(p));
 		p.argv = args;
@@ -46,7 +48,12 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		if (start_command(&p) < 0)
 			return error("unable to start editor '%s'", editor);
 
-		if (finish_command(&p))
+		sigchain_push(SIGINT, SIG_IGN);
+		sigchain_push(SIGQUIT, SIG_IGN);
+		ret = finish_command(&p);
+		sigchain_pop(SIGINT);
+		sigchain_pop(SIGQUIT);
+		if (ret)
 			return error("There was a problem with the editor '%s'.",
 					editor);
 	}
-- 
1.8.0.1.620.g558b0aa

[PATCH 4/5] run-command: do not warn about child death from terminal

[Jeff King]

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

diff --git a/run-command.c b/run-command.c
index 3aae270..757f263 100644
--- a/run-command.c
+++ b/run-command.c
@@ -242,7 +242,8 @@ static int wait_or_whine(pid_t pid, const char *argv0)
 		error("waitpid is confused (%s)", argv0);
 	} else if (WIFSIGNALED(status)) {
 		code = WTERMSIG(status);
-		error("%s died of signal %d", argv0, code);
+		if (code != SIGINT && code != SIGQUIT)
+			error("%s died of signal %d", argv0, code);
 		/*
 		 * This return value is chosen so that code & 0xff
 		 * mimics the exit code that a POSIX shell would report for
-- 
1.8.0.1.620.g558b0aa

[PATCH 5/5] launch_editor: propagate signals from editor to git

[Jeff King]

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

diff --git a/editor.c b/editor.c
index c892a81..065a7ab 100644
--- a/editor.c
+++ b/editor.c
@@ -39,7 +39,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 	if (strcmp(editor, ":")) {
 		const char *args[] = { editor, path, NULL };
 		struct child_process p;
-		int ret;
+		int ret, sig;
 
 		memset(&p, 0, sizeof(p));
 		p.argv = args;
@@ -51,8 +51,11 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		sigchain_push(SIGINT, SIG_IGN);
 		sigchain_push(SIGQUIT, SIG_IGN);
 		ret = finish_command(&p);
+		sig = ret + 128;
 		sigchain_pop(SIGINT);
 		sigchain_pop(SIGQUIT);
+		if (sig == SIGINT || sig == SIGQUIT)
+			raise(sig);
 		if (ret)
 			return error("There was a problem with the editor '%s'.",
 					editor);
-- 
1.8.0.1.620.g558b0aa

Re: [PATCH 0/5] ignore SIG{INT,QUIT} when launching editor

[Krzysztof Mazur]

On Fri, Nov 30, 2012 at 05:39:43PM -0500, Jeff King wrote:
> This is a re-roll of the pf/editor-ignore-sigint series.
> 
> 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.
> 

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

Re: [PATCH 0/5] ignore SIG{INT,QUIT} when launching editor

[Paul Fox]

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)

Re: [PATCH 0/5] ignore SIG{INT,QUIT} when launching editor

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Nicely explained.  Very much appreciated.