2009/9/26 Jeff King <peff@xxxxxxxx>: > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html