Commit 46c8f7f8459e7062ca04d1cb8e11cf84e5cfbd0f

Carlos Martín Nieto 2014-10-24T16:25:59

remote: accept a repo and name for renaming 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.

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 90b5502..11a1bff 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 977e9b8..600b8ef 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 ea510c7..362c226 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 1b819a4..e1aea02 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 1071780..a3e3502 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)