Merge pull request #3127 from libgit2/cmn/remote-fixups

Tackle remote API issues from bindings

8 files changed, 88 insertions, 96 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8a0ae0e03..19463420e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -100,15 +100,19 @@ support for HTTPS connections insead of OpenSSL.
 
 ### API removals
 
-* `git_remote_save()` and `git_remote_clear_refspecs()` has been
+* `git_remote_save()` and `git_remote_clear_refspecs()` have been
   removed. Remote's configuration is changed via the configuration
   directly or through a convenience function which performs changes to
   the configuration directly.
 
 * `git_remote_set_callbacks()`, `git_remote_get_callbacks()` and
-  `git_remote_set_transport()` have been removed a the remote no
+  `git_remote_set_transport()` have been removed and the remote no
   longer stores this configuration.
 
+* `git_remote_set_fetch_refpecs()` and
+  `git_remote_set_push_refspecs()` have been removed. There is no
+  longer a way to set the base refspecs at run-time.
+
 ### Breaking API changes
 
 * `git_smart_subtransport_cb` now has a `param` parameter.
@@ -180,7 +184,7 @@ support for HTTPS connections insead of OpenSSL.
 * `git_remote_connect()` and `git_remote_prune()` now take a pointer
   to the callbacks.
 
-* `git_remote_fetch()` and `git_remote_download()` now take a poitner
+* `git_remote_fetch()` and `git_remote_download()` now take a pointer
   to fetch options which determine the runtime configuration.
 
 * The `git_remote_autotag_option_t` values have been changed. It has
@@ -191,6 +195,9 @@ support for HTTPS connections insead of OpenSSL.
   well as a boolean whether to write `FETCH_HEAD` and the autotag
   setting.
 
+* `git_remote_create_anonymous()` no longer takes a fetch refspec as
+  url-only remotes cannot have configured refspecs.
+
 * The `git_submodule_update_options` struct now has fetch options in
   the `fetch_opts` field instead of callbacks in the
   `remote_callbacks` field.
diff --git a/examples/network/fetch.c b/examples/network/fetch.c
index e47f5be4f..67444cb4a 100644
--- a/examples/network/fetch.c
+++ b/examples/network/fetch.c
@@ -92,7 +92,7 @@ int fetch(git_repository *repo, int argc, char **argv)
 	// Figure out whether it's a named remote or a URL
 	printf("Fetching %s for repo %p\n", argv[1], repo);
 	if (git_remote_lookup(&remote, repo, argv[1]) < 0) {
-		if (git_remote_create_anonymous(&remote, repo, argv[1], NULL) < 0)
+		if (git_remote_create_anonymous(&remote, repo, argv[1]) < 0)
 			return -1;
 	}
 
diff --git a/examples/network/ls-remote.c b/examples/network/ls-remote.c
index a26092265..21026562f 100644
--- a/examples/network/ls-remote.c
+++ b/examples/network/ls-remote.c
@@ -15,7 +15,7 @@ static int use_remote(git_repository *repo, char *name)
 	// Find the remote by name
 	error = git_remote_lookup(&remote, repo, name);
 	if (error < 0) {
-		error = git_remote_create_anonymous(&remote, repo, name, NULL);
+		error = git_remote_create_anonymous(&remote, repo, name);
 		if (error < 0)
 			goto cleanup;
 	}
