Bug#469250: Commit f5bbc322 to git broke pre-commit hooks which read stdin
Johannes Sixt <[EMAIL PROTECTED]> 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.
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
Bug#469250: Commit f5bbc322 to git broke pre-commit hooks which read stdin
Hi, On Tue, 4 Mar 2008, Jakub Narebski wrote: > Johannes Schindelin <[EMAIL PROTECTED]> 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 -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
Bug#469250: Commit f5bbc322 to git broke pre-commit hooks which read stdin
Johannes Schindelin <[EMAIL PROTECTED]> 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 -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
Bug#469250: Commit f5bbc322 to git broke pre-commit hooks which read stdin
Junio C Hamano schrieb:
> Johannes Schindelin <[EMAIL PROTECTED]> 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
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
Bug#469250: Commit f5bbc322 to git broke pre-commit hooks which read stdin
Johannes Schindelin <[EMAIL PROTECTED]> 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. -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
Bug#469250: Commit f5bbc322 to git broke pre-commit hooks which read stdin
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 -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
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 -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
