From 359dce726d215be65aee719c7316b43e3851c031 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Sat, 2 Nov 2013 00:05:32 +0100 Subject: [PATCH] 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 | 26 ++++++++-------- include/git2/remote.h | 13 ++++---- include/git2/transport.h | 12 ++++---- src/clone.c | 25 +++++---------- src/fetch.c | 45 +++++++++++++-------------- src/push.c | 20 +++++++----- src/remote.c | 21 +++---------- src/transports/local.c | 10 ++---- src/transports/smart.c | 34 ++++++++++++--------- src/transports/smart.h | 1 + src/transports/smart_protocol.c | 1 + tests-clar/network/remote/local.c | 51 +++++++++++++------------------ tests-clar/online/fetch.c | 19 +++--------- tests-clar/online/push.c | 7 +++-- tests-clar/online/push_util.c | 15 +++++---- tests-clar/online/push_util.h | 5 +-- 16 files changed, 141 insertions(+), 164 deletions(-) diff --git a/examples/network/ls-remote.c b/examples/network/ls-remote.c index 18cd02367..1e08b293e 100644 --- a/examples/network/ls-remote.c +++ b/examples/network/ls-remote.c @@ -4,21 +4,12 @@ #include #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); diff --git a/include/git2/remote.h b/include/git2/remote.h index 1f33c92e8..9e7d218a8 100644 --- a/include/git2/remote.h +++ b/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 diff --git a/include/git2/transport.h b/include/git2/transport.h index 147b3fdcb..81ebf4dc9 100644 --- a/include/git2/transport.h +++ b/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); diff --git a/src/clone.c b/src/clone.c index f9338b746..23aacd478 100644 --- a/src/clone.c +++ b/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); diff --git a/src/fetch.c b/src/fetch.c index 295b036ef..19afddcd8 100644 --- a/src/fetch.c +++ b/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(&p.odb, remote->repo) < 0) + if (git_repository_odb__weakptr(&odb, remote->repo) < 0) goto cleanup; - error = git_remote_ls(remote, filter_ref__cb, &p); + if (git_remote_ls((const git_remote_head ***)&heads, &heads_len, remote) < 0) + goto cleanup; + + for (i = 0; i < heads_len; i++) { + if ((error = maybe_want(remote, heads[i], odb, &tagspec)) < 0) + break; + } cleanup: git_refspec__free(&tagspec); diff --git a/src/push.c b/src/push.c index a799db8d0..4f442c6a2 100644 --- a/src/push.c +++ b/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) diff --git a/src/remote.c b/src/remote.c index a2fd21007..289ead802 100644 --- a/src/remote.c +++ b/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; } diff --git a/src/transports/local.c b/src/transports/local.c index 3163d2eac..4502f0202 100644 --- a/src/transports/local.c +++ b/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; } diff --git a/src/transports/smart.c b/src/transports/smart.c index a681d5f40..53f880583 100644 --- a/src/transports/smart.c +++ b/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; diff --git a/src/transports/smart.h b/src/transports/smart.h index 5232e54de..b46a798a4 100644 --- a/src/transports/smart.h +++ b/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; diff --git a/src/transports/smart_protocol.c b/src/transports/smart_protocol.c index ce3f115ee..4fe7c0d09 100644 --- a/src/transports/smart_protocol.c +++ b/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); diff --git a/tests-clar/network/remote/local.c b/tests-clar/network/remote/local.c index 6d658a2e4..309142925 100644 --- a/tests-clar/network/remote/local.c +++ b/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) diff --git a/tests-clar/online/fetch.c b/tests-clar/online/fetch.c index df1b2e288..5153a7ae0 100644 --- a/tests-clar/online/fetch.c +++ b/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); } diff --git a/tests-clar/online/push.c b/tests-clar/online/push.c index 5a747bba7..9ed97174b 100644 --- a/tests-clar/online/push.c +++ b/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); } diff --git a/tests-clar/online/push_util.c b/tests-clar/online/push_util.c index 2e457844d..e3f326932 100644 --- a/tests-clar/online/push_util.c +++ b/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; diff --git a/tests-clar/online/push_util.h b/tests-clar/online/push_util.h index 64f02cf2f..e6ddb7133 100644 --- a/tests-clar/online/push_util.h +++ b/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__ */