diff --git a/include/git2/remote.h b/include/git2/remote.h
index 5c43136ac..02d73a0e6 100644
--- a/include/git2/remote.h
+++ b/include/git2/remote.h
@@ -63,24 +63,18 @@ GIT_EXTERN(int) git_remote_create_with_fetchspec(
 /**
  * Create an anonymous remote
  *
- * Create a remote with the given url and refspec in memory. You can use
- * this when you have a URL instead of a remote's name.  Note that anonymous
- * remotes cannot be converted to persisted remotes.
+ * Create a remote with the given url in-memory. You can use this when
+ * you have a URL instead of a remote's name.
  *
- * The name, when provided, will be checked for validity.
- * See `git_tag_create()` for rules about valid names.
- *
- * @param out pointer to the new remote object
+ * @param out pointer to the new remote objects
  * @param repo the associated repository
  * @param url the remote repository's URL
- * @param fetch the fetch refspec to use for this remote.
  * @return 0 or an error code
  */
 GIT_EXTERN(int) git_remote_create_anonymous(
 		git_remote **out,
 		git_repository *repo,
-		const char *url,
-		const char *fetch);
+		const char *url);
 
 /**
  * Get the information for a particular remote
@@ -174,7 +168,7 @@ GIT_EXTERN(int) git_remote_set_pushurl(git_repository *repo, const char *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
+ * @return 0, GIT_EINVALIDSPEC if refspec is invalid or an error value
  */
 GIT_EXTERN(int) git_remote_add_fetch(git_repository *repo, const char *remote, const char *refspec);
 
