{"thread":{"id":"32036","subject":"[PATCH/RFC] launch_editor: ignore SIGINT while the editor has control","startedAt":"2012-11-07T19:16:52Z","lastAt":"2012-11-08T15:33:31Z","messageCount":4,"participants":["Paul Fox","Krzysztof Mazur","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"202618","messageId":"20121107191652.842C52E8089@grass.foxharp.boston.ma.us","threadId":"32036","inReplyTo":null,"subject":"[PATCH/RFC] launch_editor: ignore SIGINT while the editor has control","fromName":"Paul Fox","fromEmail":"pgf@foxharp.boston.ma.us","sentAt":"2012-11-07T19:16:52Z","receivedAt":"2012-11-07T19:16:52Z","isPatch":true,"sender":{"key":"pgf@foxharp.boston.ma.us","avatar":"https://avatars.githubusercontent.com/u/4249842?v=4"},"body":"the user's editor likely catches SIGINT (ctrl-C).  but if the user\nspawns a command from the editor and uses ctrl-C to kill that command,\nthe SIGINT will likely also kill git itself.  (depending on the\neditor, this can leave the terminal in an unusable state.)\n\nSigned-off-by: Paul Fox <pgf@foxharp.boston.ma.us>\n---\n\ni often shell out of my editor while composing a git commit message,\nin order to recheck the diffs or the log, do a final test build, etc.\nwhen i interrupt one of these operations, the spawned program gets\nkilled.  in addition git itself gets killed, which in turn kills my\neditor.  this is never what i intended.  :-)\n\nthe problem is easy to demonstrate with vim, vile, or em.  in a vi-like\neditor:\n    git commit foo\n    :!sleep 10\n    ^C\n\nboth CVS and my usual mailer (MH) protect against this behavior when\nspawning editors by using code similar to the patch below, which\ncauses the spawning process to ignore SIGINT while the editor is\nrunning.\n\ni looked at the other invocations of run_command_v_opt_xxx() in git,\nbut couldn't convince myself that any of the others needed similar\nprotection.  i also couldn't convince myself that i wouldn't cause\ncollateral damage if i tried moving the sigchain_push/pop into\nrun-command.c.  (but perhaps it's simple -- maybe the RUN_USING_SHELL\nflag should always imply this behavior.)\n\nthe patch is against current master.\n\npaul\n\n editor.c |    6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/editor.c b/editor.c\nindex d834003..775f22d 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -37,8 +37,12 @@ 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\tint ret;\n \n-\t\tif (run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env))\n+\t\tsigchain_push(SIGINT, SIG_IGN);\n+\t\tret = run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env);\n+\t\tsigchain_pop(SIGINT);\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.7.5.4\n\n\n=---------------------\n paul fox, pgf@foxharp.boston.ma.us (arlington, ma, where it's 31.8 degrees)\n"},{"id":"202624","messageId":"20121107220027.GA17463@shrek.podlesie.net","threadId":"32036","inReplyTo":"20121107191652.842C52E8089@grass.foxharp.boston.ma.us","subject":"Re: [PATCH/RFC] launch_editor: ignore SIGINT while the editor has control","fromName":"Krzysztof Mazur","fromEmail":"krzysiek@podlesie.net","sentAt":"2012-11-07T22:00:28Z","receivedAt":"2012-11-07T22:00:28Z","isPatch":true,"sender":{"key":"krzysiek@podlesie.net","avatar":null},"body":"On Wed, Nov 07, 2012 at 02:16:52PM -0500, Paul Fox wrote:\n> the user's editor likely catches SIGINT (ctrl-C).  but if the user\n> spawns a command from the editor and uses ctrl-C to kill that command,\n> the SIGINT will likely also kill git itself.  (depending on the\n> editor, this can leave the terminal in an unusable state.)\n> \n> Signed-off-by: Paul Fox <pgf@foxharp.boston.ma.us>\n> \n>  editor.c |    6 +++++-\n>  1 files changed, 5 insertions(+), 1 deletions(-)\n> \n> diff --git a/editor.c b/editor.c\n> index d834003..775f22d 100644\n> --- a/editor.c\n> +++ b/editor.c\n> @@ -37,8 +37,12 @@ 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\tint ret;\n>  \n> -\t\tif (run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env))\n> +\t\tsigchain_push(SIGINT, SIG_IGN);\n> +\t\tret = run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env);\n> +\t\tsigchain_pop(SIGINT);\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\nLooks and works good, except for warnings:\n\neditor.c: In function 'launch_editor':\neditor.c:42:3: warning: implicit declaration of function 'sigchain_push' [-Wimplicit-function-declaration]\neditor.c:44:3: warning: implicit declaration of function 'sigchain_pop' [-Wimplicit-function-declaration]\n\n\"sigchain.h\" should be included, something like:\n\ndiff --git a/editor.c b/editor.c\nindex 775f22d..3ca361b 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\nKrzysiek\n"},{"id":"202625","messageId":"20121107233515.107ED2E8089@grass.foxharp.boston.ma.us","threadId":"32036","inReplyTo":"20121107220027.GA17463@shrek.podlesie.net","subject":"Re: [PATCH/RFC] launch_editor: ignore SIGINT while the editor has control","fromName":"Paul Fox","fromEmail":"pgf@foxharp.boston.ma.us","sentAt":"2012-11-07T23:35:15Z","receivedAt":"2012-11-07T23:35:15Z","isPatch":true,"sender":{"key":"pgf@foxharp.boston.ma.us","avatar":"https://avatars.githubusercontent.com/u/4249842?v=4"},"body":"the user's editor likely catches SIGINT (ctrl-C).  but if the user\nspawns a command from the editor and uses ctrl-C to kill that command,\nthe SIGINT will likely also kill git itself.  (depending on the\neditor, this can leave the terminal in an unusable state.)\n\nSigned-off-by: Paul Fox <pgf@foxharp.boston.ma.us>\n---\n\nkrzysztof wrote:\n...\n > editor.c: In function 'launch_editor':\n > editor.c:42:3: warning: implicit declaration of function 'sigchain_push' [-Wimplicit-function-declaration]\n > editor.c:44:3: warning: implicit declaration of function 'sigchain_pop' [-Wimplicit-function-declaration]\n\nsigh.  i had that initially, lost the patch, and then recreated\nwithout it.  but i'm surprised my build (i did rebuild! :-) doesn't\nemit those errors.  in any case, here's the fixed patch.\n\n editor.c |    7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/editor.c b/editor.c\nindex d834003..3ca361b 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@@ -37,8 +38,12 @@ 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\tint ret;\n \n-\t\tif (run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env))\n+\t\tsigchain_push(SIGINT, SIG_IGN);\n+\t\tret = run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env);\n+\t\tsigchain_pop(SIGINT);\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.7.5.4\n\n\n=---------------------\n paul fox, pgf@foxharp.boston.ma.us (arlington, ma, where it's 26.6 degrees)\n"},{"id":"202645","messageId":"20121108153331.GC15560@sigill.intra.peff.net","threadId":"32036","inReplyTo":"20121107233515.107ED2E8089@grass.foxharp.boston.ma.us","subject":"Re: [PATCH/RFC] launch_editor: ignore SIGINT while the editor has control","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-08T15:33:31Z","receivedAt":"2012-11-08T15:33:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 07, 2012 at 06:35:15PM -0500, Paul Fox wrote:\n\n> the user's editor likely catches SIGINT (ctrl-C).  but if the user\n> spawns a command from the editor and uses ctrl-C to kill that command,\n> the SIGINT will likely also kill git itself.  (depending on the\n> editor, this can leave the terminal in an unusable state.)\n> \n> Signed-off-by: Paul Fox <pgf@foxharp.boston.ma.us>\n\nThanks, I think this makes sense.\n\n> krzysztof wrote:\n> ...\n>  > editor.c: In function 'launch_editor':\n>  > editor.c:42:3: warning: implicit declaration of function 'sigchain_push' [-Wimplicit-function-declaration]\n>  > editor.c:44:3: warning: implicit declaration of function 'sigchain_pop' [-Wimplicit-function-declaration]\n> \n> sigh.  i had that initially, lost the patch, and then recreated\n> without it.  but i'm surprised my build (i did rebuild! :-) doesn't\n> emit those errors.  in any case, here's the fixed patch.\n\ngcc will not warn about implicit declarations by default; try compiling\nwith \"-Wall\".\n\n-Peff\n"}]}