remote: remove url and pushurl from the save logic

As a first step in removing the repository-saving logic, don't allow
chaning the url or push url from a remote object, but change the
configuration on the configuration immediately.

include/git2/remote.h

@@ -153,26 +153,30 @@ GIT_EXTERN(const char *) git_remote_url(const git_remote *remote);
 GIT_EXTERN(const char *) git_remote_pushurl(const git_remote *remote);
 
 /**
- * Set the remote's url
+ * Set the remote's url in the configuration
  *
- * Existing connections will not be updated.
+ * Remote objects already in memory will not be affected. This assumes
+ * the common case of a single-url remote and will otherwise return an error.
  *
- * @param remote the remote
+ * @param repo the repository in which to perform the change
+ * @param remote the remote's name
  * @param url the url to set
  * @return 0 or an error value
  */
-GIT_EXTERN(int) git_remote_set_url(git_remote *remote, const char* url);
+GIT_EXTERN(int) git_remote_set_url(git_repository *repo, const char *remote, const char* url);
 
 /**
- * Set the remote's url for pushing
+ * Set the remote's url for pushing in the configuration.
  *
- * Existing connections will not be updated.
+ * Remote objects already in memory will not be affected. This assumes
+ * the common case of a single-url remote and will otherwise return an error.
  *
- * @param remote the remote
- * @param url the url to set or NULL to clear the pushurl
- * @return 0 or an error value
+ *
+ * @param repo the repository in which to perform the change
+ * @param remote the remote's name
+ * @param url the url to set
  */
-GIT_EXTERN(int) git_remote_set_pushurl(git_remote *remote, const char* url);
+GIT_EXTERN(int) git_remote_set_pushurl(git_repository *repo, const char *remote, const char* url);
 
 /**
  * Add a fetch refspec to the remote

src/remote.c

@@ -20,6 +20,10 @@
 #include "fetchhead.h"
 #include "push.h"
 
+#define CONFIG_URL_FMT "remote.%s.url"
+#define CONFIG_PUSHURL_FMT "remote.%s.pushurl"
+#define CONFIG_FETCH_FMT "remote.%s.fetch"
+
 static int dwim_refspecs(git_vector *out, git_vector *refspecs, git_vector *refs);
 static int lookup_remote_prune_config(git_remote *remote, git_config *config, const char *name);
 
@@ -141,12 +145,16 @@ static int create_internal(git_remote **out, git_repository *repo, const char *n
 {
 	git_remote *remote;
 	git_config *config = NULL;
-	git_buf canonical_url = GIT_BUF_INIT, fetchbuf = GIT_BUF_INIT;
+	git_buf canonical_url = GIT_BUF_INIT;
+	git_buf var = GIT_BUF_INIT;
 	int error = -1;
 
 	/* name is optional */
 	assert(out && repo && url);
 
+	if ((error = git_repository_config__weakptr(&config, repo)) < 0)
+		return error;
+
 	remote = git__calloc(1, sizeof(git_remote));
 	GITERR_CHECK_ALLOC(remote);
 
