Re: [PATCH] branch: handle errors on setting tracking branches

[Jeff King <peff@peff.net>]

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