@@ -190,16 +184,6 @@ GIT_EXTERN(int) git_remote_add_fetch(git_repository *repo, const char *remote, c
 GIT_EXTERN(int) git_remote_get_fetch_refspecs(git_strarray *array, const git_remote *remote);
 
 /**
- * Set the remote's list of fetch refspecs
- *
- * The contents of the string array are copied.
- *
- * @param remote the remote to modify
- * @param array the new list of fetch resfpecs
- */
-GIT_EXTERN(int) git_remote_set_fetch_refspecs(git_remote *remote, git_strarray *array);
-
-/**
  * Add a push refspec to the remote's configuration
  *
  * Add the given refspec to the push list in the configuration. No
@@ -208,7 +192,7 @@ GIT_EXTERN(int) git_remote_set_fetch_refspecs(git_remote *remote, git_strarray *
  * @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
+ * @return 0, GIT_EINVALIDSPEC if refspec is invalid or an error value
  */
 GIT_EXTERN(int) git_remote_add_push(git_repository *repo, const char *remote, const char *refspec);
 
@@ -224,16 +208,6 @@ GIT_EXTERN(int) git_remote_add_push(git_repository *repo, const char *remote, co
 GIT_EXTERN(int) git_remote_get_push_refspecs(git_strarray *array, const git_remote *remote);
 
 /**
- * Set the remote's list of push refspecs
- *
- * The contents of the string array are copied.
- *
- * @param remote the remote to modify
- * @param array the new list of push resfpecs
- */
-GIT_EXTERN(int) git_remote_set_push_refspecs(git_remote *remote, git_strarray *array);
-
-/**
  * Get the number of refspecs for a remote
  *
  * @param remote the remote
diff --git a/src/remote.c b/src/remote.c
index 44885bd17..43b34561d 100644
--- a/src/remote.c
+++ b/src/remote.c
@@ -97,6 +97,7 @@ static int write_add_refspec(git_repository *repo, const char *name, const char
 {
 	git_config *cfg;
 	git_buf var = GIT_BUF_INIT;
+	git_refspec spec;
 	const char *fmt;
 	int error;
 
@@ -108,6 +109,15 @@ static int write_add_refspec(git_repository *repo, const char *name, const char
 	if ((error = ensure_remote_name_is_valid(name)) < 0)
 		return error;
 
+	if ((error = git_refspec__parse(&spec, refspec, fetch)) < 0) {
+		if (giterr_last()->klass != GITERR_NOMEMORY)
+			error = GIT_EINVALIDSPEC;
+
+		return error;
+	}
+
+	git_refspec__free(&spec);
+
 	if ((error = git_buf_printf(&var, fmt, name)) < 0)
 		return error;
 
@@ -311,15 +321,16 @@ on_error:
 	return -1;
 }
 
-int git_remote_create_anonymous(git_remote **out, git_repository *repo, const char *url, const char *fetch)
+int git_remote_create_anonymous(git_remote **out, git_repository *repo, const char *url)
 {
-	return create_internal(out, repo, NULL, url, fetch);
+	return create_internal(out, repo, NULL, url, NULL);
 }
 
 int git_remote_dup(git_remote **dest, git_remote *source)
 {
+	size_t i;
 	int error = 0;
-	git_strarray refspecs = { 0 };
+	git_refspec *spec;
 	git_remote *remote = git__calloc(1, sizeof(git_remote));
 	GITERR_CHECK_ALLOC(remote);
 
@@ -349,22 +360,15 @@ int git_remote_dup(git_remote **dest, git_remote *source)
 		goto cleanup;
 	}
 
-	if ((error = git_remote_get_fetch_refspecs(&refspecs, source)) < 0 ||
-	    (error = git_remote_set_fetch_refspecs(remote, &refspecs)) < 0)
-		goto cleanup;
-
-	git_strarray_free(&refspecs);
-
-	if ((error = git_remote_get_push_refspecs(&refspecs, source)) < 0 ||
-	    (error = git_remote_set_push_refspecs(remote, &refspecs)) < 0)
-		goto cleanup;
+	git_vector_foreach(&source->refspecs, i, spec) {
+		if ((error = add_refspec(remote, spec->string, !spec->push)) < 0)
+			goto cleanup;
+	}
 
 	*dest = remote;
 
 cleanup:
 
-	git_strarray_free(&refspecs);
-
 	if (error < 0)
 		git__free(remote);
 
@@ -1449,18 +1453,20 @@ static int next_head(const git_remote *remote, git_vector *refs,
 	return GIT_ITEROVER;
 }
 
-static int opportunistic_updates(const git_remote *remote, git_vector *refs, const char *msg)
+static int opportunistic_updates(const git_remote *remote, const git_remote_callbacks *callbacks,
+				 git_vector *refs, const char *msg)
 {
 	size_t i, j, k;
 	git_refspec *spec;
 	git_remote_head *head;
 	git_reference *ref;
 	git_buf refname = GIT_BUF_INIT;
-	int error;
+	int error = 0;
 
 	i = j = k = 0;
 
 	while ((error = next_head(remote, refs, &spec, &head, &i, &j, &k)) == 0) {
+		git_oid old = {{ 0 }};
 		/*
 		 * If we got here, there is a refspec which was used
 		 * for fetching which matches the source of one of the
@@ -1469,18 +1475,38 @@ static int opportunistic_updates(const git_remote *remote, git_vector *refs, con
 		 * FETCH_HEAD
 		 */
 
+		git_buf_clear(&refname);
 		if ((error = git_refspec_transform(&refname, spec, head->name)) < 0)
-			return error;
+			goto cleanup;
 
-		error = git_reference_create(&ref, remote->repo, refname.ptr, &head->oid, true, msg);
-		git_buf_free(&refname);
-		git_reference_free(ref);
+		error = git_reference_name_to_id(&old, remote->repo, refname.ptr);
+		if (error < 0 && error != GIT_ENOTFOUND)
+			goto cleanup;
 
+		if (!git_oid_cmp(&old, &head->oid))
+			continue;
+
+		/* If we did find a current reference, make sure we haven't lost a race */
+		if (error)
+			error = git_reference_create(&ref, remote->repo, refname.ptr, &head->oid, true, msg);
+		else
+			error = git_reference_create_matching(&ref, remote->repo, refname.ptr, &head->oid, true, &old, msg);
+		git_reference_free(ref);
 		if (error < 0)
-			return error;
+			goto cleanup;
+
+		if (callbacks && callbacks->update_tips != NULL) {
+			if (callbacks->update_tips(refname.ptr, &old, &head->oid, callbacks->payload) < 0)
+				goto cleanup;
+		}
 	}
 
-	return 0;
+	if (error == GIT_ITEROVER)
+		error = 0;
+
+cleanup:
+	git_buf_free(&refname);
+	return error;
 }
 
 int git_remote_update_tips(
@@ -1528,7 +1554,7 @@ int git_remote_update_tips(
 
 	/* only try to do opportunisitic updates if the refpec lists differ */
 	if (remote->passed_refspecs)
-		error = opportunistic_updates(remote, &refs, reflog_message);
+		error = opportunistic_updates(remote, callbacks, &refs, reflog_message);
 
 out:
 	git_vector_free(&refs);
@@ -2046,16 +2072,6 @@ static int set_refspecs(git_remote *remote, git_strarray *array, int push)
 	return 0;
 }
 
-int git_remote_set_fetch_refspecs(git_remote *remote, git_strarray *array)
-{
-	return set_refspecs(remote, array, false);
-}
-
-int git_remote_set_push_refspecs(git_remote *remote, git_strarray *array)
-{
-	return set_refspecs(remote, array, true);
-}
-
 static int copy_refspecs(git_strarray *array, const git_remote *remote, unsigned int push)
 {
 	size_t i;
diff --git a/tests/network/remote/local.c b/tests/network/remote/local.c
index 9d96184a2..5d726c958 100644
--- a/tests/network/remote/local.c
+++ b/tests/network/remote/local.c
@@ -39,7 +39,7 @@ static void connect_to_local_repository(const char *local_repository)
 {
 	git_buf_sets(&file_path_buf, cl_git_path_url(local_repository));
 
-	cl_git_pass(git_remote_create_anonymous(&remote, repo, git_buf_cstr(&file_path_buf), NULL));
+	cl_git_pass(git_remote_create_anonymous(&remote, repo, git_buf_cstr(&file_path_buf)));
 	cl_git_pass(git_remote_connect(remote, GIT_DIRECTION_FETCH, NULL));
 }
 
@@ -71,7 +71,7 @@ void test_network_remote_local__retrieve_advertised_before_connect(void)
 
 	git_buf_sets(&file_path_buf, cl_git_path_url(cl_fixture("testrepo.git")));
 
-	cl_git_pass(git_remote_create_anonymous(&remote, repo, git_buf_cstr(&file_path_buf), NULL));
+	cl_git_pass(git_remote_create_anonymous(&remote, repo, git_buf_cstr(&file_path_buf)));
 	cl_git_fail(git_remote_ls(&refs, &refs_len, remote));
 }
 
@@ -213,7 +213,7 @@ void test_network_remote_local__push_to_bare_remote(void)
 	}
 
 	/* Connect to the bare repo */
