git does not wait on hook exes (^C)

> Here's a small example:
> 
> ```bash
> #!/usr/bin/env bash
> foo() {
>     echo zzz part 2
>     sleep 1
>     echo exiting now
> }
> trap foo SIGINT
> 
> echo zzz part 1
> sleep 10
> ```
> 
> ```console
> $ git --version
> git version 2.25.GIT
> $ git commit --allow-empty -m foo
> zzz part 1
> ^Czzz part 2
> 
> $ exiting now
> ```
> 
> - I pressed ^C during the first sleep
> - control was returned back to my terminal
> - the hook script was still running in the background

>
> On 2020-02-20 at 21:14:55, Anthony Sottile wrote:
> > Here's a small example:
> >
> > ```bash
> > #!/usr/bin/env bash
> > foo() {
> >     echo zzz part 2
> >     sleep 1
> >     echo exiting now
> > }
> > trap foo SIGINT
> >
> > echo zzz part 1
> > sleep 10
> > ```
> >
> > ```console
> > $ git --version
> > git version 2.25.GIT
> > $ git commit --allow-empty -m foo
> > zzz part 1
> > ^Czzz part 2
> >
> > $ exiting now
> > ```
> >
> > - I pressed ^C during the first sleep
> > - control was returned back to my terminal
> > - the hook script was still running in the background
>
> I believe the way that SIGINT works on a terminal is that it sends the
> signal to all processes in the foreground process group.  So my guess of
> what's happening here is that Git and your script both get SIGINT, Git
> cleans up and exits quickly, leaving your script running.
>
> If so, I'm not sure that Git can do much here.  If Git waited for the
> hook to exit, then a broken or hung hook would cause Git to hang
> indefinitely, which is not what the user intended when they pressed
> Ctrl-C.  Usually what the user wants is an immediate return to the
> terminal in such a case, and I think most users would consider it a bug
> if Git were to wait for its children.
>
> Certainly I'm open to hearing other views on this, though.
> --
> brian m. carlson: Houston, Texas, US
> OpenPGP: https://keybase.io/bk2204

> On Thu, Feb 20, 2020 at 4:01 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > I believe the way that SIGINT works on a terminal is that it sends the
> > signal to all processes in the foreground process group.  So my guess of
> > what's happening here is that Git and your script both get SIGINT, Git
> > cleans up and exits quickly, leaving your script running.
> >
> > If so, I'm not sure that Git can do much here.  If Git waited for the
> > hook to exit, then a broken or hung hook would cause Git to hang
> > indefinitely, which is not what the user intended when they pressed
> > Ctrl-C.  Usually what the user wants is an immediate return to the
> > terminal in such a case, and I think most users would consider it a bug
> > if Git were to wait for its children.
> 
> Taking git out of the situation:
> 
> Create a shell script:
> 
> ```bash
> #!/usr/bin/env bash
> .git/hooks/pre-commit
> ```
> 
> ```console
> $ ./t.sh
> zzz part 1
> ^Czzz part 2
> exiting
> $
> ```
> 
> that works fine (and is the expected case for `subprocess` calls for
> example in python)

>
> On 2020-02-21 at 00:12:18, Anthony Sottile wrote:
> > On Thu, Feb 20, 2020 at 4:01 PM brian m. carlson
> > <sandals@crustytoothpaste.net> wrote:
> > > I believe the way that SIGINT works on a terminal is that it sends the
> > > signal to all processes in the foreground process group.  So my guess of
> > > what's happening here is that Git and your script both get SIGINT, Git
> > > cleans up and exits quickly, leaving your script running.
> > >
> > > If so, I'm not sure that Git can do much here.  If Git waited for the
> > > hook to exit, then a broken or hung hook would cause Git to hang
> > > indefinitely, which is not what the user intended when they pressed
> > > Ctrl-C.  Usually what the user wants is an immediate return to the
> > > terminal in such a case, and I think most users would consider it a bug
> > > if Git were to wait for its children.
> >
> > Taking git out of the situation:
> >
> > Create a shell script:
> >
> > ```bash
> > #!/usr/bin/env bash
> > .git/hooks/pre-commit
> > ```
> >
> > ```console
> > $ ./t.sh
> > zzz part 1
> > ^Czzz part 2
> > exiting
> > $
> > ```
> >
> > that works fine (and is the expected case for `subprocess` calls for
> > example in python)
>
> Yeah, I think this is the case I was discussing up above, where the
> parent process waits for the subprocess to exit.  If I modify the foo
> function in your hook to also have a "sleep 10", then the parent process
> hangs until the child process exits, which again would mean that Git
> would hang indefinitely if the hook hung.
>
> Can you maybe tell us a little more about your use case?  What are you
> doing in your hook that makes this case come up?  Why is your hook
> trapping SIGINT?
> --
> brian m. carlson: Houston, Texas, US
> OpenPGP: https://keybase.io/bk2204

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

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

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