# [PATCH 0/5] ignore SIG{INT,QUIT} when launching editor 9 messages from 2012-11-30 to 2012-12-02. Participants: Jeff King, Krzysztof Mazur, Paul Fox, Junio C Hamano. Thread: https://gitlist.dev/t/32254 ## Jeff King, 2012-11-30 22:39 Subject: [PATCH 0/5] ignore SIG{INT,QUIT} when launching editor Message-ID: <20121130223943.GA27120@sigill.intra.peff.net> URL: https://gitlist.dev/e/20121130223943.GA27120%40sigill.intra.peff.net ``` 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 ``` ## Jeff King, 2012-11-30 22:40 Subject: [PATCH 1/5] run-command: drop silent_exec_failure arg from wait_or_whine Message-ID: <20121130224050.GA23772@sigill.intra.peff.net> URL: https://gitlist.dev/e/20121130224050.GA23772%40sigill.intra.peff.net In-Reply-To: <20121130223943.GA27120@sigill.intra.peff.net> ``` 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 --- 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 ``` ## Jeff King, 2012-11-30 22:41 Subject: [PATCH 2/5] launch_editor: refactor to use start/finish_command Message-ID: <20121130224104.GB23772@sigill.intra.peff.net> URL: https://gitlist.dev/e/20121130224104.GB23772%40sigill.intra.peff.net In-Reply-To: <20121130223943.GA27120@sigill.intra.peff.net> ``` 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 --- 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 ``` ## Jeff King, 2012-11-30 22:41 Subject: [PATCH 3/5] launch_editor: ignore terminal signals while editor has control Message-ID: <20121130224125.GC23772@sigill.intra.peff.net> URL: https://gitlist.dev/e/20121130224125.GC23772%40sigill.intra.peff.net In-Reply-To: <20121130223943.GA27120@sigill.intra.peff.net> ``` From: Paul Fox 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 Signed-off-by: Jeff King --- 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 ``` ## Jeff King, 2012-11-30 22:41 Subject: [PATCH 4/5] run-command: do not warn about child death from terminal Message-ID: <20121130224138.GD23772@sigill.intra.peff.net> URL: https://gitlist.dev/e/20121130224138.GD23772%40sigill.intra.peff.net In-Reply-To: <20121130223943.GA27120@sigill.intra.peff.net> ``` 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 --- 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 ``` ## Jeff King, 2012-11-30 22:41 Subject: [PATCH 5/5] launch_editor: propagate signals from editor to git Message-ID: <20121130224149.GE23772@sigill.intra.peff.net> URL: https://gitlist.dev/e/20121130224149.GE23772%40sigill.intra.peff.net In-Reply-To: <20121130223943.GA27120@sigill.intra.peff.net> ``` 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 --- 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 ``` ## Krzysztof Mazur, 2012-12-01 12:34 Subject: Re: [PATCH 0/5] ignore SIG{INT,QUIT} when launching editor Message-ID: <20121201123437.GA10287@shrek.podlesie.net> URL: https://gitlist.dev/e/20121201123437.GA10287%40shrek.podlesie.net In-Reply-To: <20121130223943.GA27120@sigill.intra.peff.net> ``` 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 Thanks, Krzysiek ``` ## Paul Fox, 2012-12-01 15:48 Subject: Re: [PATCH 0/5] ignore SIG{INT,QUIT} when launching editor Message-ID: <20121201154805.55C492E932A@grass.foxharp.boston.ma.us> URL: https://gitlist.dev/e/20121201154805.55C492E932A%40grass.foxharp.boston.ma.us In-Reply-To: <20121130223943.GA27120@sigill.intra.peff.net> ``` 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) ``` ## Junio C Hamano, 2012-12-02 10:04 Subject: Re: [PATCH 0/5] ignore SIG{INT,QUIT} when launching editor Message-ID: <7vboeclqh0.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vboeclqh0.fsf%40alter.siamese.dyndns.org In-Reply-To: <20121130223943.GA27120@sigill.intra.peff.net> ``` Jeff King 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. ```