@@ -162,6 +170,12 @@ static int create_internal(git_remote **out, git_repository *repo, const char *n
 	if (name != NULL) {
 		remote->name = git__strdup(name);
 		GITERR_CHECK_ALLOC(remote->name);
+
+		if ((error = git_buf_printf(&var, CONFIG_URL_FMT, name)) < 0)
+			goto on_error;
+
+		if ((error = git_config_set_string(config, var.ptr, remote->url)) < 0)
+			goto on_error;
 	}
 
 	if (fetch != NULL) {
@@ -183,6 +197,8 @@ static int create_internal(git_remote **out, git_repository *repo, const char *n
 		/* A remote without a name doesn't download tags */
 		remote->download_tags = GIT_REMOTE_DOWNLOAD_TAGS_NONE;
 
+	git_buf_free(&var);
+
 	*out = remote;
 	error = 0;
 
@@ -191,8 +207,8 @@ on_error:
 		git_remote_free(remote);
 
 	git_config_free(config);
-	git_buf_free(&fetchbuf);
 	git_buf_free(&canonical_url);
+	git_buf_free(&var);
 	return error;
 }
 
@@ -564,23 +580,8 @@ int git_remote_save(const git_remote *remote)
 	if ((error = git_repository_config__weakptr(&cfg, remote->repo)) < 0)
 		return error;
 
-	if ((error = git_buf_printf(&buf, "remote.%s.url", remote->name)) < 0)
-		return error;
-
 	/* after this point, buffer is allocated so end with cleanup */
 
-	if ((error = git_config_set_string(
-			cfg, git_buf_cstr(&buf), remote->url)) < 0)
-		goto cleanup;
-
-	git_buf_clear(&buf);
-	if ((error = git_buf_printf(&buf, "remote.%s.pushurl", remote->name)) < 0)
-		goto cleanup;
-
-	if ((error = git_config__update_entry(
-			cfg, git_buf_cstr(&buf), remote->pushurl, true, false)) < 0)
-		goto cleanup;
-
 	if ((error = update_config_refspec(remote, cfg, GIT_DIRECTION_FETCH)) < 0)
 		goto cleanup;
 
@@ -642,16 +643,42 @@ const char *git_remote_url(const git_remote *remote)
 	return remote->url;
 }
 
-int git_remote_set_url(git_remote *remote, const char* url)
+static int set_url(git_repository *repo, const char *remote, const char *pattern, const char *url)
 {
-	assert(remote);
-	assert(url);
+	git_config *cfg;
+	git_buf buf = GIT_BUF_INIT, canonical_url = GIT_BUF_INIT;
+	int error;
 
-	git__free(remote->url);
-	remote->url = git__strdup(url);
-	GITERR_CHECK_ALLOC(remote->url);
+	assert(repo && remote);
 
-	return 0;
+	if ((error = ensure_remote_name_is_valid(remote)) < 0)
+		return error;
+
+	if ((error = git_repository_config__weakptr(&cfg, repo)) < 0)
+		return error;
+
+	if ((error = git_buf_printf(&buf, pattern, remote)) < 0)
+		return error;
+
+	if (url) {
+		if ((error = canonicalize_url(&canonical_url, url)) < 0)
+			goto cleanup;
+
+		error = git_config_set_string(cfg, buf.ptr, url);
+	} else {
+		error = git_config_delete_entry(cfg, buf.ptr);
+	}
+
+cleanup:
+	git_buf_free(&canonical_url);
+	git_buf_free(&buf);
+
+	return error;
+}
+
+int git_remote_set_url(git_repository *repo, const char *remote, const char *url)
+{
+	return set_url(repo, remote, CONFIG_URL_FMT, url);
 }
 
 const char *git_remote_pushurl(const git_remote *remote)
@@ -660,18 +687,9 @@ const char *git_remote_pushurl(const git_remote *remote)
 	return remote->pushurl;
 }
 
-int git_remote_set_pushurl(git_remote *remote, const char* url)
+int git_remote_set_pushurl(git_repository *repo, const char *remote, const char* url)
 {
-	assert(remote);
-
-	git__free(remote->pushurl);
-	if (url) {
-		remote->pushurl = git__strdup(url);
-		GITERR_CHECK_ALLOC(remote->pushurl);
-	} else {
-		remote->pushurl = NULL;
-	}
-	return 0;
+	return set_url(repo, remote, CONFIG_PUSHURL_FMT, url);
 }
 
 const char* git_remote__urlfordirection(git_remote *remote, int direction)

tests/fetchhead/nonetwork.c

@@ -331,9 +331,8 @@ void test_fetchhead_nonetwork__unborn_with_upstream(void)
 	cl_git_pass(git_clone(&repo, "./test1", "./repowithunborn", NULL));
 
 	/* Simulate someone pushing to it by changing to one that has stuff */
+	cl_git_pass(git_remote_set_url(repo, "origin", cl_fixture("testrepo.git")));
 	cl_git_pass(git_remote_lookup(&remote, repo, "origin"));
-	cl_git_pass(git_remote_set_url(remote, cl_fixture("testrepo.git")));
-	cl_git_pass(git_remote_save(remote));
 
 	cl_git_pass(git_remote_fetch(remote, NULL, NULL, NULL));
 	git_remote_free(remote);

tests/network/fetchlocal.c

@@ -400,16 +400,16 @@ void test_network_fetchlocal__multi_remotes(void)
 
 	cl_set_cleanup(&cleanup_sandbox, NULL);
 	options.callbacks.transfer_progress = transfer_cb;
