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

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, p...@foxharp.boston.ma.us (arlington, ma, where it's 31.8 degrees)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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, p...@foxharp.boston.ma.us (arlington, ma, where it's 26.6 degrees)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Paul Fox]

kalle olavi niemitalo wrote:
 > Jeff King  writes:
 > 
 > >  Comments welcome from people using unusual editors (e.g., a script that
 > >  starts an editor in another window then blocks, waiting for the user to
 > >  finish).
 > 
 > I often run a shell in Emacs in X, then start git commit in that
 > shell.  $EDITOR is emacsclient --current-frame, which asks the
 > existing Emacs instance to load the file and waits until I press
 > C-x # in Emacs to mark the file done.  If I want to abort the
 > commit, it is most intuitive to return to the *Shell* buffer in
 > Emacs and press C-c C-c (comint-interrupt-subjob) to send SIGINT
 > to git from there.  (I see that "an empty message aborts the
 > commit", and indeed it does, but well, I prefer not to trust such
 > a feature if I can instead just interrupt the thing.)
 > 
 > With pf/editor-ignore-sigint, C-c C-c in the *Shell* buffer kills
 > neither git nor the emacsclient started by git.  This is not good.
 > SIGQUIT from C-c C-\ (comint-quit-subjob) still works though.

when i implemented the change, i wondered if some twisted emacs
workflow would be an issue. ;-)  and i almost blocked SIGQUIT as
well -- the two programs i looked at for precedent (CVS and MH) both
block both SIGQUIT and SIGINT when spawning an editor.

but since emacs users must have dealt with CVS for a long time before
dealing with git, how might they have done so?

the existing git behavior is bad for non-emacs users, and git itself
provides an abort-the-operation mechanism (i.e., writing an empty
message), so i'm not convinced your use case invalidates the new
behavior.  (though it might spotlight a need for this being prominent
in release notes.)

paul
=-
 paul fox, p...@foxharp.boston.ma.us (arlington, ma, where it's 40.6 degrees)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Paul Fox]

kalle olavi niemitalo wrote:
 > Paul Fox  writes:
 > 
 > > when i implemented the change, i wondered if some twisted emacs
 > > workflow would be an issue. ;-)  and i almost blocked SIGQUIT as
 > > well -- the two programs i looked at for precedent (CVS and MH) both
 > > block both SIGQUIT and SIGINT when spawning an editor.
 > >
 > > but since emacs users must have dealt with CVS for a long time before
 > > dealing with git, how might they have done so?
 > 
 > I think I usually ran CVS via vc.el, which prompts for a commit
 > message in Emacs before it runs cvs commit.  So CVS did not need
 > to run $EDITOR.
 > 
 > I just tried emacsclient with CVS 1.12.13-MirDebian-9, and it
 > behaves somewhat differently from Git with pf/editor-ignore-sigint.
 > When I tell Emacs to send SIGINT to the *Shell* buffer, CVS prompts:
 > 
 > cvs commit: warning: editor session failed
 > 
 > Log message unchanged or not specified
 > a)bort, c)ontinue, e)dit, !)reuse this message unchanged for remaining dirs
 > Action: (continue) 

you're sending SIGINT to the cvs commit command, and that causes the
editor to die right away?  that's surprising.  i can replicate your
described behavior by setting $VISUAL to a script that just sleeps, and
sending SIGTERM to the cvs commit process.  but not by sending SIGINT.

well, i'm not sure what to say.  there's a real problem when using the
current code and traditional editors.  i thought that the patch in
pf/editor-ignore-sigint reflected standard practice, and indeed it
accomplishes exactly the right thing with those editors.  you've shown
a particular work flow involving emacsclient that won't work anymore
with the change made, though there are workarounds.  perhaps there's
something the other editors themselves should be doing differently,
but i don't know what that might be.

paul

 > 
 > and then I can choose to abort.
 > 
 > With strace, it looks like CVS sets SIG_IGN as the handler of
 > SIGINT and SIGQUIT only in the parent process after forking, not
 > in the child process that executes the editor.
 > 
 > CVS also temporarily blocks signals by calling sigprocmask, but
 > it undoes that before it forks or waits for the child process.

=-
 paul fox, p...@foxharp.boston.ma.us (arlington, ma, where it's 45.5 degrees)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] 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 
Signed-off-by: Jeff King 
---
 editor.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/editor.c b/editor.c
index 842f782..28aae85 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,10 @@ 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);
+   ret = finish_command(&p);
+   sigchain_pop(SIGINT);
+   if (ret)
return error("There was a problem with the editor 
'%s'.",
editor);
}
-- 
1.8.0.207.gdf2154c

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] ignore SIGINT while editor runs

[Paul Fox]

