[PATCH/RFC] launch_editor: ignore SIGINT while the editor has control

[PATCH/RFC] launch_editor: ignore SIGINT while the editor has control

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

Signed-off-by: Paul Fox <pgf@foxharp.boston.ma.us>
---

i often shell out of my editor while composing a git commit message,
in order to recheck the diffs or the log, do a final test build, etc.
when i interrupt one of these operations, the spawned program gets
killed.  in addition git itself gets killed, which in turn kills my
editor.  this is never what i intended.  :-)

the problem is easy to demonstrate with vim, vile, or em.  in a vi-like
editor:
    git commit foo
    :!sleep 10
    ^C

both CVS and my usual mailer (MH) protect against this behavior when
spawning editors by using code similar to the patch below, which
causes the spawning process to ignore SIGINT while the editor is
running.

i looked at the other invocations of run_command_v_opt_xxx() in git,
but couldn't convince myself that any of the others needed similar
protection.  i also couldn't convince myself that i wouldn't cause
collateral damage if i tried moving the sigchain_push/pop into
run-command.c.  (but perhaps it's simple -- maybe the RUN_USING_SHELL
flag should always imply this behavior.)

the patch is against current master.

paul

 editor.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/editor.c b/editor.c
index d834003..775f22d 100644
--- a/editor.c
+++ b/editor.c
@@ -37,8 +37,12 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 
 	if (strcmp(editor, ":")) {
 		const char *args[] = { editor, path, NULL };
+		int ret;
 
-		if (run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env))
+		sigchain_push(SIGINT, SIG_IGN);
+		ret = run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env);
+		sigchain_pop(SIGINT);
+		if (ret)
 			return error("There was a problem with the editor '%s'.",
 					editor);
 	}
-- 
1.7.5.4


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

Re: [PATCH/RFC] launch_editor: ignore SIGINT while the editor has control

[Krzysztof Mazur]

On Wed, Nov 07, 2012 at 02:16:52PM -0500, Paul Fox wrote:
> 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.)
> 
> Signed-off-by: Paul Fox <pgf@foxharp.boston.ma.us>
> 
>  editor.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/editor.c b/editor.c
> index d834003..775f22d 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -37,8 +37,12 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
>  
>  	if (strcmp(editor, ":")) {
>  		const char *args[] = { editor, path, NULL };
> +		int ret;
>  
> -		if (run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env))
> +		sigchain_push(SIGINT, SIG_IGN);
> +		ret = run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env);
> +		sigchain_pop(SIGINT);
> +		if (ret)
>  			return error("There was a problem with the editor '%s'.",
>  					editor);
>  	}

Looks and works good, except for warnings:

editor.c: In function 'launch_editor':
editor.c:42:3: warning: implicit declaration of function 'sigchain_push' [-Wimplicit-function-declaration]
editor.c:44:3: warning: implicit declaration of function 'sigchain_pop' [-Wimplicit-function-declaration]

"sigchain.h" should be included, something like:

diff --git a/editor.c b/editor.c
index 775f22d..3ca361b 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"

Krzysiek

Re: [PATCH/RFC] launch_editor: ignore SIGINT while the editor has control

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

Signed-off-by: Paul Fox <pgf@foxharp.boston.ma.us>
---

krzysztof wrote:
...
 > editor.c: In function 'launch_editor':
 > editor.c:42:3: warning: implicit declaration of function 'sigchain_push' [-Wimplicit-function-declaration]
 > editor.c:44:3: warning: implicit declaration of function 'sigchain_pop' [-Wimplicit-function-declaration]

sigh.  i had that initially, lost the patch, and then recreated
without it.  but i'm surprised my build (i did rebuild! :-) doesn't
emit those errors.  in any case, here's the fixed patch.

 editor.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/editor.c b/editor.c
index d834003..3ca361b 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"
@@ -37,8 +38,12 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 
 	if (strcmp(editor, ":")) {
 		const char *args[] = { editor, path, NULL };
+		int ret;
 
-		if (run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env))
+		sigchain_push(SIGINT, SIG_IGN);
+		ret = run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env);
+		sigchain_pop(SIGINT);
+		if (ret)
 			return error("There was a problem with the editor '%s'.",
 					editor);
 	}
-- 
1.7.5.4


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

Re: [PATCH/RFC] launch_editor: ignore SIGINT while the editor has control

[Jeff King]

On Wed, Nov 07, 2012 at 06:35:15PM -0500, Paul Fox wrote:

> 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.)
> 
> Signed-off-by: Paul Fox <pgf@foxharp.boston.ma.us>

Thanks, I think this makes sense.

> krzysztof wrote:
> ...
>  > editor.c: In function 'launch_editor':
>  > editor.c:42:3: warning: implicit declaration of function 'sigchain_push' [-Wimplicit-function-declaration]
>  > editor.c:44:3: warning: implicit declaration of function 'sigchain_pop' [-Wimplicit-function-declaration]
> 
> sigh.  i had that initially, lost the patch, and then recreated
> without it.  but i'm surprised my build (i did rebuild! :-) doesn't
> emit those errors.  in any case, here's the fixed patch.

gcc will not warn about implicit declarations by default; try compiling
with "-Wall".

-Peff