remote: remove live changing of refspecs

The base refspecs changing can be a cause of confusion as to what is the
current base refspec set and complicate saving the remote's
configuration.

Change `git_remote_add_{fetch,push}()` to update the configuration
instead of an instance.

This finally makes `git_remote_save()` a no-op, it will be removed in a
later commit.

9 files changed, 80 insertions, 136 deletions

diff --git a/include/git2/remote.h b/include/git2/remote.h
index e2350f4f5..73440a966 100644
--- a/include/git2/remote.h
+++ b/include/git2/remote.h
@@ -179,16 +179,17 @@ GIT_EXTERN(int) git_remote_set_url(git_repository *repo, const char *remote, con
 GIT_EXTERN(int) git_remote_set_pushurl(git_repository *repo, const char *remote, const char* url);
 
 /**
- * Add a fetch refspec to the remote
+ * Add a fetch refspec to the remote's configuration
  *
- * Convenience function for adding a single fetch refspec to the
- * current list in the remote.
+ * Add the given refspec to the fetch list in the configuration. No
+ * loaded remote instances will be affected.
  *
- * @param remote the remote
+ * @param repo the repository in which to change the configuration
+ * @param remote the name of the remote to change
  * @param refspec the new fetch refspec
  * @return 0 or an error value
  */
-GIT_EXTERN(int) git_remote_add_fetch(git_remote *remote, const char *refspec);
+GIT_EXTERN(int) git_remote_add_fetch(git_repository *repo, const char *remote, const char *refspec);
 
 /**
  * Get the remote's list of fetch refspecs
@@ -212,16 +213,17 @@ GIT_EXTERN(int) git_remote_get_fetch_refspecs(git_strarray *array, const git_rem
 GIT_EXTERN(int) git_remote_set_fetch_refspecs(git_remote *remote, git_strarray *array);
 
 /**
- * Add a push refspec to the remote
+ * Add a push refspec to the remote's configuration
  *
- * Convenience function for adding a single push refspec to the
- * current list in the remote.
+ * Add the given refspec to the push list in the configuration. No
+ * loaded remote instances will be affected.
  *
- * @param remote the remote
+ * @param repo the repository in which to change the configuration
+ * @param remote the name of the remote to change
  * @param refspec the new push refspec
  * @return 0 or an error value
  */
-GIT_EXTERN(int) git_remote_add_push(git_remote *remote, const char *refspec);
+GIT_EXTERN(int) git_remote_add_push(git_repository *repo, const char *remote, const char *refspec);
 
 /**
  * Get the remote's list of push refspecs
@@ -245,15 +247,6 @@ GIT_EXTERN(int) git_remote_get_push_refspecs(git_strarray *array, const git_remo
 GIT_EXTERN(int) git_remote_set_push_refspecs(git_remote *remote, git_strarray *array);
 
 /**
- * Clear the refspecs
- *
- * Remove all configured fetch and push refspecs from the remote.
- *
- * @param remote the remote
- */
-GIT_EXTERN(void) git_remote_clear_refspecs(git_remote *remote);
-
-/**
  * Get the number of refspecs for a remote
  *
  * @param remote the remote
diff --git a/src/clone.c b/src/clone.c
index 7dcbb8a1d..c44cf599f 100644
--- a/src/clone.c
+++ b/src/clone.c
@@ -341,11 +341,9 @@ static int clone_into(git_repository *repo, git_remote *_remote, const git_fetch
 	if ((error = git_remote_dup(&remote, _remote)) < 0)
 		return error;
 
-	if ((error = git_remote_add_fetch(remote, "refs/tags/*:refs/tags/*")) < 0)
-		goto cleanup;
-
 	memcpy(&fetch_opts, opts, sizeof(git_fetch_options));
 	fetch_opts.update_fetchhead = 0;
+	fetch_opts.download_tags = GIT_REMOTE_DOWNLOAD_TAGS_ALL;
 	git_buf_printf(&reflog_message, "clone: from %s", git_remote_url(remote));
 
 	if ((error = git_remote_fetch(remote, NULL, &fetch_opts, git_buf_cstr(&reflog_message))) != 0)
diff --git a/src/remote.c b/src/remote.c
index ccbc46bbb..f4a2f0452 100644
--- a/src/remote.c
+++ b/src/remote.c
@@ -23,6 +23,7 @@
 #define CONFIG_URL_FMT "remote.%s.url"
 #define CONFIG_PUSHURL_FMT "remote.%s.pushurl"
 #define CONFIG_FETCH_FMT "remote.%s.fetch"
+#define CONFIG_PUSH_FMT "remote.%s.push"
 #define CONFIG_TAGOPT_FMT "remote.%s.tagopt"
 
 static int dwim_refspecs(git_vector *out, git_vector *refspecs, git_vector *refs);
@@ -92,6 +93,38 @@ static int ensure_remote_name_is_valid(const char *name)
 	return error;
 }
 
+static int write_add_refspec(git_repository *repo, const char *name, const char *refspec, bool fetch)
+{
+	git_config *cfg;
+	git_buf var = GIT_BUF_INIT;
+	const char *fmt;
+	int error;
+
+	if ((error = git_repository_config__weakptr(&cfg, repo)) < 0)
+	    return error;
+
+	fmt = fetch ? CONFIG_FETCH_FMT : CONFIG_PUSH_FMT;
+
+	if ((error = ensure_remote_name_is_valid(name)) < 0)
+		return error;
+
+	if ((error = git_buf_printf(&var, fmt, name)) < 0)
+		return error;
+
+	/*
+	 * "$^" is a unmatcheable regexp: it will not match anything at all, so
+	 * all values will be considered new and we will not replace any
+	 * present value.
+	 */
+	if ((error = git_config_set_multivar(cfg, var.ptr, "$^", refspec)) < 0) {
+		goto cleanup;
+	}
+
+cleanup:
+	git_buf_free(&var);
+	return 0;
+}
+
 #if 0
 /* We could export this as a helper */
 static int get_check_cert(int *out, git_repository *repo)
