From 36913b8cb497487728ee9bab383d4c1205685927 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 6 Mar 2014 15:11:11 +0100 Subject: [PATCH 01/13] config: document current write behaviour in a test On set, we set/add the value written to the config's internal values, but we do not refresh old values. Document this in a test in preparation for the refresh changes. --- tests/config/write.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/config/write.c b/tests/config/write.c index 922d75557..f269c9571 100644 --- a/tests/config/write.c +++ b/tests/config/write.c @@ -304,3 +304,30 @@ void test_config_write__updating_a_locked_config_file_returns_ELOCKED(void) git_config_free(cfg); } +void test_config_write__outside_change(void) +{ + int32_t tmp; + git_config *cfg; + const char *filename = "config-ext-change"; + + cl_git_mkfile(filename, "[old]\nvalue = 5\n"); + + cl_git_pass(git_config_open_ondisk(&cfg, filename)); + + cl_git_pass(git_config_get_int32(&tmp, cfg, "old.value")); + + /* Change the value on the file itself (simulate external process) */ + cl_git_mkfile(filename, "[old]\nvalue = 6\n"); + + cl_git_pass(git_config_set_int32(cfg, "new.value", 7)); + + cl_git_pass(git_config_get_int32(&tmp, cfg, "old.value")); + cl_assert_equal_i(5, tmp); + + cl_git_pass(git_config_refresh(cfg)); + + cl_git_pass(git_config_get_int32(&tmp, cfg, "old.value")); + cl_assert_equal_i(6, tmp); + + git_config_free(cfg); +} From 55ebd7d369a789f27fe1ad6b8ec8965aa1335d08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 13 Mar 2014 17:11:34 +0100 Subject: [PATCH 02/13] config: implement config snapshotting In order to have consistent views of the config files for remotes, submodules et al. and a configuration that represents what is currently stored on-disk, we need a way to provide a view of the configuration that does not change. The goal here is to provide the snapshotting part by creating a read-only copy of the state of the configuration at a particular point in time, which does not change when a repository's main config changes. --- include/git2/config.h | 13 ++ include/git2/sys/config.h | 2 + src/config.c | 32 ++++ src/config_file.c | 332 ++++++++++++++++++++++++++++++-------- tests/config/snapshot.c | 69 ++++++++ 5 files changed, 380 insertions(+), 68 deletions(-) create mode 100644 tests/config/snapshot.c diff --git a/include/git2/config.h b/include/git2/config.h index 663b4f6ba..86c4012ed 100644 --- a/include/git2/config.h +++ b/include/git2/config.h @@ -226,6 +226,19 @@ GIT_EXTERN(int) git_config_open_level( */ GIT_EXTERN(int) git_config_open_global(git_config **out, git_config *config); +/** + * Create a snapshot of the configuration + * + * Create a snapshot of the current state of a configuration, which + * allows you to look into a consistent view of the configuration for + * looking up complex values (e.g. a remote, submodule). + * + * @param out pointer in which to store the snapshot config object + * @param config configuration to snapshot + * @return 0 or an error code + */ +GIT_EXTERN(int) git_config_snapshot(git_config **out, git_config *config); + /** * Reload changed config files diff --git a/include/git2/sys/config.h b/include/git2/sys/config.h index 3df2ba327..090588999 100644 --- a/include/git2/sys/config.h +++ b/include/git2/sys/config.h @@ -64,6 +64,8 @@ struct git_config_backend { int (*del_multivar)(struct git_config_backend *, const char *key, const char *regexp); int (*iterator)(git_config_iterator **, struct git_config_backend *); int (*refresh)(struct git_config_backend *); + /** Produce a read-only version of this backend */ + int (*snapshot)(struct git_config_backend **, struct git_config_backend *); void (*free)(struct git_config_backend *); }; #define GIT_CONFIG_BACKEND_VERSION 1 diff --git a/src/config.c b/src/config.c index b3168f735..01a1f7d33 100644 --- a/src/config.c +++ b/src/config.c @@ -137,6 +137,38 @@ int git_config_open_ondisk(git_config **out, const char *path) return error; } +int git_config_snapshot(git_config **out, git_config *in) +{ + int error; + size_t i; + file_internal *internal; + git_config *config; + + *out = NULL; + + if (git_config_new(&config) < 0) + return -1; + + git_vector_foreach(&in->files, i, internal) { + git_config_backend *b; + + if ((error = internal->file->snapshot(&b, internal->file)) < 0) + goto on_error; + + if ((error = git_config_add_backend(config, b, internal->level, 0)) < 0) { + b->free(b); + goto on_error; + } + } + + *out = config; + return error; + +on_error: + git_config_free(config); + return error; +} + static int find_internal_file_by_level( file_internal **internal_out, const git_config *cfg, diff --git a/src/config_file.c b/src/config_file.c index bb26aa8a3..fcf9cab04 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -88,27 +88,44 @@ struct reader { typedef struct { git_config_backend parent; - git_strmap *values; +} diskfile_header; + +typedef struct { + diskfile_header header; + + git_config_level_t level; git_array_t(struct reader) readers; char *file_path; - - git_config_level_t level; } diskfile_backend; +typedef struct { + diskfile_header header; + + diskfile_backend *snapshot_from; +} diskfile_readonly_backend; + static int config_parse(diskfile_backend *cfg_file, struct reader *reader, git_config_level_t level, int depth); static int parse_variable(struct reader *reader, char **var_name, char **var_value); static int config_write(diskfile_backend *cfg, const char *key, const regex_t *preg, const char *value); static char *escape_value(const char *ptr); +int git_config_file__snapshot(git_config_backend **out, diskfile_backend *in); + static void set_parse_error(struct reader *reader, int col, const char *error_str) { giterr_set(GITERR_CONFIG, "Failed to parse config file: %s (in %s:%d, column %d)", error_str, reader->file_path, reader->line_number, col); } +static int config_error_readonly(void) +{ + giterr_set(GITERR_CONFIG, "this backend is read-only"); + return -1; +} + static void cvar_free(cvar_t *var) { if (var == NULL) @@ -155,6 +172,30 @@ int git_config_file_normalize_section(char *start, char *end) return 0; } +/* Add or append the new config option */ +static int append_entry(git_strmap *values, cvar_t *var) +{ + git_strmap_iter pos; + cvar_t *existing; + int error = 0; + + pos = git_strmap_lookup_index(values, var->entry->name); + if (!git_strmap_valid_index(values, pos)) { + git_strmap_insert(values, var->entry->name, var, error); + } else { + existing = git_strmap_value_at(values, pos); + while (existing->next != NULL) { + existing = existing->next; + } + existing->next = var; + } + + if (error > 0) + error = 0; + + return error; +} + static void free_vars(git_strmap *values) { cvar_t *var = NULL; @@ -180,13 +221,13 @@ static int config_open(git_config_backend *cfg, git_config_level_t level) b->level = level; - if ((res = git_strmap_alloc(&b->values)) < 0) + if ((res = git_strmap_alloc(&b->header.values)) < 0) return res; git_array_init(b->readers); reader = git_array_alloc(b->readers); if (!reader) { - git_strmap_free(b->values); + git_strmap_free(b->header.values); return -1; } memset(reader, 0, sizeof(struct reader)); @@ -203,8 +244,8 @@ static int config_open(git_config_backend *cfg, git_config_level_t level) return 0; if (res < 0 || (res = config_parse(b, reader, level, 0)) < 0) { - free_vars(b->values); - b->values = NULL; + free_vars(b->header.values); + b->header.values = NULL; } reader = git_array_get(b->readers, 0); @@ -239,16 +280,21 @@ static int config_refresh(git_config_backend *cfg) return (res == GIT_ENOTFOUND) ? 0 : res; /* need to reload - store old values and prep for reload */ - old_values = b->values; - if ((res = git_strmap_alloc(&b->values)) < 0) { - b->values = old_values; - } else if ((res = config_parse(b, reader, b->level, 0)) < 0) { - free_vars(b->values); - b->values = old_values; - } else { - free_vars(old_values); + old_values = b->header.values; + if ((res = git_strmap_alloc(&b->header.values)) < 0) { + b->header.values = old_values; + goto cleanup; } + if ((res = config_parse(b, reader, b->level, 0)) < 0) { + free_vars(b->header.values); + b->header.values = old_values; + goto cleanup; + } + + free_vars(old_values); + +cleanup: git_buf_free(&reader->buffer); return res; } @@ -268,7 +314,7 @@ static void backend_free(git_config_backend *_backend) git_array_clear(backend->readers); git__free(backend->file_path); - free_vars(backend->values); + free_vars(backend->header.values); git__free(backend); } @@ -283,12 +329,13 @@ static int config_iterator_next( git_config_iterator *iter) { git_config_file_iter *it = (git_config_file_iter *) iter; - diskfile_backend *b = (diskfile_backend *) it->parent.backend; + diskfile_header *h = (diskfile_header *) it->parent.backend; + git_strmap *values = h->values; int err = 0; cvar_t * var; if (it->next_var == NULL) { - err = git_strmap_next((void**) &var, &(it->iter), b->values); + err = git_strmap_next((void**) &var, &(it->iter), values); } else { var = it->next_var; } @@ -308,15 +355,16 @@ static int config_iterator_new( git_config_iterator **iter, struct git_config_backend* backend) { - diskfile_backend *b = (diskfile_backend *)backend; + diskfile_header *h = (diskfile_header *)backend; git_config_file_iter *it = git__calloc(1, sizeof(git_config_file_iter)); - GIT_UNUSED(b); - GITERR_CHECK_ALLOC(it); + /* strmap_begin() is currently a macro returning 0 */ + GIT_UNUSED(h); + it->parent.backend = backend; - it->iter = git_strmap_begin(b->values); + it->iter = git_strmap_begin(h->values); it->next_var = NULL; it->parent.next = config_iterator_next; @@ -330,6 +378,7 @@ static int config_set(git_config_backend *cfg, const char *name, const char *val { cvar_t *var = NULL, *old_var = NULL; diskfile_backend *b = (diskfile_backend *)cfg; + git_strmap *values = b->header.values; char *key, *esc_value = NULL; khiter_t pos; int rval, ret; @@ -341,9 +390,9 @@ static int config_set(git_config_backend *cfg, const char *name, const char *val * Try to find it in the existing values and update it if it * only has one value. */ - pos = git_strmap_lookup_index(b->values, key); - if (git_strmap_valid_index(b->values, pos)) { - cvar_t *existing = git_strmap_value_at(b->values, pos); + pos = git_strmap_lookup_index(values, key); + if (git_strmap_valid_index(values, pos)) { + cvar_t *existing = git_strmap_value_at(values, pos); char *tmp = NULL; git__free(key); @@ -398,7 +447,7 @@ static int config_set(git_config_backend *cfg, const char *name, const char *val } git__free(esc_value); - git_strmap_insert2(b->values, key, var, old_var, rval); + git_strmap_insert2(values, key, var, old_var, rval); if (rval < 0) return -1; if (old_var != NULL) @@ -412,15 +461,16 @@ static int config_set(git_config_backend *cfg, const char *name, const char *val */ static int config_get(const git_config_backend *cfg, const char *key, const git_config_entry **out) { - diskfile_backend *b = (diskfile_backend *)cfg; - khiter_t pos = git_strmap_lookup_index(b->values, key); + diskfile_header *h = (diskfile_header *)cfg; + git_strmap *values = h->values; + khiter_t pos = git_strmap_lookup_index(values, key); cvar_t *var; /* no error message; the config system will write one */ - if (!git_strmap_valid_index(b->values, pos)) + if (!git_strmap_valid_index(values, pos)) return GIT_ENOTFOUND; - var = git_strmap_value_at(b->values, pos); + var = git_strmap_value_at(values, pos); while (var->next) var = var->next; @@ -434,6 +484,7 @@ static int config_set_multivar( int replaced = 0; cvar_t *var, *newvar; diskfile_backend *b = (diskfile_backend *)cfg; + git_strmap *values = b->header.values; char *key; regex_t preg; int result; @@ -444,15 +495,15 @@ static int config_set_multivar( if ((result = git_config__normalize_name(name, &key)) < 0) return result; - pos = git_strmap_lookup_index(b->values, key); - if (!git_strmap_valid_index(b->values, pos)) { + pos = git_strmap_lookup_index(values, key); + if (!git_strmap_valid_index(values, pos)) { /* If we don't have it, behave like a normal set */ result = config_set(cfg, name, value); git__free(key); return result; } - var = git_strmap_value_at(b->values, pos); + var = git_strmap_value_at(values, pos); result = regcomp(&preg, regexp, REG_EXTENDED); if (result < 0) { @@ -510,6 +561,7 @@ static int config_delete(git_config_backend *cfg, const char *name) { cvar_t *var; diskfile_backend *b = (diskfile_backend *)cfg; + git_strmap *values = b->header.values; char *key; int result; khiter_t pos; @@ -517,22 +569,22 @@ static int config_delete(git_config_backend *cfg, const char *name) if ((result = git_config__normalize_name(name, &key)) < 0) return result; - pos = git_strmap_lookup_index(b->values, key); + pos = git_strmap_lookup_index(values, key); git__free(key); - if (!git_strmap_valid_index(b->values, pos)) { + if (!git_strmap_valid_index(values, pos)) { giterr_set(GITERR_CONFIG, "Could not find key '%s' to delete", name); return GIT_ENOTFOUND; } - var = git_strmap_value_at(b->values, pos); + var = git_strmap_value_at(values, pos); if (var->next != NULL) { giterr_set(GITERR_CONFIG, "Cannot delete multivar with a single delete"); return -1; } - git_strmap_delete_at(b->values, pos); + git_strmap_delete_at(values, pos); result = config_write(b, var->entry->name, NULL, NULL); @@ -546,6 +598,7 @@ static int config_delete_multivar(git_config_backend *cfg, const char *name, con cvar_t **to_delete; int to_delete_idx; diskfile_backend *b = (diskfile_backend *)cfg; + git_strmap *values = b->header.values; char *key; regex_t preg; int result; @@ -554,15 +607,15 @@ static int config_delete_multivar(git_config_backend *cfg, const char *name, con if ((result = git_config__normalize_name(name, &key)) < 0) return result; - pos = git_strmap_lookup_index(b->values, key); + pos = git_strmap_lookup_index(values, key); - if (!git_strmap_valid_index(b->values, pos)) { + if (!git_strmap_valid_index(values, pos)) { giterr_set(GITERR_CONFIG, "Could not find key '%s' to delete", name); git__free(key); return GIT_ENOTFOUND; } - var = git_strmap_value_at(b->values, pos); + var = git_strmap_value_at(values, pos); result = regcomp(&preg, regexp, REG_EXTENDED); if (result < 0) { @@ -597,9 +650,9 @@ static int config_delete_multivar(git_config_backend *cfg, const char *name, con } if (new_head != NULL) { - git_strmap_set_value_at(b->values, pos, new_head); + git_strmap_set_value_at(values, pos, new_head); } else { - git_strmap_delete_at(b->values, pos); + git_strmap_delete_at(values, pos); } if (to_delete_idx > 0) @@ -614,6 +667,13 @@ static int config_delete_multivar(git_config_backend *cfg, const char *name, con return result; } +static int config_snapshot(git_config_backend **out, git_config_backend *in) +{ + diskfile_backend *b = (diskfile_backend *) in; + + return git_config_file__snapshot(out, b); +} + int git_config_file__ondisk(git_config_backend **out, const char *path) { diskfile_backend *backend; @@ -621,20 +681,164 @@ int git_config_file__ondisk(git_config_backend **out, const char *path) backend = git__calloc(1, sizeof(diskfile_backend)); GITERR_CHECK_ALLOC(backend); - backend->parent.version = GIT_CONFIG_BACKEND_VERSION; + backend->header.parent.version = GIT_CONFIG_BACKEND_VERSION; backend->file_path = git__strdup(path); GITERR_CHECK_ALLOC(backend->file_path); - backend->parent.open = config_open; - backend->parent.get = config_get; - backend->parent.set = config_set; - backend->parent.set_multivar = config_set_multivar; - backend->parent.del = config_delete; - backend->parent.del_multivar = config_delete_multivar; - backend->parent.iterator = config_iterator_new; - backend->parent.refresh = config_refresh; - backend->parent.free = backend_free; + backend->header.parent.open = config_open; + backend->header.parent.get = config_get; + backend->header.parent.set = config_set; + backend->header.parent.set_multivar = config_set_multivar; + backend->header.parent.del = config_delete; + backend->header.parent.del_multivar = config_delete_multivar; + backend->header.parent.iterator = config_iterator_new; + backend->header.parent.refresh = config_refresh; + backend->header.parent.snapshot = config_snapshot; + backend->header.parent.free = backend_free; + + *out = (git_config_backend *)backend; + + return 0; +} + +static int config_set_readonly(git_config_backend *cfg, const char *name, const char *value) +{ + GIT_UNUSED(cfg); + GIT_UNUSED(name); + GIT_UNUSED(value); + + return config_error_readonly(); +} + +static int config_set_multivar_readonly( + git_config_backend *cfg, const char *name, const char *regexp, const char *value) +{ + GIT_UNUSED(cfg); + GIT_UNUSED(name); + GIT_UNUSED(regexp); + GIT_UNUSED(value); + + return config_error_readonly(); +} + +static int config_delete_multivar_readonly(git_config_backend *cfg, const char *name, const char *regexp) +{ + GIT_UNUSED(cfg); + GIT_UNUSED(name); + GIT_UNUSED(regexp); + + return config_error_readonly(); +} + +static int config_delete_readonly(git_config_backend *cfg, const char *name) +{ + GIT_UNUSED(cfg); + GIT_UNUSED(name); + + return config_error_readonly(); +} + +static int config_refresh_readonly(git_config_backend *cfg) +{ + GIT_UNUSED(cfg); + + return config_error_readonly(); +} + +static void backend_readonly_free(git_config_backend *_backend) +{ + diskfile_backend *backend = (diskfile_backend *)_backend; + + if (backend == NULL) + return; + + free_vars(backend->header.values); + git__free(backend); +} + +static int config_entry_dup(git_config_entry **out, git_config_entry *src) +{ + git_config_entry *entry; + + entry = git__calloc(1, sizeof(git_config_entry)); + GITERR_CHECK_ALLOC(entry); + + entry->level = src->level; + entry->name = git__strdup(src->name); + GITERR_CHECK_ALLOC(entry->name); + entry->value = git__strdup(src->value); + GITERR_CHECK_ALLOC(entry->value); + + *out = entry; + + return 0; +} + +static int config_readonly_open(git_config_backend *cfg, git_config_level_t level) +{ + diskfile_readonly_backend *b = (diskfile_readonly_backend *) cfg; + diskfile_backend *src = b->snapshot_from; + git_strmap *src_values = src->header.values; + git_strmap *values; + git_strmap_iter i; + cvar_t *src_var; + int error; + + /* We're just copying data, don't care about the level */ + GIT_UNUSED(level); + + if ((error = git_strmap_alloc(&b->header.values)) < 0) + return error; + + values = b->header.values; + + i = git_strmap_begin(src_values); + while ((error = git_strmap_next((void **) &src_var, &i, src_values)) == 0) { + do { + git_config_entry *entry; + cvar_t *var; + + var = git__calloc(1, sizeof(cvar_t)); + GITERR_CHECK_ALLOC(var); + + if (config_entry_dup(&entry, src_var->entry) < 0) + return -1; + + var->entry = entry; + + error = append_entry(values, var); + src_var = CVAR_LIST_NEXT(src_var); + } while (src_var != NULL && error == 0); + } + + if (error == GIT_ITEROVER) + error = 0; + + return error; +} + +int git_config_file__snapshot(git_config_backend **out, diskfile_backend *in) +{ + diskfile_readonly_backend *backend; + + backend = git__calloc(1, sizeof(diskfile_readonly_backend)); + GITERR_CHECK_ALLOC(backend); + + backend->header.parent.version = GIT_CONFIG_BACKEND_VERSION; + + backend->snapshot_from = in; + + backend->header.parent.version = GIT_CONFIG_BACKEND_VERSION; + backend->header.parent.open = config_readonly_open; + backend->header.parent.get = config_get; + backend->header.parent.set = config_set_readonly; + backend->header.parent.set_multivar = config_set_multivar_readonly; + backend->header.parent.del = config_delete_readonly; + backend->header.parent.del_multivar = config_delete_multivar_readonly; + backend->header.parent.iterator = config_iterator_new; + backend->header.parent.refresh = config_refresh_readonly; + backend->header.parent.free = backend_readonly_free; *out = (git_config_backend *)backend; @@ -1020,11 +1224,11 @@ static int config_parse(diskfile_backend *cfg_file, struct reader *reader, git_c char *current_section = NULL; char *var_name; char *var_value; - cvar_t *var, *existing; + cvar_t *var; git_buf buf = GIT_BUF_INIT; int result = 0; - khiter_t pos; uint32_t reader_idx; + git_strmap *values = cfg_file->header.values; if (depth >= MAX_INCLUDE_DEPTH) { giterr_set(GITERR_CONFIG, "Maximum config include depth reached"); @@ -1088,21 +1292,13 @@ static int config_parse(diskfile_backend *cfg_file, struct reader *reader, git_c var->entry->level = level; var->included = !!depth; - /* Add or append the new config option */ - pos = git_strmap_lookup_index(cfg_file->values, var->entry->name); - if (!git_strmap_valid_index(cfg_file->values, pos)) { - git_strmap_insert(cfg_file->values, var->entry->name, var, result); - if (result < 0) - break; - result = 0; - } else { - existing = git_strmap_value_at(cfg_file->values, pos); - while (existing->next != NULL) { - existing = existing->next; - } - existing->next = var; - } + if ((result = append_entry(values, var)) < 0) + break; + else + result = 0; + + /* Add or append the new config option */ if (!git__strcmp(var->entry->name, "include.path")) { struct reader *r; git_buf path = GIT_BUF_INIT; diff --git a/tests/config/snapshot.c b/tests/config/snapshot.c new file mode 100644 index 000000000..a9d3eadd3 --- /dev/null +++ b/tests/config/snapshot.c @@ -0,0 +1,69 @@ +#include "clar_libgit2.h" + +void test_config_snapshot__create_snapshot(void) +{ + int32_t tmp; + git_config *cfg, *snapshot; + const char *filename = "config-ext-change"; + + cl_git_mkfile(filename, "[old]\nvalue = 5\n"); + + cl_git_pass(git_config_open_ondisk(&cfg, filename)); + + cl_git_pass(git_config_get_int32(&tmp, cfg, "old.value")); + cl_assert_equal_i(5, tmp); + + cl_git_pass(git_config_snapshot(&snapshot, cfg)); + + /* Change the value on the file itself (simulate external process) */ + cl_git_mkfile(filename, "[old]\nvalue = 56\n"); + + cl_git_pass(git_config_get_int32(&tmp, cfg, "old.value")); + cl_assert_equal_i(5, tmp); + + cl_git_pass(git_config_refresh(cfg)); + + cl_git_pass(git_config_get_int32(&tmp, cfg, "old.value")); + cl_assert_equal_i(56, tmp); + + cl_git_pass(git_config_get_int32(&tmp, snapshot, "old.value")); + cl_assert_equal_i(5, tmp); + + git_config_free(snapshot); + git_config_free(cfg); +} + +static int count_me(const git_config_entry *entry, void *payload) +{ + int *n = (int *) payload; + + GIT_UNUSED(entry); + + (*n)++; + + return 0; +} + +void test_config_snapshot__multivar(void) +{ + int count = 0; + git_config *cfg, *snapshot; + const char *filename = "config-file"; + + cl_git_mkfile(filename, "[old]\nvalue = 5\nvalue = 6\n"); + + cl_git_pass(git_config_open_ondisk(&cfg, filename)); + cl_git_pass(git_config_get_multivar_foreach(cfg, "old.value", NULL, count_me, &count)); + + cl_assert_equal_i(2, count); + + cl_git_pass(git_config_snapshot(&snapshot, cfg)); + git_config_free(cfg); + + count = 0; + cl_git_pass(git_config_get_multivar_foreach(snapshot, "old.value", NULL, count_me, &count)); + + cl_assert_equal_i(2, count); + + git_config_free(snapshot); +} From 29c4cb0965b67ad1ae7f640dfee67cf3ce358c26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Sat, 15 Mar 2014 03:53:36 +0100 Subject: [PATCH 03/13] Use config snapshotting This way we can assume we have a consistent view of the config situation when we're looking up remote, branch, pack-objects, etc. --- src/branch.c | 18 +++++++++++------- src/diff_driver.c | 8 ++++++-- src/pack-objects.c | 11 ++++++++--- src/remote.c | 8 ++++++-- src/repository.c | 25 +++++++++++++------------ src/signature.c | 7 +++++-- 6 files changed, 49 insertions(+), 28 deletions(-) diff --git a/src/branch.c b/src/branch.c index 63c6ec110..d8c82b73e 100644 --- a/src/branch.c +++ b/src/branch.c @@ -306,17 +306,13 @@ int git_branch_name( static int retrieve_upstream_configuration( const char **out, - git_repository *repo, + const git_config *config, const char *canonical_branch_name, const char *format) { - git_config *config; git_buf buf = GIT_BUF_INIT; int error; - if (git_repository_config__weakptr(&config, repo) < 0) - return -1; - if (git_buf_printf(&buf, format, canonical_branch_name + strlen(GIT_REFS_HEADS_DIR)) < 0) return -1; @@ -336,6 +332,7 @@ int git_branch_upstream_name( int error = -1; git_remote *remote = NULL; const git_refspec *refspec; + git_config *config, *repo_config; assert(out && refname); @@ -344,12 +341,18 @@ int git_branch_upstream_name( if (!git_reference__is_branch(refname)) return not_a_local_branch(refname); + if ((error = git_repository_config__weakptr(&repo_config, repo)) < 0) + return error; + + if ((error = git_config_snapshot(&config, repo_config)) < 0) + return error; + if ((error = retrieve_upstream_configuration( - &remote_name, repo, refname, "branch.%s.remote")) < 0) + &remote_name, config, refname, "branch.%s.remote")) < 0) goto cleanup; if ((error = retrieve_upstream_configuration( - &merge_name, repo, refname, "branch.%s.merge")) < 0) + &merge_name, config, refname, "branch.%s.merge")) < 0) goto cleanup; if (!*remote_name || !*merge_name) { @@ -378,6 +381,7 @@ int git_branch_upstream_name( error = git_buf_set(out, git_buf_cstr(&buf), git_buf_len(&buf)); cleanup: + git_config_free(config); git_remote_free(remote); git_buf_free(&buf); return error; diff --git a/src/diff_driver.c b/src/diff_driver.c index 8136e0dd9..6e87fd6f8 100644 --- a/src/diff_driver.c +++ b/src/diff_driver.c @@ -219,7 +219,7 @@ static int git_diff_driver_load( git_diff_driver *drv = NULL; size_t namelen = strlen(driver_name); khiter_t pos; - git_config *cfg; + git_config *cfg, *repo_cfg; git_buf name = GIT_BUF_INIT; const git_config_entry *ce; bool found_driver = false; @@ -234,11 +234,14 @@ static int git_diff_driver_load( } /* if you can't read config for repo, just use default driver */ - if (git_repository_config__weakptr(&cfg, repo) < 0) { + if (git_repository_config__weakptr(&repo_cfg, repo) < 0) { giterr_clear(); goto done; } + if ((error = git_config_snapshot(&cfg, repo_cfg)) < 0) + return error; + drv = git__calloc(1, sizeof(git_diff_driver) + namelen + 1); GITERR_CHECK_ALLOC(drv); drv->type = DIFF_DRIVER_AUTO; @@ -321,6 +324,7 @@ static int git_diff_driver_load( done: git_buf_free(&name); + git_config_free(cfg); if (!*out) { int error2 = git_diff_driver_builtin(out, reg, driver_name); diff --git a/src/pack-objects.c b/src/pack-objects.c index c881e6d99..e7c7f391f 100644 --- a/src/pack-objects.c +++ b/src/pack-objects.c @@ -86,12 +86,15 @@ static unsigned name_hash(const char *name) static int packbuilder_config(git_packbuilder *pb) { - git_config *config; + git_config *config, *repo_config; int ret; int64_t val; - if (git_repository_config__weakptr(&config, pb->repo) < 0) - return -1; + if ((ret = git_repository_config__weakptr(&repo_config, pb->repo)) < 0) + return ret; + + if ((ret = git_config_snapshot(&config, repo_config)) < 0) + return ret; #define config_get(KEY,DST,DFLT) do { \ ret = git_config_get_int64(&val, config, KEY); \ @@ -109,6 +112,8 @@ static int packbuilder_config(git_packbuilder *pb) #undef config_get + git_config_free(config); + return 0; } diff --git a/src/remote.c b/src/remote.c index 243086bf9..3a754d4bd 100644 --- a/src/remote.c +++ b/src/remote.c @@ -347,7 +347,7 @@ int git_remote_load(git_remote **out, git_repository *repo, const char *name) git_buf buf = GIT_BUF_INIT; const char *val; int error = 0; - git_config *config; + git_config *config, *repo_config; struct refspec_cb_data data = { NULL }; bool optional_setting_found = false, found; @@ -356,9 +356,12 @@ int git_remote_load(git_remote **out, git_repository *repo, const char *name) if ((error = ensure_remote_name_is_valid(name)) < 0) return error; - if (git_repository_config__weakptr(&config, repo) < 0) + if (git_repository_config__weakptr(&repo_config, repo) < 0) return -1; + if ((error = git_config_snapshot(&config, repo_config)) < 0) + return error; + remote = git__malloc(sizeof(git_remote)); GITERR_CHECK_ALLOC(remote); @@ -437,6 +440,7 @@ int git_remote_load(git_remote **out, git_repository *repo, const char *name) *out = remote; cleanup: + git_config_free(config); git_buf_free(&buf); if (error < 0) diff --git a/src/repository.c b/src/repository.c index 6b2705bfa..c1d98300c 100644 --- a/src/repository.c +++ b/src/repository.c @@ -165,13 +165,9 @@ int git_repository_new(git_repository **out) return 0; } -static int load_config_data(git_repository *repo) +static int load_config_data(git_repository *repo, const git_config *config) { int is_bare; - git_config *config; - - if (git_repository_config__weakptr(&config, repo) < 0) - return -1; /* Try to figure out if it's bare, default to non-bare if it's not set */ if (git_config_get_bool(&is_bare, config, "core.bare") < 0) @@ -182,19 +178,15 @@ static int load_config_data(git_repository *repo) return 0; } -static int load_workdir(git_repository *repo, git_buf *parent_path) +static int load_workdir(git_repository *repo, git_config *config, git_buf *parent_path) { int error; - git_config *config; const git_config_entry *ce; git_buf worktree = GIT_BUF_INIT; if (repo->is_bare) return 0; - if ((error = git_repository_config__weakptr(&config, repo)) < 0) - return error; - if ((error = git_config__lookup_entry( &ce, config, "core.worktree", false)) < 0) return error; @@ -447,6 +439,7 @@ int git_repository_open_ext( int error; git_buf path = GIT_BUF_INIT, parent = GIT_BUF_INIT; git_repository *repo; + git_config *repo_config, *config; if (repo_ptr) *repo_ptr = NULL; @@ -461,15 +454,23 @@ int git_repository_open_ext( repo->path_repository = git_buf_detach(&path); GITERR_CHECK_ALLOC(repo->path_repository); + if ((error = git_repository_config__weakptr(&repo_config, repo)) < 0) + return error; + + if ((error = git_config_snapshot(&config, repo_config)) < 0) + return error; + if ((flags & GIT_REPOSITORY_OPEN_BARE) != 0) repo->is_bare = 1; - else if ((error = load_config_data(repo)) < 0 || - (error = load_workdir(repo, &parent)) < 0) + else if ((error = load_config_data(repo, config)) < 0 || + (error = load_workdir(repo, config, &parent)) < 0) { + git_config_free(config); git_repository_free(repo); return error; } + git_config_free(config); git_buf_free(&parent); *repo_ptr = repo; return 0; diff --git a/src/signature.c b/src/signature.c index f501cd8b6..b0ee0da74 100644 --- a/src/signature.c +++ b/src/signature.c @@ -141,10 +141,13 @@ int git_signature_now(git_signature **sig_out, const char *name, const char *ema int git_signature_default(git_signature **out, git_repository *repo) { int error; - git_config *cfg; + git_config *cfg, *repo_cfg; const char *user_name, *user_email; - if ((error = git_repository_config(&cfg, repo)) < 0) + if ((error = git_repository_config__weakptr(&repo_cfg, repo)) < 0) + return error; + + if ((error = git_config_snapshot(&cfg, repo_cfg)) < 0) return error; if (!(error = git_config_get_string(&user_name, cfg, "user.name")) && From c047317e400efc75b07aa0fff61ee1fcd794e74e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 18 Mar 2014 17:54:32 +0100 Subject: [PATCH 04/13] config: make refresh atomic Current code sets the active map to a new one and builds it whilst it's active. This is a race condition with someone else trying to access the same config. Instead, let's build up our new map and swap the active and new one. --- src/config_file.c | 39 ++++++++++++++++----------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/src/config_file.c b/src/config_file.c index fcf9cab04..437a5c592 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -107,7 +107,7 @@ typedef struct { diskfile_backend *snapshot_from; } diskfile_readonly_backend; -static int config_parse(diskfile_backend *cfg_file, struct reader *reader, git_config_level_t level, int depth); +static int config_parse(git_strmap *values, diskfile_backend *cfg_file, struct reader *reader, git_config_level_t level, int depth); static int parse_variable(struct reader *reader, char **var_name, char **var_value); static int config_write(diskfile_backend *cfg, const char *key, const regex_t *preg, const char *value); static char *escape_value(const char *ptr); @@ -243,7 +243,7 @@ static int config_open(git_config_backend *cfg, git_config_level_t level) if (res == GIT_ENOTFOUND) return 0; - if (res < 0 || (res = config_parse(b, reader, level, 0)) < 0) { + if (res < 0 || (res = config_parse(b->header.values, b, reader, level, 0)) < 0) { free_vars(b->header.values); b->header.values = NULL; } @@ -256,47 +256,41 @@ static int config_open(git_config_backend *cfg, git_config_level_t level) static int config_refresh(git_config_backend *cfg) { - int res = 0, updated = 0, any_updated = 0; + int error = 0, updated = 0, any_updated = 0; diskfile_backend *b = (diskfile_backend *)cfg; - git_strmap *old_values; + git_strmap *values = NULL; struct reader *reader = NULL; uint32_t i; for (i = 0; i < git_array_size(b->readers); i++) { reader = git_array_get(b->readers, i); - - res = git_futils_readbuffer_updated( + error = git_futils_readbuffer_updated( &reader->buffer, reader->file_path, &reader->file_mtime, &reader->file_size, &updated); - if (res < 0) - return (res == GIT_ENOTFOUND) ? 0 : res; + if (error < 0) + return (error == GIT_ENOTFOUND) ? 0 : error; if (updated) any_updated = 1; } if (!any_updated) - return (res == GIT_ENOTFOUND) ? 0 : res; + return (error == GIT_ENOTFOUND) ? 0 : error; - /* need to reload - store old values and prep for reload */ - old_values = b->header.values; - if ((res = git_strmap_alloc(&b->header.values)) < 0) { - b->header.values = old_values; + + if ((error = git_strmap_alloc(&values)) < 0) goto cleanup; - } - if ((res = config_parse(b, reader, b->level, 0)) < 0) { - free_vars(b->header.values); - b->header.values = old_values; + if ((error = config_parse(values, b, reader, b->level, 0)) < 0) goto cleanup; - } - free_vars(old_values); + values = git__swap(b->header.values, values); cleanup: + free_vars(values); git_buf_free(&reader->buffer); - return res; + return error; } static void backend_free(git_config_backend *_backend) @@ -1218,7 +1212,7 @@ static int included_path(git_buf *out, const char *dir, const char *path) return git_path_join_unrooted(out, path, dir, NULL); } -static int config_parse(diskfile_backend *cfg_file, struct reader *reader, git_config_level_t level, int depth) +static int config_parse(git_strmap *values, diskfile_backend *cfg_file, struct reader *reader, git_config_level_t level, int depth) { int c; char *current_section = NULL; @@ -1228,7 +1222,6 @@ static int config_parse(diskfile_backend *cfg_file, struct reader *reader, git_c git_buf buf = GIT_BUF_INIT; int result = 0; uint32_t reader_idx; - git_strmap *values = cfg_file->header.values; if (depth >= MAX_INCLUDE_DEPTH) { giterr_set(GITERR_CONFIG, "Maximum config include depth reached"); @@ -1327,7 +1320,7 @@ static int config_parse(diskfile_backend *cfg_file, struct reader *reader, git_c &r->file_size, NULL)) < 0) break; - result = config_parse(cfg_file, r, level, depth+1); + result = config_parse(values, cfg_file, r, level, depth+1); r = git_array_get(cfg_file->readers, index); git_buf_free(&r->buffer); From bd95f836f51804011b8a8c532471733f92f3e119 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Mon, 31 Mar 2014 08:08:17 +0200 Subject: [PATCH 05/13] config: split out the refresh step This will be used by the writing commands in a later step. --- src/config_file.c | 39 +++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/src/config_file.c b/src/config_file.c index 437a5c592..24ace6039 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -254,11 +254,34 @@ static int config_open(git_config_backend *cfg, git_config_level_t level) return res; } +/* The meat of the refresh, as we want to use it in different places */ +static int config__refresh(git_config_backend *cfg) +{ + git_strmap *values = NULL; + diskfile_backend *b = (diskfile_backend *)cfg; + struct reader *reader = NULL; + int error = 0; + + if ((error = git_strmap_alloc(&values)) < 0) + goto out; + + reader = git_array_get(b->readers, git_array_size(b->readers) - 1); + + if ((error = config_parse(values, b, reader, b->level, 0)) < 0) + goto out; + + values = git__swap(b->header.values, values); + +out: + free_vars(values); + git_buf_free(&reader->buffer); + return error; +} + static int config_refresh(git_config_backend *cfg) { int error = 0, updated = 0, any_updated = 0; diskfile_backend *b = (diskfile_backend *)cfg; - git_strmap *values = NULL; struct reader *reader = NULL; uint32_t i; @@ -278,19 +301,7 @@ static int config_refresh(git_config_backend *cfg) if (!any_updated) return (error == GIT_ENOTFOUND) ? 0 : error; - - if ((error = git_strmap_alloc(&values)) < 0) - goto cleanup; - - if ((error = config_parse(values, b, reader, b->level, 0)) < 0) - goto cleanup; - - values = git__swap(b->header.values, values); - -cleanup: - free_vars(values); - git_buf_free(&reader->buffer); - return error; + return config__refresh(cfg); } static void backend_free(git_config_backend *_backend) From 0500a1ef4e61449a7ad375c1f42d3bd25a4e36bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Mon, 31 Mar 2014 08:32:45 +0200 Subject: [PATCH 06/13] config: use a snapshot for the iterator --- src/config_file.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/config_file.c b/src/config_file.c index 24ace6039..81828ef23 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -113,6 +113,7 @@ static int config_write(diskfile_backend *cfg, const char *key, const regex_t *p static char *escape_value(const char *ptr); int git_config_file__snapshot(git_config_backend **out, diskfile_backend *in); +static int config_snapshot(git_config_backend **out, git_config_backend *in); static void set_parse_error(struct reader *reader, int col, const char *error_str) { @@ -326,6 +327,7 @@ static void backend_free(git_config_backend *_backend) static void config_iterator_free( git_config_iterator* iter) { + iter->backend->free(iter->backend); git__free(iter); } @@ -360,15 +362,27 @@ static int config_iterator_new( git_config_iterator **iter, struct git_config_backend* backend) { - diskfile_header *h = (diskfile_header *)backend; - git_config_file_iter *it = git__calloc(1, sizeof(git_config_file_iter)); + diskfile_header *h; + git_config_file_iter *it; + git_config_backend *snapshot; + diskfile_backend *b = (diskfile_backend *) backend; + int error; + if ((error = config_snapshot(&snapshot, backend)) < 0) + return error; + + if ((error = snapshot->open(snapshot, b->level)) < 0) + return error; + + it = git__calloc(1, sizeof(git_config_file_iter)); GITERR_CHECK_ALLOC(it); + h = (diskfile_header *)snapshot; + /* strmap_begin() is currently a macro returning 0 */ GIT_UNUSED(h); - it->parent.backend = backend; + it->parent.backend = snapshot; it->iter = git_strmap_begin(h->values); it->next_var = NULL; From eaf3703401a64d59fd8bf2609449343d739ef056 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Mon, 31 Mar 2014 08:53:56 +0200 Subject: [PATCH 07/13] config: refresh the values on write When writing out, parse the resulting file instead of adding or replacing the value locally. This has the effect of reading external changes as well. --- src/config_file.c | 99 ++++++++------------------------------------ tests/config/write.c | 2 +- 2 files changed, 18 insertions(+), 83 deletions(-) diff --git a/src/config_file.c b/src/config_file.c index 81828ef23..621a7c3e2 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -395,7 +395,6 @@ static int config_iterator_new( static int config_set(git_config_backend *cfg, const char *name, const char *value) { - cvar_t *var = NULL, *old_var = NULL; diskfile_backend *b = (diskfile_backend *)cfg; git_strmap *values = b->header.values; char *key, *esc_value = NULL; @@ -412,67 +411,38 @@ static int config_set(git_config_backend *cfg, const char *name, const char *val pos = git_strmap_lookup_index(values, key); if (git_strmap_valid_index(values, pos)) { cvar_t *existing = git_strmap_value_at(values, pos); - char *tmp = NULL; - - git__free(key); if (existing->next != NULL) { + git__free(key); giterr_set(GITERR_CONFIG, "Multivar incompatible with simple set"); return -1; } /* don't update if old and new values already match */ if ((!existing->entry->value && !value) || - (existing->entry->value && value && !strcmp(existing->entry->value, value))) + (existing->entry->value && value && + !strcmp(existing->entry->value, value))) { + git__free(key); return 0; - - if (value) { - tmp = git__strdup(value); - GITERR_CHECK_ALLOC(tmp); - esc_value = escape_value(value); - GITERR_CHECK_ALLOC(esc_value); } - - git__free((void *)existing->entry->value); - existing->entry->value = tmp; - - ret = config_write(b, existing->entry->name, NULL, esc_value); - - git__free(esc_value); - return ret; } - var = git__malloc(sizeof(cvar_t)); - GITERR_CHECK_ALLOC(var); - memset(var, 0x0, sizeof(cvar_t)); - var->entry = git__malloc(sizeof(git_config_entry)); - GITERR_CHECK_ALLOC(var->entry); - memset(var->entry, 0x0, sizeof(git_config_entry)); - - var->entry->name = key; - var->entry->value = NULL; + /* No early returns due to sanity checks, let's write it out and refresh */ if (value) { - var->entry->value = git__strdup(value); - GITERR_CHECK_ALLOC(var->entry->value); esc_value = escape_value(value); GITERR_CHECK_ALLOC(esc_value); } - if ((ret = config_write(b, key, NULL, esc_value)) < 0) { - git__free(esc_value); - cvar_free(var); - return ret; - } + if ((ret = config_write(b, key, NULL, esc_value)) < 0) + goto out; + ret = config_refresh(cfg); + +out: git__free(esc_value); - git_strmap_insert2(values, key, var, old_var, rval); - if (rval < 0) - return -1; - if (old_var != NULL) - cvar_free(old_var); - - return 0; + git__free(key); + return ret; } /* @@ -500,8 +470,6 @@ static int config_get(const git_config_backend *cfg, const char *key, const git_ static int config_set_multivar( git_config_backend *cfg, const char *name, const char *regexp, const char *value) { - int replaced = 0; - cvar_t *var, *newvar; diskfile_backend *b = (diskfile_backend *)cfg; git_strmap *values = b->header.values; char *key; @@ -522,8 +490,6 @@ static int config_set_multivar( return result; } - var = git_strmap_value_at(values, pos); - result = regcomp(&preg, regexp, REG_EXTENDED); if (result < 0) { git__free(key); @@ -532,44 +498,13 @@ static int config_set_multivar( return -1; } - for (;;) { - if (regexec(&preg, var->entry->value, 0, NULL, 0) == 0) { - char *tmp = git__strdup(value); - GITERR_CHECK_ALLOC(tmp); + /* If we do have it, set call config_write() and reload */ + if ((result = config_write(b, key, &preg, value)) < 0) + goto out; - git__free((void *)var->entry->value); - var->entry->value = tmp; - replaced = 1; - } - - if (var->next == NULL) - break; - - var = var->next; - } - - /* If we've reached the end of the variables and we haven't found it yet, we need to append it */ - if (!replaced) { - newvar = git__malloc(sizeof(cvar_t)); - GITERR_CHECK_ALLOC(newvar); - memset(newvar, 0x0, sizeof(cvar_t)); - newvar->entry = git__malloc(sizeof(git_config_entry)); - GITERR_CHECK_ALLOC(newvar->entry); - memset(newvar->entry, 0x0, sizeof(git_config_entry)); - - newvar->entry->name = git__strdup(var->entry->name); - GITERR_CHECK_ALLOC(newvar->entry->name); - - newvar->entry->value = git__strdup(value); - GITERR_CHECK_ALLOC(newvar->entry->value); - - newvar->entry->level = var->entry->level; - - var->next = newvar; - } - - result = config_write(b, key, &preg, value); + result = config_refresh(cfg); +out: git__free(key); regfree(&preg); diff --git a/tests/config/write.c b/tests/config/write.c index f269c9571..402be9317 100644 --- a/tests/config/write.c +++ b/tests/config/write.c @@ -322,7 +322,7 @@ void test_config_write__outside_change(void) cl_git_pass(git_config_set_int32(cfg, "new.value", 7)); cl_git_pass(git_config_get_int32(&tmp, cfg, "old.value")); - cl_assert_equal_i(5, tmp); + cl_assert_equal_i(6, tmp); cl_git_pass(git_config_refresh(cfg)); From 523032cd24e5b238752394bd1b691a3e28ba321e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Mon, 31 Mar 2014 09:58:44 +0200 Subject: [PATCH 08/13] config: refresh before reading a value With the isolation of complex reads, we can now try to refresh the on-disk file before reading a value from it. This changes the semantics a bit, as before we could be sure that a string we got from the configuration was valid until we wrote or refreshed. This is no longer the case, as a read can also invalidate the pointer. --- include/git2/sys/config.h | 2 +- src/config_file.c | 15 ++++++++++++--- tests/config/refresh.c | 9 +++------ tests/config/snapshot.c | 5 ----- 4 files changed, 16 insertions(+), 15 deletions(-) diff --git a/include/git2/sys/config.h b/include/git2/sys/config.h index 090588999..5d606ed06 100644 --- a/include/git2/sys/config.h +++ b/include/git2/sys/config.h @@ -57,7 +57,7 @@ struct git_config_backend { /* Open means open the file/database and parse if necessary */ int (*open)(struct git_config_backend *, git_config_level_t level); - int (*get)(const struct git_config_backend *, const char *key, const git_config_entry **entry); + int (*get)(struct git_config_backend *, const char *key, const git_config_entry **entry); int (*set)(struct git_config_backend *, const char *key, const char *value); int (*set_multivar)(git_config_backend *cfg, const char *name, const char *regexp, const char *value); int (*del)(struct git_config_backend *, const char *key); diff --git a/src/config_file.c b/src/config_file.c index 621a7c3e2..227ec82e9 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -89,6 +89,7 @@ struct reader { typedef struct { git_config_backend parent; git_strmap *values; + int readonly; } diskfile_header; typedef struct { @@ -448,12 +449,19 @@ out: /* * Internal function that actually gets the value in string form */ -static int config_get(const git_config_backend *cfg, const char *key, const git_config_entry **out) +static int config_get(git_config_backend *cfg, const char *key, const git_config_entry **out) { diskfile_header *h = (diskfile_header *)cfg; - git_strmap *values = h->values; - khiter_t pos = git_strmap_lookup_index(values, key); + git_strmap *values; + khiter_t pos; cvar_t *var; + int error; + + if (!h->readonly && ((error = config_refresh(cfg)) < 0)) + return error; + + values = h->values; + pos = git_strmap_lookup_index(values, key); /* no error message; the config system will write one */ if (!git_strmap_valid_index(values, pos)) @@ -783,6 +791,7 @@ int git_config_file__snapshot(git_config_backend **out, diskfile_backend *in) backend->snapshot_from = in; + backend->header.readonly = 1; backend->header.parent.version = GIT_CONFIG_BACKEND_VERSION; backend->header.parent.open = config_readonly_open; backend->header.parent.get = config_get; diff --git a/tests/config/refresh.c b/tests/config/refresh.c index 99d677f0e..08cd45b95 100644 --- a/tests/config/refresh.c +++ b/tests/config/refresh.c @@ -26,9 +26,6 @@ void test_config_refresh__update_value(void) cl_git_rewritefile(TEST_FILE, "[section]\n\tvalue = 10\n\n"); - cl_git_pass(git_config_get_int32(&v, cfg, "section.value")); - cl_assert_equal_i(1, v); - cl_git_pass(git_config_refresh(cfg)); cl_git_pass(git_config_get_int32(&v, cfg, "section.value")); @@ -53,9 +50,9 @@ void test_config_refresh__delete_value(void) cl_git_rewritefile(TEST_FILE, "[section]\n\tnewval = 10\n\n"); - cl_git_pass(git_config_get_int32(&v, cfg, "section.value")); - cl_assert_equal_i(1, v); - cl_git_fail(git_config_get_int32(&v, cfg, "section.newval")); + cl_git_fail_with(GIT_ENOTFOUND, git_config_get_int32(&v, cfg, "section.value")); + + cl_git_pass(git_config_get_int32(&v, cfg, "section.newval")); cl_git_pass(git_config_refresh(cfg)); diff --git a/tests/config/snapshot.c b/tests/config/snapshot.c index a9d3eadd3..c9f15921a 100644 --- a/tests/config/snapshot.c +++ b/tests/config/snapshot.c @@ -18,11 +18,6 @@ void test_config_snapshot__create_snapshot(void) /* Change the value on the file itself (simulate external process) */ cl_git_mkfile(filename, "[old]\nvalue = 56\n"); - cl_git_pass(git_config_get_int32(&tmp, cfg, "old.value")); - cl_assert_equal_i(5, tmp); - - cl_git_pass(git_config_refresh(cfg)); - cl_git_pass(git_config_get_int32(&tmp, cfg, "old.value")); cl_assert_equal_i(56, tmp); From c20d71eac928fef34a89ced3cbb6502ab2763dc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Mon, 31 Mar 2014 10:13:40 +0200 Subject: [PATCH 09/13] config: document the how long the pointers are valid for --- include/git2/config.h | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/include/git2/config.h b/include/git2/config.h index 86c4012ed..21a5825a5 100644 --- a/include/git2/config.h +++ b/include/git2/config.h @@ -233,6 +233,9 @@ GIT_EXTERN(int) git_config_open_global(git_config **out, git_config *config); * allows you to look into a consistent view of the configuration for * looking up complex values (e.g. a remote, submodule). * + * The string returned when querying such a config object is valid + * until it is freed. + * * @param out pointer in which to store the snapshot config object * @param config configuration to snapshot * @return 0 or an error code @@ -325,7 +328,8 @@ GIT_EXTERN(int) git_config_get_bool(int *out, const git_config *cfg, const char * Get the value of a string config variable. * * The string is owned by the variable and should not be freed by the - * user. + * user. The pointer will be valid until the next operation on this + * config object. * * All config files will be looked into, in the order of their * defined level. A higher level means a higher priority. The @@ -366,6 +370,9 @@ GIT_EXTERN(int) git_config_multivar_iterator_new(git_config_iterator **out, cons /** * Return the current entry and advance the iterator * + * The pointers returned by this function are valid until the iterator + * is freed. + * * @param entry pointer to store the entry * @param iter the iterator * @return 0 or an error code. GIT_ITEROVER if the iteration has completed @@ -464,6 +471,9 @@ GIT_EXTERN(int) git_config_delete_multivar(git_config *cfg, const char *name, co * If the callback returns a non-zero value, the function stops iterating * and returns that value to the caller. * + * The pointers passed to the callback are only valid as long as the + * iteration is ongoing. + * * @param cfg where to get the variables from * @param callback the function to call on each variable * @param payload the data to pass to the callback @@ -504,6 +514,9 @@ GIT_EXTERN(int) git_config_iterator_glob_new(git_config_iterator **out, const gi * regular expression that filters which config keys are passed to the * callback. * + * The pointers passed to the callback are only valid as long as the + * iteration is ongoing. + * * @param cfg where to get the variables from * @param regexp regular expression to match against config names * @param callback the function to call on each variable From 8c1f4ab4abdb47c7926fa678577973272be6e2df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 9 Apr 2014 12:26:53 +0200 Subject: [PATCH 10/13] config: refresh on delete When we delete an entry, we also want to refresh the configuration to catch any changes that happened externally. This allows us to simplify the logic, as we no longer need to delete these variables internally. The whole state will be refreshed and the deleted entries won't be there. --- src/config_file.c | 63 ++++++++--------------------------------- tests/config/multivar.c | 6 ++-- 2 files changed, 15 insertions(+), 54 deletions(-) diff --git a/src/config_file.c b/src/config_file.c index 227ec82e9..e5b5655fc 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -26,7 +26,7 @@ GIT__USE_STRMAP; typedef struct cvar_t { struct cvar_t *next; git_config_entry *entry; - int included; /* whether this is part of [include] */ + bool included; /* whether this is part of [include] */ } cvar_t; typedef struct git_config_file_iter { @@ -546,19 +546,14 @@ static int config_delete(git_config_backend *cfg, const char *name) return -1; } - git_strmap_delete_at(values, pos); + if ((result = config_write(b, var->entry->name, NULL, NULL)) < 0) + return result; - result = config_write(b, var->entry->name, NULL, NULL); - - cvar_free(var); - return result; + return config_refresh(cfg); } static int config_delete_multivar(git_config_backend *cfg, const char *name, const char *regexp) { - cvar_t *var, *prev = NULL, *new_head = NULL; - cvar_t **to_delete; - int to_delete_idx; diskfile_backend *b = (diskfile_backend *)cfg; git_strmap *values = b->header.values; char *key; @@ -572,59 +567,25 @@ static int config_delete_multivar(git_config_backend *cfg, const char *name, con pos = git_strmap_lookup_index(values, key); if (!git_strmap_valid_index(values, pos)) { - giterr_set(GITERR_CONFIG, "Could not find key '%s' to delete", name); git__free(key); + giterr_set(GITERR_CONFIG, "Could not find key '%s' to delete", name); return GIT_ENOTFOUND; } - var = git_strmap_value_at(values, pos); - result = regcomp(&preg, regexp, REG_EXTENDED); if (result < 0) { - git__free(key); giterr_set_regex(&preg, result); - regfree(&preg); - return -1; + result = -1; + goto out; } - to_delete = git__calloc(cvar_length(var), sizeof(cvar_t *)); - GITERR_CHECK_ALLOC(to_delete); - to_delete_idx = 0; + if ((result = config_write(b, key, &preg, NULL)) < 0) + goto out; - while (var != NULL) { - cvar_t *next = var->next; - - if (regexec(&preg, var->entry->value, 0, NULL, 0) == 0) { - // If we are past the head, reattach previous node to next one, - // otherwise set the new head for the strmap. - if (prev != NULL) { - prev->next = next; - } else { - new_head = next; - } - - to_delete[to_delete_idx++] = var; - } else { - prev = var; - } - - var = next; - } - - if (new_head != NULL) { - git_strmap_set_value_at(values, pos, new_head); - } else { - git_strmap_delete_at(values, pos); - } - - if (to_delete_idx > 0) - result = config_write(b, key, &preg, NULL); - - while (to_delete_idx-- > 0) - cvar_free(to_delete[to_delete_idx]); + result = config_refresh(cfg); +out: git__free(key); - git__free(to_delete); regfree(&preg); return result; } @@ -1532,7 +1493,7 @@ static int config_write(diskfile_backend *cfg, const char *key, const regex_t *p * this, but instead we'll handle it gracefully with an error. */ if (value == NULL) { giterr_set(GITERR_CONFIG, - "Race condition when writing a config file (a cvar has been removed)"); + "race condition when writing a config file (a cvar has been removed)"); goto rewrite_fail; } diff --git a/tests/config/multivar.c b/tests/config/multivar.c index afdb1e5f4..015008992 100644 --- a/tests/config/multivar.c +++ b/tests/config/multivar.c @@ -231,13 +231,13 @@ void test_config_multivar__delete(void) n = 0; cl_git_pass(git_config_get_multivar_foreach(cfg, _name, NULL, cb, &n)); - cl_assert(n == 2); + cl_assert_equal_i(2, n); cl_git_pass(git_config_delete_multivar(cfg, _name, "github")); n = 0; cl_git_pass(git_config_get_multivar_foreach(cfg, _name, NULL, cb, &n)); - cl_assert(n == 1); + cl_assert_equal_i(1, n); git_config_free(cfg); @@ -245,7 +245,7 @@ void test_config_multivar__delete(void) n = 0; cl_git_pass(git_config_get_multivar_foreach(cfg, _name, NULL, cb, &n)); - cl_assert(n == 1); + cl_assert_equal_i(1, n); git_config_free(cfg); } From 4b99b8f52844cb66b2b61b9e88b37b5f98515d50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 9 Apr 2014 13:08:01 +0200 Subject: [PATCH 11/13] config: refcount the values map This is mostly groundwork to let us re-use the map in the snapshots. --- src/config_file.c | 174 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 131 insertions(+), 43 deletions(-) diff --git a/src/config_file.c b/src/config_file.c index e5b5655fc..c53ec015f 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -87,8 +87,15 @@ struct reader { }; typedef struct { - git_config_backend parent; + git_atomic refcount; git_strmap *values; +} refcounted_strmap; + +typedef struct { + git_config_backend parent; + /* mutex to coordinate accessing the values */ + git_mutex values_mutex; + refcounted_strmap *values; int readonly; } diskfile_header; @@ -139,18 +146,6 @@ static void cvar_free(cvar_t *var) git__free(var); } -static int cvar_length(cvar_t *var) -{ - int length = 0; - - while (var) { - length++; - var = var->next; - } - - return length; -} - int git_config_file_normalize_section(char *start, char *end) { char *scan; @@ -215,6 +210,58 @@ static void free_vars(git_strmap *values) git_strmap_free(values); } +static void refcounted_strmap_free(refcounted_strmap *map) +{ + if (!map) + return; + + if (git_atomic_dec(&map->refcount) != 0) + return; + + free_vars(map->values); + git__free(map); +} + +/** + * Take the current values map from the backend and increase its + * refcount. This is its own function to make sure we use the mutex to + * avoid the map pointer from changing under us. + */ +static refcounted_strmap *refcounted_strmap_take(diskfile_header *h) +{ + refcounted_strmap *map; + + git_mutex_lock(&h->values_mutex); + + map = h->values; + git_atomic_inc(&map->refcount); + + git_mutex_unlock(&h->values_mutex); + + return map; +} + +static int refcounted_strmap_alloc(refcounted_strmap **out) +{ + refcounted_strmap *map; + int error; + + map = git__calloc(1, sizeof(refcounted_strmap)); + if (!map) { + giterr_set_oom(); + return -1; + } + + git_atomic_set(&map->refcount, 1); + if ((error = git_strmap_alloc(&map->values)) < 0) { + git__free(map); + return error; + } + + *out = map; + return error; +} + static int config_open(git_config_backend *cfg, git_config_level_t level) { int res; @@ -223,13 +270,14 @@ static int config_open(git_config_backend *cfg, git_config_level_t level) b->level = level; - if ((res = git_strmap_alloc(&b->header.values)) < 0) + if ((res = refcounted_strmap_alloc(&b->header.values)) < 0) return res; + git_mutex_init(&b->header.values_mutex); git_array_init(b->readers); reader = git_array_alloc(b->readers); if (!reader) { - git_strmap_free(b->header.values); + refcounted_strmap_free(b->header.values); return -1; } memset(reader, 0, sizeof(struct reader)); @@ -245,8 +293,8 @@ static int config_open(git_config_backend *cfg, git_config_level_t level) if (res == GIT_ENOTFOUND) return 0; - if (res < 0 || (res = config_parse(b->header.values, b, reader, level, 0)) < 0) { - free_vars(b->header.values); + if (res < 0 || (res = config_parse(b->header.values->values, b, reader, level, 0)) < 0) { + refcounted_strmap_free(b->header.values); b->header.values = NULL; } @@ -259,23 +307,29 @@ static int config_open(git_config_backend *cfg, git_config_level_t level) /* The meat of the refresh, as we want to use it in different places */ static int config__refresh(git_config_backend *cfg) { - git_strmap *values = NULL; + refcounted_strmap *values = NULL, *tmp; diskfile_backend *b = (diskfile_backend *)cfg; struct reader *reader = NULL; int error = 0; - if ((error = git_strmap_alloc(&values)) < 0) + if ((error = refcounted_strmap_alloc(&values)) < 0) goto out; reader = git_array_get(b->readers, git_array_size(b->readers) - 1); - if ((error = config_parse(values, b, reader, b->level, 0)) < 0) + if ((error = config_parse(values->values, b, reader, b->level, 0)) < 0) goto out; - values = git__swap(b->header.values, values); + git_mutex_lock(&b->header.values_mutex); + + tmp = b->header.values; + b->header.values = values; + values = tmp; + + git_mutex_unlock(&b->header.values_mutex); out: - free_vars(values); + refcounted_strmap_free(values); git_buf_free(&reader->buffer); return error; } @@ -321,7 +375,7 @@ static void backend_free(git_config_backend *_backend) git_array_clear(backend->readers); git__free(backend->file_path); - free_vars(backend->header.values); + refcounted_strmap_free(backend->header.values); git__free(backend); } @@ -338,7 +392,7 @@ static int config_iterator_next( { git_config_file_iter *it = (git_config_file_iter *) iter; diskfile_header *h = (diskfile_header *) it->parent.backend; - git_strmap *values = h->values; + git_strmap *values = h->values->values; int err = 0; cvar_t * var; @@ -397,7 +451,8 @@ static int config_iterator_new( static int config_set(git_config_backend *cfg, const char *name, const char *value) { diskfile_backend *b = (diskfile_backend *)cfg; - git_strmap *values = b->header.values; + refcounted_strmap *map; + git_strmap *values; char *key, *esc_value = NULL; khiter_t pos; int rval, ret; @@ -405,6 +460,9 @@ static int config_set(git_config_backend *cfg, const char *name, const char *val if ((rval = git_config__normalize_name(name, &key)) < 0) return rval; + map = refcounted_strmap_take(&b->header); + values = map->values; + /* * Try to find it in the existing values and update it if it * only has one value. @@ -414,17 +472,17 @@ static int config_set(git_config_backend *cfg, const char *name, const char *val cvar_t *existing = git_strmap_value_at(values, pos); if (existing->next != NULL) { - git__free(key); giterr_set(GITERR_CONFIG, "Multivar incompatible with simple set"); - return -1; + ret = -1; + goto out; } /* don't update if old and new values already match */ if ((!existing->entry->value && !value) || (existing->entry->value && value && !strcmp(existing->entry->value, value))) { - git__free(key); - return 0; + ret = 0; + goto out; } } @@ -441,6 +499,7 @@ static int config_set(git_config_backend *cfg, const char *name, const char *val ret = config_refresh(cfg); out: + refcounted_strmap_free(map); git__free(esc_value); git__free(key); return ret; @@ -452,6 +511,7 @@ out: static int config_get(git_config_backend *cfg, const char *key, const git_config_entry **out) { diskfile_header *h = (diskfile_header *)cfg; + refcounted_strmap *map; git_strmap *values; khiter_t pos; cvar_t *var; @@ -460,17 +520,22 @@ static int config_get(git_config_backend *cfg, const char *key, const git_config if (!h->readonly && ((error = config_refresh(cfg)) < 0)) return error; - values = h->values; + map = refcounted_strmap_take(h); + values = map->values; + pos = git_strmap_lookup_index(values, key); /* no error message; the config system will write one */ - if (!git_strmap_valid_index(values, pos)) + if (!git_strmap_valid_index(values, pos)) { + refcounted_strmap_free(map); return GIT_ENOTFOUND; + } var = git_strmap_value_at(values, pos); while (var->next) var = var->next; + refcounted_strmap_free(map); *out = var->entry; return 0; } @@ -479,7 +544,8 @@ static int config_set_multivar( git_config_backend *cfg, const char *name, const char *regexp, const char *value) { diskfile_backend *b = (diskfile_backend *)cfg; - git_strmap *values = b->header.values; + refcounted_strmap *map; + git_strmap *values; char *key; regex_t preg; int result; @@ -490,20 +556,23 @@ static int config_set_multivar( if ((result = git_config__normalize_name(name, &key)) < 0) return result; + map = refcounted_strmap_take(&b->header); + values = b->header.values->values; + pos = git_strmap_lookup_index(values, key); if (!git_strmap_valid_index(values, pos)) { /* If we don't have it, behave like a normal set */ result = config_set(cfg, name, value); + refcounted_strmap_free(map); git__free(key); return result; } result = regcomp(&preg, regexp, REG_EXTENDED); if (result < 0) { - git__free(key); giterr_set_regex(&preg, result); - regfree(&preg); - return -1; + result = -1; + goto out; } /* If we do have it, set call config_write() and reload */ @@ -513,6 +582,7 @@ static int config_set_multivar( result = config_refresh(cfg); out: + refcounted_strmap_free(map); git__free(key); regfree(&preg); @@ -523,7 +593,7 @@ static int config_delete(git_config_backend *cfg, const char *name) { cvar_t *var; diskfile_backend *b = (diskfile_backend *)cfg; - git_strmap *values = b->header.values; + refcounted_strmap *map; git_strmap *values; char *key; int result; khiter_t pos; @@ -531,15 +601,20 @@ static int config_delete(git_config_backend *cfg, const char *name) if ((result = git_config__normalize_name(name, &key)) < 0) return result; + map = refcounted_strmap_take(&b->header); + values = b->header.values->values; + pos = git_strmap_lookup_index(values, key); git__free(key); if (!git_strmap_valid_index(values, pos)) { + refcounted_strmap_free(map); giterr_set(GITERR_CONFIG, "Could not find key '%s' to delete", name); return GIT_ENOTFOUND; } var = git_strmap_value_at(values, pos); + refcounted_strmap_free(map); if (var->next != NULL) { giterr_set(GITERR_CONFIG, "Cannot delete multivar with a single delete"); @@ -555,7 +630,8 @@ static int config_delete(git_config_backend *cfg, const char *name) static int config_delete_multivar(git_config_backend *cfg, const char *name, const char *regexp) { diskfile_backend *b = (diskfile_backend *)cfg; - git_strmap *values = b->header.values; + refcounted_strmap *map; + git_strmap *values; char *key; regex_t preg; int result; @@ -564,14 +640,20 @@ static int config_delete_multivar(git_config_backend *cfg, const char *name, con if ((result = git_config__normalize_name(name, &key)) < 0) return result; + map = refcounted_strmap_take(&b->header); + values = b->header.values->values; + pos = git_strmap_lookup_index(values, key); if (!git_strmap_valid_index(values, pos)) { + refcounted_strmap_free(map); git__free(key); giterr_set(GITERR_CONFIG, "Could not find key '%s' to delete", name); return GIT_ENOTFOUND; } + refcounted_strmap_free(map); + result = regcomp(&preg, regexp, REG_EXTENDED); if (result < 0) { giterr_set_regex(&preg, result); @@ -676,7 +758,7 @@ static void backend_readonly_free(git_config_backend *_backend) if (backend == NULL) return; - free_vars(backend->header.values); + refcounted_strmap_free(backend->header.values); git__free(backend); } @@ -702,8 +784,8 @@ static int config_readonly_open(git_config_backend *cfg, git_config_level_t leve { diskfile_readonly_backend *b = (diskfile_readonly_backend *) cfg; diskfile_backend *src = b->snapshot_from; - git_strmap *src_values = src->header.values; - git_strmap *values; + refcounted_strmap *src_map; + git_strmap *src_values, *values; git_strmap_iter i; cvar_t *src_var; int error; @@ -711,10 +793,12 @@ static int config_readonly_open(git_config_backend *cfg, git_config_level_t leve /* We're just copying data, don't care about the level */ GIT_UNUSED(level); - if ((error = git_strmap_alloc(&b->header.values)) < 0) + if ((error = refcounted_strmap_alloc(&b->header.values)) < 0) return error; - values = b->header.values; + src_map = refcounted_strmap_take(&src->header); + src_values = src->header.values->values; + values = b->header.values->values; i = git_strmap_begin(src_values); while ((error = git_strmap_next((void **) &src_var, &i, src_values)) == 0) { @@ -725,8 +809,11 @@ static int config_readonly_open(git_config_backend *cfg, git_config_level_t leve var = git__calloc(1, sizeof(cvar_t)); GITERR_CHECK_ALLOC(var); - if (config_entry_dup(&entry, src_var->entry) < 0) + if (config_entry_dup(&entry, src_var->entry) < 0) { + refcounted_strmap_free(b->header.values); + refcounted_strmap_free(src_map); return -1; + } var->entry = entry; @@ -738,6 +825,7 @@ static int config_readonly_open(git_config_backend *cfg, git_config_level_t leve if (error == GIT_ITEROVER) error = 0; + refcounted_strmap_free(src_map); return error; } From 2280b388c913cbc4eee35ce99c760316206e2703 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 9 Apr 2014 13:17:51 +0200 Subject: [PATCH 12/13] config: share the strmap on snapshot Now that our strmap is no longer modified but replaced, we can use the same strmap for the snapshot's values and it will be freed when we don't need it anymore. --- src/config_file.c | 56 ++--------------------------------------------- 1 file changed, 2 insertions(+), 54 deletions(-) diff --git a/src/config_file.c b/src/config_file.c index c53ec015f..c09a032be 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -762,71 +762,19 @@ static void backend_readonly_free(git_config_backend *_backend) git__free(backend); } -static int config_entry_dup(git_config_entry **out, git_config_entry *src) -{ - git_config_entry *entry; - - entry = git__calloc(1, sizeof(git_config_entry)); - GITERR_CHECK_ALLOC(entry); - - entry->level = src->level; - entry->name = git__strdup(src->name); - GITERR_CHECK_ALLOC(entry->name); - entry->value = git__strdup(src->value); - GITERR_CHECK_ALLOC(entry->value); - - *out = entry; - - return 0; -} - static int config_readonly_open(git_config_backend *cfg, git_config_level_t level) { diskfile_readonly_backend *b = (diskfile_readonly_backend *) cfg; diskfile_backend *src = b->snapshot_from; refcounted_strmap *src_map; - git_strmap *src_values, *values; - git_strmap_iter i; - cvar_t *src_var; - int error; /* We're just copying data, don't care about the level */ GIT_UNUSED(level); - if ((error = refcounted_strmap_alloc(&b->header.values)) < 0) - return error; - src_map = refcounted_strmap_take(&src->header); - src_values = src->header.values->values; - values = b->header.values->values; + b->header.values = src_map; - i = git_strmap_begin(src_values); - while ((error = git_strmap_next((void **) &src_var, &i, src_values)) == 0) { - do { - git_config_entry *entry; - cvar_t *var; - - var = git__calloc(1, sizeof(cvar_t)); - GITERR_CHECK_ALLOC(var); - - if (config_entry_dup(&entry, src_var->entry) < 0) { - refcounted_strmap_free(b->header.values); - refcounted_strmap_free(src_map); - return -1; - } - - var->entry = entry; - - error = append_entry(values, var); - src_var = CVAR_LIST_NEXT(src_var); - } while (src_var != NULL && error == 0); - } - - if (error == GIT_ITEROVER) - error = 0; - - refcounted_strmap_free(src_map); - return error; + return 0; } int git_config_file__snapshot(git_config_backend **out, diskfile_backend *in) From ac99d86ba5e2a9d2332b7f82737e1231c621dc43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 7 May 2014 11:34:32 +0200 Subject: [PATCH 13/13] repository: introduce a convenience config snapshot method Accessing the repository's config and immediately taking a snapshot of it is a common operation, so let's provide a convenience function for it. --- include/git2/repository.h | 14 +++++++++++++- src/branch.c | 7 ++----- src/diff_driver.c | 5 +---- src/pack-objects.c | 7 ++----- src/remote.c | 7 ++----- src/repository.c | 17 ++++++++++++----- src/signature.c | 7 ++----- 7 files changed, 34 insertions(+), 30 deletions(-) diff --git a/include/git2/repository.h b/include/git2/repository.h index 4433e71a2..37140d48a 100644 --- a/include/git2/repository.h +++ b/include/git2/repository.h @@ -409,12 +409,24 @@ GIT_EXTERN(int) git_repository_is_bare(git_repository *repo); * The configuration file must be freed once it's no longer * being used by the user. * - * @param out Pointer to store the loaded config file + * @param out Pointer to store the loaded configuration * @param repo A repository object * @return 0, or an error code */ GIT_EXTERN(int) git_repository_config(git_config **out, git_repository *repo); +/** + * Get a snapshot of the repository's configuration + * + * Convenience function to take a snapshot from the repository's + * configuration. + * + * @param out Pointer to store the loaded configuration + * @param repo the repository + * @return 0, or an error code + */ +GIT_EXTERN(int) git_repository_config_snapshot(git_config **out, git_repository *repo); + /** * Get the Object Database for this repository. * diff --git a/src/branch.c b/src/branch.c index d8c82b73e..52760853b 100644 --- a/src/branch.c +++ b/src/branch.c @@ -332,7 +332,7 @@ int git_branch_upstream_name( int error = -1; git_remote *remote = NULL; const git_refspec *refspec; - git_config *config, *repo_config; + git_config *config; assert(out && refname); @@ -341,10 +341,7 @@ int git_branch_upstream_name( if (!git_reference__is_branch(refname)) return not_a_local_branch(refname); - if ((error = git_repository_config__weakptr(&repo_config, repo)) < 0) - return error; - - if ((error = git_config_snapshot(&config, repo_config)) < 0) + if ((error = git_repository_config_snapshot(&config, repo)) < 0) return error; if ((error = retrieve_upstream_configuration( diff --git a/src/diff_driver.c b/src/diff_driver.c index 6e87fd6f8..28c0a6b17 100644 --- a/src/diff_driver.c +++ b/src/diff_driver.c @@ -234,14 +234,11 @@ static int git_diff_driver_load( } /* if you can't read config for repo, just use default driver */ - if (git_repository_config__weakptr(&repo_cfg, repo) < 0) { + if (git_repository_config_snapshot(&cfg, repo) < 0) { giterr_clear(); goto done; } - if ((error = git_config_snapshot(&cfg, repo_cfg)) < 0) - return error; - drv = git__calloc(1, sizeof(git_diff_driver) + namelen + 1); GITERR_CHECK_ALLOC(drv); drv->type = DIFF_DRIVER_AUTO; diff --git a/src/pack-objects.c b/src/pack-objects.c index e7c7f391f..882ddf544 100644 --- a/src/pack-objects.c +++ b/src/pack-objects.c @@ -86,14 +86,11 @@ static unsigned name_hash(const char *name) static int packbuilder_config(git_packbuilder *pb) { - git_config *config, *repo_config; + git_config *config; int ret; int64_t val; - if ((ret = git_repository_config__weakptr(&repo_config, pb->repo)) < 0) - return ret; - - if ((ret = git_config_snapshot(&config, repo_config)) < 0) + if ((ret = git_repository_config_snapshot(&config, pb->repo)) < 0) return ret; #define config_get(KEY,DST,DFLT) do { \ diff --git a/src/remote.c b/src/remote.c index 3a754d4bd..0a6b72fb9 100644 --- a/src/remote.c +++ b/src/remote.c @@ -347,7 +347,7 @@ int git_remote_load(git_remote **out, git_repository *repo, const char *name) git_buf buf = GIT_BUF_INIT; const char *val; int error = 0; - git_config *config, *repo_config; + git_config *config; struct refspec_cb_data data = { NULL }; bool optional_setting_found = false, found; @@ -356,10 +356,7 @@ int git_remote_load(git_remote **out, git_repository *repo, const char *name) if ((error = ensure_remote_name_is_valid(name)) < 0) return error; - if (git_repository_config__weakptr(&repo_config, repo) < 0) - return -1; - - if ((error = git_config_snapshot(&config, repo_config)) < 0) + if ((error = git_repository_config_snapshot(&config, repo)) < 0) return error; remote = git__malloc(sizeof(git_remote)); diff --git a/src/repository.c b/src/repository.c index c1d98300c..dfddc5966 100644 --- a/src/repository.c +++ b/src/repository.c @@ -439,7 +439,7 @@ int git_repository_open_ext( int error; git_buf path = GIT_BUF_INIT, parent = GIT_BUF_INIT; git_repository *repo; - git_config *repo_config, *config; + git_config *config; if (repo_ptr) *repo_ptr = NULL; @@ -454,10 +454,7 @@ int git_repository_open_ext( repo->path_repository = git_buf_detach(&path); GITERR_CHECK_ALLOC(repo->path_repository); - if ((error = git_repository_config__weakptr(&repo_config, repo)) < 0) - return error; - - if ((error = git_config_snapshot(&config, repo_config)) < 0) + if ((error = git_repository_config_snapshot(&config, repo)) < 0) return error; if ((flags & GIT_REPOSITORY_OPEN_BARE) != 0) @@ -624,6 +621,16 @@ int git_repository_config(git_config **out, git_repository *repo) return 0; } +int git_repository_config_snapshot(git_config **out, git_repository *repo) +{ + git_config *weak; + + if (git_repository_config__weakptr(&weak, repo) < 0) + return -1; + + return git_config_snapshot(out, weak); +} + void git_repository_set_config(git_repository *repo, git_config *config) { assert(repo && config); diff --git a/src/signature.c b/src/signature.c index b0ee0da74..2545b7519 100644 --- a/src/signature.c +++ b/src/signature.c @@ -141,13 +141,10 @@ int git_signature_now(git_signature **sig_out, const char *name, const char *ema int git_signature_default(git_signature **out, git_repository *repo) { int error; - git_config *cfg, *repo_cfg; + git_config *cfg; const char *user_name, *user_email; - if ((error = git_repository_config__weakptr(&repo_cfg, repo)) < 0) - return error; - - if ((error = git_config_snapshot(&cfg, repo_cfg)) < 0) + if ((error = git_repository_config_snapshot(&cfg, repo)) < 0) return error; if (!(error = git_config_get_string(&user_name, cfg, "user.name")) &&