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

From: Reece Dunn <msclrhd@googlemail.com>
To: Jeff King <peff@peff.net>
Cc: Giuseppe Scrivano <gscrivano@gnu.org>,
	git@vger.kernel.org,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Sverre Rabbelier <srabbelier@gmail.com>
Subject: Re: [PATCH] Remove various dead assignments and dead increments found  by the clang static analyzer
Date: Sat, 26 Sep 2009 22:20:18 +0100	[thread overview]
Message-ID: <3f4fd2640909261420h2588df4cld8dd3e49f9654e9e@mail.gmail.com> (raw)
In-Reply-To: <20090926211220.GA3387@coredump.intra.peff.net>

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

Sure.

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

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.

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

Including being called from a script?

- Reece