From b83c92dd6fe11adae0ff29e1db381c31f0f88cb7 Mon Sep 17 00:00:00 2001 From: nulltoken Date: Wed, 21 Aug 2013 13:16:17 +0200 Subject: [PATCH 1/6] remote: Assert proper GIT_DIRECTION_XXXX values --- src/remote.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/remote.c b/src/remote.c index 7677e56b2..14ed740a8 100644 --- a/src/remote.c +++ b/src/remote.c @@ -534,6 +534,8 @@ const char* git_remote__urlfordirection(git_remote *remote, int direction) { assert(remote); + assert(direction == GIT_DIRECTION_FETCH || direction == GIT_DIRECTION_PUSH); + if (direction == GIT_DIRECTION_FETCH) { return remote->url; } From 44bc0c6ac3b939d3dfc1102be77e82e00e919ae4 Mon Sep 17 00:00:00 2001 From: nulltoken Date: Wed, 21 Aug 2013 13:20:17 +0200 Subject: [PATCH 2/6] remote: Warn the user when connecting with no url --- src/remote.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/remote.c b/src/remote.c index 14ed740a8..948c755bb 100644 --- a/src/remote.c +++ b/src/remote.c @@ -558,8 +558,11 @@ int git_remote_connect(git_remote *remote, git_direction direction) t = remote->transport; url = git_remote__urlfordirection(remote, direction); - if (url == NULL ) + if (url == NULL ) { + giterr_set(GITERR_INVALID, + "Malformed remote '%s' - missing URL", remote->name); return -1; + } /* A transport could have been supplied in advance with * git_remote_set_transport */ From ece24ef7c4bb31eb2c715948bcf6dff6ed9d7dfc Mon Sep 17 00:00:00 2001 From: nulltoken Date: Wed, 21 Aug 2013 13:37:21 +0200 Subject: [PATCH 3/6] remote: Don't parse missing urls as empty strings --- src/remote.c | 2 +- tests-clar/network/remote/remotes.c | 6 ++++++ tests-clar/resources/testrepo.git/config | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/remote.c b/src/remote.c index 948c755bb..4bba1d57e 100644 --- a/src/remote.c +++ b/src/remote.c @@ -308,7 +308,7 @@ int git_remote_load(git_remote **out, git_repository *repo, const char *name) if ((error = get_optional_config(config, &buf, NULL, (void *)&val)) < 0) goto cleanup; - if (val) { + if (val && strlen(val) > 0) { remote->pushurl = git__strdup(val); GITERR_CHECK_ALLOC(remote->pushurl); } diff --git a/tests-clar/network/remote/remotes.c b/tests-clar/network/remote/remotes.c index dec646526..e356526ed 100644 --- a/tests-clar/network/remote/remotes.c +++ b/tests-clar/network/remote/remotes.c @@ -367,8 +367,14 @@ void test_network_remote_remotes__can_load_with_an_empty_url(void) cl_git_pass(git_remote_load(&remote, _repo, "empty-remote-url")); + cl_assert(remote->url == NULL); + cl_assert(remote->pushurl == NULL); + cl_git_fail(git_remote_connect(remote, GIT_DIRECTION_FETCH)); + cl_assert(giterr_last() != NULL); + cl_assert(giterr_last()->klass == GITERR_INVALID); + git_remote_free(remote); } diff --git a/tests-clar/resources/testrepo.git/config b/tests-clar/resources/testrepo.git/config index 904a4e3f3..1264f6ea7 100644 --- a/tests-clar/resources/testrepo.git/config +++ b/tests-clar/resources/testrepo.git/config @@ -10,7 +10,7 @@ url = git://github.com/libgit2/libgit2 [remote "empty-remote-url"] url = - + pushurl = [remote "test_with_pushurl"] url = git://github.com/libgit2/fetchlibgit2 pushurl = git://github.com/libgit2/pushlibgit2 From c9ffa84bde45021c40623553822916fb3d13b20a Mon Sep 17 00:00:00 2001 From: nulltoken Date: Wed, 21 Aug 2013 16:04:25 +0200 Subject: [PATCH 4/6] remote: Relax the parsing logic even more In order to be loaded, a remote needs to be configured with at least a `url` or a `pushurl`. ENOTFOUND will be returned when trying to git_remote_load() a remote with neither of these entries defined. --- src/remote.c | 33 ++++++++++++++++++------ tests-clar/network/remote/remotes.c | 22 ++++++++++++++++ tests-clar/resources/testrepo.git/config | 4 +++ 3 files changed, 51 insertions(+), 8 deletions(-) diff --git a/src/remote.c b/src/remote.c index 4bba1d57e..7276daa39 100644 --- a/src/remote.c +++ b/src/remote.c @@ -233,7 +233,8 @@ static int refspec_cb(const git_config_entry *entry, void *payload) } static int get_optional_config( - git_config *config, git_buf *buf, git_config_foreach_cb cb, void *payload) + bool *found, git_config *config, git_buf *buf, + git_config_foreach_cb cb, void *payload) { int error = 0; const char *key = git_buf_cstr(buf); @@ -246,6 +247,9 @@ static int get_optional_config( else error = git_config_get_string(payload, config, key); + if (found) + *found = !error; + if (error == GIT_ENOTFOUND) { giterr_clear(); error = 0; @@ -265,6 +269,7 @@ int git_remote_load(git_remote **out, git_repository *repo, const char *name) int error = 0; git_config *config; struct refspec_cb_data data; + bool optional_setting_found = false, found; assert(out && repo && name); @@ -294,21 +299,33 @@ int git_remote_load(git_remote **out, git_repository *repo, const char *name) goto cleanup; } - if ((error = git_config_get_string(&val, config, git_buf_cstr(&buf))) < 0) + if ((error = get_optional_config(&found, config, &buf, NULL, (void *)&val)) < 0) goto cleanup; + optional_setting_found |= found; + remote->repo = repo; - remote->url = git__strdup(val); - GITERR_CHECK_ALLOC(remote->url); + + if (found && strlen(val) > 0) { + remote->url = git__strdup(val); + GITERR_CHECK_ALLOC(remote->url); + } val = NULL; git_buf_clear(&buf); git_buf_printf(&buf, "remote.%s.pushurl", name); - if ((error = get_optional_config(config, &buf, NULL, (void *)&val)) < 0) + if ((error = get_optional_config(&found, config, &buf, NULL, (void *)&val)) < 0) goto cleanup; - if (val && strlen(val) > 0) { + optional_setting_found |= found; + + if (!optional_setting_found) { + error = GIT_ENOTFOUND; + goto cleanup; + } + + if (found && strlen(val) > 0) { remote->pushurl = git__strdup(val); GITERR_CHECK_ALLOC(remote->pushurl); } @@ -318,14 +335,14 @@ int git_remote_load(git_remote **out, git_repository *repo, const char *name) git_buf_clear(&buf); git_buf_printf(&buf, "remote.%s.fetch", name); - if ((error = get_optional_config(config, &buf, refspec_cb, &data)) < 0) + if ((error = get_optional_config(NULL, config, &buf, refspec_cb, &data)) < 0) goto cleanup; data.fetch = false; git_buf_clear(&buf); git_buf_printf(&buf, "remote.%s.push", name); - if ((error = get_optional_config(config, &buf, refspec_cb, &data)) < 0) + if ((error = get_optional_config(NULL, config, &buf, refspec_cb, &data)) < 0) goto cleanup; if (download_tags_value(remote, config) < 0) diff --git a/tests-clar/network/remote/remotes.c b/tests-clar/network/remote/remotes.c index e356526ed..55b233eda 100644 --- a/tests-clar/network/remote/remotes.c +++ b/tests-clar/network/remote/remotes.c @@ -378,6 +378,28 @@ void test_network_remote_remotes__can_load_with_an_empty_url(void) git_remote_free(remote); } +void test_network_remote_remotes__can_load_with_only_an_empty_pushurl(void) +{ + git_remote *remote = NULL; + + cl_git_pass(git_remote_load(&remote, _repo, "empty-remote-pushurl")); + + cl_assert(remote->url == NULL); + cl_assert(remote->pushurl == NULL); + + cl_git_fail(git_remote_connect(remote, GIT_DIRECTION_FETCH)); + + git_remote_free(remote); +} + +void test_network_remote_remotes__returns_ENOTFOUND_when_neither_url_nor_pushurl(void) +{ + git_remote *remote = NULL; + + cl_git_fail_with( + git_remote_load(&remote, _repo, "no-remote-url"), GIT_ENOTFOUND); +} + void test_network_remote_remotes__check_structure_version(void) { git_transport transport = GIT_TRANSPORT_INIT; diff --git a/tests-clar/resources/testrepo.git/config b/tests-clar/resources/testrepo.git/config index 1264f6ea7..dfab4eeb4 100644 --- a/tests-clar/resources/testrepo.git/config +++ b/tests-clar/resources/testrepo.git/config @@ -11,6 +11,10 @@ [remote "empty-remote-url"] url = pushurl = +[remote "empty-remote-pushurl"] + pushurl = +[remote "no-remote-url"] + fetch = [remote "test_with_pushurl"] url = git://github.com/libgit2/fetchlibgit2 pushurl = git://github.com/libgit2/pushlibgit2 From 191adce8751e728111a3b1c3e9b2c02fe9f5d775 Mon Sep 17 00:00:00 2001 From: nulltoken Date: Tue, 27 Aug 2013 20:00:28 +0200 Subject: [PATCH 5/6] vector: Teach git_vector_uniq() to free while deduplicating --- src/util.h | 5 ++++- src/vector.c | 9 ++++++--- src/vector.h | 2 +- tests-clar/core/vector.c | 2 +- 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/util.h b/src/util.h index a784390c1..bd93b46b5 100644 --- a/src/util.h +++ b/src/util.h @@ -82,7 +82,10 @@ GIT_INLINE(void *) git__realloc(void *ptr, size_t size) return new_ptr; } -#define git__free(ptr) free(ptr) +GIT_INLINE(void) git__free(void *ptr) +{ + free(ptr); +} #define STRCMP_CASESELECT(IGNORE_CASE, STR1, STR2) \ ((IGNORE_CASE) ? strcasecmp((STR1), (STR2)) : strcmp((STR1), (STR2))) diff --git a/src/vector.c b/src/vector.c index 5ba2fab18..362e7b0c0 100644 --- a/src/vector.c +++ b/src/vector.c @@ -220,7 +220,7 @@ void git_vector_pop(git_vector *v) v->length--; } -void git_vector_uniq(git_vector *v) +void git_vector_uniq(git_vector *v, void (*git_free_cb)(void *)) { git_vector_cmp cmp; size_t i, j; @@ -232,9 +232,12 @@ void git_vector_uniq(git_vector *v) cmp = v->_cmp ? v->_cmp : strict_comparison; for (i = 0, j = 1 ; j < v->length; ++j) - if (!cmp(v->contents[i], v->contents[j])) + if (!cmp(v->contents[i], v->contents[j])) { + if (git_free_cb) + git_free_cb(v->contents[i]); + v->contents[i] = v->contents[j]; - else + } else v->contents[++i] = v->contents[j]; v->length -= j - i - 1; diff --git a/src/vector.h b/src/vector.h index 1bda9c93d..cca846f43 100644 --- a/src/vector.h +++ b/src/vector.h @@ -71,7 +71,7 @@ int git_vector_insert_sorted(git_vector *v, void *element, int (*on_dup)(void **old, void *new)); int git_vector_remove(git_vector *v, size_t idx); void git_vector_pop(git_vector *v); -void git_vector_uniq(git_vector *v); +void git_vector_uniq(git_vector *v, void (*git_free_cb)(void *)); void git_vector_remove_matching( git_vector *v, int (*match)(const git_vector *v, size_t idx)); diff --git a/tests-clar/core/vector.c b/tests-clar/core/vector.c index c9e43a149..db52c004f 100644 --- a/tests-clar/core/vector.c +++ b/tests-clar/core/vector.c @@ -54,7 +54,7 @@ void test_core_vector__2(void) cl_git_pass(git_vector_insert(&x, ptrs[1])); cl_assert(x.length == 5); - git_vector_uniq(&x); + git_vector_uniq(&x, NULL); cl_assert(x.length == 2); git_vector_free(&x); From aec87f712fd1e84038d5d14b83a97d78e2e1b1ad Mon Sep 17 00:00:00 2001 From: nulltoken Date: Tue, 27 Aug 2013 19:14:18 +0200 Subject: [PATCH 6/6] remote: Make git_remote_list() detect pushurl --- src/remote.c | 6 ++++-- tests-clar/network/remote/remotes.c | 10 ++++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/remote.c b/src/remote.c index 7276daa39..10c1b5b81 100644 --- a/src/remote.c +++ b/src/remote.c @@ -1109,10 +1109,10 @@ int git_remote_list(git_strarray *remotes_list, git_repository *repo) if (git_repository_config__weakptr(&cfg, repo) < 0) return -1; - if (git_vector_init(&list, 4, NULL) < 0) + if (git_vector_init(&list, 4, git__strcmp_cb) < 0) return -1; - if (regcomp(&preg, "^remote\\.(.*)\\.url$", REG_EXTENDED) < 0) { + if (regcomp(&preg, "^remote\\.(.*)\\.(push)?url$", REG_EXTENDED) < 0) { giterr_set(GITERR_OS, "Remote catch regex failed to compile"); return -1; } @@ -1137,6 +1137,8 @@ int git_remote_list(git_strarray *remotes_list, git_repository *repo) return error; } + git_vector_uniq(&list, git__free); + remotes_list->strings = (char **)list.contents; remotes_list->count = list.length; diff --git a/tests-clar/network/remote/remotes.c b/tests-clar/network/remote/remotes.c index 55b233eda..6e0eeeb05 100644 --- a/tests-clar/network/remote/remotes.c +++ b/tests-clar/network/remote/remotes.c @@ -243,13 +243,19 @@ void test_network_remote_remotes__list(void) git_config *cfg; cl_git_pass(git_remote_list(&list, _repo)); - cl_assert(list.count == 4); + cl_assert(list.count == 5); git_strarray_free(&list); cl_git_pass(git_repository_config(&cfg, _repo)); + + /* Create a new remote */ cl_git_pass(git_config_set_string(cfg, "remote.specless.url", "http://example.com")); + + /* Update a remote (previously without any url/pushurl entry) */ + cl_git_pass(git_config_set_string(cfg, "remote.no-remote-url.pushurl", "http://example.com")); + cl_git_pass(git_remote_list(&list, _repo)); - cl_assert(list.count == 5); + cl_assert(list.count == 7); git_strarray_free(&list); git_config_free(cfg);