Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

From: Jeff King <peff@peff.net>
Date: Sep 26, 2009, 21:36 UTC
Message-ID: <20090926213602.GA3756@coredump.intra.peff.net>
In-Reply-To: <3f4fd2640909261420h2588df4cld8dd3e49f9654e9e@mail.gmail.com>

On Sat, Sep 26, 2009 at 10:20:18PM +0100, Reece Dunn wrote:

Show 8 quoted lines

> > 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.

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.

> > There is exactly one caller, and it doesn't care about the return code
> > for the reasons mentioned above.
> 
> Including being called from a script?

I suppose people can be scripting around "git receive-pack" itself, though I find it pretty unlikely. I find it much more likely for them to script around "git push", which calls receive-pack on the remote end, and may or may not get the actual status (without checking, I imagine the exit code is lost anyway for git:// pushes, but probably passed back along for pushes over ssh).

At any rate, even if we assume that people are scripting around it, and that they can in fact see the exit status, I think we would want to keep it the same for compatibility reasons, as mentioned above.

-Peff