Bug#469250: Commit f5bbc322 to git broke pre-commit hooks which read stdin

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

Hi,

On Tue, 4 Mar 2008, David Bremner wrote:

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

Junio C Hamano schrieb:

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

Johannes Sixt <j.sixt@viscovery.net> writes:

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.

    Junio> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

Hi,

On Tue, 4 Mar 2008, David Bremner 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?

Ciao, Dscho

Johannes Schindelin wrote:

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.

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

David Bremner <bremner@unb.ca> wrote:

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 O. Pearce" <spearce@spearce.org> writes:

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

Junio C Hamano <gitster@pobox.com> wrote:

Yup. That I think is key, because then hook authors know what they can assume, and what they cannot.

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.

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Never mind pre-commit. What about pre-receive and post-receive hooks, both of which gets data on stdin?

Hi,

On Tue, 4 Mar 2008, Jakub Narebski wrote:

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