libgit2/libgit2 — pull request #1986 by arrbee (2013-12-05 05:55:15+00:00)
Title: Clean up various error handling situations

This PR has a few error handling improvements that I've been playing with. You folks may consider them to be too intrusive, but I figured I'd open it up and get the conversation started.

There are two main areas that I've worked on so far: creating internal APIs for looking up config values without a GIT_ENOTFOUND error, and reliably clearing the error when returning GIT_EUSER (paired with reliably preserving the error code when such a function is used internally).

----

For handling config value access, I added a new internal-only function:

```c
extern int git_config__lookup_entry(
const git_config_entry **out,
const git_config *cfg,
const char *key,
bool no_errors);
```

This looks up a specific key (no normalization is applied, so it must already be in normal form) and returns the found entry or NULL if it was not found. Not finding the key is not considered an error. There is an extra `no_errors` parameter that suppresses all errors completely - I'm not sure what core Git does in cases where there is config file corruption and an operation where accessing a config value is non-critical is performed.

There are also three helper functions to get a string, bool, and int from the config with a fallback value to use if the value is not found or if an error is encountered. These are named `git_config__get_string_force`, `git_config__get_bool_force`, and `git_config__get_int_force`. I'm not sure about those names, so opinions are welcome -- I'm was trying to imagine a `--force` flag being applied to config lookups.

In places where a known configuration setting is being looked up (i.e. the name of the config is not coming from user input) and a `GIT_ENOTFOUND` is to be ignored, I inserted this new function which has a streamlined path and leaves no error to be cleared.

----

For handling `GIT_EUSER` return values, I ended up adding three new error helper functions: `giterr_user_cancel()` which clears the error message and returns `GIT_EUSER`, `giterr_capture` which takes a snapshot of an error code and error message, and `giterr_restore` which restores the snapshot.

Everywhere that `GIT_EUSER` was being returned, I tried to replace with a return of `giterr_user_cancel()` (to ensure that `giterr_clear()` was called). This meant that several functions which were not previously reliably clearing the error message with an EUSER error now were - however, when these functions were used internally to the library, this could now result in an error code being returned without a message.

To resolve this, I tried to go through every place that used one of the `GIT_EUSER`-returning functions and if there was an error code and message that needed to be preserved, I would grab it using `giterr_capture` and then intercept the `GIT_EUSER` return value and restore it using `giterr_restore`. Using this, I think I ended up fixing a number of places that previously would have either returned `GIT_EUSER` from a function where the user would not have expected it to be returned or fixed places where `-1` would have been returned without an error message.

----

There are a variety of other unrelated error handling fixes that I put in as I was going through the code. There were a number of functions that were either not checking allocations or were replacing error return codes with `-1`. Those fixes are scattered around.

I also fixed an apparent bug in `git_branch_move` where the config was updated before the reference was altered. As a result, if the reference change encountered a conflict, the config settings would be trashed and not restored. I swapped the order in which the steps are executed to fix this and added some new tests to cover this case.

I also fixed an inefficiency in `reference_matches_remote_head` inside the clone code where once a match was found, we continued to iterate over all reference names (just immediately returning from the callback) instead of returning something like `GIT_ITEROVER` to stop the iteration.

----

Anyhow, hopefully this hasn't gone too far off the tracks and there is some value in it. There are a number of other fix ups I'd still like to do (such as dealing with the `GIT_ENOTFOUND` from looking up environment files -- i.e. the system, global, and XDG files) and there are definitely more places where we are not propagating error codes correctly (I saw several in the networking code). However, I'd prefer not to let this grow to be too large (particularly since it gets conflicts easily) and see if we can get something mergeable that will be an incremental improvement for some of the error issues.