threads / discuss / 7138

Git push failure with update hook success

Subject: Git push failure with update hook success

## tl;dr

12 messages between Mar 7, 2007 and Mar 8, 2007.

replies: 11people: 4as markdown or json

Bill Lear· Mar 7, 2007, 16:29 UTC · lore

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
Bill Lear· Mar 7, 2007, 16:46 UTC · re: Bill Lear · lore

Re: Git push failure with update hook success

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
Shawn O. Pearce· Mar 7, 2007, 17:09 UTC · re: Bill Lear · lore

Re: Git push failure with update hook success

Bill Lear <rael@zopyra.com> wrote:
Show 8 quoted lines
> 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.
Bill Lear· Mar 7, 2007, 17:25 UTC · re: Shawn O. Pearce · lore

Re: Git push failure with update hook success

On Wednesday, March 7, 2007 at 12:09:04 (-0500) Shawn O. Pearce writes:
Show 13 quoted lines
>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.

Show 6 quoted lines
>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
Shawn O. Pearce· Mar 7, 2007, 17:38 UTC · re: Bill Lear · lore

Re: Git push failure with update hook success

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.  :-)
 
Show 19 quoted lines
> >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.
Shawn O. Pearce· Mar 7, 2007, 23:09 UTC · re: Bill Lear · lore

Re: Git push failure with update hook success

Bill Lear <rael@zopyra.com> wrote:
Show 6 quoted lines
> 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.
Bill Lear· Mar 7, 2007, 23:15 UTC · re: Shawn O. Pearce · lore

Re: Git push failure with update hook success

On Wednesday, March 7, 2007 at 18:09:48 (-0500) Shawn O. Pearce writes:
Show 11 quoted lines
>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
Andy Parkins· Mar 8, 2007, 09:22 UTC · re: Bill Lear · lore

Re: Git push failure with update hook success

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
Shawn O. Pearce· Mar 8, 2007, 09:28 UTC · re: Andy Parkins · lore

Re: Git push failure with update hook success

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.
Junio C Hamano· Mar 8, 2007, 09:54 UTC · re: Shawn O. Pearce · lore

Re: Git push failure with update hook success

"Shawn O. Pearce" <spearce@spearce.org> writes:
Show 7 quoted lines
> 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).

Andy Parkins· Mar 8, 2007, 10:04 UTC · re: Shawn O. Pearce · lore

Re: Git push failure with update hook success

On Thursday 2007 March 08 09:28, Shawn O. Pearce wrote:
Show 7 quoted lines
> 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
Shawn O. Pearce· Mar 8, 2007, 10:06 UTC · re: Andy Parkins · lore

Re: Git push failure with update hook success

Andy Parkins <andyparkins@gmail.com> wrote:
Show 17 quoted lines
> 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.

← back to recent threads