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

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: [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, #02; Fri, 9)

[Paul Fox]

kalle olavi niemitalo wrote:
  Jeff King p...@peff.net 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 p...@foxharp.boston.ma.us 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/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 p...@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, 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 p...@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, 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