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

Jeff King [off-list ref] writes:

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.

quoted

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.

quoted

 branch.c | 24 ++++++++++++++++--------
 branch.h |  3 ++-
 2 files changed, 18 insertions(+), 9 deletions(-)

The patch itself looks OK to me from a cursory read.

I agree with everything you said in the analysis, including that
this patch is a good first step in the right direction, but at the
same time it is a glorified no-op whose only external effect is to
make the error diagnosis a bit noisier.  A good first step in the
right direction with stride length of zero ;-)