remote: make _ls return the list directly

The callback-based method of listing remote references dates back to the
beginning of the network code's lifetime, when we didn't know any
better.

We need to keep the list around for update_tips() after disconnect() so
let's make use of this to simply give the user a pointer to the array so
they can write straightforward code instead of having to go through a
callback.

examples/network/ls-remote.c

@@ -4,21 +4,12 @@
 #include <string.h>
 #include "common.h"
 
-/** Callback to show each item */
-static int show_ref__cb(git_remote_head *head, void *payload)
-{
-	char oid[GIT_OID_HEXSZ + 1] = {0};
-
-	(void)payload;
-	git_oid_fmt(oid, &head->oid);
-	printf("%s\t%s\n", oid, head->name);
-	return 0;
-}
-
 static int use_remote(git_repository *repo, char *name)
 {
 	git_remote *remote = NULL;
 	int error;
+	const git_remote_head **refs;
+	size_t refs_len, i;
 	git_remote_callbacks callbacks = GIT_REMOTE_CALLBACKS_INIT;
 
 	// Find the remote by name
@@ -40,7 +31,18 @@ static int use_remote(git_repository *repo, char *name)
 	if (error < 0)
 		goto cleanup;
 
-	error = git_remote_ls(remote, &show_ref__cb, NULL);
+	/**
+	 * Get the list of references on the remote and print out
+	 * their name next to what they point to.
+	 */
+	if (git_remote_ls(&refs, &refs_len, remote) < 0)
+		goto cleanup;
+
+	for (i = 0; i < refs_len; i++) {
+		char oid[GIT_OID_HEXSZ + 1] = {0};
+		git_oid_fmt(oid, &refs[i]->oid);
+		printf("%s\t%s\n", oid, refs[i]->name);
+	}
 
 cleanup:
 	git_remote_free(remote);

include/git2/remote.h

@@ -254,15 +254,16 @@ GIT_EXTERN(int) git_remote_connect(git_remote *remote, git_direction direction);
  * The remote (or more exactly its transport) must be connected. The
  * memory belongs to the remote.
  *
- * If you a return a non-zero value from the callback, this will stop
- * looping over the refs.
+ * The array will stay valid as long as the remote object exists and
+ * its transport isn't changed, but a copy is recommended for usage of
+ * the data.
  *
+ * @param out pointer to the array
+ * @param size the number of remote heads
  * @param remote the remote
- * @param list_cb function to call with each ref discovered at the remote
- * @param payload additional data to pass to the callback
- * @return 0 on success, GIT_EUSER on non-zero callback, or error code
+ * @return 0 on success, or an error code
  */
-GIT_EXTERN(int) git_remote_ls(git_remote *remote, git_headlist_cb list_cb, void *payload);
+GIT_EXTERN(int) git_remote_ls(const git_remote_head ***out,  size_t *size, git_remote *remote);
 
 /**
  * Download and index the packfile

include/git2/transport.h

@@ -203,13 +203,13 @@ struct git_transport {
 		int direction,
 		int flags);
 
-	/* This function may be called after a successful call to connect(). The
-	 * provided callback is invoked for each ref discovered on the remote
-	 * end. */
+	/* This function may be called after a successful call to
+	 * connect(). The array returned is owned by the transport and
+	 * is guranteed until the next call of a transport function. */
 	int (*ls)(
-		git_transport *transport,
-		git_headlist_cb list_cb,
-		void *payload);
+		const git_remote_head ***out,
+		size_t *size,
+		git_transport *transport);
 
 	/* Executes the push whose context is in the git_push object. */
 	int (*push)(git_transport *transport, git_push *push);

src/clone.c

@@ -176,25 +176,20 @@ static int update_head_to_new_branch(
 	return error;
 }
 
