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.

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.
 

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);
 
 /**

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)

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);
 

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)