{"thread":{"id":"12487","subject":"Bug#469250: Commit f5bbc322 to git broke pre-commit hooks which read stdin","startedAt":"2008-03-04T08:38:21Z","lastAt":"2008-03-05T05:46:46Z","messageCount":14,"participants":["David Bremner","Johannes Schindelin","Junio C Hamano","Johannes Sixt","Jakub Narebski","Joey Hess","Shawn O. Pearce"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"70910","messageId":"0tableanpe.wl%bremner@pivot.cs.unb.ca","threadId":"12487","inReplyTo":null,"subject":"Bug#469250: Commit f5bbc322 to git broke pre-commit hooks which read stdin","fromName":"David Bremner","fromEmail":"bremner@unb.ca","sentAt":"2008-03-04T08:38:21Z","receivedAt":"2008-03-04T08:38:21Z","isPatch":false,"sender":{"key":"bremner@unb.ca","avatar":null},"body":"\nIt looks like line 435 of builtin-commit.c disables stdin for hooks\n(with the disclaimer that I first looked at the git source ten minutes\nago).\n\n\t hook.no_stdin = 1\n\nI'm not sure if this was by design, but just so you know, this breaks\npeople's hooks. In particular the default metastore pre-commit hook\nno longer works. I didn't find anything relevant in the docs [1].\n\nDavid\n\n[1]: http://www.kernel.org/pub/software/scm/git/docs/hooks.html\n"},{"id":"70917","messageId":"alpine.LSU.1.00.0803041044120.22527@racer.site","threadId":"12487","inReplyTo":"0tableanpe.wl%bremner@pivot.cs.unb.ca","subject":"Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-03-04T10:45:12Z","receivedAt":"2008-03-04T10:45:12Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 4 Mar 2008, David Bremner wrote:\n\n> It looks like line 435 of builtin-commit.c disables stdin for hooks \n> (with the disclaimer that I first looked at the git source ten minutes \n> ago).\n> \n> \t hook.no_stdin = 1\n> \n> I'm not sure if this was by design, but just so you know, this breaks \n> people's hooks. In particular the default metastore pre-commit hook no \n> longer works. I didn't find anything relevant in the docs [1].\n\nPardon my ignorance, but what business has metastore reading stdin? There \nshould be nothing coming in, so the change you mentioned should be \ncorrect, and your hook relies on something it should not rely on.\n\nCiao,\nDscho\n\n"},{"id":"70919","messageId":"7vk5ki91zj.fsf@gitster.siamese.dyndns.org","threadId":"12487","inReplyTo":"alpine.LSU.1.00.0803041044120.22527@racer.site","subject":"Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-04T11:12:48Z","receivedAt":"2008-03-04T11:12:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi,\n>\n> On Tue, 4 Mar 2008, David Bremner wrote:\n>\n>> It looks like line 435 of builtin-commit.c disables stdin for hooks \n>> (with the disclaimer that I first looked at the git source ten minutes \n>> ago).\n>> \n>> \t hook.no_stdin = 1\n>> \n>> I'm not sure if this was by design, but just so you know, this breaks \n>> people's hooks. In particular the default metastore pre-commit hook no \n>> longer works. I didn't find anything relevant in the docs [1].\n>\n> Pardon my ignorance, but what business has metastore reading stdin? There \n> should be nothing coming in, so the change you mentioned should be \n> correct, and your hook relies on something it should not rely on.\n\nIt is not metastore. It is an interactive hook that reads from the user\nwho is sitting on the terminal and invoked the git-commit program.\n"},{"id":"70926","messageId":"47CD370A.3070404@viscovery.net","threadId":"12487","inReplyTo":"7vk5ki91zj.fsf@gitster.siamese.dyndns.org","subject":"Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-03-04T11:48:26Z","receivedAt":"2008-03-04T11:48:26Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Johannes Schindelin writes:\n>> On Tue, 4 Mar 2008, David Bremner wrote:\n>>\n>>> It looks like line 435 of builtin-commit.c disables stdin for hooks \n>>> (with the disclaimer that I first looked at the git source ten minutes \n>>> ago).\n>>>\n>>> \t hook.no_stdin = 1\n>>>\n>>> I'm not sure if this was by design, but just so you know, this breaks \n>>> people's hooks. In particular the default metastore pre-commit hook no \n>>> longer works. I didn't find anything relevant in the docs [1].\n>> Pardon my ignorance, but what business has metastore reading stdin? There \n>> should be nothing coming in, so the change you mentioned should be \n>> correct, and your hook relies on something it should not rely on.\n> \n> It is not metastore. It is an interactive hook that reads from the user\n> who is sitting on the terminal and invoked the git-commit program.\n\nAre you saying stdin should not be directed to /dev/null, or that an\ninteractive hook is required to do\n\n exec < /dev/tty || { echo 2>&1 \"not interactive\"; exit 1; }\n\nbefore it reads from stdin?\n\n-- Hannes\n\n"},{"id":"70934","messageId":"0t4pbmaew9.wl%bremner@pivot.cs.unb.ca","threadId":"12487","inReplyTo":"7vk5ki91zj.fsf@gitster.siamese.dyndns.org","subject":"Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin","fromName":"David Bremner","fromEmail":"bremner@unb.ca","sentAt":"2008-03-04T11:48:38Z","receivedAt":"2008-03-04T11:48:38Z","isPatch":false,"sender":{"key":"bremner@unb.ca","avatar":null},"body":">>>>> \"Junio\" == Junio C Hamano writes:\n\n Junio> Johannes Schindelin writes:\n\n >> Pardon my ignorance, but what business has metastore reading\n >> stdin? There should be nothing coming in, so the change you\n >> mentioned should be correct, and your hook relies on something\n >> it should not rely on.\n\n Junio> It is not metastore. It is an interactive hook that reads\n Junio> from the user who is sitting on the terminal and invoked\n Junio> the git-commit program.\n\nYeah, I should have been more explicit. The problem is a line \n\n read -N1 VAR\n\nThis used to work, and now it doesn't. Maybe there are good reasons\nfor change in git, I don't know. I'm just reporting that something\nbroke. And asking nicely that what hooks can and can not depend on be\ndocumented somewhere to avoid future problems of this type.\n\nAll the best,\n\nDavid\n\nP.S. I removed 469250@bugs.debian.org from CC; I'm not sure Debian BTS needs all of the discussion.\nFeel free to put it back if you disagree. \n\n\n"},{"id":"70928","messageId":"m3k5kiiu60.fsf@localhost.localdomain","threadId":"12487","inReplyTo":"alpine.LSU.1.00.0803041044120.22527@racer.site","subject":"Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-03-04T11:51:22Z","receivedAt":"2008-03-04T11:51:22Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Johannes Schindelin writes:\n> On Tue, 4 Mar 2008, David Bremner wrote:\n> \n>> It looks like line 435 of builtin-commit.c disables stdin for hooks \n>> (with the disclaimer that I first looked at the git source ten minutes \n>> ago).\n>> \n>> \t hook.no_stdin = 1\n>> \n>> I'm not sure if this was by design, but just so you know, this breaks \n>> people's hooks. In particular the default metastore pre-commit hook no \n>> longer works. I didn't find anything relevant in the docs [1].\n> \n> Pardon my ignorance, but what business has metastore reading stdin? There \n> should be nothing coming in, so the change you mentioned should be \n> correct, and your hook relies on something it should not rely on.\n\nNever mind pre-commit. What about pre-receive and post-receive hooks,\nboth of which gets data on stdin?\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"70930","messageId":"alpine.LSU.1.00.0803041202150.22527@racer.site","threadId":"12487","inReplyTo":"m3k5kiiu60.fsf@localhost.localdomain","subject":"Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-03-04T12:03:16Z","receivedAt":"2008-03-04T12:03:16Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 4 Mar 2008, Jakub Narebski wrote:\n\n> Johannes Schindelin writes:\n> > On Tue, 4 Mar 2008, David Bremner wrote:\n> > \n> >> It looks like line 435 of builtin-commit.c disables stdin for hooks \n> >> (with the disclaimer that I first looked at the git source ten minutes \n> >> ago).\n> >> \n> >> \t hook.no_stdin = 1\n> \n> Never mind pre-commit. What about pre-receive and post-receive hooks, \n> both of which gets data on stdin?\n\nHe was talking about builtin-commit.c. AFAIR there is no code to call \npre-receive or post-receive in that file. ;-)\n\nIOW the issue does not apply to the hooks you mentioned.\n\nCiao,\nDscho\n\n"},{"id":"70931","messageId":"alpine.LSU.1.00.0803041203320.22527@racer.site","threadId":"12487","inReplyTo":"0t4pbmaew9.wl%bremner@pivot.cs.unb.ca","subject":"Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-03-04T12:04:12Z","receivedAt":"2008-03-04T12:04:12Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 4 Mar 2008, David Bremner wrote:\n\n> >>>>> \"Junio\" == Junio C Hamano writes:\n> \n> Junio> Johannes Schindelin writes:\n> \n> >> Pardon my ignorance, but what business has metastore reading\n> >> stdin? There should be nothing coming in, so the change you\n> >> mentioned should be correct, and your hook relies on something\n> >> it should not rely on.\n> \n> Junio> It is not metastore. It is an interactive hook that reads\n> Junio> from the user who is sitting on the terminal and invoked\n> Junio> the git-commit program.\n> \n> Yeah, I should have been more explicit. The problem is a line \n> \n> read -N1 VAR\n\nCan you be even more explicit? IOW why does this have to be a pre-commit \nhook, and cannot be done before calling git-commit itself?\n\nCiao,\nDscho\n"},{"id":"70935","messageId":"7vbq5u7kf9.fsf@gitster.siamese.dyndns.org","threadId":"12487","inReplyTo":"47CD370A.3070404@viscovery.net","subject":"Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-04T12:17:30Z","receivedAt":"2008-03-04T12:17:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n>> It is not metastore. It is an interactive hook that reads from the user\n>> who is sitting on the terminal and invoked the git-commit program.\n>\n> Are you saying stdin should not be directed to /dev/null, or that an\n> interactive hook is required to do\n>\n> exec < /dev/tty || { echo 2>&1 \"not interactive\"; exit 1; }\n>\n> before it reads from stdin?\n\nI am saying that scripted version left the stdin as-is but somehow we\nended up spawning with .no_stdin = 1 in the C-rewrite, which is a change\nin established behaviour. It is often called a regression, unless the\nchange has a very good reason. And I tend to think this particular one\nfalls into the former.\n\nWe should audit how the hooks are called from various commands\nre-implemented, comparing the environment the scripted version used to\ngive them, which includes:\n\n - what directory the hook is run in;\n - what environment variables are exported to it;\n - what temporary files are visible to them for inspection;\n - in what order they are run;\n - which file descriptor is connected to what;\n\nI think we already caught some of the environment and ordering issues in\ncommit and checkout, but I am far from confident to say that what we have\nbehave identically to the scripted version.\n"},{"id":"70982","messageId":"20080304181625.GA30366@kodama.kitenet.net","threadId":"12487","inReplyTo":"alpine.LSU.1.00.0803041203320.22527@racer.site","subject":"Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2008-03-04T18:16:25Z","receivedAt":"2008-03-04T18:16:25Z","isPatch":false,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"Johannes Schindelin wrote:\n> Can you be even more explicit? IOW why does this have to be a pre-commit \n> hook, and cannot be done before calling git-commit itself?\n\nmetastore adds a .metadata file that contains information (permissions,\nowners, etc) not normally tracked with git. The pre-commit hook updates\nthe metadata just before committing, to ensure that the metadata\nincluded in the commit is consistent with the rest of the commit. You\ncould do it by hand, but that would be an extra step that would have to\nbe rememebered every time.\n\nThere's no particular reason for the pre-commit hook to interactively\nprompt before updating the metadata. I suspect it prompts only because\nit's an example.\n\n-- \nsee shy jo\n"},{"id":"71007","messageId":"0tr6eq87az.wl%bremner@pivot.cs.unb.ca","threadId":"12487","inReplyTo":"alpine.LSU.1.00.0803041203320.22527@racer.site","subject":"Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin","fromName":"David Bremner","fromEmail":"bremner@unb.ca","sentAt":"2008-03-04T22:15:32Z","receivedAt":"2008-03-04T22:15:32Z","isPatch":false,"sender":{"key":"bremner@unb.ca","avatar":null},"body":">>>>> \"Johannes\" == Johannes Schindelin writes:\n\n >> Tue, 4 Mar 2008, David Bremner wrote:\n >> \n >> Yeah, I should have been more explicit. The problem is a line\n >> \n >> read -N1 VAR\n\n Johannes> Can you be even more explicit? IOW why does this have\n Johannes> to be a pre-commit hook, and cannot be done before\n Johannes> calling git-commit itself?\n\nI have this feeling I don't really understand the question. Yes, in\nprinciple, whatever I am doing in a pre-commit hook could be done by\nhand first. I guess it is primarily a user interface issue. The goal\nis modify the behaviour of git-commit in a particular repository;\nisn't this the purpose of pre-commit hooks? \n\nAll the best,\n\nDavid\n\n"},{"id":"71026","messageId":"20080305051212.GZ8410@spearce.org","threadId":"12487","inReplyTo":"0tr6eq87az.wl%bremner@pivot.cs.unb.ca","subject":"Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-03-05T05:12:12Z","receivedAt":"2008-03-05T05:12:12Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"David Bremner wrote:\n> >>>>> \"Johannes\" == Johannes Schindelin writes:\n> \n> >> Tue, 4 Mar 2008, David Bremner wrote:\n> >> \n> >> Yeah, I should have been more explicit. The problem is a line\n> >> \n> >> read -N1 VAR\n> \n> Johannes> Can you be even more explicit? IOW why does this have\n> Johannes> to be a pre-commit hook, and cannot be done before\n> Johannes> calling git-commit itself?\n> \n> I have this feeling I don't really understand the question. Yes, in\n> principle, whatever I am doing in a pre-commit hook could be done by\n> hand first. I guess it is primarily a user interface issue. The goal\n> is modify the behaviour of git-commit in a particular repository;\n> isn't this the purpose of pre-commit hooks? \n\nWhat happens to such hooks under git-gui?\n\ngit-gui invokes the pre-commit hook with stdin coming off the stdin\nthat the wish process inherited when it was spawned. This may not\nbe the best way to interact with the end-user of that repository.\n\n-- \nShawn.\n"},{"id":"71029","messageId":"7vtzjl3f6d.fsf@gitster.siamese.dyndns.org","threadId":"12487","inReplyTo":"20080305051212.GZ8410@spearce.org","subject":"Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-05T05:36:42Z","receivedAt":"2008-03-05T05:36:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> What happens to such hooks under git-gui?\n>\n> git-gui invokes the pre-commit hook with stdin coming off the stdin\n> that the wish process inherited when it was spawned. This may not\n> be the best way to interact with the end-user of that repository.\n\nWell, if git-gui is designed to interoperate with CLI git (and I think\nthat is a sensible thing to aim for), we probably should revisit the list\nof hooks in hooks.txt and make sure we define the environment these hooks\nare invoked in precisely enough (this incidentally will help C rewrite\neffort to avoid regressing). Then, hooks that take input from and give\noutput to the user could be launched with I/O redirected to talk with wish\n(which I presume has a terminal lookalike widget you can embed in your\napplication).\n"},{"id":"71030","messageId":"20080305054646.GB8410@spearce.org","threadId":"12487","inReplyTo":"7vtzjl3f6d.fsf@gitster.siamese.dyndns.org","subject":"Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-03-05T05:46:46Z","receivedAt":"2008-03-05T05:46:46Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n> \n> > What happens to such hooks under git-gui?\n> >\n> > git-gui invokes the pre-commit hook with stdin coming off the stdin\n> > that the wish process inherited when it was spawned. This may not\n> > be the best way to interact with the end-user of that repository.\n> \n> Well, if git-gui is designed to interoperate with CLI git (and I think\n> that is a sensible thing to aim for), we probably should revisit the list\n> of hooks in hooks.txt and make sure we define the environment these hooks\n> are invoked in precisely enough (this incidentally will help C rewrite\n> effort to avoid regressing).\n\nYup. That I think is key, because then hook authors know what they\ncan assume, and what they cannot.\n\n> Then, hooks that take input from and give\n> output to the user could be launched with I/O redirected to talk with wish\n> (which I presume has a terminal lookalike widget you can embed in your\n> application).\n\nNope. In fact the one that I already have right now is not dealing\nwith the \\r in the progress meters from git correctly. Someone needs\nto revisit that code. Something's not right with it.\n\nOh, and the one that git-gui does have only handles output. It does\nnot permit input. Nor does it actually honor weird stty modes like\nsay noecho. Its most decidely *not* a tty.\n\n-- \nShawn.\n"}]}