threads / rfc / 32036

RFC patchlaunch_editor: ignore SIGINT while the editor has control

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

## tl;dr

4 messages between Nov 7, 2012 and Nov 8, 2012. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Paul Fox· Nov 7, 2012, 19:16 UTC · lore

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

Show changes to editor.c +5 −1

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)

Krzysztof Mazur· Nov 7, 2012, 22:00 UTC · re: Paul Fox · lore

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

On Wed, Nov 07, 2012 at 02:16:52PM -0500, Paul Fox wrote:

Show 28 quoted lines

> 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:

Show changes to editor.c +1 −0

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

Paul Fox· Nov 7, 2012, 23:35 UTC · re: Krzysztof Mazur · lore

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

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

Show changes to editor.c +6 −1

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)

Jeff King· Nov 8, 2012, 15:33 UTC · re: Paul Fox · lore

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

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

Show 6 quoted lines

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

Show 9 quoted lines

> 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