remote: accept a repo and name for renamingcmn/remote-rename

Remote objects are not meant to be changed from under the user. We did
this in rename, but only the name and left the refspecs, such that a
save would save the wrong refspecs (and a fetch and anything else would
use the wrong refspecs).

Instead, let's simply take a name and not change any loaded remote from
under the user.

5 files changed, 54 insertions, 74 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 90b550240..11a1bfff0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -43,6 +43,11 @@ v0.21 + 1
   path of the programs to execute for receive-pack and upload-pack on
   the server, git_transport_ssh_with_paths.
 
+* git_remote_rename() now takes the repository and the remote's
+  current name. Accepting a remote indicates we want to change it,
+  which we only did partially. It is much clearer if we accept a name
+  and no loaded objects are changed.
+
 * git_remote_delete() now accepts the repository and the remote's name
   instead of a loaded remote.
 
diff --git a/include/git2/remote.h b/include/git2/remote.h
index 977e9b809..600b8ef05 100644
--- a/include/git2/remote.h
+++ b/include/git2/remote.h
@@ -557,18 +557,21 @@ GIT_EXTERN(void) git_remote_set_autotag(
  * The new name will be checked for validity.
  * See `git_tag_create()` for rules about valid names.
  *
- * A temporary in-memory remote cannot be given a name with this method.
+ * No loaded instances of a the remote with the old name will change
+ * their name or their list of refspecs.
  *
  * @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 repo the repository in which to rename
+ * @param name the current name of the reamote
  * @param new_name the new name the remote should bear
  * @return 0, GIT_EINVALIDSPEC, GIT_EEXISTS or an error code
  */
 GIT_EXTERN(int) git_remote_rename(
 	git_strarray *problems,
-	git_remote *remote,
+	git_repository *repo,
+	const char *name,
 	const char *new_name);
 
 /**
diff --git a/src/remote.c b/src/remote.c
index ea510c7d4..362c226c4 100644
--- a/src/remote.c
+++ b/src/remote.c
@@ -1686,48 +1686,44 @@ static int rename_fetch_refspecs(git_vector *problems, git_remote *remote, const
 	return error;
 }
 
-int git_remote_rename(git_strarray *out, git_remote *remote, const char *new_name)
+int git_remote_rename(git_strarray *out, git_repository *repo, const char *name, const char *new_name)
 {
 	int error;
-	git_vector problem_refspecs;
-	char *tmp, *dup;
+	git_vector problem_refspecs = GIT_VECTOR_INIT;
+	git_remote *remote;
 
-	assert(out && remote && new_name);
+	assert(out && repo && name && new_name);
 
-	if (!remote->name) {
-		giterr_set(GITERR_INVALID, "Can't rename an anonymous remote.");
-		return GIT_EINVALIDSPEC;
-	}
+	if ((error = git_remote_load(&remote, repo, name)) < 0)
+		return -1;
 
 	if ((error = ensure_remote_name_is_valid(new_name)) < 0)
-		return error;
+		goto cleanup;
 
-	if ((error = ensure_remote_doesnot_exist(remote->repo, new_name)) < 0)
-		return error;
+	if ((error = ensure_remote_doesnot_exist(repo, new_name)) < 0)
+		goto cleanup;
 
-	if ((error = rename_remote_config_section(remote->repo, remote->name, new_name)) < 0)
-		return error;
+	if ((error = rename_remote_config_section(repo, name, new_name)) < 0)
+		goto cleanup;
 
-	if ((error = update_branch_remote_config_entry(remote->repo, remote->name, new_name)) < 0)
-		return error;
+	if ((error = update_branch_remote_config_entry(repo, name, new_name)) < 0)
+		goto cleanup;
 
-	if ((error = rename_remote_references(remote->repo, remote->name, new_name)) < 0)
-		return error;
+	if ((error = rename_remote_references(repo, name, new_name)) < 0)
+		goto cleanup;
 
 	if ((error = rename_fetch_refspecs(&problem_refspecs, remote, new_name)) < 0)
-		return error;
+		goto cleanup;
 
 	out->count = problem_refspecs.length;
 	out->strings = (char **) problem_refspecs.contents;
 
-	dup = git__strdup(new_name);
-	GITERR_CHECK_ALLOC(dup);
-
-	tmp = remote->name;
-	remote->name = dup;
-	git__free(tmp);
+cleanup:
+	if (error < 0)
+		git_vector_free(&problem_refspecs);
 
-	return 0;
+	git_remote_free(remote);
+	return error;
 }
 
 int git_remote_update_fetchhead(git_remote *remote)
diff --git a/tests/network/remote/rename.c b/tests/network/remote/rename.c
index 1b819a445..e1aea0297 100644
--- a/tests/network/remote/rename.c
+++ b/tests/network/remote/rename.c
@@ -3,21 +3,16 @@
 
 #include "repository.h"
 
-static git_remote *_remote;
 static git_repository *_repo;
+static const char *_remote_name = "test";
 
 void test_network_remote_rename__initialize(void)
 {
 	_repo = cl_git_sandbox_init("testrepo.git");
-
-	cl_git_pass(git_remote_load(&_remote, _repo, "test"));
 }
 
 void test_network_remote_rename__cleanup(void)
 {
-	git_remote_free(_remote);
-	_remote = NULL;
-
 	cl_git_sandbox_cleanup();
 }
 
@@ -38,7 +33,7 @@ void test_network_remote_rename__renaming_a_remote_moves_related_configuration_s
 	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(&problems, _remote, "just/renamed"));
+	cl_git_pass(git_remote_rename(&problems, _repo, _remote_name, "just/renamed"));
 	cl_assert_equal_i(0, problems.count);
 	git_strarray_free(&problems);
 
@@ -52,7 +47,7 @@ void test_network_remote_rename__renaming_a_remote_updates_branch_related_config
 
 	assert_config_entry_value(_repo, "branch.master.remote", "test");
 
-	cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed"));
+	cl_git_pass(git_remote_rename(&problems, _repo, _remote_name, "just/renamed"));
 	cl_assert_equal_i(0, problems.count);
 	git_strarray_free(&problems);
 
@@ -63,7 +58,7 @@ void test_network_remote_rename__renaming_a_remote_updates_default_fetchrefspec(
 {
 	git_strarray problems = {0};
 
-	cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed"));
+	cl_git_pass(git_remote_rename(&problems, _repo, _remote_name, "just/renamed"));
 	cl_assert_equal_i(0, problems.count);
 	git_strarray_free(&problems);
 
@@ -73,17 +68,18 @@ 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_remote *remote;
 	git_strarray problems = {0};
 
-	git_remote_free(_remote);
 	cl_git_pass(git_repository_config__weakptr(&config, _repo));
 	cl_git_pass(git_config_delete_entry(config, "remote.test.fetch"));
 
-	cl_git_pass(git_remote_load(&_remote, _repo, "test"));
+	cl_git_pass(git_remote_load(&remote, _repo, "test"));
+	git_remote_free(remote);
 
 	assert_config_entry_existence(_repo, "remote.test.fetch", false);
 
-	cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed"));
+	cl_git_pass(git_remote_rename(&problems, _repo, _remote_name, "just/renamed"));
 	cl_assert_equal_i(0, problems.count);
 	git_strarray_free(&problems);
 
@@ -93,15 +89,15 @@ void test_network_remote_rename__renaming_a_remote_without_a_fetchrefspec_doesnt
 void test_network_remote_rename__renaming_a_remote_notifies_of_non_default_fetchrefspec(void)
 {
 	git_config *config;
-
+	git_remote *remote;
 	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_load(&remote, _repo, "test"));
+	git_remote_free(remote);
 
-	cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed"));
+	cl_git_pass(git_remote_rename(&problems, _repo, _remote_name, "just/renamed"));
 	cl_assert_equal_i(1, problems.count);
 	cl_assert_equal_s("+refs/*:refs/*", problems.strings[0]);
 	git_strarray_free(&problems);
@@ -115,10 +111,10 @@ void test_network_remote_rename__new_name_can_contain_dots(void)
 {
 	git_strarray problems = {0};
 
-	cl_git_pass(git_remote_rename(&problems, _remote, "just.renamed"));
+	cl_git_pass(git_remote_rename(&problems, _repo, _remote_name, "just.renamed"));
 	cl_assert_equal_i(0, problems.count);
 	git_strarray_free(&problems);
-	cl_assert_equal_s("just.renamed", git_remote_name(_remote));
+	assert_config_entry_existence(_repo, "remote.just.renamed.fetch", true);
 }
 
 void test_network_remote_rename__new_name_must_conform_to_reference_naming_conventions(void)
@@ -127,7 +123,7 @@ void test_network_remote_rename__new_name_must_conform_to_reference_naming_conve
 
 	cl_assert_equal_i(
 		GIT_EINVALIDSPEC,
-		git_remote_rename(&problems, _remote, "new@{name"));
+		git_remote_rename(&problems, _repo, _remote_name, "new@{name"));
 }
 
 void test_network_remote_rename__renamed_name_is_persisted(void)
@@ -138,7 +134,7 @@ void test_network_remote_rename__renamed_name_is_persisted(void)
 
 	cl_git_fail(git_remote_load(&renamed, _repo, "just/renamed"));
 
-	cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed"));
+	cl_git_pass(git_remote_rename(&problems, _repo, _remote_name, "just/renamed"));
 	cl_assert_equal_i(0, problems.count);
 	git_strarray_free(&problems);
 
@@ -153,8 +149,8 @@ void test_network_remote_rename__cannot_overwrite_an_existing_remote(void)
 {
 	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"));
+	cl_assert_equal_i(GIT_EEXISTS, git_remote_rename(&problems, _repo, _remote_name, "test"));
+	cl_assert_equal_i(GIT_EEXISTS, git_remote_rename(&problems, _repo, _remote_name, "test_with_pushurl"));
 }
 
 void test_network_remote_rename__renaming_a_remote_moves_the_underlying_reference(void)
@@ -166,7 +162,7 @@ void test_network_remote_rename__renaming_a_remote_moves_the_underlying_referenc
 	cl_git_pass(git_reference_lookup(&underlying, _repo, "refs/remotes/test/master"));
 	git_reference_free(underlying);
 
-	cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed"));
+	cl_git_pass(git_remote_rename(&problems, _repo, _remote_name, "just/renamed"));
 	cl_assert_equal_i(0, problems.count);
 	git_strarray_free(&problems);
 
@@ -175,23 +171,10 @@ void test_network_remote_rename__renaming_a_remote_moves_the_underlying_referenc
 	git_reference_free(underlying);
 }
 
-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(&problems, remote, "newname"));
-
-	git_strarray_free(&problems);
-	git_remote_free(remote);
-}
-
 void test_network_remote_rename__overwrite_ref_in_target(void)
 {
 	git_oid id;
 	char idstr[GIT_OID_HEXSZ + 1] = {0};
-	git_remote *remote;
 	git_reference *ref;
 	git_branch_t btype;
 	git_branch_iterator *iter;
@@ -201,9 +184,7 @@ void test_network_remote_rename__overwrite_ref_in_target(void)
 	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(&problems, remote, "renamed"));
-	git_remote_free(remote);
+	cl_git_pass(git_remote_rename(&problems, _repo, _remote_name, "renamed"));
 	cl_assert_equal_i(0, problems.count);
 	git_strarray_free(&problems);
 
@@ -222,7 +203,6 @@ void test_network_remote_rename__overwrite_ref_in_target(void)
 void test_network_remote_rename__symref_head(void)
 {
 	int error;
-	git_remote *remote;
 	git_reference *ref;
 	git_branch_t btype;
 	git_branch_iterator *iter;
@@ -233,9 +213,7 @@ void test_network_remote_rename__symref_head(void)
 	cl_git_pass(git_reference_symbolic_create(&ref, _repo, "refs/remotes/test/HEAD", "refs/remotes/test/master", 0, NULL, NULL));
 	git_reference_free(ref);
 
-	cl_git_pass(git_remote_load(&remote, _repo, "test"));
-	cl_git_pass(git_remote_rename(&problems, remote, "renamed"));
-	git_remote_free(remote);
+	cl_git_pass(git_remote_rename(&problems, _repo, _remote_name, "renamed"));
 	cl_assert_equal_i(0, problems.count);
 	git_strarray_free(&problems);
 
diff --git a/tests/submodule/add.c b/tests/submodule/add.c
index 10717809e..a3e3502b7 100644
--- a/tests/submodule/add.c
+++ b/tests/submodule/add.c
@@ -94,12 +94,10 @@ void test_submodule_add__url_relative(void)
 	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(&problems, remote, "test_remote"));
+	cl_git_pass(git_remote_rename(&problems, g_repo, "origin", "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);
 
 	cl_git_pass(
 		git_submodule_add_setup(&sm, g_repo, "../TestGitRepository", "TestGitRepository", 1)