-static int get_head_callback(git_remote_head *head, void *payload)
-{
-	git_remote_head **destination = (git_remote_head **)payload;
-
-	/* Save the first entry, and terminate the enumeration */
-	*destination = head;
-	return 1;
-}
-
 static int update_head_to_remote(git_repository *repo, git_remote *remote)
 {
 	int retcode = -1;
+	size_t refs_len;
 	git_refspec dummy_spec;
-	git_remote_head *remote_head;
+	const git_remote_head *remote_head, **refs;
 	struct head_info head_info;
 	git_buf remote_master_name = GIT_BUF_INIT;
 
+	if (git_remote_ls(&refs, &refs_len, remote) < 0)
+		return -1;
+
 	/* Did we just clone an empty repository? */
-	if (remote->refs.length == 0) {
+	if (refs_len == 0) {
 		return setup_tracking_config(
 			repo,
 			"master",
@@ -202,12 +197,8 @@ static int update_head_to_remote(git_repository *repo, git_remote *remote)
 			GIT_REFS_HEADS_MASTER_FILE);
 	}
 
-	/* Get the remote's HEAD. This is always the first ref in remote->refs. */
-	remote_head = NULL;
-
-	if (!remote->transport->ls(remote->transport, get_head_callback, &remote_head))
-		return -1;
-
+	/* Get the remote's HEAD. This is always the first ref in the list. */
+	remote_head = refs[0];
 	assert(remote_head);
 
 	git_oid_cpy(&head_info.remote_head_oid, &remote_head->oid);

src/fetch.c

@@ -19,52 +19,47 @@
 #include "repository.h"
 #include "refs.h"
 
-struct filter_payload {
-	git_remote *remote;
-	const git_refspec *spec, *tagspec;
-	git_odb *odb;
-};
-
-static int filter_ref__cb(git_remote_head *head, void *payload)
+static int maybe_want(git_remote *remote, git_remote_head *head, git_odb *odb, git_refspec *tagspec)
 {
-	struct filter_payload *p = payload;
 	int match = 0;
 
 	if (!git_reference_is_valid_name(head->name))
 		return 0;
 
-	if (p->remote->download_tags == GIT_REMOTE_DOWNLOAD_TAGS_ALL) {
+	if (remote->download_tags == GIT_REMOTE_DOWNLOAD_TAGS_ALL) {
 		/*
 		 * If tagopt is --tags, then we only use the default
 		 * tags refspec and ignore the remote's
 		 */
-		if (git_refspec_src_matches(p->tagspec, head->name))
+		if (git_refspec_src_matches(tagspec, head->name))
 			match = 1;
 		else
 			return 0;
-	} else if (git_remote__matching_refspec(p->remote, head->name))
+	} else if (git_remote__matching_refspec(remote, head->name))
 			match = 1;
 
 	if (!match)
 		return 0;
 
 	/* If we have the object, mark it so we don't ask for it */
-	if (git_odb_exists(p->odb, &head->oid))
+	if (git_odb_exists(odb, &head->oid))
 		head->local = 1;
 	else
-		p->remote->need_pack = 1;
+		remote->need_pack = 1;
 
-	return git_vector_insert(&p->remote->refs, head);
+	return git_vector_insert(&remote->refs, head);
 }
 
 static int filter_wants(git_remote *remote)
 {
-	struct filter_payload p;
+	git_remote_head **heads;
 	git_refspec tagspec, head;
-	int error = -1;
+	int error = 0;
+	git_odb *odb;
+	size_t i, heads_len;
 
-	//git_vector_clear(&remote->refs);
-	if (git_refspec__parse(&tagspec, GIT_REFSPEC_TAGS, true) < 0)
+	git_vector_clear(&remote->refs);
+	if ((error = git_refspec__parse(&tagspec, GIT_REFSPEC_TAGS, true)) < 0)
 		return error;
 
 	/*
@@ -73,8 +68,6 @@ static int filter_wants(git_remote *remote)
 	 * not interested in any particular branch but just the remote's
 	 * HEAD, which will be stored in FETCH_HEAD after the fetch.
 	 */
-	p.tagspec = &tagspec;
-	p.remote = remote;
 	if (remote->active_refspecs.length == 0) {
 		if ((error = git_refspec__parse(&head, "HEAD", true)) < 0)
 			goto cleanup;
@@ -83,12 +76,16 @@ static int filter_wants(git_remote *remote)
 			goto cleanup;
 	}
 
-	git_vector_clear(&remote->refs);
+	if (git_repository_odb__weakptr(&odb, remote->repo) < 0)
+		goto cleanup;
 
-	if (git_repository_odb__weakptr(&p.odb, remote->repo) < 0)
+	if (git_remote_ls((const git_remote_head ***)&heads, &heads_len, remote) < 0)
 		goto cleanup;
 
-	error = git_remote_ls(remote, filter_ref__cb, &p);
+	for (i = 0; i < heads_len; i++) {
+		if ((error = maybe_want(remote, heads[i], odb, &tagspec)) < 0)
+			break;
+	}
 
 cleanup:
 	git_refspec__free(&tagspec);

src/push.c

@@ -616,16 +616,22 @@ on_error:
 	return error;
 }
 
-static int cb_filter_refs(git_remote_head *ref, void *data)
-{
-	git_remote *remote = (git_remote *) data;
-	return git_vector_insert(&remote->refs, ref);
-}
-
 static int filter_refs(git_remote *remote)
 {
+	const git_remote_head **heads;
+	size_t heads_len, i;
+
 	git_vector_clear(&remote->refs);
-	return git_remote_ls(remote, cb_filter_refs, remote);
+
+	if (git_remote_ls(&heads, &heads_len, remote) < 0)
+		return -1;
+
+	for (i = 0; i < heads_len; i++) {
+		if (git_vector_insert(&remote->refs, heads[i]) < 0)
+			return -1;
+	}
+
+	return 0;
 }
 
 int git_push_finish(git_push *push)

src/remote.c

@@ -620,11 +620,11 @@ on_error:
 	return -1;
 }
 
-int git_remote_ls(git_remote *remote, git_headlist_cb list_cb, void *payload)
+int git_remote_ls(const git_remote_head ***out, size_t *size, git_remote *remote)
 {
 	assert(remote);
 
-	return remote->transport->ls(remote->transport, list_cb, payload);
+	return remote->transport->ls(out, size, remote->transport);
 }
 
 int git_remote__get_http_proxy(git_remote *remote, bool use_ssl, char **proxy_url)
@@ -684,13 +684,6 @@ int git_remote__get_http_proxy(git_remote *remote, bool use_ssl, char **proxy_ur
 	return 0;
 }
 
-static int store_refs(git_remote_head *head, void *payload)
-{
-	git_vector *refs = (git_vector *)payload;
-
-	return git_vector_insert(refs, head);
-}
-
 /* DWIM `refspecs` based on `refs` and append the output to `out` */
 static int dwim_refspecs(git_vector *out, git_vector *refspecs, git_vector *refs)
 {
@@ -736,15 +729,12 @@ int git_remote_download(git_remote *remote)
 	if (git_vector_init(&refs, 8, remote_head_cmp) < 0)
 		return -1;
 
-	if (git_remote_ls(remote, store_refs, &refs) < 0) {
+	if (git_remote_ls((const git_remote_head ***)&refs.contents, &refs.length, remote) < 0)
 		return -1;
-	}
 
 	free_refspecs(&remote->active_refspecs);
 
-	error = dwim_refspecs(&remote->active_refspecs, &remote->refspecs, &refs);
-	git_vector_free(&refs);
-	if (error < 0)
+	if (dwim_refspecs(&remote->active_refspecs, &remote->refspecs, &refs) < 0)
 		return -1;
 
 	if ((error = git_fetch_negotiate(remote)) < 0)
@@ -996,7 +986,7 @@ int git_remote_update_tips(git_remote *remote)
 	if (git_vector_init(&refs, 16, NULL) < 0)
 		return -1;
 
-	if ((error = git_remote_ls(remote, store_refs, &refs)) < 0)
+	if ((error = git_remote_ls((const git_remote_head ***)&refs.contents, &refs.length, remote)) < 0)
 		goto out;
 
 	if (remote->download_tags == GIT_REMOTE_DOWNLOAD_TAGS_ALL) {
@@ -1014,7 +1004,6 @@ int git_remote_update_tips(git_remote *remote)
 
 out:
 	git_refspec__free(&tagspec);
-	git_vector_free(&refs);
 	return error;
 }
 

src/transports/local.c

@@ -213,21 +213,17 @@ static int local_connect(
 	return 0;
 }
 
-static int local_ls(git_transport *transport, git_headlist_cb list_cb, void *payload)
+static int local_ls(const git_remote_head ***out, size_t *size, git_transport *transport)
 {
 	transport_local *t = (transport_local *)transport;
-	unsigned int i;
-	git_remote_head *head = NULL;
 
 	if (!t->have_refs) {
 		giterr_set(GITERR_NET, "The transport has not yet loaded the refs");
 		return -1;
 	}
 
-	git_vector_foreach(&t->refs, i, head) {
-		if (list_cb(head, payload))
-			return GIT_EUSER;
-	}
+	*out = (const git_remote_head **) t->refs.contents;
+	*size = t->refs.length;
 
 	return 0;
 }

src/transports/smart.c

@@ -74,6 +74,7 @@ static int git_smart__connect(
 	transport_smart *t = (transport_smart *)transport;
 	git_smart_subtransport_stream *stream;
 	int error;
+	size_t i;
 	git_pkt *pkt;
 	git_pkt_ref *first;
 	git_smart_service_t service;
@@ -140,6 +141,16 @@ static int git_smart__connect(
 		git_pkt_free((git_pkt *)first);
 	}
 
+	/* Keep a list of heads for _ls */
+	git_vector_foreach(&t->refs, i, pkt) {
+		git_pkt_ref *ref = (git_pkt_ref *) pkt;
+		if (pkt->type != GIT_PKT_REF)
+			continue;
+
+		if (git_vector_insert(&t->heads, &ref->head) < 0)
+			return -1;
+	}
+
 	if (t->rpc && git_smart__reset_stream(t, false) < 0)
 		return -1;
 
@@ -149,28 +160,17 @@ static int git_smart__connect(
 	return 0;
 }
 
-static int git_smart__ls(git_transport *transport, git_headlist_cb list_cb, void *payload)
+static int git_smart__ls(const git_remote_head ***out, size_t *size, git_transport *transport)
 {
 	transport_smart *t = (transport_smart *)transport;
-	unsigned int i;
-	git_pkt *p = NULL;
 
 	if (!t->have_refs) {
 		giterr_set(GITERR_NET, "The transport has not yet loaded the refs");
 		return -1;
 	}
 
-	git_vector_foreach(&t->refs, i, p) {
-		git_pkt_ref *pkt = NULL;
-
-		if (p->type != GIT_PKT_REF)
-			continue;
-
-		pkt = (git_pkt_ref *)p;
-
-		if (list_cb(&pkt->head, payload))
-			return GIT_EUSER;
-	}
+	*out = (const git_remote_head **) t->heads.contents;
+	*size = t->heads.length;
 
 	return 0;
 }
@@ -293,6 +293,7 @@ static void git_smart__free(git_transport *transport)
 	/* Free the subtransport */
 	t->wrapped->free(t->wrapped);
 
+	git_vector_free(&t->heads);
 	git_vector_foreach(refs, i, p)
 		git_pkt_free(p);
 
@@ -340,6 +341,11 @@ int git_transport_smart(git_transport **out, git_remote *owner, void *param)
 		return -1;
 	}
 
+	if (git_vector_init(&t->heads, 16, ref_name_cmp) < 0) {
+		git__free(t);
+		return -1;
+	}
+
 	if (definition->callback(&t->wrapped, &t->parent) < 0) {
 		git__free(t);
 		return -1;

src/transports/smart.h

@@ -140,6 +140,7 @@ typedef struct {
 	git_smart_subtransport_stream *current_stream;
 	transport_smart_caps caps;
 	git_vector refs;
+	git_vector heads;
 	git_vector common;
 	git_atomic cancelled;
 	packetsize_cb packetsize_cb;

src/transports/smart_protocol.c

@@ -32,6 +32,7 @@ int git_smart__store_refs(transport_smart *t, int flushes)
 	/* Clear existing refs in case git_remote_connect() is called again
 	 * after git_remote_disconnect().
 	 */
+	git_vector_clear(&t->heads);
 	git_vector_foreach(refs, i, ref) {
 		git__free(ref->head.name);
 		git__free(ref);

tests-clar/network/remote/local.c

@@ -26,26 +26,6 @@ void test_network_remote_local__cleanup(void)
 	cl_fixture_cleanup("remotelocal");
 }
 
-static int count_ref__cb(git_remote_head *head, void *payload)
-{
-	int *count = (int *)payload;
-
-	(void)head;
-	(*count)++;
-
-	return 0;
-}
-
-static int ensure_peeled__cb(git_remote_head *head, void *payload)
-{
-	GIT_UNUSED(payload);
-
-	if(strcmp(head->name, "refs/tags/test^{}") != 0)
-		return 0;
-
-	return git_oid_streq(&head->oid, "e90810b8df3e80c413d903f631643c716887138d");
-}
-
 static void connect_to_local_repository(const char *local_repository)
 {
 	git_buf_sets(&file_path_buf, cl_git_path_url(local_repository));
@@ -65,39 +45,42 @@ void test_network_remote_local__connected(void)
 
 void test_network_remote_local__retrieve_advertised_references(void)
 {
-	int how_many_refs = 0;
+	const git_remote_head **refs;
+	size_t refs_len;
 
 	connect_to_local_repository(cl_fixture("testrepo.git"));
 
-	cl_git_pass(git_remote_ls(remote, &count_ref__cb, &how_many_refs));
+	cl_git_pass(git_remote_ls(&refs, &refs_len, remote));
 
-	cl_assert_equal_i(how_many_refs, 28);
+	cl_assert_equal_i(refs_len, 28);
 }
 
 void test_network_remote_local__retrieve_advertised_references_after_disconnect(void)
 {
-	int how_many_refs = 0;
+	const git_remote_head **refs;
+	size_t refs_len;
 
 	connect_to_local_repository(cl_fixture("testrepo.git"));
 	git_remote_disconnect(remote);
 
-	cl_git_pass(git_remote_ls(remote, &count_ref__cb, &how_many_refs));
+	cl_git_pass(git_remote_ls(&refs, &refs_len, remote));
 
-	cl_assert_equal_i(how_many_refs, 28);
+	cl_assert_equal_i(refs_len, 28);
 }
 
 void test_network_remote_local__retrieve_advertised_references_from_spaced_repository(void)
 {
-	int how_many_refs = 0;
+	const git_remote_head **refs;
+	size_t refs_len;
 
 	cl_fixture_sandbox("testrepo.git");
 	cl_git_pass(p_rename("testrepo.git", "spaced testrepo.git"));
 
 	connect_to_local_repository("spaced testrepo.git");
 
-	cl_git_pass(git_remote_ls(remote, &count_ref__cb, &how_many_refs));
+	cl_git_pass(git_remote_ls(&refs, &refs_len, remote));
 
-	cl_assert_equal_i(how_many_refs, 28);
+	cl_assert_equal_i(refs_len, 28);
 
 	git_remote_free(remote);	/* Disconnect from the "spaced repo" before the cleanup */
 	remote = NULL;
@@ -107,9 +90,17 @@ void test_network_remote_local__retrieve_advertised_references_from_spaced_repos
 
 void test_network_remote_local__nested_tags_are_completely_peeled(void)
 {
+	const git_remote_head **refs;
+	size_t refs_len, i;
+
 	connect_to_local_repository(cl_fixture("testrepo.git"));
 
-	cl_git_pass(git_remote_ls(remote, &ensure_peeled__cb, NULL));
+	cl_git_pass(git_remote_ls(&refs, &refs_len, remote));
+
+	for (i = 0; i < refs_len; i++) {
+		if (!strcmp(refs[i]->name, "refs/tags/test^{}"))
+			cl_git_pass(git_oid_streq(&refs[i]->oid, "e90810b8df3e80c413d903f631643c716887138d"));
+	}
 }
 
 void test_network_remote_local__shorthand_fetch_refspec0(void)

tests-clar/online/fetch.c

@@ -152,29 +152,20 @@ void test_online_fetch__can_cancel(void)
 	git_remote_free(remote);
 }
 
-int ls_cb(git_remote_head *rhead, void *payload)
-{
-	int *nr = payload;
-	GIT_UNUSED(rhead);
-
-	(*nr)++;
-
-	return 0;
-}
-
 void test_online_fetch__ls_disconnected(void)
 {
+	const git_remote_head **refs;
+	size_t refs_len_before, refs_len_after;
 	git_remote *remote;
-	int nr_before = 0, nr_after = 0;
 
 	cl_git_pass(git_remote_create(&remote, _repo, "test",
 				"http://github.com/libgit2/TestGitRepository.git"));
 	cl_git_pass(git_remote_connect(remote, GIT_DIRECTION_FETCH));
-	cl_git_pass(git_remote_ls(remote, ls_cb, &nr_before));
+	cl_git_pass(git_remote_ls(&refs, &refs_len_before, remote));
 	git_remote_disconnect(remote);
-	cl_git_pass(git_remote_ls(remote, ls_cb, &nr_after));
+	cl_git_pass(git_remote_ls(&refs, &refs_len_after, remote));
 
-	cl_assert_equal_i(nr_before, nr_after);
+	cl_assert_equal_i(refs_len_before, refs_len_after);
 
 	git_remote_free(remote);
 }

tests-clar/online/push.c

@@ -155,10 +155,11 @@ static void do_verify_push_status(git_push *push, const push_status expected[],
  */
 static void verify_refs(git_remote *remote, expected_ref expected_refs[], size_t expected_refs_len)
 {
-	git_vector actual_refs = GIT_VECTOR_INIT;
+	const git_remote_head **actual_refs;
+	size_t actual_refs_len;
 
-	git_remote_ls(remote, record_ref_cb, &actual_refs);
-	verify_remote_refs(&actual_refs, expected_refs, expected_refs_len);
+	git_remote_ls(&actual_refs, &actual_refs_len, remote);
+	verify_remote_refs(actual_refs, actual_refs_len, expected_refs, expected_refs_len);
 
 	git_vector_free(&actual_refs);
 }

tests-clar/online/push_util.c

@@ -69,26 +69,28 @@ int record_ref_cb(git_remote_head *head, void *payload)
 	return git_vector_insert(refs, head);
 }
 
-void verify_remote_refs(git_vector *actual_refs, const expected_ref expected_refs[], size_t expected_refs_len)
+void verify_remote_refs(const git_remote_head *actual_refs[], size_t actual_refs_len, const expected_ref expected_refs[], size_t expected_refs_len)
 {
 	size_t i, j = 0;
 	git_buf msg = GIT_BUF_INIT;
-	git_remote_head *actual;
+	const git_remote_head *actual;
 	char *oid_str;
 	bool master_present = false;
 
 	/* We don't care whether "master" is present on the other end or not */
-	git_vector_foreach(actual_refs, i, actual) {
+	for (i = 0; i < actual_refs_len; i++) {
+		actual = actual_refs[i];
 		if (!strcmp(actual->name, "refs/heads/master")) {
 			master_present = true;
 			break;
 		}
 	}
 
-	if (expected_refs_len + (master_present ? 1 : 0) != actual_refs->length)
+	if (expected_refs_len + (master_present ? 1 : 0) != actual_refs_len)
 		goto failed;
 
-	git_vector_foreach(actual_refs, i, actual) {
+	for (i = 0; i < actual_refs_len; i++) {
+		actual = actual_refs[i];
 		if (master_present && !strcmp(actual->name, "refs/heads/master"))
 			continue;
 
@@ -111,7 +113,8 @@ failed:
 	}
 
 	git_buf_puts(&msg, "\nACTUAL:\n");
-	git_vector_foreach(actual_refs, i, actual) {
+	for (i = 0; i < actual_refs_len; i++) {
+		actual = actual_refs[i];
 		if (master_present && !strcmp(actual->name, "refs/heads/master"))
 			continue;
 

tests-clar/online/push_util.h

@@ -60,10 +60,11 @@ int record_ref_cb(git_remote_head *head, void *payload);
  * Verifies that refs on remote stored by record_ref_cb match the expected
  * names, oids, and order.
  *
- * @param actual_refs actual refs stored by record_ref_cb()
+ * @param actual_refs actual refs in the remote
+ * @param actual_refs_len length of actual_refs
  * @param expected_refs expected remote refs
  * @param expected_refs_len length of expected_refs
  */
-void verify_remote_refs(git_vector *actual_refs, const expected_ref expected_refs[], size_t expected_refs_len);
+void verify_remote_refs(const git_remote_head *actual_refs[], size_t actual_refs_len, const expected_ref expected_refs[], size_t expected_refs_len);
 
 #endif /* INCLUDE_cl_push_util_h__ */