From 3793fa9b181f3595c24a1cc517646f8e7a4a7175 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Rodri=CC=81guez=20Troitin=CC=83o?= Date: Thu, 31 Oct 2013 01:08:50 +0100 Subject: [PATCH 1/3] Fix saving remotes with several fetch/push ref specs. At some moment git_config_delete_entry lost the ability to delete one entry of a multivar configuration. The moment you had more than one fetch or push ref spec for a remote you will not be able to save that remote anymore. The changes in network::remote::remotes::save show that problem. I needed to create a new git_config_delete_multivar because I was not able to remove one or several entries of a multivar config with the current API. Several tries modifying how git_config_set_multivar(..., NULL) behaved were not successful. git_config_delete_multivar is very similar to git_config_set_multivar, and delegates into config_delete_multivar of config_file. This function search for the cvar_t that will be deleted, storing them in a temporal array, and rebuilding the linked list. After calling config_write to delete the entries, the cvar_t stored in the temporal array are freed. There is a little fix in config_write, it avoids an infinite loop when using a regular expression (case for the multivars). This error was found by the test network::remote::remotes::tagopt. --- include/git2/config.h | 11 ++++ include/git2/sys/config.h | 1 + src/config.c | 13 +++++ src/config_file.c | 90 ++++++++++++++++++++++++++++- src/remote.c | 6 +- tests-clar/config/multivar.c | 65 +++++++++++++++++++++ tests-clar/network/remote/remotes.c | 25 +++++--- 7 files changed, 199 insertions(+), 12 deletions(-) diff --git a/include/git2/config.h b/include/git2/config.h index f14415148..95da4bc03 100644 --- a/include/git2/config.h +++ b/include/git2/config.h @@ -434,6 +434,17 @@ GIT_EXTERN(int) git_config_set_multivar(git_config *cfg, const char *name, const */ GIT_EXTERN(int) git_config_delete_entry(git_config *cfg, const char *name); +/** + * Deletes one or several entries from a multivar in the local config file. + * + * @param cfg where to look for the variables + * @param name the variable's name + * @param regexp a regular expression to indicate which values to delete + * + * @return 0 or an error code + */ +GIT_EXTERN(int) git_config_delete_multivar(git_config *cfg, const char *name, const char *regexp); + /** * Perform an operation on each config variable. * diff --git a/include/git2/sys/config.h b/include/git2/sys/config.h index 7572ace51..419ad7ea7 100644 --- a/include/git2/sys/config.h +++ b/include/git2/sys/config.h @@ -61,6 +61,7 @@ struct git_config_backend { 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); + 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 *); void (*free)(struct git_config_backend *); diff --git a/src/config.c b/src/config.c index c98d6a52d..0d9471383 100644 --- a/src/config.c +++ b/src/config.c @@ -862,6 +862,19 @@ int git_config_set_multivar(git_config *cfg, const char *name, const char *regex return file->set_multivar(file, name, regexp, value); } +int git_config_delete_multivar(git_config *cfg, const char *name, const char *regexp) +{ + git_config_backend *file; + file_internal *internal; + + internal = git_vector_get(&cfg->files, 0); + if (!internal || !internal->file) + return config_error_nofiles(name); + file = internal->file; + + return file->del_multivar(file, name, regexp); +} + int git_config_next(git_config_entry **entry, git_config_iterator *iter) { return iter->next(entry, iter); diff --git a/src/config_file.c b/src/config_file.c index 8fb43b990..8c3d8121c 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -120,6 +120,18 @@ static void cvar_free(cvar_t *var) git__free(var); } +static int cvar_length(cvar_t *var) +{ + int length = 0; + + while (var) { + length += 1; + var = var->next; + } + + return length; +} + int git_config_file_normalize_section(char *start, char *end) { char *scan; @@ -531,6 +543,81 @@ static int config_delete(git_config_backend *cfg, const char *name) return result; } +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; + char *key; + regex_t preg; + int result; + khiter_t pos; + + 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)) { + 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); + + result = regcomp(&preg, regexp, REG_EXTENDED); + if (result < 0) { + git__free(key); + giterr_set_regex(&preg, result); + regfree(&preg); + return -1; + } + + to_delete = git__calloc(cvar_length(var), sizeof(cvar_t *)); + GITERR_CHECK_ALLOC(to_delete); + to_delete_idx = 0; + + for (;;) { + cvar_t *var_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 = var_next; + } else { + new_head = var_next; + } + + to_delete[to_delete_idx++] = var; + } else { + prev = var; + } + + if (var_next == NULL) + break; + + var = var_next; + } + + if (new_head != NULL) { + git_strmap_set_value_at(b->values, pos, new_head); + } else { + git_strmap_delete_at(b->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]); + + git__free(key); + return result; +} + int git_config_file__ondisk(git_config_backend **out, const char *path) { diskfile_backend *backend; @@ -548,6 +635,7 @@ int git_config_file__ondisk(git_config_backend **out, const char *path) 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; @@ -1214,7 +1302,7 @@ static int config_write(diskfile_backend *cfg, const char *key, const regex_t *p } /* multiline variable? we need to keep reading lines to match */ - if (preg != NULL) { + if (preg != NULL && section_matches) { data_start = post_start; continue; } diff --git a/src/remote.c b/src/remote.c index bdfa08642..e2b40347a 100644 --- a/src/remote.c +++ b/src/remote.c @@ -365,16 +365,18 @@ static int update_config_refspec(const git_remote *remote, git_config *config, i const char *dir; size_t i; int error = 0; + const char *cname; push = direction == GIT_DIRECTION_PUSH; dir = push ? "push" : "fetch"; if (git_buf_printf(&name, "remote.%s.%s", remote->name, dir) < 0) return -1; + cname = git_buf_cstr(&name); /* Clear out the existing config */ while (!error) - error = git_config_delete_entry(config, git_buf_cstr(&name)); + error = git_config_delete_multivar(config, cname, ".*"); if (error != GIT_ENOTFOUND) return error; @@ -386,7 +388,7 @@ static int update_config_refspec(const git_remote *remote, git_config *config, i continue; if ((error = git_config_set_multivar( - config, git_buf_cstr(&name), "", spec->string)) < 0) { + config, cname, "", spec->string)) < 0) { goto cleanup; } } diff --git a/tests-clar/config/multivar.c b/tests-clar/config/multivar.c index 0d552d65e..afdb1e5f4 100644 --- a/tests-clar/config/multivar.c +++ b/tests-clar/config/multivar.c @@ -221,3 +221,68 @@ void test_config_multivar__replace_multiple(void) git_config_free(cfg); } + +void test_config_multivar__delete(void) +{ + git_config *cfg; + int n; + + cl_git_pass(git_config_open_ondisk(&cfg, "config/config11")); + + n = 0; + cl_git_pass(git_config_get_multivar_foreach(cfg, _name, NULL, cb, &n)); + cl_assert(n == 2); + + 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); + + git_config_free(cfg); + + cl_git_pass(git_config_open_ondisk(&cfg, "config/config11")); + + n = 0; + cl_git_pass(git_config_get_multivar_foreach(cfg, _name, NULL, cb, &n)); + cl_assert(n == 1); + + git_config_free(cfg); +} + +void test_config_multivar__delete_multiple(void) +{ + git_config *cfg; + int n; + + cl_git_pass(git_config_open_ondisk(&cfg, "config/config11")); + + n = 0; + cl_git_pass(git_config_get_multivar_foreach(cfg, _name, NULL, cb, &n)); + cl_assert(n == 2); + + cl_git_pass(git_config_delete_multivar(cfg, _name, "git")); + + n = 0; + cl_git_fail_with(git_config_get_multivar_foreach(cfg, _name, NULL, cb, &n), GIT_ENOTFOUND); + + git_config_free(cfg); + + cl_git_pass(git_config_open_ondisk(&cfg, "config/config11")); + + n = 0; + cl_git_fail_with(git_config_get_multivar_foreach(cfg, _name, NULL, cb, &n), GIT_ENOTFOUND); + + git_config_free(cfg); +} + +void test_config_multivar__delete_notfound(void) +{ + git_config *cfg; + + cl_git_pass(git_config_open_ondisk(&cfg, "config/config11")); + + cl_git_fail_with(git_config_delete_multivar(cfg, "remote.ab.noturl", "git"), GIT_ENOTFOUND); + + git_config_free(cfg); +} diff --git a/tests-clar/network/remote/remotes.c b/tests-clar/network/remote/remotes.c index 6e0eeeb05..7c79b8318 100644 --- a/tests-clar/network/remote/remotes.c +++ b/tests-clar/network/remote/remotes.c @@ -150,8 +150,10 @@ void test_network_remote_remotes__add_pushspec(void) void test_network_remote_remotes__save(void) { git_strarray array; - const char *fetch_refspec = "refs/heads/*:refs/remotes/upstream/*"; - const char *push_refspec = "refs/heads/*:refs/heads/*"; + const char *fetch_refspec1 = "refs/heads/ns1/*:refs/remotes/upstream/ns1/*"; + const char *fetch_refspec2 = "refs/heads/ns2/*:refs/remotes/upstream/ns2/*"; + const char *push_refspec1 = "refs/heads/ns1/*:refs/heads/ns1/*"; + const char *push_refspec2 = "refs/heads/ns2/*:refs/heads/ns2/*"; git_remote_free(_remote); _remote = NULL; @@ -160,8 +162,10 @@ void test_network_remote_remotes__save(void) cl_git_pass(git_remote_create(&_remote, _repo, "upstream", "git://github.com/libgit2/libgit2")); git_remote_clear_refspecs(_remote); - cl_git_pass(git_remote_add_fetch(_remote, fetch_refspec)); - cl_git_pass(git_remote_add_push(_remote, push_refspec)); + cl_git_pass(git_remote_add_fetch(_remote, fetch_refspec1)); + cl_git_pass(git_remote_add_fetch(_remote, fetch_refspec2)); + cl_git_pass(git_remote_add_push(_remote, push_refspec1)); + cl_git_pass(git_remote_add_push(_remote, push_refspec2)); cl_git_pass(git_remote_set_pushurl(_remote, "git://github.com/libgit2/libgit2_push")); cl_git_pass(git_remote_save(_remote)); git_remote_free(_remote); @@ -171,16 +175,19 @@ void test_network_remote_remotes__save(void) cl_git_pass(git_remote_load(&_remote, _repo, "upstream")); cl_git_pass(git_remote_get_fetch_refspecs(&array, _remote)); - cl_assert_equal_i(1, (int)array.count); - cl_assert_equal_s(fetch_refspec, array.strings[0]); + cl_assert_equal_i(2, (int)array.count); + cl_assert_equal_s(fetch_refspec1, array.strings[0]); + cl_assert_equal_s(fetch_refspec2, array.strings[1]); git_strarray_free(&array); cl_git_pass(git_remote_get_push_refspecs(&array, _remote)); - cl_assert_equal_i(1, (int)array.count); - cl_assert_equal_s(push_refspec, array.strings[0]); + cl_assert_equal_i(2, (int)array.count); + cl_assert_equal_s(push_refspec1, array.strings[0]); + cl_assert_equal_s(push_refspec2, array.strings[1]); + git_strarray_free(&array); + cl_assert_equal_s(git_remote_url(_remote), "git://github.com/libgit2/libgit2"); cl_assert_equal_s(git_remote_pushurl(_remote), "git://github.com/libgit2/libgit2_push"); - git_strarray_free(&array); /* remove the pushurl again and see if we can save that too */ cl_git_pass(git_remote_set_pushurl(_remote, NULL)); From a71331ebc40b445a58d73a1fe3fb2dc21665c09c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Rodri=CC=81guez=20Troitin=CC=83o?= Date: Thu, 31 Oct 2013 23:41:48 +0100 Subject: [PATCH 2/3] Fix memory leaks. --- src/config_file.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/config_file.c b/src/config_file.c index 8c3d8121c..5f36a3f8b 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -615,6 +615,8 @@ static int config_delete_multivar(git_config_backend *cfg, const char *name, con cvar_free(to_delete[to_delete_idx]); git__free(key); + git__free(to_delete); + regfree(&preg); return result; } From 376454d03dbb0c78b1266a85b29ec8bf48930a4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Rodri=CC=81guez=20Troitin=CC=83o?= Date: Thu, 31 Oct 2013 23:42:04 +0100 Subject: [PATCH 3/3] Set new multivar values using unmatcheable regexp. Seems that regexp in Mac OS X and Linux were behaving differently: while in OS X the empty string didn't match any value, in Linux it was matching all of them, so the the second fetch refspec was overwritting the first one, instead of creating a new one. Using an unmatcheable regular expression solves the problem (and seems to be portable). --- src/remote.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/remote.c b/src/remote.c index e2b40347a..3528b1c46 100644 --- a/src/remote.c +++ b/src/remote.c @@ -387,8 +387,11 @@ static int update_config_refspec(const git_remote *remote, git_config *config, i if (spec->push != push) continue; + // "$^" is a unmatcheable regexp: it will not match anything at all, so + // all values will be considered new and we will not replace any + // present value. if ((error = git_config_set_multivar( - config, cname, "", spec->string)) < 0) { + config, cname, "$^", spec->string)) < 0) { goto cleanup; } }