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.

4 files changed, 87 insertions, 56 deletions

diff --git a/include/git2/remote.h b/include/git2/remote.h
index 28771ac4..8d374426 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 c7b97ca6..4e6f350d 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 b7ec4472..48a33038 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 af81713f..9fdc7cc5 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);