{"thread":{"id":"7138","subject":"Git push failure with update hook success","startedAt":"2007-03-07T16:29:29Z","lastAt":"2007-03-08T10:06:55Z","messageCount":12,"participants":["Bill Lear","Shawn O. Pearce","Andy Parkins","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"36552","messageId":"17902.59497.831409.218529@lisa.zopyra.com","threadId":"7138","inReplyTo":null,"subject":"Git push failure with update hook success","fromName":"Bill Lear","fromEmail":"rael@zopyra.com","sentAt":"2007-03-07T16:29:29Z","receivedAt":"2007-03-07T16:29:29Z","isPatch":false,"sender":{"key":"rael@zopyra.com","avatar":"https://gravatar.com/avatar/c4f2d2790ca3828d3b4e7dfebabf61d2fe94fd82fa49cdac2a5295dd2d46a874?d=mp&s=160"},"body":"We seem to have a few permission problems using git through ssh that\nwe hopefully will resolve. However, in the course of this, we have\nnoticed that our update hook runs with no errors, sends out a\nconfirmation email, but the push to our repo fails (we found that the\nlog file had improper permissions due to umask botchitude).\n\nSo, I surmised that the rough order of things is on our git company\nrepo is:\n\no receive the git \"package\" from the person who is pushing it\no call the update hook, telling it the package that is coming in\no the update hook examines things, forms an email, and sends it out\no the rest of the git machinery then actually applies the changes,\n logs them, etc., but fails (after the email is long gone), and\n on the remote (client) side, the push fails.\no the user gets a confirmation email that the push went ok\n\nSo, I was wondering if this is correct (more or less) and if so\nwhether it might be better to call the update hook after everything\nhad actually been written, including the log file.\n\n\nBill\n"},{"id":"36554","messageId":"17902.60536.5890.608883@lisa.zopyra.com","threadId":"7138","inReplyTo":"17902.59497.831409.218529@lisa.zopyra.com","subject":"Re: Git push failure with update hook success","fromName":"Bill Lear","fromEmail":"rael@zopyra.com","sentAt":"2007-03-07T16:46:48Z","receivedAt":"2007-03-07T16:46:48Z","isPatch":false,"sender":{"key":"rael@zopyra.com","avatar":"https://gravatar.com/avatar/c4f2d2790ca3828d3b4e7dfebabf61d2fe94fd82fa49cdac2a5295dd2d46a874?d=mp&s=160"},"body":"On Wednesday, March 7, 2007 at 10:29:29 (-0600) Bill Lear writes:\n>...\n>whether it might be better to call the update hook after everything\n>had actually been written, including the log file.\n\nI dug into the code: in receive-pack.c, the command 'update(struct\ncommand *cmd)', calls write_ref_sha1() after run_update_hook()\nand does not check the return code of write_ref_sha1(). write_ref_sha1()\nwill return an error if log_ref_write() fails, as it seems to in\nour case, here:\n\n\t\tif (logfd < 0)\n return error(\"Unable to append to %s: %s\",\n log_file, strerror(errno));\n\nOn the client push side, we get:\n\nerror: Unable to append to logs/refs/heads/master: Permission denied\n\n\nBill\n"},{"id":"36557","messageId":"20070307170904.GB27922@spearce.org","threadId":"7138","inReplyTo":"17902.60536.5890.608883@lisa.zopyra.com","subject":"Re: Git push failure with update hook success","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-03-07T17:09:04Z","receivedAt":"2007-03-07T17:09:04Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Bill Lear wrote:\n> On Wednesday, March 7, 2007 at 10:29:29 (-0600) Bill Lear writes:\n> >...\n> >whether it might be better to call the update hook after everything\n> >had actually been written, including the log file.\n> \n> I dug into the code: in receive-pack.c, the command 'update(struct\n> command *cmd)', calls write_ref_sha1() after run_update_hook()\n> and does not check the return code of write_ref_sha1().\n\nPlease see the patch I just posted (and CC'd you on). We should\nhave caught the return error of write_ref_sha1, the patch now\ndoes that. :)\n\nYou probably want to use the post-update hook to send email.\nThis hook will always run if it exists and is executable, but it\nwon't be given a ref that failed to be updated.\n\nOf course an unfortunate downside to the post-update hook is it\ndoes not receive the old SHA-1 of the ref; it just gets the ref name.\n\n-- \nShawn.\n"},{"id":"36559","messageId":"17902.62836.920473.810183@lisa.zopyra.com","threadId":"7138","inReplyTo":"20070307170904.GB27922@spearce.org","subject":"Re: Git push failure with update hook success","fromName":"Bill Lear","fromEmail":"rael@zopyra.com","sentAt":"2007-03-07T17:25:08Z","receivedAt":"2007-03-07T17:25:08Z","isPatch":false,"sender":{"key":"rael@zopyra.com","avatar":"https://gravatar.com/avatar/c4f2d2790ca3828d3b4e7dfebabf61d2fe94fd82fa49cdac2a5295dd2d46a874?d=mp&s=160"},"body":"On Wednesday, March 7, 2007 at 12:09:04 (-0500) Shawn O. Pearce writes:\n>Bill Lear wrote:\n>> On Wednesday, March 7, 2007 at 10:29:29 (-0600) Bill Lear writes:\n>> >...\n>> >whether it might be better to call the update hook after everything\n>> >had actually been written, including the log file.\n>> \n>> I dug into the code: in receive-pack.c, the command 'update(struct\n>> command *cmd)', calls write_ref_sha1() after run_update_hook()\n>> and does not check the return code of write_ref_sha1().\n>\n>Please see the patch I just posted (and CC'd you on). We should\n>have caught the return error of write_ref_sha1, the patch now\n>does that. :)\n\nOk, thank you for fixing this. I guess the call run_update_hook()\nshould not in fact come after the write_ref_sha1() block, and just\nbefore the 'return 0' line, because you don't want to run the\nwrite_ref_sha1() at all if the update hook complains.\n\n>You probably want to use the post-update hook to send email.\n>This hook will always run if it exists and is executable, but it\n>won't be given a ref that failed to be updated.\n>\n>Of course an unfortunate downside to the post-update hook is it\n>does not receive the old SHA-1 of the ref; it just gets the ref name.\n\nHmm, I agree that this sounds like the better place, logically\nspeaking, for the email report to be generated, but unfortunate since\nI'm lame with git, so writing a post-update hook from scratch will\nprobably be beyond my abilities, but since I just watched \"Touching\nthe Void\" last night, I might be inspired to brave it.\n\nSince it just gets the ref name, would one (of sufficient skill) be\nable to reconstruct the same sort of report that the \"pre\" update hook\ndoes? That is, from the ref name can I get the old SHA-1? If I try\nto write this, what I think I would like to do is just call the\nexisting update hook from the post-update hook, with the post update\nhook figuring out the proper arguments to pass along.\n\nAlso, I do notice in refs.c:create_symref() that it does not\ncheck the return code of log_ref_write(). I don't know if that\nis done explicitly or not ...\n\n\nBill\n"},{"id":"36560","messageId":"20070307173804.GC27922@spearce.org","threadId":"7138","inReplyTo":"17902.62836.920473.810183@lisa.zopyra.com","subject":"Re: Git push failure with update hook success","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-03-07T17:38:04Z","receivedAt":"2007-03-07T17:38:04Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Bill Lear wrote:\n> Ok, thank you for fixing this. I guess the call run_update_hook()\n> should not in fact come after the write_ref_sha1() block, and just\n> before the 'return 0' line, because you don't want to run the\n> write_ref_sha1() at all if the update hook complains.\n\nExactly. :-)\n \n> >You probably want to use the post-update hook to send email.\n> >This hook will always run if it exists and is executable, but it\n> >won't be given a ref that failed to be updated.\n> >\n> >Of course an unfortunate downside to the post-update hook is it\n> >does not receive the old SHA-1 of the ref; it just gets the ref name.\n> \n> Hmm, I agree that this sounds like the better place, logically\n> speaking, for the email report to be generated, but unfortunate since\n> I'm lame with git, so writing a post-update hook from scratch will\n> probably be beyond my abilities, but since I just watched \"Touching\n> the Void\" last night, I might be inspired to brave it.\n>\n> Since it just gets the ref name, would one (of sufficient skill) be\n> able to reconstruct the same sort of report that the \"pre\" update hook\n> does? That is, from the ref name can I get the old SHA-1? If I try\n> to write this, what I think I would like to do is just call the\n> existing update hook from the post-update hook, with the post update\n> hook figuring out the proper arguments to pass along.\n\nIf you have a reflog enabled you can use name@{1}..name to generate\nthe list, but this is subject to a race condition as the ref could\nbe updated by someone else before you get to look at it to generate\nthe output.\n\nI'm actually working on a patch right now to create a new hook\n(hooks/post-receive ?) that takes the 3 arg form like hooks/update\ndoes, avoiding the race condition entirely.\n \n-- \nShawn.\n"},{"id":"36578","messageId":"20070307230948.GE27922@spearce.org","threadId":"7138","inReplyTo":"17902.62836.920473.810183@lisa.zopyra.com","subject":"Re: Git push failure with update hook success","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-03-07T23:09:48Z","receivedAt":"2007-03-07T23:09:48Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Bill Lear wrote:\n> Since it just gets the ref name, would one (of sufficient skill) be\n> able to reconstruct the same sort of report that the \"pre\" update hook\n> does? That is, from the ref name can I get the old SHA-1? If I try\n> to write this, what I think I would like to do is just call the\n> existing update hook from the post-update hook, with the post update\n> hook figuring out the proper arguments to pass along.\n\nFor what its worth, the receive-pack patch series that I posted a\nfew hours ago creates a new post-receive hook that would be suitable\nfor generating the report you are looking for.\n\n-- \nShawn.\n"},{"id":"36580","messageId":"17903.18303.797634.693414@lisa.zopyra.com","threadId":"7138","inReplyTo":"20070307230948.GE27922@spearce.org","subject":"Re: Git push failure with update hook success","fromName":"Bill Lear","fromEmail":"rael@zopyra.com","sentAt":"2007-03-07T23:15:11Z","receivedAt":"2007-03-07T23:15:11Z","isPatch":false,"sender":{"key":"rael@zopyra.com","avatar":"https://gravatar.com/avatar/c4f2d2790ca3828d3b4e7dfebabf61d2fe94fd82fa49cdac2a5295dd2d46a874?d=mp&s=160"},"body":"On Wednesday, March 7, 2007 at 18:09:48 (-0500) Shawn O. Pearce writes:\n>Bill Lear wrote:\n>> Since it just gets the ref name, would one (of sufficient skill) be\n>> able to reconstruct the same sort of report that the \"pre\" update hook\n>> does? That is, from the ref name can I get the old SHA-1? If I try\n>> to write this, what I think I would like to do is just call the\n>> existing update hook from the post-update hook, with the post update\n>> hook figuring out the proper arguments to pass along.\n>\n>For what its worth, the receive-pack patch series that I posted a\n>few hours ago creates a new post-receive hook that would be suitable\n>for generating the report you are looking for.\n\nOk, I'll have a look at them. Thanks for the quick work.\n\n\nBill\n"},{"id":"36602","messageId":"200703080922.54978.andyparkins@gmail.com","threadId":"7138","inReplyTo":"17902.62836.920473.810183@lisa.zopyra.com","subject":"Re: Git push failure with update hook success","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-03-08T09:22:53Z","receivedAt":"2007-03-08T09:22:53Z","isPatch":false,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Wednesday 2007 March 07 17:25, Bill Lear wrote:\n\n> Ok, thank you for fixing this. I guess the call run_update_hook()\n> should not in fact come after the write_ref_sha1() block, and just\n> before the 'return 0' line, because you don't want to run the\n> write_ref_sha1() at all if the update hook complains.\n\nYep. The problem is definitely real, it's a bodge to have to do the email \ngeneration in the update hook. When I started writing a new update hook, I \nwanted to put it in post-update but found I couldn't.\n\nHere's my assumption for an email generator: you want every change of revision \nto generate an email; you want only the revisions added to make that change \nto appear on an email; you want every new revision to appear on /only/ one \nemail.\n\nA lot of the magic for making emails with useful information comes from the \nfact that the update hook is given three pieces of information:\n - ref being changed\n - current revision of ref\n - potential new revision of ref\n\nThe first job of the update hook is to see if this update is allowed. The \nsample update hook defaults to rejecting un-annotated tags - generally \nun-annotated tags are ones that you wouldn't want to distribute to the whole \nproject. It also rejects updates to remote tracking branches - obviously a \nremote shouldn't be allowed to update your local tracking branches - that's \ndone by your own git-fetch; not someone elses git-push (in fact a central \nrepository probably shouldn't have tracking branches at all). This is the \nbit that in an ideal world stay in the pre-update hook.\n\nThe second job (which is the one that really should go into post-update but \ncan't) is to build a useful email. The ref parameter tells us what sort of \nref we're updating; branch updates (i.e. refs/heads/*) and annotated tags \n(i.e. refs/tags/*) updates are the most interesting.\n\nSo; you want to send an email summarising the changes that are being added to \nthe repository. You don't want to send the entire log for every change to \nthe branch in question since the beginning of time, because that would just \nmake ever increasing emails. Therefore you need the old revision. e.g.\n\n * -- * --- O (oldrev)\n \\\n N -- N -- N (newrev)\n\nIn this case, the new revisions are found with \"git-rev-list $oldrev..\n$newrev\". Easy.\n\nMore difficult is when you want to be able to cope when the ref update \nwas --forced. e.g.\n\n * -- * -- B -- O -- O -- O (oldrev)\n \\\n N -- N -- N (newrev)\n\nIn this example, the ref update was not a fast forward, newrev points at the \ntip of a branch which does not contain oldrev. Oh dear, \"git-rev-list \n$oldrev..$newrev\" wouldn't return just the \"N\" revisions. To get around \nthis, the hook makes the assumption that emails for oldrev will already have \nbeen issued (otherwise how did oldrev get into the repository :-)) and finds \nthe base revision of the old and new, B. Then the new commits are given \nby \"git-rev-list $baserev..$newrev\".\n\nThe final tricky thing to get around is the killer for why this email \ngeneration has to be done in the pre-update hook. Merges.\n\n * -- O (master) -- A -- A -- A -- M\n \\ /\n B --- B ---- B --- B --- B (maint)\n\nLet's imagine that the master branch is what is being pushed, so it is about \nto be updated from O to M. Sometime earlier, the \"B\" branch referenced by \nmaint has received updates as normal.\n\nThe new revisions that need an email are all of the \"A\" nodes and the \"M\" \nnode. We can't simply use \"git rev-list O..M\" as we have before because that \nwould include all of the \"B\" branch. We've already had emails for those \nrevisions so we'd simply be making noise. The solution then is to cheat a \nlittle and say \"all the revisions from O to M, but not any that reachable by \nrefs already in the repository\". In git-speak:\n\n git-rev-list $oldrev..$newrev --not --all\n\nOr, close to how it's written in the update-hook\n\n git-rev-list $newrev --not $oldrev --all\n\nThe \"--all\" means all of \"refs/\", and the revisions reachable by them. Now \nhere's the killer. If we did that in the post-update hook (even if all the \nother information were available), this wouldn't work because the master ref \nwould have been updated and would already point at \"M\". In that \ncase \"--not --all\" would include the new revisions.\n\nWithout adding some nasty switches to git-rev-list \n(like --all-except-this-branch), I can't see how the post-update hook could \never send emails with the necessary amount of detail.\n\n\nAndy\n-- \nDr Andy Parkins, M Eng (hons), MIET\nandyparkins@gmail.com\n"},{"id":"36603","messageId":"20070308092809.GE30289@spearce.org","threadId":"7138","inReplyTo":"200703080922.54978.andyparkins@gmail.com","subject":"Re: Git push failure with update hook success","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-03-08T09:28:09Z","receivedAt":"2007-03-08T09:28:09Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Andy Parkins wrote:\n> Without adding some nasty switches to git-rev-list \n> (like --all-except-this-branch), I can't see how the post-update hook could \n> ever send emails with the necessary amount of detail.\n\nWhich is why `master` now has support for a post-receive hook,\nthat has the magic three parameters. ;-)\n\n-- \nShawn.\n"},{"id":"36606","messageId":"7vvehc9i1x.fsf@assigned-by-dhcp.cox.net","threadId":"7138","inReplyTo":"20070308092809.GE30289@spearce.org","subject":"Re: Git push failure with update hook success","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-08T09:54:34Z","receivedAt":"2007-03-08T09:54:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Andy Parkins wrote:\n>> Without adding some nasty switches to git-rev-list \n>> (like --all-except-this-branch), I can't see how the post-update hook could \n>> ever send emails with the necessary amount of detail.\n>\n> Which is why `master` now has support for a post-receive hook,\n> that has the magic three parameters. ;-)\n\n... but do not use that yet, as its public interface will change\nto take the refs parameters in different ways (and the final\ninterface still under discussion).\n"},{"id":"36610","messageId":"200703081004.32666.andyparkins@gmail.com","threadId":"7138","inReplyTo":"20070308092809.GE30289@spearce.org","subject":"Re: Git push failure with update hook success","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-03-08T10:04:30Z","receivedAt":"2007-03-08T10:04:30Z","isPatch":false,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Thursday 2007 March 08 09:28, Shawn O. Pearce wrote:\n> Andy Parkins wrote:\n> > Without adding some nasty switches to git-rev-list\n> > (like --all-except-this-branch), I can't see how the post-update hook\n> > could ever send emails with the necessary amount of detail.\n>\n> Which is why `master` now has support for a post-receive hook,\n> that has the magic three parameters. ;-)\n\nIt still won't work - what about the \n\n git-rev-list --not --all\n\nProblem?\n\nBy the time that hook runs the master ref (for example) has changed, \nso \"--not --all\" includes the new revisions, and you get no output.\n\n\nAndy\n-- \nDr Andy Parkins, M Eng (hons), MIET\nandyparkins@gmail.com\n"},{"id":"36611","messageId":"20070308100655.GG30289@spearce.org","threadId":"7138","inReplyTo":"200703081004.32666.andyparkins@gmail.com","subject":"Re: Git push failure with update hook success","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-03-08T10:06:55Z","receivedAt":"2007-03-08T10:06:55Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Andy Parkins wrote:\n> On Thursday 2007 March 08 09:28, Shawn O. Pearce wrote:\n> > Andy Parkins wrote:\n> > > Without adding some nasty switches to git-rev-list\n> > > (like --all-except-this-branch), I can't see how the post-update hook\n> > > could ever send emails with the necessary amount of detail.\n> >\n> > Which is why `master` now has support for a post-receive hook,\n> > that has the magic three parameters. ;-)\n> \n> It still won't work - what about the \n> \n> git-rev-list --not --all\n> \n> Problem?\n> \n> By the time that hook runs the master ref (for example) has changed, \n> so \"--not --all\" includes the new revisions, and you get no output.\n\nDammit. That's so obvious. And I missed it.\n\nI got nothing at this hour of the morning.\n\n-- \nShawn.\n"}]}