Git push failure with update hook success

Git push failure with update hook success

[Bill Lear]

We seem to have a few permission problems using git through ssh that
we hopefully will resolve.  However, in the course of this, we have
noticed that our update hook runs with no errors, sends out a
confirmation email, but the push to our repo fails (we found that the
log file had improper permissions due to umask botchitude).

So, I surmised that the rough order of things is on our git company
repo is:

o receive the git "package" from the person who is pushing it
o call the update hook, telling it the package that is coming in
o the update hook examines things, forms an email, and sends it out
o the rest of the git machinery then actually applies the changes,
  logs them, etc., but fails (after the email is long gone), and
  on the remote (client) side, the push fails.
o the user gets a confirmation email that the push went ok

So, I was wondering if this is correct (more or less) and if so
whether it might be better to call the update hook after everything
had actually been written, including the log file.


Bill

Re: Git push failure with update hook success

[Bill Lear]

On Wednesday, March 7, 2007 at 10:29:29 (-0600) Bill Lear writes:
>...
>whether it might be better to call the update hook after everything
>had actually been written, including the log file.

I dug into the code: in receive-pack.c, the command 'update(struct
command *cmd)', calls write_ref_sha1() after run_update_hook()
and does not check the return code of write_ref_sha1().  write_ref_sha1()
will return an error if log_ref_write() fails, as it seems to in
our case, here:

		if (logfd < 0)
                        return error("Unable to append to %s: %s",
                                     log_file, strerror(errno));

On the client push side, we get:

error: Unable to append to logs/refs/heads/master: Permission denied


Bill

Re: Git push failure with update hook success

[Shawn O. Pearce]

Bill Lear <rael@zopyra.com> wrote:
> On Wednesday, March 7, 2007 at 10:29:29 (-0600) Bill Lear writes:
> >...
> >whether it might be better to call the update hook after everything
> >had actually been written, including the log file.
> 
> I dug into the code: in receive-pack.c, the command 'update(struct
> command *cmd)', calls write_ref_sha1() after run_update_hook()
> and does not check the return code of write_ref_sha1().

Please see the patch I just posted (and CC'd you on).  We should
have caught the return error of write_ref_sha1, the patch now
does that.  :)

You probably want to use the post-update hook to send email.
This hook will always run if it exists and is executable, but it
won't be given a ref that failed to be updated.

Of course an unfortunate downside to the post-update hook is it
does not receive the old SHA-1 of the ref; it just gets the ref name.

-- 
Shawn.

Re: Git push failure with update hook success

[Bill Lear]

On Wednesday, March 7, 2007 at 12:09:04 (-0500) Shawn O. Pearce writes:
>Bill Lear <rael@zopyra.com> wrote:
>> On Wednesday, March 7, 2007 at 10:29:29 (-0600) Bill Lear writes:
>> >...
>> >whether it might be better to call the update hook after everything
>> >had actually been written, including the log file.
>> 
>> I dug into the code: in receive-pack.c, the command 'update(struct
>> command *cmd)', calls write_ref_sha1() after run_update_hook()
>> and does not check the return code of write_ref_sha1().
>
>Please see the patch I just posted (and CC'd you on).  We should
>have caught the return error of write_ref_sha1, the patch now
>does that.  :)

Ok, thank you for fixing this.  I guess the call run_update_hook()
should not in fact come after the write_ref_sha1() block, and just
before the 'return 0' line, because you don't want to run the
write_ref_sha1() at all if the update hook complains.

>You probably want to use the post-update hook to send email.
>This hook will always run if it exists and is executable, but it
>won't be given a ref that failed to be updated.
>
>Of course an unfortunate downside to the post-update hook is it
>does not receive the old SHA-1 of the ref; it just gets the ref name.

Hmm, I agree that this sounds like the better place, logically
speaking, for the email report to be generated, but unfortunate since
I'm lame with git, so writing a post-update hook from scratch will
probably be beyond my abilities, but since I just watched "Touching
the Void" last night, I might be inspired to brave it.

Since it just gets the ref name, would one (of sufficient skill) be
able to reconstruct the same sort of report that the "pre" update hook
does?  That is, from the ref name can I get the old SHA-1?  If I try
to write this, what I think I would like to do is just call the
existing update hook from the post-update hook, with the post update
hook figuring out the proper arguments to pass along.

Also, I do notice in refs.c:create_symref() that it does not
check the return code of log_ref_write().  I don't know if that
is done explicitly or not ...


Bill

Re: Git push failure with update hook success

[Shawn O. Pearce]

