Re: git does not wait on hook exes (^C)

[Jeff King <peff@peff.net>]

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