+	cl_git_pass(git_remote_set_url(repo, "test", cl_git_fixture_url("testrepo.git")));
 	cl_git_pass(git_remote_lookup(&test, repo, "test"));
-	cl_git_pass(git_remote_set_url(test, cl_git_fixture_url("testrepo.git")));
 	cl_git_pass(git_remote_fetch(test, NULL, &options, NULL));
 
 	cl_git_pass(git_reference_list(&refnames, repo));
 	cl_assert_equal_i(32, (int)refnames.count);
 	git_strarray_free(&refnames);
 
+	cl_git_pass(git_remote_set_url(repo, "test_with_pushurl", cl_git_fixture_url("testrepo.git")));
 	cl_git_pass(git_remote_lookup(&test2, repo, "test_with_pushurl"));
-	cl_git_pass(git_remote_set_url(test2, cl_git_fixture_url("testrepo.git")));
 	cl_git_pass(git_remote_fetch(test2, NULL, &options, NULL));
 
 	cl_git_pass(git_reference_list(&refnames, repo));

tests/network/remote/remotes.c

@@ -54,11 +54,18 @@ void test_network_remote_remotes__parsing(void)
 
 void test_network_remote_remotes__pushurl(void)
 {
-	cl_git_pass(git_remote_set_pushurl(_remote, "git://github.com/libgit2/notlibgit2"));
-	cl_assert_equal_s(git_remote_pushurl(_remote), "git://github.com/libgit2/notlibgit2");
+	const char *name = git_remote_name(_remote);
+	git_remote *mod;
 
-	cl_git_pass(git_remote_set_pushurl(_remote, NULL));
-	cl_assert(git_remote_pushurl(_remote) == NULL);
+	cl_git_pass(git_remote_set_pushurl(_repo, name, "git://github.com/libgit2/notlibgit2"));
+	cl_git_pass(git_remote_lookup(&mod, _repo, name));
+	cl_assert_equal_s(git_remote_pushurl(mod), "git://github.com/libgit2/notlibgit2");
+	git_remote_free(mod);
+
+	cl_git_pass(git_remote_set_pushurl(_repo, name, NULL));
+	cl_git_pass(git_remote_lookup(&mod, _repo, name));
+	cl_assert(git_remote_pushurl(mod) == NULL);
+	git_remote_free(mod);
 }
 
 void test_network_remote_remotes__error_when_not_found(void)
@@ -174,13 +181,16 @@ void test_network_remote_remotes__save(void)
 
 	/* Set up the remote and save it to config */
 	cl_git_pass(git_remote_create(&_remote, _repo, "upstream", "git://github.com/libgit2/libgit2"));
+	git_remote_free(_remote);
+	cl_git_pass(git_remote_set_pushurl(_repo, "upstream", "git://github.com/libgit2/libgit2_push"));
+	cl_git_pass(git_remote_lookup(&_remote, _repo, "upstream"));
+
 	git_remote_clear_refspecs(_remote);
 
 	cl_git_pass(git_remote_add_fetch(_remote, fetch_refspec1));
 	cl_git_pass(git_remote_add_fetch(_remote, fetch_refspec2));
 	cl_git_pass(git_remote_add_push(_remote, push_refspec1));
 	cl_git_pass(git_remote_add_push(_remote, push_refspec2));
-	cl_git_pass(git_remote_set_pushurl(_remote, "git://github.com/libgit2/libgit2_push"));
 	cl_git_pass(git_remote_save(_remote));
 	git_remote_free(_remote);
 	_remote = NULL;
@@ -203,12 +213,11 @@ void test_network_remote_remotes__save(void)
 	cl_assert_equal_s(git_remote_url(_remote), "git://github.com/libgit2/libgit2");
 	cl_assert_equal_s(git_remote_pushurl(_remote), "git://github.com/libgit2/libgit2_push");
 
-	/* remove the pushurl again and see if we can save that too */
-	cl_git_pass(git_remote_set_pushurl(_remote, NULL));
-	cl_git_pass(git_remote_save(_remote));
 	git_remote_free(_remote);
 	_remote = NULL;
 
+	/* remove the pushurl again and see if we can save that too */
+	cl_git_pass(git_remote_set_pushurl(_repo, "upstream", NULL));
 	cl_git_pass(git_remote_lookup(&_remote, _repo, "upstream"));
 	cl_assert(git_remote_pushurl(_remote) == NULL);
 }