Bill Lear <rael@zopyra.com> wrote:
> Ok, thank you for fixing this.  I guess the call run_update_hook()
> should not in fact come after the write_ref_sha1() block, and just
> before the 'return 0' line, because you don't want to run the
> write_ref_sha1() at all if the update hook complains.

Exactly.  :-)
 
> >You probably want to use the post-update hook to send email.
> >This hook will always run if it exists and is executable, but it
> >won't be given a ref that failed to be updated.
> >
> >Of course an unfortunate downside to the post-update hook is it
> >does not receive the old SHA-1 of the ref; it just gets the ref name.
> 
> Hmm, I agree that this sounds like the better place, logically
> speaking, for the email report to be generated, but unfortunate since
> I'm lame with git, so writing a post-update hook from scratch will
> probably be beyond my abilities, but since I just watched "Touching
> the Void" last night, I might be inspired to brave it.
>
> Since it just gets the ref name, would one (of sufficient skill) be
> able to reconstruct the same sort of report that the "pre" update hook
> does?  That is, from the ref name can I get the old SHA-1?  If I try
> to write this, what I think I would like to do is just call the
> existing update hook from the post-update hook, with the post update
> hook figuring out the proper arguments to pass along.

If you have a reflog enabled you can use name@{1}..name to generate
the list, but this is subject to a race condition as the ref could
be updated by someone else before you get to look at it to generate
the output.

I'm actually working on a patch right now to create a new hook
(hooks/post-receive ?) that takes the 3 arg form like hooks/update
does, avoiding the race condition entirely.
 
-- 
Shawn.

Re: Git push failure with update hook success

[Shawn O. Pearce]

Bill Lear <rael@zopyra.com> wrote:
> Since it just gets the ref name, would one (of sufficient skill) be
> able to reconstruct the same sort of report that the "pre" update hook
> does?  That is, from the ref name can I get the old SHA-1?  If I try
> to write this, what I think I would like to do is just call the
> existing update hook from the post-update hook, with the post update
> hook figuring out the proper arguments to pass along.

For what its worth, the receive-pack patch series that I posted a
few hours ago creates a new post-receive hook that would be suitable
for generating the report you are looking for.

-- 
Shawn.

Re: Git push failure with update hook success

[Bill Lear]

On Wednesday, March 7, 2007 at 18:09:48 (-0500) Shawn O. Pearce writes:
>Bill Lear <rael@zopyra.com> wrote:
>> Since it just gets the ref name, would one (of sufficient skill) be
>> able to reconstruct the same sort of report that the "pre" update hook
>> does?  That is, from the ref name can I get the old SHA-1?  If I try
>> to write this, what I think I would like to do is just call the
>> existing update hook from the post-update hook, with the post update
>> hook figuring out the proper arguments to pass along.
>
>For what its worth, the receive-pack patch series that I posted a
>few hours ago creates a new post-receive hook that would be suitable
>for generating the report you are looking for.

Ok, I'll have a look at them.  Thanks for the quick work.


Bill

Re: Git push failure with update hook success

[Andy Parkins]

On Wednesday 2007 March 07 17:25, Bill Lear wrote:

> Ok, thank you for fixing this.  I guess the call run_update_hook()
> should not in fact come after the write_ref_sha1() block, and just
> before the 'return 0' line, because you don't want to run the
> write_ref_sha1() at all if the update hook complains.

Yep.  The problem is definitely real, it's a bodge to have to do the email 
generation in the update hook.  When I started writing a new update hook, I 
wanted to put it in post-update but found I couldn't.

Here's my assumption for an email generator: you want every change of revision 
to generate an email; you want only the revisions added to make that change 
to appear on an email; you want every new revision to appear on /only/ one 
email.

A lot of the magic for making emails with useful information comes from the 
fact that the update hook is given three pieces of information:
 - ref being changed
 - current revision of ref
 - potential new revision of ref

