From 72bca13e5d0d421da7992f029e275d950c864105 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 6 Jun 2014 16:33:54 +0200 Subject: [PATCH] remote: return problem refspecs instead of using a callback There is no reason why we need to use a callback here. A string array fits better with the usage, as this is not an event and we don't need anything from the user. --- include/git2/remote.h | 8 ++-- src/remote.c | 40 ++++++++++------ tests/network/remote/rename.c | 90 +++++++++++++++++++++-------------- tests/submodule/add.c | 5 +- 4 files changed, 87 insertions(+), 56 deletions(-) diff --git a/include/git2/remote.h b/include/git2/remote.h index 28771ac42..8d3744265 100644 --- a/include/git2/remote.h +++ b/include/git2/remote.h @@ -572,6 +572,9 @@ GIT_EXTERN(void) git_remote_set_autotag( * * A temporary in-memory remote cannot be given a name with this method. * + * @param problems non-default refspecs cannot be renamed and will be + * stored here for further processing by the caller. Always free this + * strarray on succesful return. * @param remote the remote to rename * @param new_name the new name the remote should bear * @param callback Optional callback to notify the consumer of fetch refspecs @@ -580,10 +583,9 @@ GIT_EXTERN(void) git_remote_set_autotag( * @return 0, GIT_EINVALIDSPEC, GIT_EEXISTS or an error code */ GIT_EXTERN(int) git_remote_rename( + git_strarray *problems, git_remote *remote, - const char *new_name, - git_remote_rename_problem_cb callback, - void *payload); + const char *new_name); /** * Retrieve the update FETCH_HEAD setting. diff --git a/src/remote.c b/src/remote.c index c7b97ca62..4e6f350d5 100644 --- a/src/remote.c +++ b/src/remote.c @@ -1419,11 +1419,7 @@ static int rename_remote_references( return (error == GIT_ITEROVER) ? 0 : error; } -static int rename_fetch_refspecs( - git_remote *remote, - const char *new_name, - int (*callback)(const char *problematic_refspec, void *payload), - void *payload) +static int rename_fetch_refspecs(git_vector *problems, git_remote *remote, const char *new_name) { git_config *config; git_buf base = GIT_BUF_INIT, var = GIT_BUF_INIT, val = GIT_BUF_INIT; @@ -1434,6 +1430,9 @@ static int rename_fetch_refspecs( if ((error = git_repository_config__weakptr(&config, remote->repo)) < 0) return error; + if ((error = git_vector_init(problems, 1, NULL)) < 0) + return error; + if ((error = git_buf_printf( &base, "+refs/heads/*:refs/remotes/%s/*", remote->name)) < 0) return error; @@ -1444,11 +1443,13 @@ static int rename_fetch_refspecs( /* Does the dst part of the refspec follow the expected format? */ if (strcmp(git_buf_cstr(&base), spec->string)) { + char *dup; - if ((error = callback(spec->string, payload)) != 0) { - giterr_set_after_callback(error); + dup = git__strdup(spec->string); + GITERR_CHECK_ALLOC(dup); + + if ((error = git_vector_insert(problems, dup)) < 0) break; - } continue; } @@ -1474,19 +1475,25 @@ static int rename_fetch_refspecs( git_buf_free(&base); git_buf_free(&var); git_buf_free(&val); + + if (error < 0) { + char *str; + git_vector_foreach(problems, i, str) + git__free(str); + + git_vector_free(problems); + } + return error; } -int git_remote_rename( - git_remote *remote, - const char *new_name, - git_remote_rename_problem_cb callback, - void *payload) +int git_remote_rename(git_strarray *out, git_remote *remote, const char *new_name) { int error; + git_vector problem_refspecs; char *tmp, *dup; - assert(remote && new_name); + assert(out && remote && new_name); if (!remote->name) { giterr_set(GITERR_INVALID, "Can't rename an anonymous remote."); @@ -1508,9 +1515,12 @@ int git_remote_rename( if ((error = rename_remote_references(remote->repo, remote->name, new_name)) < 0) return error; - if ((error = rename_fetch_refspecs(remote, new_name, callback, payload)) < 0) + if ((error = rename_fetch_refspecs(&problem_refspecs, remote, new_name)) < 0) return error; + out->count = problem_refspecs.length; + out->strings = (char **) problem_refspecs.contents; + dup = git__strdup(new_name); GITERR_CHECK_ALLOC(dup); diff --git a/tests/network/remote/rename.c b/tests/network/remote/rename.c index b7ec44726..48a33038a 100644 --- a/tests/network/remote/rename.c +++ b/tests/network/remote/rename.c @@ -33,10 +33,14 @@ static int dont_call_me_cb(const char *fetch_refspec, void *payload) void test_network_remote_rename__renaming_a_remote_moves_related_configuration_section(void) { + git_strarray problems = {0}; + assert_config_entry_existence(_repo, "remote.test.fetch", true); assert_config_entry_existence(_repo, "remote.just/renamed.fetch", false); - cl_git_pass(git_remote_rename(_remote, "just/renamed", dont_call_me_cb, NULL)); + cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed")); + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); assert_config_entry_existence(_repo, "remote.test.fetch", false); assert_config_entry_existence(_repo, "remote.just/renamed.fetch", true); @@ -44,16 +48,24 @@ void test_network_remote_rename__renaming_a_remote_moves_related_configuration_s void test_network_remote_rename__renaming_a_remote_updates_branch_related_configuration_entries(void) { + git_strarray problems = {0}; + assert_config_entry_value(_repo, "branch.master.remote", "test"); - cl_git_pass(git_remote_rename(_remote, "just/renamed", dont_call_me_cb, NULL)); + cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed")); + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); assert_config_entry_value(_repo, "branch.master.remote", "just/renamed"); } void test_network_remote_rename__renaming_a_remote_updates_default_fetchrefspec(void) { - cl_git_pass(git_remote_rename(_remote, "just/renamed", dont_call_me_cb, NULL)); + git_strarray problems = {0}; + + cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed")); + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); assert_config_entry_value(_repo, "remote.just/renamed.fetch", "+refs/heads/*:refs/remotes/just/renamed/*"); } @@ -61,6 +73,7 @@ void test_network_remote_rename__renaming_a_remote_updates_default_fetchrefspec( void test_network_remote_rename__renaming_a_remote_without_a_fetchrefspec_doesnt_create_one(void) { git_config *config; + git_strarray problems = {0}; git_remote_free(_remote); cl_git_pass(git_repository_config__weakptr(&config, _repo)); @@ -70,70 +83,64 @@ void test_network_remote_rename__renaming_a_remote_without_a_fetchrefspec_doesnt assert_config_entry_existence(_repo, "remote.test.fetch", false); - cl_git_pass(git_remote_rename(_remote, "just/renamed", dont_call_me_cb, NULL)); + cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed")); + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); assert_config_entry_existence(_repo, "remote.just/renamed.fetch", false); } -static int ensure_refspecs(const char* refspec_name, void *payload) -{ - int i = 0; - bool found = false; - const char ** exp = (const char **)payload; - - while (exp[i]) { - if (strcmp(exp[i++], refspec_name)) - continue; - - found = true; - break; - } - - cl_assert(found); - - return 0; -} - void test_network_remote_rename__renaming_a_remote_notifies_of_non_default_fetchrefspec(void) { git_config *config; - char *expected_refspecs[] = { - "+refs/*:refs/*", - NULL - }; + git_strarray problems = {0}; git_remote_free(_remote); cl_git_pass(git_repository_config__weakptr(&config, _repo)); cl_git_pass(git_config_set_string(config, "remote.test.fetch", "+refs/*:refs/*")); cl_git_pass(git_remote_load(&_remote, _repo, "test")); - cl_git_pass(git_remote_rename(_remote, "just/renamed", ensure_refspecs, &expected_refspecs)); + cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed")); + cl_assert_equal_i(1, problems.count); + cl_assert_equal_s("+refs/*:refs/*", problems.strings[0]); + git_strarray_free(&problems); assert_config_entry_value(_repo, "remote.just/renamed.fetch", "+refs/*:refs/*"); + + git_strarray_free(&problems); } void test_network_remote_rename__new_name_can_contain_dots(void) { - cl_git_pass(git_remote_rename(_remote, "just.renamed", dont_call_me_cb, NULL)); + git_strarray problems = {0}; + + cl_git_pass(git_remote_rename(&problems, _remote, "just.renamed")); + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); cl_assert_equal_s("just.renamed", git_remote_name(_remote)); } void test_network_remote_rename__new_name_must_conform_to_reference_naming_conventions(void) { + git_strarray problems = {0}; + cl_assert_equal_i( GIT_EINVALIDSPEC, - git_remote_rename(_remote, "new@{name", dont_call_me_cb, NULL)); + git_remote_rename(&problems, _remote, "new@{name")); } void test_network_remote_rename__renamed_name_is_persisted(void) { git_remote *renamed; git_repository *another_repo; + git_strarray problems = {0}; cl_git_fail(git_remote_load(&renamed, _repo, "just/renamed")); - cl_git_pass(git_remote_rename(_remote, "just/renamed", dont_call_me_cb, NULL)); + cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed")); + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); cl_git_pass(git_repository_open(&another_repo, "testrepo.git")); cl_git_pass(git_remote_load(&renamed, _repo, "just/renamed")); @@ -144,19 +151,24 @@ void test_network_remote_rename__renamed_name_is_persisted(void) void test_network_remote_rename__cannot_overwrite_an_existing_remote(void) { - cl_assert_equal_i(GIT_EEXISTS, git_remote_rename(_remote, "test", dont_call_me_cb, NULL)); - cl_assert_equal_i(GIT_EEXISTS, git_remote_rename(_remote, "test_with_pushurl", dont_call_me_cb, NULL)); + git_strarray problems = {0}; + + cl_assert_equal_i(GIT_EEXISTS, git_remote_rename(&problems, _remote, "test")); + cl_assert_equal_i(GIT_EEXISTS, git_remote_rename(&problems, _remote, "test_with_pushurl")); } void test_network_remote_rename__renaming_a_remote_moves_the_underlying_reference(void) { git_reference *underlying; + git_strarray problems = {0}; cl_assert_equal_i(GIT_ENOTFOUND, git_reference_lookup(&underlying, _repo, "refs/remotes/just/renamed")); cl_git_pass(git_reference_lookup(&underlying, _repo, "refs/remotes/test/master")); git_reference_free(underlying); - cl_git_pass(git_remote_rename(_remote, "just/renamed", dont_call_me_cb, NULL)); + cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed")); + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); cl_assert_equal_i(GIT_ENOTFOUND, git_reference_lookup(&underlying, _repo, "refs/remotes/test/master")); cl_git_pass(git_reference_lookup(&underlying, _repo, "refs/remotes/just/renamed/master")); @@ -166,10 +178,12 @@ void test_network_remote_rename__renaming_a_remote_moves_the_underlying_referenc void test_network_remote_rename__cannot_rename_an_inmemory_remote(void) { git_remote *remote; + git_strarray problems = {0}; cl_git_pass(git_remote_create_anonymous(&remote, _repo, "file:///blah", NULL)); - cl_git_fail(git_remote_rename(remote, "newname", NULL, NULL)); + cl_git_fail(git_remote_rename(&problems, remote, "newname")); + git_strarray_free(&problems); git_remote_free(remote); } @@ -181,15 +195,17 @@ void test_network_remote_rename__overwrite_ref_in_target(void) git_reference *ref; git_branch_t btype; git_branch_iterator *iter; + git_strarray problems = {0}; cl_git_pass(git_oid_fromstr(&id, "a65fedf39aefe402d3bb6e24df4d4f5fe4547750")); cl_git_pass(git_reference_create(&ref, _repo, "refs/remotes/renamed/master", &id, 1, NULL, NULL)); git_reference_free(ref); cl_git_pass(git_remote_load(&remote, _repo, "test")); - cl_git_pass(git_remote_rename(remote, "renamed", dont_call_me_cb, NULL)); + cl_git_pass(git_remote_rename(&problems, remote, "renamed")); git_remote_free(remote); - + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); /* make sure there's only one remote-tracking branch */ cl_git_pass(git_branch_iterator_new(&iter, _repo, GIT_BRANCH_REMOTE)); diff --git a/tests/submodule/add.c b/tests/submodule/add.c index af81713f1..9fdc7cc57 100644 --- a/tests/submodule/add.c +++ b/tests/submodule/add.c @@ -68,13 +68,16 @@ void test_submodule_add__url_relative(void) { git_submodule *sm; git_remote *remote; + git_strarray problems = {0}; /* default remote url is https://github.com/libgit2/false.git */ g_repo = cl_git_sandbox_init("testrepo2"); /* make sure we don't default to origin - rename origin -> test_remote */ cl_git_pass(git_remote_load(&remote, g_repo, "origin")); - cl_git_pass(git_remote_rename(remote, "test_remote", NULL, NULL)); + cl_git_pass(git_remote_rename(&problems, remote, "test_remote")); + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); cl_git_fail(git_remote_load(&remote, g_repo, "origin")); git_remote_free(remote);