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

[Reece Dunn <msclrhd@googlemail.com>]

2009/9/26 Jeff King <peff@peff.net>:
> 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