Jeff King <peff@xxxxxxxx> 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. > >> 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. 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 ;-) -- 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