{"thread":{"id":"52853","subject":"git does not wait on hook exes (^C)","startedAt":"2020-02-20T21:15:08Z","lastAt":"2020-02-21T06:08:22Z","messageCount":8,"participants":["Anthony Sottile","brian m. carlson","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"392201","messageId":"CA+dzEB=jJEkuvcX5qDfeXS=mbJu_m=-yUsmHd_KrKAXC3YDe7w@mail.gmail.com","threadId":"52853","inReplyTo":null,"subject":"git does not wait on hook exes (^C)","fromName":"Anthony Sottile","fromEmail":"asottile@umich.edu","sentAt":"2020-02-20T21:14:55Z","receivedAt":"2020-02-20T21:15:08Z","isPatch":false,"sender":{"key":"asottile@umich.edu","avatar":"https://avatars.githubusercontent.com/u/1810591?v=4"},"body":"Here's a small example:\n\n```bash\n#!/usr/bin/env bash\nfoo() {\n    echo zzz part 2\n    sleep 1\n    echo exiting now\n}\ntrap foo SIGINT\n\necho zzz part 1\nsleep 10\n```\n\n```console\n$ git --version\ngit version 2.25.GIT\n$ git commit --allow-empty -m foo\nzzz part 1\n^Czzz part 2\n\n$ exiting now\n```\n\n- I pressed ^C during the first sleep\n- control was returned back to my terminal\n- the hook script was still running in the background\n\nAnthony\n"},{"id":"392225","messageId":"20200221000141.GA6462@camp.crustytoothpaste.net","threadId":"52853","inReplyTo":"CA+dzEB=jJEkuvcX5qDfeXS=mbJu_m=-yUsmHd_KrKAXC3YDe7w@mail.gmail.com","subject":"Re: git does not wait on hook exes (^C)","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-02-21T00:01:41Z","receivedAt":"2020-02-21T00:01:49Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-02-20 at 21:14:55, Anthony Sottile wrote:\n> Here's a small example:\n> \n> ```bash\n> #!/usr/bin/env bash\n> foo() {\n>     echo zzz part 2\n>     sleep 1\n>     echo exiting now\n> }\n> trap foo SIGINT\n> \n> echo zzz part 1\n> sleep 10\n> ```\n> \n> ```console\n> $ git --version\n> git version 2.25.GIT\n> $ git commit --allow-empty -m foo\n> zzz part 1\n> ^Czzz part 2\n> \n> $ exiting now\n> ```\n> \n> - I pressed ^C during the first sleep\n> - control was returned back to my terminal\n> - the hook script was still running in the background\n\nI believe the way that SIGINT works on a terminal is that it sends the\nsignal to all processes in the foreground process group.  So my guess of\nwhat's happening here is that Git and your script both get SIGINT, Git\ncleans up and exits quickly, leaving your script running.\n\nIf so, I'm not sure that Git can do much here.  If Git waited for the\nhook to exit, then a broken or hung hook would cause Git to hang\nindefinitely, which is not what the user intended when they pressed\nCtrl-C.  Usually what the user wants is an immediate return to the\nterminal in such a case, and I think most users would consider it a bug\nif Git were to wait for its children.\n\nCertainly I'm open to hearing other views on this, though.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"392227","messageId":"CA+dzEBkP==U7iS=6vS4=g7LYwh_cKzqbPApDraiYv7tiwPOVHA@mail.gmail.com","threadId":"52853","inReplyTo":"20200221000141.GA6462@camp.crustytoothpaste.net","subject":"Re: git does not wait on hook exes (^C)","fromName":"Anthony Sottile","fromEmail":"asottile@umich.edu","sentAt":"2020-02-21T00:12:18Z","receivedAt":"2020-02-21T00:12:30Z","isPatch":false,"sender":{"key":"asottile@umich.edu","avatar":"https://avatars.githubusercontent.com/u/1810591?v=4"},"body":"On Thu, Feb 20, 2020 at 4:01 PM brian m. carlson\n<sandals@crustytoothpaste.net> wrote:\n>\n> On 2020-02-20 at 21:14:55, Anthony Sottile wrote:\n> > Here's a small example:\n> >\n> > ```bash\n> > #!/usr/bin/env bash\n> > foo() {\n> >     echo zzz part 2\n> >     sleep 1\n> >     echo exiting now\n> > }\n> > trap foo SIGINT\n> >\n> > echo zzz part 1\n> > sleep 10\n> > ```\n> >\n> > ```console\n> > $ git --version\n> > git version 2.25.GIT\n> > $ git commit --allow-empty -m foo\n> > zzz part 1\n> > ^Czzz part 2\n> >\n> > $ exiting now\n> > ```\n> >\n> > - I pressed ^C during the first sleep\n> > - control was returned back to my terminal\n> > - the hook script was still running in the background\n>\n> I believe the way that SIGINT works on a terminal is that it sends the\n> signal to all processes in the foreground process group.  So my guess of\n> what's happening here is that Git and your script both get SIGINT, Git\n> cleans up and exits quickly, leaving your script running.\n>\n> If so, I'm not sure that Git can do much here.  If Git waited for the\n> hook to exit, then a broken or hung hook would cause Git to hang\n> indefinitely, which is not what the user intended when they pressed\n> Ctrl-C.  Usually what the user wants is an immediate return to the\n> terminal in such a case, and I think most users would consider it a bug\n> if Git were to wait for its children.\n>\n> Certainly I'm open to hearing other views on this, though.\n> --\n> brian m. carlson: Houston, Texas, US\n> OpenPGP: https://keybase.io/bk2204\n\n\nTaking git out of the situation:\n\nCreate a shell script:\n\n```bash\n#!/usr/bin/env bash\n.git/hooks/pre-commit\n```\n\n```console\n$ ./t.sh\nzzz part 1\n^Czzz part 2\nexiting\n$\n```\n\nthat works fine (and is the expected case for `subprocess` calls for\nexample in python)\n\nAnthony\n"},{"id":"392234","messageId":"20200221011138.GB6462@camp.crustytoothpaste.net","threadId":"52853","inReplyTo":"CA+dzEBkP==U7iS=6vS4=g7LYwh_cKzqbPApDraiYv7tiwPOVHA@mail.gmail.com","subject":"Re: git does not wait on hook exes (^C)","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-02-21T01:11:38Z","receivedAt":"2020-02-21T01:11:46Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-02-21 at 00:12:18, Anthony Sottile wrote:\n> On Thu, Feb 20, 2020 at 4:01 PM brian m. carlson\n> <sandals@crustytoothpaste.net> wrote:\n> > I believe the way that SIGINT works on a terminal is that it sends the\n> > signal to all processes in the foreground process group.  So my guess of\n> > what's happening here is that Git and your script both get SIGINT, Git\n> > cleans up and exits quickly, leaving your script running.\n> >\n> > If so, I'm not sure that Git can do much here.  If Git waited for the\n> > hook to exit, then a broken or hung hook would cause Git to hang\n> > indefinitely, which is not what the user intended when they pressed\n> > Ctrl-C.  Usually what the user wants is an immediate return to the\n> > terminal in such a case, and I think most users would consider it a bug\n> > if Git were to wait for its children.\n> \n> Taking git out of the situation:\n> \n> Create a shell script:\n> \n> ```bash\n> #!/usr/bin/env bash\n> .git/hooks/pre-commit\n> ```\n> \n> ```console\n> $ ./t.sh\n> zzz part 1\n> ^Czzz part 2\n> exiting\n> $\n> ```\n> \n> that works fine (and is the expected case for `subprocess` calls for\n> example in python)\n\nYeah, I think this is the case I was discussing up above, where the\nparent process waits for the subprocess to exit.  If I modify the foo\nfunction in your hook to also have a \"sleep 10\", then the parent process\nhangs until the child process exits, which again would mean that Git\nwould hang indefinitely if the hook hung.\n\nCan you maybe tell us a little more about your use case?  What are you\ndoing in your hook that makes this case come up?  Why is your hook\ntrapping SIGINT?\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"392235","messageId":"CA+dzEBk5D9x=sQnKDwAPHXOS_8gnA37X9sn+ccpfViEauiC5ww@mail.gmail.com","threadId":"52853","inReplyTo":"20200221011138.GB6462@camp.crustytoothpaste.net","subject":"Re: git does not wait on hook exes (^C)","fromName":"Anthony Sottile","fromEmail":"asottile@umich.edu","sentAt":"2020-02-21T01:20:51Z","receivedAt":"2020-02-21T01:21:08Z","isPatch":false,"sender":{"key":"asottile@umich.edu","avatar":"https://avatars.githubusercontent.com/u/1810591?v=4"},"body":"On Thu, Feb 20, 2020 at 5:11 PM brian m. carlson\n<sandals@crustytoothpaste.net> wrote:\n>\n> On 2020-02-21 at 00:12:18, Anthony Sottile wrote:\n> > On Thu, Feb 20, 2020 at 4:01 PM brian m. carlson\n> > <sandals@crustytoothpaste.net> wrote:\n> > > I believe the way that SIGINT works on a terminal is that it sends the\n> > > signal to all processes in the foreground process group.  So my guess of\n> > > what's happening here is that Git and your script both get SIGINT, Git\n> > > cleans up and exits quickly, leaving your script running.\n> > >\n> > > If so, I'm not sure that Git can do much here.  If Git waited for the\n> > > hook to exit, then a broken or hung hook would cause Git to hang\n> > > indefinitely, which is not what the user intended when they pressed\n> > > Ctrl-C.  Usually what the user wants is an immediate return to the\n> > > terminal in such a case, and I think most users would consider it a bug\n> > > if Git were to wait for its children.\n> >\n> > Taking git out of the situation:\n> >\n> > Create a shell script:\n> >\n> > ```bash\n> > #!/usr/bin/env bash\n> > .git/hooks/pre-commit\n> > ```\n> >\n> > ```console\n> > $ ./t.sh\n> > zzz part 1\n> > ^Czzz part 2\n> > exiting\n> > $\n> > ```\n> >\n> > that works fine (and is the expected case for `subprocess` calls for\n> > example in python)\n>\n> Yeah, I think this is the case I was discussing up above, where the\n> parent process waits for the subprocess to exit.  If I modify the foo\n> function in your hook to also have a \"sleep 10\", then the parent process\n> hangs until the child process exits, which again would mean that Git\n> would hang indefinitely if the hook hung.\n>\n> Can you maybe tell us a little more about your use case?  What are you\n> doing in your hook that makes this case come up?  Why is your hook\n> trapping SIGINT?\n> --\n> brian m. carlson: Houston, Texas, US\n> OpenPGP: https://keybase.io/bk2204\n\nMy hook in question is a python process: https://pre-commit.com\n\nIt doesn't really do all that much on SIGINT but prints \"(^C)\nInterrupted\" and offers a crash log when receiving ^C -- this races\nwith the git process terminating and causes terminal spew (sometimes\nwith pretty bad consequences with input breaking until `reset`\ndepending on which thing wins the tty reset race).\n\nAnthony\n"},{"id":"392241","messageId":"20200221022558.GD6462@camp.crustytoothpaste.net","threadId":"52853","inReplyTo":"CA+dzEBk5D9x=sQnKDwAPHXOS_8gnA37X9sn+ccpfViEauiC5ww@mail.gmail.com","subject":"Re: git does not wait on hook exes (^C)","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-02-21T02:25:58Z","receivedAt":"2020-02-21T02:26:08Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-02-21 at 01:20:51, Anthony Sottile wrote:\n> My hook in question is a python process: https://pre-commit.com\n> \n> It doesn't really do all that much on SIGINT but prints \"(^C)\n> Interrupted\" and offers a crash log when receiving ^C -- this races\n> with the git process terminating and causes terminal spew (sometimes\n> with pretty bad consequences with input breaking until `reset`\n> depending on which thing wins the tty reset race).\n\nThanks, this is helpful context.  I don't know that Git waiting for the\nprocess is going to fix the broken terminal state, although it will\nlikely fix the jumbled output.\n\nI'm not planning on writing a patch, but I think an interesting patch to\nsee might be if we called wait(2) in a loop in a SIGINT handler but\ndidn't reinstall the signal handler, which means that a second Ctrl-C\nwould kill Git.  I believe that's what certain other programs do, and\nthat might address many of the problems in both scenarios.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"392254","messageId":"20200221053221.GC1280313@coredump.intra.peff.net","threadId":"52853","inReplyTo":"20200221022558.GD6462@camp.crustytoothpaste.net","subject":"Re: git does not wait on hook exes (^C)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-21T05:32:21Z","receivedAt":"2020-02-21T05:32:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 21, 2020 at 02:25:58AM +0000, brian m. carlson wrote:\n\n> On 2020-02-21 at 01:20:51, Anthony Sottile wrote:\n> > My hook in question is a python process: https://pre-commit.com\n> > \n> > It doesn't really do all that much on SIGINT but prints \"(^C)\n> > Interrupted\" and offers a crash log when receiving ^C -- this races\n> > with the git process terminating and causes terminal spew (sometimes\n> > with pretty bad consequences with input breaking until `reset`\n> > depending on which thing wins the tty reset race).\n> \n> Thanks, this is helpful context.  I don't know that Git waiting for the\n> process is going to fix the broken terminal state, although it will\n> likely fix the jumbled output.\n> \n> I'm not planning on writing a patch, but I think an interesting patch to\n> see might be if we called wait(2) in a loop in a SIGINT handler but\n> didn't reinstall the signal handler, which means that a second Ctrl-C\n> would kill Git.  I believe that's what certain other programs do, and\n> that might address many of the problems in both scenarios.\n\nThe run-command struct has a clean_on_exit flag, as well as a\nwait_after_clean flag, that would do what you want: when we're killed by\nSIGINT, we'd pass the signal on to the child and then wait for to\nfinish.  That first step should generally be unnecessary for SIGINT\n(since as you noted, it will usually be delivered to the whole process\ngroup), but it shouldn't hurt.\n\nTo get the double-^C behavior, I think cleanup_children_on_signal()\nwould have to be reordered to pop the signal handler first before\ncalling cleanup_children().\n\nI'm not quite convinced that's all worth doing, or wouldn't have other\nunforeseen consequences. But if anybody is interested in experimenting,\nI think the patch would only be a few lines (set those flags when\nrunning hooks, and then that reordering).\n\n-Peff\n"},{"id":"392258","messageId":"CA+dzEBkGWdBm_=fbw97UmWg8DXtVrfGUFLDDrtmX-+bmkQaFjA@mail.gmail.com","threadId":"52853","inReplyTo":"20200221053221.GC1280313@coredump.intra.peff.net","subject":"Re: git does not wait on hook exes (^C)","fromName":"Anthony Sottile","fromEmail":"asottile@umich.edu","sentAt":"2020-02-21T06:08:08Z","receivedAt":"2020-02-21T06:08:22Z","isPatch":false,"sender":{"key":"asottile@umich.edu","avatar":"https://avatars.githubusercontent.com/u/1810591?v=4"},"body":"On Thu, Feb 20, 2020 at 9:32 PM Jeff King <peff@peff.net> wrote:\n>\n> On Fri, Feb 21, 2020 at 02:25:58AM +0000, brian m. carlson wrote:\n>\n> > On 2020-02-21 at 01:20:51, Anthony Sottile wrote:\n> > > My hook in question is a python process: https://pre-commit.com\n> > >\n> > > It doesn't really do all that much on SIGINT but prints \"(^C)\n> > > Interrupted\" and offers a crash log when receiving ^C -- this races\n> > > with the git process terminating and causes terminal spew (sometimes\n> > > with pretty bad consequences with input breaking until `reset`\n> > > depending on which thing wins the tty reset race).\n> >\n> > Thanks, this is helpful context.  I don't know that Git waiting for the\n> > process is going to fix the broken terminal state, although it will\n> > likely fix the jumbled output.\n> >\n> > I'm not planning on writing a patch, but I think an interesting patch to\n> > see might be if we called wait(2) in a loop in a SIGINT handler but\n> > didn't reinstall the signal handler, which means that a second Ctrl-C\n> > would kill Git.  I believe that's what certain other programs do, and\n> > that might address many of the problems in both scenarios.\n>\n> The run-command struct has a clean_on_exit flag, as well as a\n> wait_after_clean flag, that would do what you want: when we're killed by\n> SIGINT, we'd pass the signal on to the child and then wait for to\n> finish.  That first step should generally be unnecessary for SIGINT\n> (since as you noted, it will usually be delivered to the whole process\n> group), but it shouldn't hurt.\n>\n> To get the double-^C behavior, I think cleanup_children_on_signal()\n> would have to be reordered to pop the signal handler first before\n> calling cleanup_children().\n>\n> I'm not quite convinced that's all worth doing, or wouldn't have other\n> unforeseen consequences. But if anybody is interested in experimenting,\n> I think the patch would only be a few lines (set those flags when\n> running hooks, and then that reordering).\n>\n> -Peff\n\nThe small patch at least solves my issues and prevents the zombie\nprocesses so I've sent it to the mailing list -- thanks for the\npointer.  I had found `wait_after_clean` in my initial investigation\nbut had missed the `clean_on_exit` flag!\n\nAnthony\n"}]}