On Thu, Sep 24, 2015 at 12:32:01PM +0200, Patrick Steinhardt wrote: > The function `install_branch_config`, which is used to set up > tracking branches, does not verify return codes of > `git_config_set`. Due to this we may incorrectly print that a > tracking branch has been set up when in fact we did not due to an > error. > > Fix this by checking the return value of `git_config_set` and > returning early in the case of an error. The > `install_branch_config` function has been changed to return an > error code that reflects whether it has been successful. I think it's nice to tell the user when something has gone wrong, so in that sense this is a good change. Though git_config_set does already print something, so the main improvement here is passing up the error code from install_branch_config. What happens to that error? I count 4 callers in the current code, and none of them currently looks at the return value. Your patch updates setup_tracking() to propagate the error, which is good. But that is called from exactly one place, create_branch(), which also ignores the outcome. :-/ I don't think we want to die() when the config fails. That might be the right thing for "branch --set-upstream-to", but probably not for "checkout -b". I think we need to look at each call site and see whether we should be propagating the error back. With the hope that we would ultimately affect the exit code of the command. In the case of branch creation, we are in a two-step procedure: create the branch, then set up its tracking config. We _could_ rollback the first step when the second step fails, but that is probably not worth the complication. > Actually, there are quite a few places where we don't check the > return values of git_config_set and related functions. I didn't > go through them yet, but might do so if you deem this to be of > value. I think more error-checking is better than less. But I do think it's worth plumbing through the error codes in each case. It's tempting to just die() when the operation fails, but as discussed above, that's not always the most appropriate thing. > branch.c | 24 ++++++++++++++++-------- > branch.h | 3 ++- > 2 files changed, 18 insertions(+), 9 deletions(-) The patch itself looks OK to me from a cursory read. -Peff -- 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