Commit 0927156a24b1c66fdb5b0c9ab4e5889ebe0fd2b1
2019-10-24T12:32:11
config_file: refactor taking entries ref to return an error code The function to take a reference to the config file's config entries currently returns the reference via return value. Due to this, it's harder than necessary to integrate into our typical coding style, as one needs to make sure that a proper error code is set before erroring out from the caller. This bites us in `config_file_delete`, where we call `goto out` directly when `config_file_entries_take` returns `NULL`, but we actually forget to set up the error code and thus return success. Fix the issue by refactoring the function to return an error code and pass the reference via an out-pointer.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77
diff --git a/src/config_file.c b/src/config_file.c
index 60e2190..4d1bcde 100644
--- a/src/config_file.c
+++ b/src/config_file.c
@@ -64,21 +64,21 @@ static char *escape_value(const char *ptr);
* refcount. This is its own function to make sure we use the mutex to
* avoid the map pointer from changing under us.
*/
-static git_config_entries *config_file_entries_take(config_file_backend *b)
+static int config_file_entries_take(git_config_entries **out, config_file_backend *b)
{
- git_config_entries *entries;
+ int error;
- if (git_mutex_lock(&b->values_mutex) < 0) {
- git_error_set(GIT_ERROR_OS, "failed to lock config backend");
- return NULL;
+ if ((error = git_mutex_lock(&b->values_mutex)) < 0) {
+ git_error_set(GIT_ERROR_OS, "failed to lock config backend");
+ return error;
}
- entries = b->entries;
- git_config_entries_incref(entries);
+ git_config_entries_incref(b->entries);
+ *out = b->entries;
git_mutex_unlock(&b->values_mutex);
- return entries;
+ return 0;
}
static void config_file_clear(config_file *file)
@@ -280,8 +280,8 @@ static int config_file_set(git_config_backend *cfg, const char *name, const char
if ((error = git_config__normalize_name(name, &key)) < 0)
return error;
- if ((entries = config_file_entries_take(b)) == NULL)
- return -1;
+ if ((error = config_file_entries_take(&entries, b)) < 0)
+ return error;
/* Check whether we'd be modifying an included or multivar key */
if ((error = git_config_entries_get_unique(&existing, entries, key)) < 0) {
@@ -331,8 +331,8 @@ static int config_file_get(git_config_backend *cfg, const char *key, git_config_
if (!h->parent.readonly && ((error = config_file_refresh(cfg)) < 0))
return error;
- if ((entries = config_file_entries_take(h)) == NULL)
- return -1;
+ if ((error = config_file_entries_take(&entries, h)) < 0)
+ return error;
if ((error = (git_config_entries_get(&entry, entries, key))) < 0) {
git_config_entries_free(entries);
@@ -384,7 +384,7 @@ static int config_file_delete(git_config_backend *cfg, const char *name)
if ((error = git_config__normalize_name(name, &key)) < 0)
goto out;
- if ((entries = config_file_entries_take(b)) == NULL)
+ if ((error = config_file_entries_take(&entries, b)) < 0)
goto out;
/* Check whether we'd be modifying an included or multivar key */
@@ -415,10 +415,8 @@ static int config_file_delete_multivar(git_config_backend *cfg, const char *name
if ((result = git_config__normalize_name(name, &key)) < 0)
goto out;
- if ((entries = config_file_entries_take(b)) == NULL) {
- result = -1;
+ if ((result = config_file_entries_take(&entries, b)) < 0)
goto out;
- }
if ((result = git_config_entries_get(&entry, entries, key)) < 0) {
if (result == GIT_ENOTFOUND)