# Bug#469250: Commit f5bbc322 to git broke pre-commit hooks which read stdin 14 messages from 2008-03-04 to 2008-03-05. Participants: David Bremner, Johannes Schindelin, Junio C Hamano, Johannes Sixt, Jakub Narebski, Joey Hess, Shawn O. Pearce. Thread: https://gitlist.dev/t/12487 ## David Bremner, 2008-03-04 08:38 Subject: Bug#469250: Commit f5bbc322 to git broke pre-commit hooks which read stdin Message-ID: <0tableanpe.wl%bremner@pivot.cs.unb.ca> URL: https://gitlist.dev/e/0tableanpe.wl%25bremner%40pivot.cs.unb.ca ``` It looks like line 435 of builtin-commit.c disables stdin for hooks (with the disclaimer that I first looked at the git source ten minutes ago). hook.no_stdin = 1 I'm not sure if this was by design, but just so you know, this breaks people's hooks. In particular the default metastore pre-commit hook no longer works. I didn't find anything relevant in the docs [1]. David [1]: http://www.kernel.org/pub/software/scm/git/docs/hooks.html ``` ## Johannes Schindelin, 2008-03-04 10:45 Subject: Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0803041044120.22527%40racer.site In-Reply-To: <0tableanpe.wl%bremner@pivot.cs.unb.ca> ``` Hi, On Tue, 4 Mar 2008, David Bremner wrote: > It looks like line 435 of builtin-commit.c disables stdin for hooks > (with the disclaimer that I first looked at the git source ten minutes > ago). > > hook.no_stdin = 1 > > I'm not sure if this was by design, but just so you know, this breaks > people's hooks. In particular the default metastore pre-commit hook no > longer works. I didn't find anything relevant in the docs [1]. Pardon my ignorance, but what business has metastore reading stdin? There should be nothing coming in, so the change you mentioned should be correct, and your hook relies on something it should not rely on. Ciao, Dscho ``` ## Junio C Hamano, 2008-03-04 11:12 Subject: Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin Message-ID: <7vk5ki91zj.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk5ki91zj.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin writes: > Hi, > > On Tue, 4 Mar 2008, David Bremner wrote: > >> It looks like line 435 of builtin-commit.c disables stdin for hooks >> (with the disclaimer that I first looked at the git source ten minutes >> ago). >> >> hook.no_stdin = 1 >> >> I'm not sure if this was by design, but just so you know, this breaks >> people's hooks. In particular the default metastore pre-commit hook no >> longer works. I didn't find anything relevant in the docs [1]. > > Pardon my ignorance, but what business has metastore reading stdin? There > should be nothing coming in, so the change you mentioned should be > correct, and your hook relies on something it should not rely on. It is not metastore. It is an interactive hook that reads from the user who is sitting on the terminal and invoked the git-commit program. ``` ## Johannes Sixt, 2008-03-04 11:48 Subject: Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin Message-ID: <47CD370A.3070404@viscovery.net> URL: https://gitlist.dev/e/47CD370A.3070404%40viscovery.net In-Reply-To: <7vk5ki91zj.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano schrieb: > Johannes Schindelin writes: >> On Tue, 4 Mar 2008, David Bremner wrote: >> >>> It looks like line 435 of builtin-commit.c disables stdin for hooks >>> (with the disclaimer that I first looked at the git source ten minutes >>> ago). >>> >>> hook.no_stdin = 1 >>> >>> I'm not sure if this was by design, but just so you know, this breaks >>> people's hooks. In particular the default metastore pre-commit hook no >>> longer works. I didn't find anything relevant in the docs [1]. >> Pardon my ignorance, but what business has metastore reading stdin? There >> should be nothing coming in, so the change you mentioned should be >> correct, and your hook relies on something it should not rely on. > > It is not metastore. It is an interactive hook that reads from the user > who is sitting on the terminal and invoked the git-commit program. Are you saying stdin should not be directed to /dev/null, or that an interactive hook is required to do exec < /dev/tty || { echo 2>&1 "not interactive"; exit 1; } before it reads from stdin? -- Hannes ``` ## David Bremner, 2008-03-04 11:48 Subject: Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin Message-ID: <0t4pbmaew9.wl%bremner@pivot.cs.unb.ca> URL: https://gitlist.dev/e/0t4pbmaew9.wl%25bremner%40pivot.cs.unb.ca In-Reply-To: <7vk5ki91zj.fsf@gitster.siamese.dyndns.org> ``` >>>>> "Junio" == Junio C Hamano writes: Junio> Johannes Schindelin writes: >> Pardon my ignorance, but what business has metastore reading >> stdin? There should be nothing coming in, so the change you >> mentioned should be correct, and your hook relies on something >> it should not rely on. Junio> It is not metastore. It is an interactive hook that reads Junio> from the user who is sitting on the terminal and invoked Junio> the git-commit program. Yeah, I should have been more explicit. The problem is a line read -N1 VAR This used to work, and now it doesn't. Maybe there are good reasons for change in git, I don't know. I'm just reporting that something broke. And asking nicely that what hooks can and can not depend on be documented somewhere to avoid future problems of this type. All the best, David P.S. I removed 469250@bugs.debian.org from CC; I'm not sure Debian BTS needs all of the discussion. Feel free to put it back if you disagree. ``` ## Jakub Narebski, 2008-03-04 11:51 Subject: Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin Message-ID: URL: https://gitlist.dev/e/m3k5kiiu60.fsf%40localhost.localdomain In-Reply-To: ``` Johannes Schindelin writes: > On Tue, 4 Mar 2008, David Bremner wrote: > >> It looks like line 435 of builtin-commit.c disables stdin for hooks >> (with the disclaimer that I first looked at the git source ten minutes >> ago). >> >> hook.no_stdin = 1 >> >> I'm not sure if this was by design, but just so you know, this breaks >> people's hooks. In particular the default metastore pre-commit hook no >> longer works. I didn't find anything relevant in the docs [1]. > > Pardon my ignorance, but what business has metastore reading stdin? There > should be nothing coming in, so the change you mentioned should be > correct, and your hook relies on something it should not rely on. Never mind pre-commit. What about pre-receive and post-receive hooks, both of which gets data on stdin? -- Jakub Narebski Poland ShadeHawk on #git ``` ## Johannes Schindelin, 2008-03-04 12:03 Subject: Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0803041202150.22527%40racer.site In-Reply-To: ``` Hi, On Tue, 4 Mar 2008, Jakub Narebski wrote: > Johannes Schindelin writes: > > On Tue, 4 Mar 2008, David Bremner wrote: > > > >> It looks like line 435 of builtin-commit.c disables stdin for hooks > >> (with the disclaimer that I first looked at the git source ten minutes > >> ago). > >> > >> hook.no_stdin = 1 > > Never mind pre-commit. What about pre-receive and post-receive hooks, > both of which gets data on stdin? He was talking about builtin-commit.c. AFAIR there is no code to call pre-receive or post-receive in that file. ;-) IOW the issue does not apply to the hooks you mentioned. Ciao, Dscho ``` ## Johannes Schindelin, 2008-03-04 12:04 Subject: Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0803041203320.22527%40racer.site In-Reply-To: <0t4pbmaew9.wl%bremner@pivot.cs.unb.ca> ``` Hi, On Tue, 4 Mar 2008, David Bremner wrote: > >>>>> "Junio" == Junio C Hamano writes: > > Junio> Johannes Schindelin writes: > > >> Pardon my ignorance, but what business has metastore reading > >> stdin? There should be nothing coming in, so the change you > >> mentioned should be correct, and your hook relies on something > >> it should not rely on. > > Junio> It is not metastore. It is an interactive hook that reads > Junio> from the user who is sitting on the terminal and invoked > Junio> the git-commit program. > > Yeah, I should have been more explicit. The problem is a line > > read -N1 VAR Can you be even more explicit? IOW why does this have to be a pre-commit hook, and cannot be done before calling git-commit itself? Ciao, Dscho ``` ## Junio C Hamano, 2008-03-04 12:17 Subject: Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin Message-ID: <7vbq5u7kf9.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vbq5u7kf9.fsf%40gitster.siamese.dyndns.org In-Reply-To: <47CD370A.3070404@viscovery.net> ``` Johannes Sixt writes: >> It is not metastore. It is an interactive hook that reads from the user >> who is sitting on the terminal and invoked the git-commit program. > > Are you saying stdin should not be directed to /dev/null, or that an > interactive hook is required to do > > exec < /dev/tty || { echo 2>&1 "not interactive"; exit 1; } > > before it reads from stdin? I am saying that scripted version left the stdin as-is but somehow we ended up spawning with .no_stdin = 1 in the C-rewrite, which is a change in established behaviour. It is often called a regression, unless the change has a very good reason. And I tend to think this particular one falls into the former. We should audit how the hooks are called from various commands re-implemented, comparing the environment the scripted version used to give them, which includes: - what directory the hook is run in; - what environment variables are exported to it; - what temporary files are visible to them for inspection; - in what order they are run; - which file descriptor is connected to what; I think we already caught some of the environment and ordering issues in commit and checkout, but I am far from confident to say that what we have behave identically to the scripted version. ``` ## Joey Hess, 2008-03-04 18:16 Subject: Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin Message-ID: <20080304181625.GA30366@kodama.kitenet.net> URL: https://gitlist.dev/e/20080304181625.GA30366%40kodama.kitenet.net In-Reply-To: ``` Johannes Schindelin wrote: > Can you be even more explicit? IOW why does this have to be a pre-commit > hook, and cannot be done before calling git-commit itself? metastore adds a .metadata file that contains information (permissions, owners, etc) not normally tracked with git. The pre-commit hook updates the metadata just before committing, to ensure that the metadata included in the commit is consistent with the rest of the commit. You could do it by hand, but that would be an extra step that would have to be rememebered every time. There's no particular reason for the pre-commit hook to interactively prompt before updating the metadata. I suspect it prompts only because it's an example. -- see shy jo ``` ## David Bremner, 2008-03-04 22:15 Subject: Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin Message-ID: <0tr6eq87az.wl%bremner@pivot.cs.unb.ca> URL: https://gitlist.dev/e/0tr6eq87az.wl%25bremner%40pivot.cs.unb.ca In-Reply-To: ``` >>>>> "Johannes" == Johannes Schindelin writes: >> Tue, 4 Mar 2008, David Bremner wrote: >> >> Yeah, I should have been more explicit. The problem is a line >> >> read -N1 VAR Johannes> Can you be even more explicit? IOW why does this have Johannes> to be a pre-commit hook, and cannot be done before Johannes> calling git-commit itself? I have this feeling I don't really understand the question. Yes, in principle, whatever I am doing in a pre-commit hook could be done by hand first. I guess it is primarily a user interface issue. The goal is modify the behaviour of git-commit in a particular repository; isn't this the purpose of pre-commit hooks? All the best, David ``` ## Shawn O. Pearce, 2008-03-05 05:12 Subject: Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin Message-ID: <20080305051212.GZ8410@spearce.org> URL: https://gitlist.dev/e/20080305051212.GZ8410%40spearce.org In-Reply-To: <0tr6eq87az.wl%bremner@pivot.cs.unb.ca> ``` David Bremner wrote: > >>>>> "Johannes" == Johannes Schindelin writes: > > >> Tue, 4 Mar 2008, David Bremner wrote: > >> > >> Yeah, I should have been more explicit. The problem is a line > >> > >> read -N1 VAR > > Johannes> Can you be even more explicit? IOW why does this have > Johannes> to be a pre-commit hook, and cannot be done before > Johannes> calling git-commit itself? > > I have this feeling I don't really understand the question. Yes, in > principle, whatever I am doing in a pre-commit hook could be done by > hand first. I guess it is primarily a user interface issue. The goal > is modify the behaviour of git-commit in a particular repository; > isn't this the purpose of pre-commit hooks? What happens to such hooks under git-gui? git-gui invokes the pre-commit hook with stdin coming off the stdin that the wish process inherited when it was spawned. This may not be the best way to interact with the end-user of that repository. -- Shawn. ``` ## Junio C Hamano, 2008-03-05 05:36 Subject: Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin Message-ID: <7vtzjl3f6d.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtzjl3f6d.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080305051212.GZ8410@spearce.org> ``` "Shawn O. Pearce" writes: > What happens to such hooks under git-gui? > > git-gui invokes the pre-commit hook with stdin coming off the stdin > that the wish process inherited when it was spawned. This may not > be the best way to interact with the end-user of that repository. Well, if git-gui is designed to interoperate with CLI git (and I think that is a sensible thing to aim for), we probably should revisit the list of hooks in hooks.txt and make sure we define the environment these hooks are invoked in precisely enough (this incidentally will help C rewrite effort to avoid regressing). Then, hooks that take input from and give output to the user could be launched with I/O redirected to talk with wish (which I presume has a terminal lookalike widget you can embed in your application). ``` ## Shawn O. Pearce, 2008-03-05 05:46 Subject: Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin Message-ID: <20080305054646.GB8410@spearce.org> URL: https://gitlist.dev/e/20080305054646.GB8410%40spearce.org In-Reply-To: <7vtzjl3f6d.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > "Shawn O. Pearce" writes: > > > What happens to such hooks under git-gui? > > > > git-gui invokes the pre-commit hook with stdin coming off the stdin > > that the wish process inherited when it was spawned. This may not > > be the best way to interact with the end-user of that repository. > > Well, if git-gui is designed to interoperate with CLI git (and I think > that is a sensible thing to aim for), we probably should revisit the list > of hooks in hooks.txt and make sure we define the environment these hooks > are invoked in precisely enough (this incidentally will help C rewrite > effort to avoid regressing). Yup. That I think is key, because then hook authors know what they can assume, and what they cannot. > Then, hooks that take input from and give > output to the user could be launched with I/O redirected to talk with wish > (which I presume has a terminal lookalike widget you can embed in your > application). Nope. In fact the one that I already have right now is not dealing with the \r in the progress meters from git correctly. Someone needs to revisit that code. Something's not right with it. Oh, and the one that git-gui does have only handles output. It does not permit input. Nor does it actually honor weird stty modes like say noecho. Its most decidely *not* a tty. -- Shawn. ```