From: Reece Dunn Date: Sat, 26 Sep 2009 21:46:06 GMT Subject: Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer Message-ID: <3f4fd2640909261446t412d0c26mcee27535be2b8954@mail.gmail.com> In-Reply-To: <20090926213602.GA3756@coredump.intra.peff.net> 2009/9/26 Jeff King : > On Sat, Sep 26, 2009 at 10:20:18PM +0100, Reece Dunn wrote: > >> > I suspect nobody has cared about this before, though, because the stderr >> > channel for the hook is also directed to the user. So if >> > update-server-info (or whatever) fails, presumably it is complaining to >> > stderr and the user sees that. Adding an additional "by the way, your >> > hook failed" is just going to be noise in most cases. >> >> It could be used to return an error status from main if it is used in >> a chained command in a script. Other than that, I agree. > > I'm not sure that's a good idea. Your push _did_ happen, and the remote > repo was updated. So you have no way of knowing from an error exit code > that changes were in fact made, and it was simply the post-update hook > failing. Ok. > Of course, you can argue that the current behavior is similarly broken: > on success, you have no idea if the post-update hook failed or not. But > I would argue that whether the push itself happened is more important > than whether the hook succeeded or not. If you really care, you should > either: > >  1. Use some sort of side channel to report hook status. > >  2. Use the pre-receive hook, which can abort the push if it wants to. > > But all of that is "if we were designing this hook from scratch". At > this point, it doesn't make sense to change the semantics. People may be > relying on the current behavior, and in fact it is documented (in > githooks(5)): > >  This hook is meant primarily for notification, and cannot >  affect the outcome of git-receive-pack. That's fine. As long as the behaviour is documented (which as you pointed out, it is). - Reece