* git does not wait on hook exes (^C) @ 2020-02-20 21:14 Anthony Sottile 2020-02-21 0:01 ` brian m. carlson 0 siblings, 1 reply; 8+ messages in thread From: Anthony Sottile @ 2020-02-20 21:14 UTC (permalink / raw) To: Git Mailing List 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 Anthony ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: git does not wait on hook exes (^C) 2020-02-20 21:14 git does not wait on hook exes (^C) Anthony Sottile @ 2020-02-21 0:01 ` brian m. carlson 2020-02-21 0:12 ` Anthony Sottile 0 siblings, 1 reply; 8+ messages in thread From: brian m. carlson @ 2020-02-21 0:01 UTC (permalink / raw) To: Anthony Sottile; +Cc: Git Mailing List [-- Attachment #1: Type: text/plain, Size: 1367 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 868 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: git does not wait on hook exes (^C) 2020-02-21 0:01 ` brian m. carlson @ 2020-02-21 0:12 ` Anthony Sottile 2020-02-21 1:11 ` brian m. carlson 0 siblings, 1 reply; 8+ messages in thread From: Anthony Sottile @ 2020-02-21 0:12 UTC (permalink / raw) To: brian m. carlson, Anthony Sottile, Git Mailing List On Thu, Feb 20, 2020 at 4:01 PM brian m. carlson <sandals@crustytoothpaste.net> wrote: > > 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 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) Anthony ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: git does not wait on hook exes (^C) 2020-02-21 0:12 ` Anthony Sottile @ 2020-02-21 1:11 ` brian m. carlson 2020-02-21 1:20 ` Anthony Sottile 0 siblings, 1 reply; 8+ messages in thread From: brian m. carlson @ 2020-02-21 1:11 UTC (permalink / raw) To: Anthony Sottile; +Cc: Git Mailing List [-- Attachment #1: Type: text/plain, Size: 1741 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 868 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: git does not wait on hook exes (^C) 2020-02-21 1:11 ` brian m. carlson @ 2020-02-21 1:20 ` Anthony Sottile 2020-02-21 2:25 ` brian m. carlson 0 siblings, 1 reply; 8+ messages in thread From: Anthony Sottile @ 2020-02-21 1:20 UTC (permalink / raw) To: brian m. carlson, Anthony Sottile, Git Mailing List On Thu, Feb 20, 2020 at 5:11 PM brian m. carlson <sandals@crustytoothpaste.net> wrote: > > 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). Anthony ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: git does not wait on hook exes (^C) 2020-02-21 1:20 ` Anthony Sottile @ 2020-02-21 2:25 ` brian m. carlson 2020-02-21 5:32 ` Jeff King 0 siblings, 1 reply; 8+ messages in thread From: brian m. carlson @ 2020-02-21 2:25 UTC (permalink / raw) To: Anthony Sottile; +Cc: Git Mailing List [-- Attachment #1: Type: text/plain, Size: 1052 bytes --] 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. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 868 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: git does not wait on hook exes (^C) 2020-02-21 2:25 ` brian m. carlson @ 2020-02-21 5:32 ` Jeff King 2020-02-21 6:08 ` Anthony Sottile 0 siblings, 1 reply; 8+ messages in thread From: Jeff King @ 2020-02-21 5:32 UTC (permalink / raw) To: brian m. carlson; +Cc: Anthony Sottile, Git Mailing List 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: git does not wait on hook exes (^C) 2020-02-21 5:32 ` Jeff King @ 2020-02-21 6:08 ` Anthony Sottile 0 siblings, 0 replies; 8+ messages in thread From: Anthony Sottile @ 2020-02-21 6:08 UTC (permalink / raw) To: Jeff King; +Cc: brian m. carlson, Git Mailing List On Thu, Feb 20, 2020 at 9:32 PM Jeff King <peff@peff.net> 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-02-21 6:08 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-20 21:14 git does not wait on hook exes (^C) Anthony Sottile 2020-02-21 0:01 ` brian m. carlson 2020-02-21 0:12 ` Anthony Sottile 2020-02-21 1:11 ` brian m. carlson 2020-02-21 1:20 ` Anthony Sottile 2020-02-21 2:25 ` brian m. carlson 2020-02-21 5:32 ` Jeff King 2020-02-21 6:08 ` Anthony Sottile
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).