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

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

[David Bremner]

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

Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin

[Johannes Schindelin]

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

Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin

[Junio C Hamano]

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

Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin

[Johannes Sixt]

Junio C Hamano schrieb:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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

Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> 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.

Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin

[David Bremner]

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

Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin

[Johannes Schindelin]

Hi,

On Tue, 4 Mar 2008, David Bremner wrote:

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

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

Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 736 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin

[David Bremner]

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

Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin

[Shawn O. Pearce]

David Bremner <bremner@unb.ca> wrote:
> >>>>> "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?  

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.

Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> 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).

Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> 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.

Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin

[Jakub Narebski]

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

Re: Commit f5bbc322 to git broke pre-commit hooks which read stdin

[Johannes Schindelin]

Hi,

On Tue, 4 Mar 2008, Jakub Narebski wrote:

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