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

--

[ message continues ]

Hi,


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

--

[ message continues ]

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

[ message continues ]

>>>>> "Junio" == Junio C Hamano <gitster@pobox.com> writes:

    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. 


--

[ message continues ]

Hi,


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

[ message continues ]

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.

--=20
see shy jo
[ message continues ]

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

--

[ message continues ]

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

[ message continues ]

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

[ message continues ]

Yup.  That I think is key, because then hook authors know what they

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

[ message continues ]

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

--

[ message continues ]

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

[ message continues ]

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

[ message continues ]

Hi,


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

--

[ message continues ]