-	cl_git_pass(git_remote_create_anonymous(&localremote, repo, "./localbare.git", NULL));
+	cl_git_pass(git_remote_create_anonymous(&localremote, repo, "./localbare.git"));
 	cl_git_pass(git_remote_connect(localremote, GIT_DIRECTION_PUSH, NULL));
 
 	/* Try to push */
@@ -252,7 +252,7 @@ void test_network_remote_local__push_to_bare_remote_with_file_url(void)
 	url = cl_git_path_url("./localbare.git");
 
 	/* Connect to the bare repo */
-	cl_git_pass(git_remote_create_anonymous(&localremote, repo, url, NULL));
+	cl_git_pass(git_remote_create_anonymous(&localremote, repo, url));
 	cl_git_pass(git_remote_connect(localremote, GIT_DIRECTION_PUSH, NULL));
 
 	/* Try to push */
@@ -289,7 +289,7 @@ void test_network_remote_local__push_to_non_bare_remote(void)
 	}
 
 	/* Connect to the bare repo */
-	cl_git_pass(git_remote_create_anonymous(&localremote, repo, "./localnonbare", NULL));
+	cl_git_pass(git_remote_create_anonymous(&localremote, repo, "./localnonbare"));
 	cl_git_pass(git_remote_connect(localremote, GIT_DIRECTION_PUSH, NULL));
 
 	/* Try to push */