@@ -178,7 +211,11 @@ static int create_internal(git_remote **out, git_repository *repo, const char *n
 	}
 
 	if (fetch != NULL) {
-		if (add_refspec(remote, fetch, true) < 0)
+		if ((error = add_refspec(remote, fetch, true)) < 0)
+			goto on_error;
+
+		/* only write for non-anonymous remotes */
+		if (name && (error = write_add_refspec(repo, name, fetch, true)) < 0)
 			goto on_error;
 
 		if ((error = git_repository_config_snapshot(&config, repo)) < 0)
@@ -562,8 +599,6 @@ int git_remote_save(const git_remote *remote)
 {
 	int error;
 	git_config *cfg;
-	git_buf buf = GIT_BUF_INIT;
-	git_config_entry *existing = NULL;
 
 	assert(remote);
 
@@ -578,17 +613,6 @@ int git_remote_save(const git_remote *remote)
 	if ((error = git_repository_config__weakptr(&cfg, remote->repo)) < 0)
 		return error;
 
-	/* after this point, buffer is allocated so end with cleanup */
-
-	if ((error = update_config_refspec(remote, cfg, GIT_DIRECTION_FETCH)) < 0)
-		goto cleanup;
-
-	if ((error = update_config_refspec(remote, cfg, GIT_DIRECTION_PUSH)) < 0)
-		goto cleanup;
-
-cleanup:
-	git_config_entry_free(existing);
-	git_buf_free(&buf);
 	return error;
 }
 
@@ -2015,26 +2039,14 @@ git_refspec *git_remote__matching_dst_refspec(git_remote *remote, const char *re
 	return NULL;
 }
 
-void git_remote_clear_refspecs(git_remote *remote)
-{
-	git_refspec *spec;
-	size_t i;
-
-	git_vector_foreach(&remote->refspecs, i, spec) {
-		git_refspec__free(spec);
-		git__free(spec);
-	}
-	git_vector_clear(&remote->refspecs);
-}
-
-int git_remote_add_fetch(git_remote *remote, const char *refspec)
+int git_remote_add_fetch(git_repository *repo, const char *remote, const char *refspec)
 {
-	return add_refspec(remote, refspec, true);
+	return write_add_refspec(repo, remote, refspec, true);
 }
 
-int git_remote_add_push(git_remote *remote, const char *refspec)
+int git_remote_add_push(git_repository *repo, const char *remote, const char *refspec)
 {
-	return add_refspec(remote, refspec, false);
+	return write_add_refspec(repo, remote, refspec, false);
 }
 
 static int set_refspecs(git_remote *remote, git_strarray *array, int push)
diff --git a/tests/network/fetchlocal.c b/tests/network/fetchlocal.c
index 73ccec031..06ee3dd36 100644
--- a/tests/network/fetchlocal.c
+++ b/tests/network/fetchlocal.c
@@ -358,16 +358,9 @@ static int remote_mirror_cb(git_remote **out, git_repository *repo,
 
 	GIT_UNUSED(payload);
 
-	if ((error = git_remote_create(&remote, repo, name, url)) < 0)
+	if ((error = git_remote_create_with_fetchspec(&remote, repo, name, url, "+refs/*:refs/*")) < 0)
 		return error;
 
-	git_remote_clear_refspecs(remote);
-
-	if ((error = git_remote_add_fetch(remote, "+refs/*:refs/*")) < 0) {
-		git_remote_free(remote);
-		return error;
-	}
-
 	*out = remote;
 	return 0;
 }
diff --git a/tests/network/remote/local.c b/tests/network/remote/local.c
index bcde50cbb..21cb93c57 100644
--- a/tests/network/remote/local.c
+++ b/tests/network/remote/local.c
@@ -161,7 +161,6 @@ void test_network_remote_local__shorthand_fetch_refspec1(void)
 	git_reference *ref;
 
 	connect_to_local_repository(cl_fixture("testrepo.git"));
-	git_remote_clear_refspecs(remote);
 
 	cl_git_pass(git_remote_fetch(remote, &array, NULL, NULL));
 
diff --git a/tests/network/remote/remotes.c b/tests/network/remote/remotes.c
index 819a22601..0da76da6c 100644
--- a/tests/network/remote/remotes.c
+++ b/tests/network/remote/remotes.c
@@ -115,9 +115,12 @@ void test_network_remote_remotes__add_fetchspec(void)
 
 	size = git_remote_refspec_count(_remote);
 
-	cl_git_pass(git_remote_add_fetch(_remote, "refs/*:refs/*"));
-
+	cl_git_pass(git_remote_add_fetch(_repo, "test", "refs/*:refs/*"));
 	size++;
+
+	git_remote_free(_remote);
+	cl_git_pass(git_remote_lookup(&_remote, _repo, "test"));
+
 	cl_assert_equal_i((int)size, (int)git_remote_refspec_count(_remote));
 
 	_refspec = git_remote_get_refspec(_remote, size - 1);
@@ -156,8 +159,12 @@ void test_network_remote_remotes__add_pushspec(void)
 
 	size = git_remote_refspec_count(_remote);
 
-	cl_git_pass(git_remote_add_push(_remote, "refs/*:refs/*"));
+	cl_git_pass(git_remote_add_push(_repo, "test", "refs/*:refs/*"));
 	size++;
+
+	git_remote_free(_remote);
+	cl_git_pass(git_remote_lookup(&_remote, _repo, "test"));
+
 	cl_assert_equal_i((int)size, (int)git_remote_refspec_count(_remote));
 
 	_refspec = git_remote_get_refspec(_remote, size - 1);
@@ -168,60 +175,6 @@ void test_network_remote_remotes__add_pushspec(void)
 	cl_assert_equal_b(_refspec->push, true);
 }
 
