From 99bcb883cb2bd2d27939a831a66d794770427e98 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Thu, 27 Sep 2018 12:24:04 -0700 Subject: [PATCH 1/4] transport: allow skipping of ref listing The get_refs_via_connect() function both performs the handshake (including determining the protocol version) and obtaining the list of remote refs. However, the fetch protocol v2 supports fetching objects without the listing of refs, so make it possible for the user to skip the listing by creating a new handshake() function. This will be used in a subsequent commit. Signed-off-by: Junio C Hamano --- transport.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/transport.c b/transport.c index 1c76d64aba..5fb9ff6b56 100644 --- a/transport.c +++ b/transport.c @@ -252,8 +252,18 @@ static int connect_setup(struct transport *transport, int for_push) return 0; } -static struct ref *get_refs_via_connect(struct transport *transport, int for_push, - const struct argv_array *ref_prefixes) +/* + * Obtains the protocol version from the transport and writes it to + * transport->data->version, first connecting if not already connected. + * + * If the protocol version is one that allows skipping the listing of remote + * refs, and must_list_refs is 0, the listing of remote refs is skipped and + * this function returns NULL. Otherwise, this function returns the list of + * remote refs. + */ +static struct ref *handshake(struct transport *transport, int for_push, + const struct argv_array *ref_prefixes, + int must_list_refs) { struct git_transport_data *data = transport->data; struct ref *refs = NULL; @@ -268,8 +278,10 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus data->version = discover_version(&reader); switch (data->version) { case protocol_v2: - get_remote_refs(data->fd[1], &reader, &refs, for_push, - ref_prefixes, transport->server_options); + if (must_list_refs) + get_remote_refs(data->fd[1], &reader, &refs, for_push, + ref_prefixes, + transport->server_options); break; case protocol_v1: case protocol_v0: @@ -283,9 +295,18 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus } data->got_remote_heads = 1; + if (reader.line_peeked) + BUG("buffer must be empty at the end of handshake()"); + return refs; } +static struct ref *get_refs_via_connect(struct transport *transport, int for_push, + const struct argv_array *ref_prefixes) +{ + return handshake(transport, for_push, ref_prefixes, 1); +} + static int fetch_refs_via_pack(struct transport *transport, int nr_heads, struct ref **to_fetch) { From 01775651481ecd9c7288a85cfb7999f7f38ab37c Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Thu, 27 Sep 2018 12:24:05 -0700 Subject: [PATCH 2/4] transport: do not list refs if possible When all refs to be fetched are exact OIDs, it is possible to perform a fetch without requiring the remote to list refs if protocol v2 is used. Teach Git to do this. This currently has an effect only for lazy fetches done from partial clones. The change necessary to likewise optimize "git fetch " will be done in a subsequent patch. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- fetch-pack.c | 2 +- t/t5702-protocol-v2.sh | 4 ++++ transport.c | 13 +++++++++++-- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index 75047a4b2a..15652b4776 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1598,7 +1598,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args, if (nr_sought) nr_sought = remove_duplicates_in_refs(sought, nr_sought); - if (!ref) { + if (version != protocol_v2 && !ref) { packet_flush(fd[1]); die(_("no matching remote head")); } diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 3beeed4546..e32b5b4e3e 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -286,6 +286,10 @@ test_expect_success 'dynamically fetch missing object' ' grep "version 2" trace ' +test_expect_success 'when dynamically fetching missing object, do not list refs' ' + ! grep "git> command=ls-refs" trace +' + test_expect_success 'partial fetch' ' rm -rf client "$(pwd)/trace" && git init client && diff --git a/transport.c b/transport.c index 5fb9ff6b56..4329cca8e5 100644 --- a/transport.c +++ b/transport.c @@ -341,8 +341,17 @@ static int fetch_refs_via_pack(struct transport *transport, args.server_options = transport->server_options; args.negotiation_tips = data->options.negotiation_tips; - if (!data->got_remote_heads) - refs_tmp = get_refs_via_connect(transport, 0, NULL); + if (!data->got_remote_heads) { + int i; + int must_list_refs = 0; + for (i = 0; i < nr_heads; i++) { + if (!to_fetch[i]->exact_oid) { + must_list_refs = 1; + break; + } + } + refs_tmp = handshake(transport, 0, NULL, must_list_refs); + } switch (data->version) { case protocol_v2: From 6ab4055775b0e560d1f2a1be2e7251d872874655 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Thu, 27 Sep 2018 12:24:06 -0700 Subject: [PATCH 3/4] transport: list refs before fetch if necessary The built-in bundle transport and the transport helper interface do not work when transport_fetch_refs() is called immediately after transport creation. This will be needed in a subsequent patch, so fix this. Evidence: fetch_refs_from_bundle() relies on data->header being initialized in get_refs_from_bundle(), and fetch() in transport-helper.c relies on either data->fetch or data->import being set by get_helper(), but neither transport_helper_init() nor fetch() calls get_helper(). Up until the introduction of the partial clone feature, this has not been a problem, because transport_fetch_refs() is always called after transport_get_remote_refs(). With the introduction of the partial clone feature, which involves calling transport_fetch_refs() (to fetch objects by their OIDs) without transport_get_remote_refs(), this is still not a problem, but only coincidentally - we do not support partially cloning a bundle, and as for cloning using a transport-helper-using protocol, it so happens that before transport_fetch_refs() is called, fetch_refs() in fetch-object.c calls transport_set_option(), which means that the aforementioned get_helper() is invoked through set_helper_option() in transport-helper.c. This could be fixed by fixing the transports themselves, but it doesn't seem like a good idea to me to open up previously untested code paths; also, there may be transport helpers in the wild that assume that "list" is always called before "fetch". Instead, fix this by having transport_fetch_refs() call transport_get_remote_refs() to ensure that the latter is always called at least once, unless the transport explicitly states that it supports fetching without listing refs. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- transport-helper.c | 1 + transport-internal.h | 6 ++++++ transport.c | 12 ++++++++++++ 3 files changed, 19 insertions(+) diff --git a/transport-helper.c b/transport-helper.c index 143ca008c8..7213fa0d32 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -1105,6 +1105,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push, } static struct transport_vtable vtable = { + 0, set_helper_option, get_refs_list, fetch, diff --git a/transport-internal.h b/transport-internal.h index 1cde6258a7..004bee5e36 100644 --- a/transport-internal.h +++ b/transport-internal.h @@ -6,6 +6,12 @@ struct transport; struct argv_array; struct transport_vtable { + /** + * This transport supports the fetch() function being called + * without get_refs_list() first being called. + */ + unsigned fetch_without_list : 1; + /** * Returns 0 if successful, positive if the option is not * recognized or is inapplicable, and negative if the option diff --git a/transport.c b/transport.c index 4329cca8e5..ea72fff6a6 100644 --- a/transport.c +++ b/transport.c @@ -733,6 +733,7 @@ static int disconnect_git(struct transport *transport) } static struct transport_vtable taken_over_vtable = { + 1, NULL, get_refs_via_connect, fetch_refs_via_pack, @@ -882,6 +883,7 @@ void transport_check_allowed(const char *type) } static struct transport_vtable bundle_vtable = { + 0, NULL, get_refs_from_bundle, fetch_refs_from_bundle, @@ -891,6 +893,7 @@ static struct transport_vtable bundle_vtable = { }; static struct transport_vtable builtin_smart_vtable = { + 1, NULL, get_refs_via_connect, fetch_refs_via_pack, @@ -1254,6 +1257,15 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs) struct ref **heads = NULL; struct ref *rm; + if (!transport->vtable->fetch_without_list) + /* + * Some transports (e.g. the built-in bundle transport and the + * transport helper interface) do not work when fetching is + * done immediately after transport creation. List the remote + * refs anyway (if not already listed) as a workaround. + */ + transport_get_remote_refs(transport, NULL); + for (rm = refs; rm; rm = rm->next) { nr_refs++; if (rm->peer_ref && From e70a3030e747312327a4f3619247bf8a986aa577 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Thu, 27 Sep 2018 12:24:07 -0700 Subject: [PATCH 4/4] fetch: do not list refs if fetching only hashes If only hash literals are given on a "git fetch" command-line, tag following is not requested, and the fetch is done using protocol v2, a list of refs is not required from the remote. Therefore, optimize by invoking transport_get_remote_refs() only if we need the refs. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- builtin/fetch.c | 32 ++++++++++++++++++++++++++------ t/t5551-http-fetch-smart.sh | 15 +++++++++++++++ t/t5702-protocol-v2.sh | 13 +++++++++++++ 3 files changed, 54 insertions(+), 6 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 0696abfc2a..4c4f8fa194 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1175,6 +1175,7 @@ static int do_fetch(struct transport *transport, int retcode = 0; const struct ref *remote_refs; struct argv_array ref_prefixes = ARGV_ARRAY_INIT; + int must_list_refs = 1; if (tags == TAGS_DEFAULT) { if (transport->remote->fetch_tags == 2) @@ -1190,17 +1191,36 @@ static int do_fetch(struct transport *transport, goto cleanup; } - if (rs->nr) + if (rs->nr) { + int i; + refspec_ref_prefixes(rs, &ref_prefixes); - else if (transport->remote && transport->remote->fetch.nr) + + /* + * We can avoid listing refs if all of them are exact + * OIDs + */ + must_list_refs = 0; + for (i = 0; i < rs->nr; i++) { + if (!rs->items[i].exact_sha1) { + must_list_refs = 1; + break; + } + } + } else if (transport->remote && transport->remote->fetch.nr) refspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes); - if (ref_prefixes.argc && - (tags == TAGS_SET || (tags == TAGS_DEFAULT))) { - argv_array_push(&ref_prefixes, "refs/tags/"); + if (tags == TAGS_SET || tags == TAGS_DEFAULT) { + must_list_refs = 1; + if (ref_prefixes.argc) + argv_array_push(&ref_prefixes, "refs/tags/"); } - remote_refs = transport_get_remote_refs(transport, &ref_prefixes); + if (must_list_refs) + remote_refs = transport_get_remote_refs(transport, &ref_prefixes); + else + remote_refs = NULL; + argv_array_clear(&ref_prefixes); ref_map = get_ref_map(transport->remote, remote_refs, rs, diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 771f36f9ff..12b339d239 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -381,6 +381,21 @@ test_expect_success 'using fetch command in remote-curl updates refs' ' test_cmp expect actual ' +test_expect_success 'fetch by SHA-1 without tag following' ' + SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" && + rm -rf "$SERVER" client && + + git init "$SERVER" && + test_commit -C "$SERVER" foo && + + git clone $HTTPD_URL/smart/server client && + + test_commit -C "$SERVER" bar && + git -C "$SERVER" rev-parse bar >bar_hash && + git -C client -c protocol.version=0 fetch \ + --no-tags origin $(cat bar_hash) +' + test_expect_success 'GIT_REDACT_COOKIES redacts cookies' ' rm -rf clone && echo "Set-Cookie: Foo=1" >cookies && diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index e32b5b4e3e..b82b5f8f7e 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -79,6 +79,19 @@ test_expect_success 'fetch with git:// using protocol v2' ' grep "fetch< version 2" log ' +test_expect_success 'fetch by hash without tag following with protocol v2 does not list refs' ' + test_when_finished "rm -f log" && + + test_commit -C "$daemon_parent" two_a && + git -C "$daemon_parent" rev-parse two_a >two_a_hash && + + GIT_TRACE_PACKET="$(pwd)/log" git -C daemon_child -c protocol.version=2 \ + fetch --no-tags origin $(cat two_a_hash) && + + grep "fetch< version 2" log && + ! grep "fetch> command=ls-refs" log +' + test_expect_success 'pull with git:// using protocol v2' ' test_when_finished "rm -f log" &&