diff --git a/tests/network/remote/remotes.c b/tests/network/remote/remotes.c
index f81c1ccc0..2fa21d460 100644
--- a/tests/network/remote/remotes.c
+++ b/tests/network/remote/remotes.c
@@ -90,7 +90,7 @@ void test_network_remote_remotes__error_when_no_push_available(void)
 	};
 
 
-	cl_git_pass(git_remote_create_anonymous(&r, _repo, cl_fixture("testrepo.git"), NULL));
+	cl_git_pass(git_remote_create_anonymous(&r, _repo, cl_fixture("testrepo.git")));
 
 	callbacks.transport = git_transport_local;
 	cl_git_pass(git_remote_connect(r, GIT_DIRECTION_PUSH, &callbacks));
@@ -128,6 +128,8 @@ void test_network_remote_remotes__add_fetchspec(void)
 	cl_assert_equal_s(git_refspec_dst(_refspec), "refs/*");
 	cl_assert_equal_s(git_refspec_string(_refspec), "refs/*:refs/*");
 	cl_assert_equal_b(_refspec->push, false);
+
+	cl_git_fail_with(GIT_EINVALIDSPEC, git_remote_add_fetch(_repo, "test", "refs/*/foo/*:refs/*"));
 }
 
 void test_network_remote_remotes__dup(void)
@@ -465,13 +467,3 @@ void test_network_remote_remotes__query_refspecs(void)
 	git_remote_free(remote);
 	git_remote_delete(_repo, "test");
 }
-
-void test_network_remote_remotes__fetch_from_anonymous(void)
-{
-	git_remote *remote;
-
-	cl_git_pass(git_remote_create_anonymous(&remote, _repo, cl_fixture("testrepo.git"),
-						"refs/heads/*:refs/other/*"));
-	cl_git_pass(git_remote_fetch(remote, NULL, NULL, NULL));
-	git_remote_free(remote);
-}
diff --git a/tests/online/remotes.c b/tests/online/remotes.c
index 70374be2f..a86f2d9ae 100644
--- a/tests/online/remotes.c
+++ b/tests/online/remotes.c
@@ -1,19 +1,12 @@
 #include "clar_libgit2.h"
 
+static const char *refspec = "refs/heads/first-merge:refs/remotes/origin/first-merge";
+
 static int remote_single_branch(git_remote **out, git_repository *repo, const char *name, const char *url, void *payload)
 {
-	char *fetch_refspecs[] = {
-		"refs/heads/first-merge:refs/remotes/origin/first-merge",
-	};
-	git_strarray fetch_refspecs_strarray = {
-		fetch_refspecs,
-		1,
-	};
-
 	GIT_UNUSED(payload);
 
-	cl_git_pass(git_remote_create(out, repo, name, url));
-	cl_git_pass(git_remote_set_fetch_refspecs(*out, &fetch_refspecs_strarray));
+	cl_git_pass(git_remote_create_with_fetchspec(out, repo, name, url, refspec));
 
 	return 0;
 }
@@ -22,6 +15,7 @@ void test_online_remotes__single_branch(void)
 {
 	git_clone_options opts = GIT_CLONE_OPTIONS_INIT;
 	git_repository *repo;
+	git_remote *remote;
 	git_strarray refs;
 	size_t i, count = 0;
 
@@ -38,6 +32,15 @@ void test_online_remotes__single_branch(void)
 	cl_assert_equal_i(1, count);
 
 	git_strarray_free(&refs);
+
+	cl_git_pass(git_remote_lookup(&remote, repo, "origin"));
+	cl_git_pass(git_remote_get_fetch_refspecs(&refs, remote));
+
+	cl_assert_equal_i(1, refs.count);
+	cl_assert_equal_s(refspec, refs.strings[0]);
+
+	git_strarray_free(&refs);
+	git_remote_free(remote);
 	git_repository_free(repo);
 }