The first job of the update hook is to see if this update is allowed.  The 
sample update hook defaults to rejecting un-annotated tags - generally 
un-annotated tags are ones that you wouldn't want to distribute to the whole 
project.  It also rejects updates to remote tracking branches - obviously a 
remote shouldn't be allowed to update your local tracking branches - that's 
done by your own git-fetch; not someone elses git-push (in fact a central 
repository probably shouldn't have tracking branches at all).  This is the 
bit that in an ideal world stay in the pre-update hook.

The second job (which is the one that really should go into post-update but 
can't) is to build a useful email.  The ref parameter tells us what sort of 
ref we're updating; branch updates (i.e. refs/heads/*) and annotated tags 
(i.e. refs/tags/*) updates are the most interesting.

So; you want to send an email summarising the changes that are being added to 
the repository.  You don't want to send the entire log for every change to 
the branch in question since the beginning of time, because that would just 
make ever increasing emails.  Therefore you need the old revision. e.g.

 * -- * --- O (oldrev)
             \
              N -- N -- N (newrev)

In this case, the new revisions are found with "git-rev-list $oldrev..
$newrev".  Easy.

More difficult is when you want to be able to cope when the ref update 
was --forced.  e.g.

  * -- * -- B -- O -- O -- O (oldrev)
             \
              N -- N -- N (newrev)

In this example, the ref update was not a fast forward, newrev points at the 
tip of a branch which does not contain oldrev.  Oh dear, "git-rev-list 
$oldrev..$newrev" wouldn't return just the "N" revisions.  To get around 
this, the hook makes the assumption that emails for oldrev will already have 
been issued (otherwise how did oldrev get into the repository :-)) and finds 
the base revision of the old and new, B.  Then the new commits are given 
by "git-rev-list $baserev..$newrev".

The final tricky thing to get around is the killer for why this email 
generation has to be done in the pre-update hook.  Merges.

 * -- O (master) -- A -- A -- A -- M
       \                          /
        B --- B ---- B --- B --- B (maint)

Let's imagine that the master branch is what is being pushed, so it is about 
to be updated from O to M.  Sometime earlier, the "B" branch referenced by 
maint has received updates as normal.

The new revisions that need an email are all of the "A" nodes and the "M" 
node.  We can't simply use "git rev-list O..M" as we have before because that 
would include all of the "B" branch.  We've already had emails for those 
revisions so we'd simply be making noise.  The solution then is to cheat a 
little and say "all the revisions from O to M, but not any that reachable by 
refs already in the repository".  In git-speak:

  git-rev-list $oldrev..$newrev --not --all

Or, close to how it's written in the update-hook

 git-rev-list $newrev --not $oldrev --all

The "--all" means all of "refs/", and the revisions reachable by them.  Now 
here's the killer.  If we did that in the post-update hook (even if all the 
other information were available), this wouldn't work because the master ref 
would have been updated and would already point at "M".  In that 
case "--not --all" would include the new revisions.

Without adding some nasty switches to git-rev-list 
(like --all-except-this-branch), I can't see how the post-update hook could 
ever send emails with the necessary amount of detail.


Andy
-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

Re: Git push failure with update hook success

[Shawn O. Pearce]

Andy Parkins <andyparkins@gmail.com> wrote:
> Without adding some nasty switches to git-rev-list 
> (like --all-except-this-branch), I can't see how the post-update hook could 
> ever send emails with the necessary amount of detail.

Which is why `master` now has support for a post-receive hook,
that has the magic three parameters.  ;-)

-- 
Shawn.

Re: Git push failure with update hook success

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Andy Parkins <andyparkins@gmail.com> wrote:
>> Without adding some nasty switches to git-rev-list 
>> (like --all-except-this-branch), I can't see how the post-update hook could 
>> ever send emails with the necessary amount of detail.
>
> Which is why `master` now has support for a post-receive hook,
> that has the magic three parameters.  ;-)

... but do not use that yet, as its public interface will change
to take the refs parameters in different ways (and the final
interface still under discussion).

Re: Git push failure with update hook success

[Andy Parkins]

On Thursday 2007 March 08 09:28, Shawn O. Pearce wrote:
> Andy Parkins <andyparkins@gmail.com> wrote:
> > Without adding some nasty switches to git-rev-list
> > (like --all-except-this-branch), I can't see how the post-update hook
> > could ever send emails with the necessary amount of detail.
>
> Which is why `master` now has support for a post-receive hook,
> that has the magic three parameters.  ;-)

It still won't work - what about the 

 git-rev-list --not --all

Problem?

By the time that hook runs the master ref (for example) has changed, 
so "--not --all" includes the new revisions, and you get no output.


Andy
-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

Re: Git push failure with update hook success

[Shawn O. Pearce]

Andy Parkins <andyparkins@gmail.com> wrote:
> On Thursday 2007 March 08 09:28, Shawn O. Pearce wrote:
> > Andy Parkins <andyparkins@gmail.com> wrote:
> > > Without adding some nasty switches to git-rev-list
> > > (like --all-except-this-branch), I can't see how the post-update hook
> > > could ever send emails with the necessary amount of detail.
> >
> > Which is why `master` now has support for a post-receive hook,
> > that has the magic three parameters.  ;-)
> 
> It still won't work - what about the 
> 
>  git-rev-list --not --all
> 
> Problem?
> 
> By the time that hook runs the master ref (for example) has changed, 
> so "--not --all" includes the new revisions, and you get no output.

Dammit.  That's so obvious.  And I missed it.

I got nothing at this hour of the morning.

-- 
Shawn.