-void test_network_remote_remotes__save(void)
-{
-	git_strarray array;
-	const char *fetch_refspec1 = "refs/heads/ns1/*:refs/remotes/upstream/ns1/*";
-	const char *fetch_refspec2 = "refs/heads/ns2/*:refs/remotes/upstream/ns2/*";
-	const char *push_refspec1 = "refs/heads/ns1/*:refs/heads/ns1/*";
-	const char *push_refspec2 = "refs/heads/ns2/*:refs/heads/ns2/*";
-
-	git_remote_free(_remote);
-	_remote = NULL;
-
-	/* 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_save(_remote));
-	git_remote_free(_remote);
-	_remote = NULL;
-
-	/* Load it from config and make sure everything matches */
-	cl_git_pass(git_remote_lookup(&_remote, _repo, "upstream"));
-
-	cl_git_pass(git_remote_get_fetch_refspecs(&array, _remote));
-	cl_assert_equal_i(2, (int)array.count);
-	cl_assert_equal_s(fetch_refspec1, array.strings[0]);
-	cl_assert_equal_s(fetch_refspec2, array.strings[1]);
-	git_strarray_free(&array);
-
-	cl_git_pass(git_remote_get_push_refspecs(&array, _remote));
-	cl_assert_equal_i(2, (int)array.count);
-	cl_assert_equal_s(push_refspec1, array.strings[0]);
-	cl_assert_equal_s(push_refspec2, array.strings[1]);
-	git_strarray_free(&array);
-
-	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");
-
-	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);
-}
-
 void test_network_remote_remotes__fnmatch(void)
 {
 	cl_assert(git_refspec_src_matches(_refspec, "refs/heads/master"));
@@ -499,13 +452,16 @@ void test_network_remote_remotes__query_refspecs(void)
 	git_strarray array;
 	int i;
 
-	cl_git_pass(git_remote_create_anonymous(&remote, _repo, "git://github.com/libgit2/libgit2", NULL));
+	cl_git_pass(git_remote_create_with_fetchspec(&remote, _repo, "query", "git://github.com/libgit2/libgit2", NULL));
+	git_remote_free(remote);
 
 	for (i = 0; i < 3; i++) {
-		cl_git_pass(git_remote_add_fetch(remote, fetch_refspecs[i]));
-		cl_git_pass(git_remote_add_push(remote, push_refspecs[i]));
+		cl_git_pass(git_remote_add_fetch(_repo, "query", fetch_refspecs[i]));
+		cl_git_pass(git_remote_add_push(_repo, "query", push_refspecs[i]));
 	}
 
+	cl_git_pass(git_remote_lookup(&remote, _repo, "query"));
+
 	cl_git_pass(git_remote_get_fetch_refspecs(&array, remote));
 	for (i = 0; i < 3; i++) {
 		cl_assert_equal_s(fetch_refspecs[i], array.strings[i]);
@@ -519,6 +475,7 @@ void test_network_remote_remotes__query_refspecs(void)
 	git_strarray_free(&array);
 
 	git_remote_free(remote);
+	git_remote_delete(_repo, "test");
 }
 
 void test_network_remote_remotes__fetch_from_anonymous(void)
diff --git a/tests/online/clone.c b/tests/online/clone.c
index 1d4196948..1930a8ba3 100644
--- a/tests/online/clone.c
+++ b/tests/online/clone.c
@@ -134,16 +134,9 @@ static int remote_mirror_cb(git_remote **out, git_repository *repo,
 
 	GIT_UNUSED(payload);
 
-	if ((error = git_remote_create(&remote, repo, name, url)) < 0)
+	if ((error = git_remote_create_with_fetchspec(&remote, repo, name, url, "+refs/*:refs/*")) < 0)
 		return error;
 
-	git_remote_clear_refspecs(remote);
-
-	if ((error = git_remote_add_fetch(remote, "+refs/*:refs/*")) < 0) {
-		git_remote_free(remote);
-		return error;
-	}
-
 	*out = remote;
 	return 0;
 }
diff --git a/tests/online/push.c b/tests/online/push.c
index b35632755..04ad7a2ed 100644
--- a/tests/online/push.c
+++ b/tests/online/push.c
@@ -861,6 +861,7 @@ void test_online_push__configured(void)
 {
 	git_oid note_oid, *target_oid, expected_oid;
 	git_signature *signature;
+	git_remote *old_remote;
 	const char *specs[] = { "refs/notes/commits:refs/notes/commits" };
 	push_status exp_stats[] = { { "refs/notes/commits", 1 } };
 	expected_ref exp_refs[] = { { "refs/notes/commits", &expected_oid } };
@@ -870,7 +871,10 @@ void test_online_push__configured(void)
 
 	target_oid = &_oid_b6;
 
-	cl_git_pass(git_remote_add_push(_remote, specs[0]));
+	cl_git_pass(git_remote_add_push(_repo, git_remote_name(_remote), specs[0]));
+	old_remote = _remote;
+	cl_git_pass(git_remote_lookup(&_remote, _repo, git_remote_name(_remote)));
+	git_remote_free(old_remote);
 
 	/* Create note to push */
 	cl_git_pass(git_signature_new(&signature, "nulltoken", "emeric.fermas@gmail.com", 1323847743, 60)); /* Wed Dec 14 08:29:03 2011 +0100 */
diff --git a/tests/refs/branches/remote.c b/tests/refs/branches/remote.c
index bac088454..47526717f 100644
--- a/tests/refs/branches/remote.c
+++ b/tests/refs/branches/remote.c
@@ -57,12 +57,7 @@ void test_refs_branches_remote__ambiguous_remote_returns_error(void)
 	git_buf buf;
 
 	/* Create the remote */
-	cl_git_pass(git_remote_create(&remote, g_repo, "addtest", "http://github.com/libgit2/libgit2"));
-
-	/* Update the remote fetch spec */
-	git_remote_clear_refspecs(remote);
-	cl_git_pass(git_remote_add_fetch(remote, "refs/heads/*:refs/remotes/test/*"));
-	cl_git_pass(git_remote_save(remote));
+	cl_git_pass(git_remote_create_with_fetchspec(&remote, g_repo, "addtest", "http://github.com/libgit2/libgit2", "refs/heads/*:refs/remotes/test/*"));
 
 	git_remote_free(remote);