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:12 UTC
Message-ID: <20090926211220.GA3387@coredump.intra.peff.net>
In-Reply-To: <3f4fd2640909261403n78a7e45cm3d2cd48408b5ff52@mail.gmail.com>

On Sat, Sep 26, 2009 at 10:03:27PM +0100, Reece Dunn wrote:

> > Now this is one that I do think is sensible. The variable isn't used, so
> > don't even bother declaring it.
> 
> The status variable is removed in this patch.

Yes. Sorry if I wasn't clear, but what I meant was "this does not fall under the same idioms as the other ones, and it is a fine thing to be removing".

> But then shouldn't the status returned be checked and acted on? That
> is, are failures from run_command_v_opt being reported to the user, or
> otherwise reacted to?

Perhaps. This is the post-update hook, so at that point we have already committed any changes to the repository. Usually it is used for running "git update-server-info" for repositories available over dumb protocols.

So there is no useful action for receive-pack to do after seeing an error. But I said "perhaps" above, because it might be useful to notify the user over the stderr sideband that the hook failed. Even though we have no action to take, the user might care or want to investigate a potential problem.

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.

> Thus having the same effect (removing the status variable). Callers of
> run_update_post_hook should be checked as well, as should other
> run_command_* calls.

There is exactly one caller, and it doesn't care about the return code for the reasons mentioned above.

-Peff