On Thu, Feb 20, 2020 at 9:32 PM Jeff King <peff@xxxxxxxx> wrote: > > On Fri, Feb 21, 2020 at 02:25:58AM +0000, brian m. carlson wrote: > > > On 2020-02-21 at 01:20:51, Anthony Sottile wrote: > > > My hook in question is a python process: https://pre-commit.com > > > > > > It doesn't really do all that much on SIGINT but prints "(^C) > > > Interrupted" and offers a crash log when receiving ^C -- this races > > > with the git process terminating and causes terminal spew (sometimes > > > with pretty bad consequences with input breaking until `reset` > > > depending on which thing wins the tty reset race). > > > > Thanks, this is helpful context. I don't know that Git waiting for the > > process is going to fix the broken terminal state, although it will > > likely fix the jumbled output. > > > > I'm not planning on writing a patch, but I think an interesting patch to > > see might be if we called wait(2) in a loop in a SIGINT handler but > > didn't reinstall the signal handler, which means that a second Ctrl-C > > would kill Git. I believe that's what certain other programs do, and > > that might address many of the problems in both scenarios. > > The run-command struct has a clean_on_exit flag, as well as a > wait_after_clean flag, that would do what you want: when we're killed by > SIGINT, we'd pass the signal on to the child and then wait for to > finish. That first step should generally be unnecessary for SIGINT > (since as you noted, it will usually be delivered to the whole process > group), but it shouldn't hurt. > > To get the double-^C behavior, I think cleanup_children_on_signal() > would have to be reordered to pop the signal handler first before > calling cleanup_children(). > > I'm not quite convinced that's all worth doing, or wouldn't have other > unforeseen consequences. But if anybody is interested in experimenting, > I think the patch would only be a few lines (set those flags when > running hooks, and then that reordering). > > -Peff The small patch at least solves my issues and prevents the zombie processes so I've sent it to the mailing list -- thanks for the pointer. I had found `wait_after_clean` in my initial investigation but had missed the `clean_on_exit` flag! Anthony
