{"thread":{"id":"32254","subject":"[PATCH 0/5] ignore SIG{INT,QUIT} when launching editor","startedAt":"2012-11-30T22:39:43Z","lastAt":"2012-12-02T10:04:43Z","messageCount":9,"participants":["Jeff King","Krzysztof Mazur","Paul Fox","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"204361","messageId":"20121130223943.GA27120@sigill.intra.peff.net","threadId":"32254","inReplyTo":null,"subject":"[PATCH 0/5] ignore SIG{INT,QUIT} when launching editor","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-30T22:39:43Z","receivedAt":"2012-11-30T22:39:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a re-roll of the pf/editor-ignore-sigint series.\n\nThere are two changes from the original:\n\n 1. We ignore both SIGINT and SIGQUIT for \"least surprise\" compared to\n system(3).\n\n 2. We now use \"code + 128\" to look for signal death (instead of\n WTERMSIG), as per run-command's documentation on how it munges the\n code.\n\nPeople mentioned some buggy editors which go into an infinite EIO loop\nwhen their parent dies due to SIGQUIT. That should be a non-issue now,\nas we will be ignoring SIGQUIT. And even if you could replicate it\n(e.g., with another signal) those programs should be (and reportedly\nhave been) fixed. It is not git's job to babysit its child processes.\n\nThe patches are:\n\n [1/5]: run-command: drop silent_exec_failure arg from wait_or_whine\n [2/5]: launch_editor: refactor to use start/finish_command\n [3/5]: launch_editor: ignore terminal signals while editor has control\n [4/5]: run-command: do not warn about child death from terminal\n [5/5]: launch_editor: propagate signals from editor to git\n\nSince this can be thought of as \"act more like system(3)\", I wondered\nwhether the signal-ignore logic should be moved into run-command, or\neven used by default for blocking calls to run_command (which are\nbasically our version of system(3)). But it is detrimental in the common\ncase that the child is not taking control of the terminal, and is just\nan implementation detail (e.g., we call \"git update-ref\" behind the\nscenes, but the user does not know or care). If they hit ^C during such\na run and we are ignoring SIGINT, then either:\n\n 1. we will notice the child died by signal and report an\n error in the subprocess rather than just dying; the end result is\n similar, but the error is unnecessarily confusing\n\n 2. we do not bother to check the child's return code (because we do\n not care whether the child succeeded or not, like a \"gc --auto\");\n we end up totally ignoring the user's request to abort the\n operation\n\nSo I do not think we care about this behavior except for launching the\neditor. And the signal-propagation behavior of 5/5 is really so weirdly\neditor-specific (because it is about behaving well whether the child\nblocks signals or not).\n\n-Peff\n"},{"id":"204362","messageId":"20121130224050.GA23772@sigill.intra.peff.net","threadId":"32254","inReplyTo":"20121130223943.GA27120@sigill.intra.peff.net","subject":"[PATCH 1/5] run-command: drop silent_exec_failure arg from wait_or_whine","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-30T22:40:50Z","receivedAt":"2012-11-30T22:40:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We do not actually use this parameter; instead we complain\nfrom the child itself (for fork/exec) or from start_command\n(if we are using spawn on Windows).\n\nSigned-off-by: Jeff King \n---\n run-command.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 3b982e4..3aae270 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -226,7 +226,7 @@ static inline void set_cloexec(int fd)\n \t\tfcntl(fd, F_SETFD, flags | FD_CLOEXEC);\n }\n \n-static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n+static int wait_or_whine(pid_t pid, const char *argv0)\n {\n \tint status, code = -1;\n \tpid_t waiting;\n@@ -432,8 +432,7 @@ fail_pipe:\n \t\t * At this point we know that fork() succeeded, but execvp()\n \t\t * failed. Errors have been reported to our stderr.\n \t\t */\n-\t\twait_or_whine(cmd->pid, cmd->argv[0],\n-\t\t\t cmd->silent_exec_failure);\n+\t\twait_or_whine(cmd->pid, cmd->argv[0]);\n \t\tfailed_errno = errno;\n \t\tcmd->pid = -1;\n \t}\n@@ -538,7 +537,7 @@ int finish_command(struct child_process *cmd)\n \n int finish_command(struct child_process *cmd)\n {\n-\treturn wait_or_whine(cmd->pid, cmd->argv[0], cmd->silent_exec_failure);\n+\treturn wait_or_whine(cmd->pid, cmd->argv[0]);\n }\n \n int run_command(struct child_process *cmd)\n-- \n1.8.0.1.620.g558b0aa\n"},{"id":"204363","messageId":"20121130224104.GB23772@sigill.intra.peff.net","threadId":"32254","inReplyTo":"20121130223943.GA27120@sigill.intra.peff.net","subject":"[PATCH 2/5] launch_editor: refactor to use start/finish_command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-30T22:41:04Z","receivedAt":"2012-11-30T22:41:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The launch_editor function uses the convenient run_command_*\ninterface. Let's use the more flexible start_command and\nfinish_command functions, which will let us manipulate the\nparent state while we're waiting for the child to finish.\n\nSigned-off-by: Jeff King \n---\n editor.c | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/editor.c b/editor.c\nindex d834003..842f782 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -37,8 +37,16 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n \n \tif (strcmp(editor, \":\")) {\n \t\tconst char *args[] = { editor, path, NULL };\n+\t\tstruct child_process p;\n \n-\t\tif (run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env))\n+\t\tmemset(&p, 0, sizeof(p));\n+\t\tp.argv = args;\n+\t\tp.env = env;\n+\t\tp.use_shell = 1;\n+\t\tif (start_command(&p) < 0)\n+\t\t\treturn error(\"unable to start editor '%s'\", editor);\n+\n+\t\tif (finish_command(&p))\n \t\t\treturn error(\"There was a problem with the editor '%s'.\",\n \t\t\t\t\teditor);\n \t}\n-- \n1.8.0.1.620.g558b0aa\n"},{"id":"204364","messageId":"20121130224125.GC23772@sigill.intra.peff.net","threadId":"32254","inReplyTo":"20121130223943.GA27120@sigill.intra.peff.net","subject":"[PATCH 3/5] launch_editor: ignore terminal signals while editor has control","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-30T22:41:26Z","receivedAt":"2012-11-30T22:41:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Paul Fox \n\nThe user's editor likely catches SIGINT (ctrl-C). but if\nthe user spawns a command from the editor and uses ctrl-C to\nkill that command, the SIGINT will likely also kill git\nitself (depending on the editor, this can leave the terminal\nin an unusable state).\n\nLet's ignore it while the editor is running, and do the same\nfor SIGQUIT, which many editors also ignore. This matches\nthe behavior if we were to use system(3) instead of\nrun-command.\n\nSigned-off-by: Paul Fox \nSigned-off-by: Jeff King \n---\n editor.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/editor.c b/editor.c\nindex 842f782..c892a81 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"strbuf.h\"\n #include \"run-command.h\"\n+#include \"sigchain.h\"\n \n #ifndef DEFAULT_EDITOR\n #define DEFAULT_EDITOR \"vi\"\n@@ -38,6 +39,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n \tif (strcmp(editor, \":\")) {\n \t\tconst char *args[] = { editor, path, NULL };\n \t\tstruct child_process p;\n+\t\tint ret;\n \n \t\tmemset(&p, 0, sizeof(p));\n \t\tp.argv = args;\n@@ -46,7 +48,12 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n \t\tif (start_command(&p) < 0)\n \t\t\treturn error(\"unable to start editor '%s'\", editor);\n \n-\t\tif (finish_command(&p))\n+\t\tsigchain_push(SIGINT, SIG_IGN);\n+\t\tsigchain_push(SIGQUIT, SIG_IGN);\n+\t\tret = finish_command(&p);\n+\t\tsigchain_pop(SIGINT);\n+\t\tsigchain_pop(SIGQUIT);\n+\t\tif (ret)\n \t\t\treturn error(\"There was a problem with the editor '%s'.\",\n \t\t\t\t\teditor);\n \t}\n-- \n1.8.0.1.620.g558b0aa\n"},{"id":"204365","messageId":"20121130224138.GD23772@sigill.intra.peff.net","threadId":"32254","inReplyTo":"20121130223943.GA27120@sigill.intra.peff.net","subject":"[PATCH 4/5] run-command: do not warn about child death from terminal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-30T22:41:38Z","receivedAt":"2012-11-30T22:41:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"SIGINT and SIGQUIT are not generally interesting signals to\nthe user, since they are typically caused by them hitting \"^C\"\nor otherwise telling their terminal to send the signal.\n\nSigned-off-by: Jeff King \n---\n run-command.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 3aae270..757f263 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -242,7 +242,8 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n \t\terror(\"waitpid is confused (%s)\", argv0);\n \t} else if (WIFSIGNALED(status)) {\n \t\tcode = WTERMSIG(status);\n-\t\terror(\"%s died of signal %d\", argv0, code);\n+\t\tif (code != SIGINT && code != SIGQUIT)\n+\t\t\terror(\"%s died of signal %d\", argv0, code);\n \t\t/*\n \t\t * This return value is chosen so that code & 0xff\n \t\t * mimics the exit code that a POSIX shell would report for\n-- \n1.8.0.1.620.g558b0aa\n"},{"id":"204366","messageId":"20121130224149.GE23772@sigill.intra.peff.net","threadId":"32254","inReplyTo":"20121130223943.GA27120@sigill.intra.peff.net","subject":"[PATCH 5/5] launch_editor: propagate signals from editor to git","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-30T22:41:50Z","receivedAt":"2012-11-30T22:41:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We block SIGINT and SIGQUIT while the editor runs so that\ngit is not killed accidentally by a stray \"^C\" meant for the\neditor or its subprocesses. This works because most editors\nignore SIGINT.\n\nHowever, some editor wrappers, like emacsclient, expect to\ndie due to ^C. We detect the signal death in the editor and\nproperly exit, but not before writing a useless error\nmessage to stderr. Instead, let's notice when the editor was\nkilled by a terminal signal and just raise the signal on\nourselves. This skips the message and looks to our parent\nlike we received SIGINT ourselves.\n\nThe end effect is that if the user's editor ignores SIGINT,\nwe will, too. And if it does not, then we will behave as if\nwe did not ignore it. That should make all users happy.\n\nNote that in the off chance that another part of git has\nignored SIGINT while calling launch_editor, we will still\nproperly detect and propagate the failed return code from\nthe editor (i.e., the worst case is that we generate the\nuseless error, not fail to notice the editor's death).\n\nSigned-off-by: Jeff King \n---\n editor.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/editor.c b/editor.c\nindex c892a81..065a7ab 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -39,7 +39,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n \tif (strcmp(editor, \":\")) {\n \t\tconst char *args[] = { editor, path, NULL };\n \t\tstruct child_process p;\n-\t\tint ret;\n+\t\tint ret, sig;\n \n \t\tmemset(&p, 0, sizeof(p));\n \t\tp.argv = args;\n@@ -51,8 +51,11 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n \t\tsigchain_push(SIGINT, SIG_IGN);\n \t\tsigchain_push(SIGQUIT, SIG_IGN);\n \t\tret = finish_command(&p);\n+\t\tsig = ret + 128;\n \t\tsigchain_pop(SIGINT);\n \t\tsigchain_pop(SIGQUIT);\n+\t\tif (sig == SIGINT || sig == SIGQUIT)\n+\t\t\traise(sig);\n \t\tif (ret)\n \t\t\treturn error(\"There was a problem with the editor '%s'.\",\n \t\t\t\t\teditor);\n-- \n1.8.0.1.620.g558b0aa\n"},{"id":"204380","messageId":"20121201123437.GA10287@shrek.podlesie.net","threadId":"32254","inReplyTo":"20121130223943.GA27120@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] ignore SIG{INT,QUIT} when launching editor","fromName":"Krzysztof Mazur","fromEmail":"krzysiek@podlesie.net","sentAt":"2012-12-01T12:34:37Z","receivedAt":"2012-12-01T12:34:37Z","isPatch":true,"sender":{"key":"krzysiek@podlesie.net","avatar":null},"body":"On Fri, Nov 30, 2012 at 05:39:43PM -0500, Jeff King wrote:\n> This is a re-roll of the pf/editor-ignore-sigint series.\n> \n> People mentioned some buggy editors which go into an infinite EIO loop\n> when their parent dies due to SIGQUIT. That should be a non-issue now,\n> as we will be ignoring SIGQUIT. And even if you could replicate it\n> (e.g., with another signal) those programs should be (and reportedly\n> have been) fixed. It is not git's job to babysit its child processes.\n> \n\nAlso some good editors printed error message after they got EIO,\nconfusing the user.\n\nLooks good to me. I've tested this with ed (always ignores SIGINT\nand SIGQUIT), vim (always ignores SIGINT, but dies after three\nSIGQUIT) and \"sleep\" (dies after SIGINT and SIGQUIT) and git works now\nas expected. Doing what editor does is probably the best thing to do. \n\nTested-by: Krzysztof Mazur \n\n\nThanks,\n\nKrzysiek\n"},{"id":"204387","messageId":"20121201154805.55C492E932A@grass.foxharp.boston.ma.us","threadId":"32254","inReplyTo":"20121130223943.GA27120@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] ignore SIG{INT,QUIT} when launching editor","fromName":"Paul Fox","fromEmail":"pgf@foxharp.boston.ma.us","sentAt":"2012-12-01T15:48:05Z","receivedAt":"2012-12-01T15:48:05Z","isPatch":true,"sender":{"key":"pgf@foxharp.boston.ma.us","avatar":"https://avatars.githubusercontent.com/u/4249842?v=4"},"body":"jeff wrote:\n > This is a re-roll of the pf/editor-ignore-sigint series.\n > \n > There are two changes from the original:\n > \n > 1. We ignore both SIGINT and SIGQUIT for \"least surprise\" compared to\n > system(3).\n > \n > 2. We now use \"code + 128\" to look for signal death (instead of\n > WTERMSIG), as per run-command's documentation on how it munges the\n > code.\n\nthis series all looks good to me. thanks for re- and re-re-rolling.\n\npaul\n\n > \n > People mentioned some buggy editors which go into an infinite EIO loop\n > when their parent dies due to SIGQUIT. That should be a non-issue now,\n > as we will be ignoring SIGQUIT. And even if you could replicate it\n > (e.g., with another signal) those programs should be (and reportedly\n > have been) fixed. It is not git's job to babysit its child processes.\n > \n > The patches are:\n > \n > [1/5]: run-command: drop silent_exec_failure arg from wait_or_whine\n > [2/5]: launch_editor: refactor to use start/finish_command\n > [3/5]: launch_editor: ignore terminal signals while editor has control\n > [4/5]: run-command: do not warn about child death from terminal\n > [5/5]: launch_editor: propagate signals from editor to git\n > \n > Since this can be thought of as \"act more like system(3)\", I wondered\n > whether the signal-ignore logic should be moved into run-command, or\n > even used by default for blocking calls to run_command (which are\n > basically our version of system(3)). But it is detrimental in the common\n > case that the child is not taking control of the terminal, and is just\n > an implementation detail (e.g., we call \"git update-ref\" behind the\n > scenes, but the user does not know or care). If they hit ^C during such\n > a run and we are ignoring SIGINT, then either:\n > \n > 1. we will notice the child died by signal and report an\n > error in the subprocess rather than just dying; the end result is\n > similar, but the error is unnecessarily confusing\n > \n > 2. we do not bother to check the child's return code (because we do\n > not care whether the child succeeded or not, like a \"gc --auto\");\n > we end up totally ignoring the user's request to abort the\n > operation\n > \n > So I do not think we care about this behavior except for launching the\n > editor. And the signal-propagation behavior of 5/5 is really so weirdly\n > editor-specific (because it is about behaving well whether the child\n > blocks signals or not).\n > \n > -Peff\n\n=---------------------\n paul fox, pgf@foxharp.boston.ma.us (arlington, ma, where it's 24.8 degrees)\n"},{"id":"204420","messageId":"7vboeclqh0.fsf@alter.siamese.dyndns.org","threadId":"32254","inReplyTo":"20121130223943.GA27120@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] ignore SIG{INT,QUIT} when launching editor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-02T10:04:43Z","receivedAt":"2012-12-02T10:04:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Since this can be thought of as \"act more like system(3)\", I wondered\n> whether the signal-ignore logic should be moved into run-command, or\n> even used by default for blocking calls to run_command (which are\n> basically our version of system(3)). But it is detrimental in the common\n> case that the child is not taking control of the terminal, and is just\n> an implementation detail (e.g., we call \"git update-ref\" behind the\n> scenes, but the user does not know or care). If they hit ^C during such\n> a run and we are ignoring SIGINT, then either:\n>\n> 1. we will notice the child died by signal and report an\n> error in the subprocess rather than just dying; the end result is\n> similar, but the error is unnecessarily confusing\n>\n> 2. we do not bother to check the child's return code (because we do\n> not care whether the child succeeded or not, like a \"gc --auto\");\n> we end up totally ignoring the user's request to abort the\n> operation\n>\n> So I do not think we care about this behavior except for launching the\n> editor. And the signal-propagation behavior of 5/5 is really so weirdly\n> editor-specific (because it is about behaving well whether the child\n> blocks signals or not).\n\nNicely explained. Very much appreciated.\n"}]}