jeff wrote:
 > On Sun, Nov 11, 2012 at 10:48:46AM -0500, Jeff King wrote:
 > 
 > > Silly me. When I thought through the impact of Paul's patch, I knew that
 > > we would notice signal death of the editor. But I totally forgot to
 > > consider that the blocked signal is inherited by the child process. I
 > > think we just need to move the signal() call to after we've forked. Like
 > > this (on top of Paul's patch):
 > > [...]
 > > Note that this will give you a slightly verbose message from git.
 > > Potentially we could notice editor death due to SIGINT and suppress the
 > > message, under the assumption that the user hit ^C and does not need to
 > > be told.
 > 
 > Here's a series that I think should resolve the situation for everybody.

thanks!  i've tested -- this certainly scratches my initial itch.

ack,
paul

 > 
 >   [1/5]: launch_editor: refactor to use start/finish_command
 > 
 > The cleanup I sent out a few minutes ago.
 > 
 >   [2/5]: launch_editor: ignore SIGINT while the editor has control
 > 
 > Paul's patch rebased on my 1/5.
 > 
 >   [3/5]: run-command: drop silent_exec_failure arg from wait_or_whine
 >   [4/5]: run-command: do not warn about child death by SIGINT
 >   [5/5]: launch_editor: propagate SIGINT from editor to git
 > 
 > Act more like current git when the editor dies from SIGINT.
 > 
 > -Peff
 > --
 > To unsubscribe from this list: send the line "unsubscribe git" in
 > the body of a message to majord...@vger.kernel.org
 > More majordomo info at  http://vger.kernel.org/majordomo-info.html

=-
 paul fox, p...@foxharp.boston.ma.us (arlington, ma, where it's 56.3 degrees)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] ignore SIGINT while editor runs

[Paul Fox]

krzysztof wrote:
 > On Sun, Nov 11, 2012 at 11:31:00AM -0500, Jeff King wrote:
 > > 
 > > Here's a series that I think should resolve the situation for everybody.
 > > 
 > >   [1/5]: launch_editor: refactor to use start/finish_command
 > > 
 > > The cleanup I sent out a few minutes ago.
 > > 
 > >   [2/5]: launch_editor: ignore SIGINT while the editor has control
 > > 
 > > Paul's patch rebased on my 1/5.
 > > 
 > >   [3/5]: run-command: drop silent_exec_failure arg from wait_or_whine
 > >   [4/5]: run-command: do not warn about child death by SIGINT
 > >   [5/5]: launch_editor: propagate SIGINT from editor to git
 > > 
 > > Act more like current git when the editor dies from SIGINT.
 > > 
 > 
 > Looks ok, but what about SIGQUIT? Some editors like GNU ed (0.4 and 1.6)
 > ignore SIGQUIT, and after SIGQUIT git dies, but editor is still running.
 > After pressing any key ed receives -EIO and prints "stdin: Input/output
 > error". GNU ed 1.6 then exits, but ed 0.4 prints this error forever.
 > Maybe git should kill the editor in such case?

there's certainly lots of precedent for treating SIGINT and SIGQUIT
the same.  but there's also some merit to saying that if the user
knows to send SIGQUIT instead of SIGINT, they may well have a reason. 
(after all, if we always treat them the same, there's no point in
having both.)

the em editor (linus' microemacs) behaves as you describe ed 0.4 does,
except without the error message -- it just spins silently getting EIO
from reading stdin.  i think em needs to be fixed, and it sounds like
GNU ed already has been.  (unless i misunderstand the relationship of
0.4 and 1.6.)

paul

 > 
 > Krzysiek
 > --
 > To unsubscribe from this list: send the line "unsubscribe git" in
 > the body of a message to majord...@vger.kernel.org
 > More majordomo info at  http://vger.kernel.org/majordomo-info.html

=-
 paul fox, p...@foxharp.boston.ma.us (arlington, ma, where it's 57.2 degrees)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Nov 2012, #06; Mon, 19)

[Paul Fox]

junio c hamano wrote:
 > [Stalled]
 > 
 > * pf/editor-ignore-sigint (2012-11-11) 5 commits
 >  - launch_editor: propagate SIGINT from editor to git
 >  - run-command: do not warn about child death by SIGINT
 >  - run-command: drop silent_exec_failure arg from wait_or_whine
 >  - launch_editor: ignore SIGINT while the editor has control
 >  - launch_editor: refactor to use start/finish_command
 > 
 >  Avoid confusing cases where the user hits Ctrl-C while in the editor
 >  session, not realizing git will receive the signal. Since most editors
 >  will take over the terminal and will block SIGINT, this is not likely
 >  to confuse anyone.
 > 
 >  Some people raised issues with emacsclient, which are addressed by this
 >  re-roll. It should probably also handle SIGQUIT, and there were a
 >  handful of other review comments.
 > 
 >  Expecting a re-roll.

i don't have strong feelings on any of the later review comments
(though i guess the check on finish_command()'s return code needed
attention), and wasn't the last submitter, but would certainly like to
see this move forward.

paul
=-
 paul fox, p...@foxharp.boston.ma.us (arlington, ma, where it's 26.1 degrees)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Topics currently in the Stalled category

[Paul Fox]

In-reply-to: <7vobirq0q2.fsf...@alter.siamese.dyndns.org> 
(sfid-20121120_190548_379327_D3EE7D14)
References: <7vpq39up0m@alter.siamese.dyndns.org> 
<7vy5hvq1ey@alter.siamese.dyndns.org> 
<7vobirq0q2.fsf...@alter.siamese.dyndns.org> 
(sfid-20121120_190548_379327_D3EE7D14)
Fcc: outbox


junio c hamano wrote:
 > Here is a list of stalled topics I am having trouble deciding what
 > to do (the default is to dismiss them around feature freeze).
...
 > * pf/editor-ignore-sigint (2012-11-11) 5 commits
 > 
 >  Avoid confusing cases where the user hits Ctrl-C while in the editor
 >  session, not realizing git will receive the signal. Since most editors
 >  will take over the terminal and will block SIGINT, this is not likely
 >  to confuse anyone.
 > 
 >  Some people raised issues with emacsclient, which are addressed by this
 >  re-roll. It should probably also handle SIGQUIT, and there were a
 >  handful of other review comments.
 > 
 >  Anybody interested in moving this forward?

i started this, but then jeff took it and ran with it and made it
right.  i think the remaining changes are small -- if jeff would
prefer, i can probably finish it.  (but i won't guarantee not to
mess up the "From:" lines.  :-)

paul
=-
 paul fox, p...@foxharp.boston.ma.us (arlington, ma, where it's 36.0 degrees)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Topics currently in the Stalled category

[Paul Fox]

jeff wrote:
 > On Wed, Nov 21, 2012 at 10:27:50AM +0100, Krzysztof Mazur wrote:
 > 
 > > >  > * pf/editor-ignore-sigint (2012-11-11) 5 commits
 > > >  > 
 > > >  >  Avoid confusing cases where the user hits Ctrl-C while in the editor
 > > >  >  session, not realizing git will receive the signal. Since most 
 > > > editors
 > > >  >  will take over the terminal and will block SIGINT, this is not likely
 > > >  >  to confuse anyone.
 > > >  > 
 > > >  >  Some people raised issues with emacsclient, which are addressed by 
 > > > this
 > > >  >  re-roll. It should probably also handle SIGQUIT, and there were a
 > > >  >  handful of other review comments.
 > > >  > 
 > > >  >  Anybody interested in moving this forward?
 > > > 
 > > > i started this, but then jeff took it and ran with it and made it
 > > > right.  i think the remaining changes are small -- if jeff would
 > > > prefer, i can probably finish it.  (but i won't guarantee not to
 > > > mess up the "From:" lines.  :-)
 > > > 
 > > 
 > > I'm also interested. I sometimes use ":r !command" in vim, so far I never
 > > needed to use Ctrl-C, but maybe in future.
 > > 
 > > The SIGINT part seems to be finished, we need to decide what about SIGQUIT.
 > 
 > My plan was to just add in SIGQUIT[1] alongside SIGINT (and I think
 > there may have been one or two other minor comments in response to the
 > series). I am on vacation this week, but can revisit it next week. If
 > somebody wants to re-roll it in the meantime, that would be fine with
 > me.
 > 
 > -Peff
 > 
 > [1] Given the core-dumping behavior of SIGQUIT, I suspect it is not
 > nearly as widely used as SIGINT, but it sounds more like the
 > principle of least surprise to treat them the same.

i see no real point in treating them the same -- as you suggest, one
would only use SIGQUIT if SIGINT didn't work, and then you'd want them
to be treated differently.  so i'd be happy with the current code in
that regard.

i think krzysiek said that since editors usually catch SIGQUIT, git
should kill the editor when receiving SIGQUIT, essentially translating
the SIGQUIT to SIGTERM for the editor.  (please correct me if i
misunderstood.)  since well-behaved editors will die quickly anyway
(they get EIO on their next read from stdin), i'm not sure there's a
compelling reason for that extra step.

but i have no real objection to that behavior if others think it's
right -- there's certainly logic in saying that if git dies it should
ensure the editor does too.

(i'm away for the rest of the week also.)

paul
=-
 paul fox, p...@foxharp.boston.ma.us (arlington, ma, where it's 44.6 degrees)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] ignore SIG{INT,QUIT} when launching editor

[Paul Fox]

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, p...@foxharp.boston.ma.us (arlington, ma, where it's 24.8 degrees)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html