{"thread":{"id":"49424","subject":"[RFC PATCH] transport: list refs before fetch if necessary","startedAt":"2018-09-25T22:54:08Z","lastAt":"2018-10-07T00:53:13Z","messageCount":12,"participants":["Jonathan Tan","Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"358912","messageId":"20180925225355.74237-1-jonathantanmy@google.com","threadId":"49424","inReplyTo":null,"subject":"[RFC PATCH] transport: list refs before fetch if necessary","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-25T22:53:55Z","receivedAt":"2018-09-25T22:54:08Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The built-in bundle transport and the transport helper interface do not\nwork when transport_fetch_refs() is called immediately after transport\ncreation.\n\nEvidence: fetch_refs_from_bundle() relies on data->header being\ninitialized in get_refs_from_bundle(), and fetch() in transport-helper.c\nrelies on either data->fetch or data->import being set by get_helper(),\nbut neither transport_helper_init() nor fetch() calls get_helper().\n\nUp until the introduction of the partial clone feature, this has not\nbeen a problem, because transport_fetch_refs() is always called after\ntransport_get_remote_refs(). With the introduction of the partial clone\nfeature, which involves calling transport_fetch_refs() (to fetch objects\nby their OIDs) without transport_get_remote_refs(), this is still not a\nproblem, but only coincidentally - we do not support partially cloning a\nbundle, and as for cloning using a transport-helper-using protocol, it\nso happens that before transport_fetch_refs() is called, fetch_refs() in\nfetch-object.c calls transport_set_option(), which means that the\naforementioned get_helper() is invoked through set_helper_option() in\ntransport-helper.c. In the future, though, there may be other use cases\nin which we want to fetch without requiring listing of remote refs, so\nthis is still worth fixing.\n\nThis could be fixed by fixing the transports themselves, but it doesn't\nseem like a good idea to me to open up previously untested code paths;\nalso, there may be transport helpers in the wild that assume that \"list\"\nis always called before \"fetch\". Instead, fix this by having\ntransport_fetch_refs() call transport_get_remote_refs() to ensure that\nthe latter is always called at least once, unless the transport\nexplicitly states that it supports fetching without listing refs.\n\nSigned-off-by: Jonathan Tan \n---\nI discovered this while investigating the possibility of taking\nadvantage of the fact that protocol v2 allows us to fetch without first\ninvoking ls-refs. This is useful both when lazily fetching to a partial\nclone, and when invoking \"git fetch --no-tags \" (note\nthat tag following must be disabled).\n\nAny comments on this (for or against) is appreciated, and suggestions of\nbetter approaches are appreciated too.\n---\n transport-helper.c | 1 +\n transport-internal.h | 6 ++++++\n transport.c | 12 ++++++++++++\n 3 files changed, 19 insertions(+)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 143ca008c8..7213fa0d32 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1105,6 +1105,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push,\n }\n \n static struct transport_vtable vtable = {\n+\t0,\n \tset_helper_option,\n \tget_refs_list,\n \tfetch,\ndiff --git a/transport-internal.h b/transport-internal.h\nindex 1cde6258a7..004bee5e36 100644\n--- a/transport-internal.h\n+++ b/transport-internal.h\n@@ -6,6 +6,12 @@ struct transport;\n struct argv_array;\n \n struct transport_vtable {\n+\t/**\n+\t * This transport supports the fetch() function being called\n+\t * without get_refs_list() first being called.\n+\t */\n+\tunsigned fetch_without_list : 1;\n+\n \t/**\n \t * Returns 0 if successful, positive if the option is not\n \t * recognized or is inapplicable, and negative if the option\ndiff --git a/transport.c b/transport.c\nindex 1c76d64aba..ee8a78ff37 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -703,6 +703,7 @@ static int disconnect_git(struct transport *transport)\n }\n \n static struct transport_vtable taken_over_vtable = {\n+\t1,\n \tNULL,\n \tget_refs_via_connect,\n \tfetch_refs_via_pack,\n@@ -852,6 +853,7 @@ void transport_check_allowed(const char *type)\n }\n \n static struct transport_vtable bundle_vtable = {\n+\t0,\n \tNULL,\n \tget_refs_from_bundle,\n \tfetch_refs_from_bundle,\n@@ -861,6 +863,7 @@ static struct transport_vtable bundle_vtable = {\n };\n \n static struct transport_vtable builtin_smart_vtable = {\n+\t1,\n \tNULL,\n \tget_refs_via_connect,\n \tfetch_refs_via_pack,\n@@ -1224,6 +1227,15 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)\n \tstruct ref **heads = NULL;\n \tstruct ref *rm;\n \n+\tif (!transport->vtable->fetch_without_list)\n+\t\t/*\n+\t\t * Some transports (e.g. the built-in bundle transport and the\n+\t\t * transport helper interface) do not work when fetching is\n+\t\t * done immediately after transport creation. List the remote\n+\t\t * refs anyway (if not already listed) as a workaround.\n+\t\t */\n+\t\ttransport_get_remote_refs(transport, NULL);\n+\n \tfor (rm = refs; rm; rm = rm->next) {\n \t\tnr_refs++;\n \t\tif (rm->peer_ref &&\n-- \n2.19.0.605.g01d371f741-goog\n\n"},{"id":"358913","messageId":"xmqq8t3pnphe.fsf@gitster-ct.c.googlers.com","threadId":"49424","inReplyTo":"20180925225355.74237-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH] transport: list refs before fetch if necessary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-25T23:09:49Z","receivedAt":"2018-09-25T23:09:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 143ca008c8..7213fa0d32 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -1105,6 +1105,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push,\n> }\n> \n> static struct transport_vtable vtable = {\n> +\t0,\n> \tset_helper_option,\n> \tget_refs_list,\n> \tfetch,\n> diff --git a/transport-internal.h b/transport-internal.h\n> index 1cde6258a7..004bee5e36 100644\n> --- a/transport-internal.h\n> +++ b/transport-internal.h\n> @@ -6,6 +6,12 @@ struct transport;\n> struct argv_array;\n> \n> struct transport_vtable {\n> +\t/**\n> +\t * This transport supports the fetch() function being called\n> +\t * without get_refs_list() first being called.\n> +\t */\n> +\tunsigned fetch_without_list : 1;\n> +\n> \t/**\n> \t * Returns 0 if successful, positive if the option is not\n> \t * recognized or is inapplicable, and negative if the option\n> diff --git a/transport.c b/transport.c\n> index 1c76d64aba..ee8a78ff37 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -703,6 +703,7 @@ static int disconnect_git(struct transport *transport)\n> }\n> \n> static struct transport_vtable taken_over_vtable = {\n> +\t1,\n> \tNULL,\n> \tget_refs_via_connect,\n> \tfetch_refs_via_pack,\n> @@ -852,6 +853,7 @@ void transport_check_allowed(const char *type)\n> }\n> \n> static struct transport_vtable bundle_vtable = {\n> +\t0,\n> \tNULL,\n> \tget_refs_from_bundle,\n> \tfetch_refs_from_bundle,\n> @@ -861,6 +863,7 @@ static struct transport_vtable bundle_vtable = {\n> };\n> \n> static struct transport_vtable builtin_smart_vtable = {\n> +\t1,\n> \tNULL,\n> \tget_refs_via_connect,\n> \tfetch_refs_via_pack,\n\nUp to this point I think I understand the change. We gain one new\ntrait for each transport, many of the transport cannot run fetch\nwithout first seeing the advertisement, some are OK, so we have 0 or\n1 in these vtables as appropriately.\n\n> @@ -1224,6 +1227,15 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)\n> \tstruct ref **heads = NULL;\n> \tstruct ref *rm;\n> \n> +\tif (!transport->vtable->fetch_without_list)\n> +\t\t/*\n> +\t\t * Some transports (e.g. the built-in bundle transport and the\n> +\t\t * transport helper interface) do not work when fetching is\n> +\t\t * done immediately after transport creation. List the remote\n> +\t\t * refs anyway (if not already listed) as a workaround.\n> +\t\t */\n> +\t\ttransport_get_remote_refs(transport, NULL);\n> +\n\nBut this I do not quite understand. It looks saying \"when asked to\nfetch, if the transport does not allow us to do so without first\ngetting the advertisement, lazily do that\", and that may be a good\nthing to do, but then aren't the current set of callers already\ncalling transport-get-remote-refs elsewhere before they call\ntransport-fetch-refs? IOW, I would have expected to see a matching\nremoval, or at least a code that turns an unconditional call to\nget-remote-refs to a conditional one that is done only for the\ntransport that lacks the capability, or something along that line.\n\n... ah, do you mean that this is not a new feature, but is a bugfix\nfor some callers that are not calling get-remote-refs before calling\nfetch-refs, and the bit is to work around the fact that some\ntransport not just can function without get-remote-refs first but do\nnot want to call it?\n\nIOW, I am a bit confused by this comment (copied from an earlier part)\n\n> +\t/**\n> +\t * This transport supports the fetch() function being called\n> +\t * without get_refs_list() first being called.\n> +\t */\n\nShouldn't it read more like \"this transport does not want its\nget-refs-list called when fetch-refs is done\"?\n\nI dunno.\n\n\n> \tfor (rm = refs; rm; rm = rm->next) {\n> \t\tnr_refs++;\n> \t\tif (rm->peer_ref &&\n"},{"id":"359100","messageId":"cover.1538075680.git.jonathantanmy@google.com","threadId":"49424","inReplyTo":"20180925225355.74237-1-jonathantanmy@google.com","subject":"[RFC PATCH v2 0/4] Avoid ls-refs when possible in protocol v2","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-27T19:24:03Z","receivedAt":"2018-09-27T19:24:19Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"To answer Junio's questions in [1], I think it's best to include the\nfull patch set that I'm developing, so here it is. The original patch is\nnow patch 3 of this set.\n\n[1] https://public-inbox.org/git/xmqq8t3pnphe.fsf@gitster-ct.c.googlers.com/\n\nRearranging Junio's questions:\n\n> ... ah, do you mean that this is not a new feature, but is a bugfix\n> for some callers that are not calling get-remote-refs before calling\n> fetch-refs, and the bit is to work around the fact that some\n> transport not just can function without get-remote-refs first but do\n> not want to call it?\n\nYes, it is the bugfix you describe, except that the bug coincidentally\ndoes not cause any bad behavior. fetch-object.c indeed does not call\nget-remote-refs before fetch-refs, but it calls transport_set_option(),\nwhich so happens to do what we need (call set_helper_option()).\n\nHowever, we need it now, because ...\n\n> But this I do not quite understand. It looks saying \"when asked to\n> fetch, if the transport does not allow us to do so without first\n> getting the advertisement, lazily do that\", and that may be a good\n> thing to do, but then aren't the current set of callers already\n> calling transport-get-remote-refs elsewhere before they call\n> transport-fetch-refs? IOW, I would have expected to see a matching\n> removal, or at least a code that turns an unconditional call to\n> get-remote-refs to a conditional one that is done only for the\n> transport that lacks the capability, or something along that line.\n\n... this \"matching removal\" you are talking about is in the subsequent\npatch 4. And there is no transport_set_option() to save us this time, so\nwe really do need this bugfix.\n\n> IOW, I am a bit confused by this comment (copied from an earlier part)\n> \n> > +\t/**\n> > +\t * This transport supports the fetch() function being called\n> > +\t * without get_refs_list() first being called.\n> > +\t */\n> \n> Shouldn't it read more like \"this transport does not want its\n> get-refs-list called when fetch-refs is done\"?\n> \n> I dunno.\n\nI'm not sure I understand - transports generally don't care if\nget-refs-list is called after fetch-refs. Also, this already happens\nwhen fetching with tag following from a server that does not support tag\nfollowing, using a transport that supports reuse.\n\nJonathan Tan (4):\n transport: allow skipping of ref listing\n transport: do not list refs if possible\n transport: list refs before fetch if necessary\n fetch: do not list refs if fetching only hashes\n\n builtin/fetch.c | 32 +++++++++++++++++-----\n fetch-pack.c | 2 +-\n t/t5551-http-fetch-smart.sh | 15 +++++++++++\n t/t5702-protocol-v2.sh | 18 +++++++++++++\n transport-helper.c | 1 +\n transport-internal.h | 6 +++++\n transport.c | 54 ++++++++++++++++++++++++++++++++-----\n 7 files changed, 115 insertions(+), 13 deletions(-)\n\n-- \n2.19.0.605.g01d371f741-goog\n\n"},{"id":"359101","messageId":"1baf4254c928865ca83f35a1b6ed2d2a9ffcc941.1538075680.git.jonathantanmy@google.com","threadId":"49424","inReplyTo":"cover.1538075680.git.jonathantanmy@google.com","subject":"[RFC PATCH v2 1/4] transport: allow skipping of ref listing","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-27T19:24:04Z","receivedAt":"2018-09-27T19:24:27Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The get_refs_via_connect() function both performs the handshake\n(including determining the protocol version) and obtaining the list of\nremote refs. However, the fetch protocol v2 supports fetching objects\nwithout the listing of refs, so make it possible for the user to skip\nthe listing by creating a new handshake() function. This will be used in\na subsequent commit.\n---\n transport.c | 29 +++++++++++++++++++++++++----\n 1 file changed, 25 insertions(+), 4 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 1c76d64aba..5fb9ff6b56 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -252,8 +252,18 @@ static int connect_setup(struct transport *transport, int for_push)\n \treturn 0;\n }\n \n-static struct ref *get_refs_via_connect(struct transport *transport, int for_push,\n-\t\t\t\t\tconst struct argv_array *ref_prefixes)\n+/*\n+ * Obtains the protocol version from the transport and writes it to\n+ * transport->data->version, first connecting if not already connected.\n+ *\n+ * If the protocol version is one that allows skipping the listing of remote\n+ * refs, and must_list_refs is 0, the listing of remote refs is skipped and\n+ * this function returns NULL. Otherwise, this function returns the list of\n+ * remote refs.\n+ */\n+static struct ref *handshake(struct transport *transport, int for_push,\n+\t\t\t const struct argv_array *ref_prefixes,\n+\t\t\t int must_list_refs)\n {\n \tstruct git_transport_data *data = transport->data;\n \tstruct ref *refs = NULL;\n@@ -268,8 +278,10 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus\n \tdata->version = discover_version(&reader);\n \tswitch (data->version) {\n \tcase protocol_v2:\n-\t\tget_remote_refs(data->fd[1], &reader, &refs, for_push,\n-\t\t\t\tref_prefixes, transport->server_options);\n+\t\tif (must_list_refs)\n+\t\t\tget_remote_refs(data->fd[1], &reader, &refs, for_push,\n+\t\t\t\t\tref_prefixes,\n+\t\t\t\t\ttransport->server_options);\n \t\tbreak;\n \tcase protocol_v1:\n \tcase protocol_v0:\n@@ -283,9 +295,18 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus\n \t}\n \tdata->got_remote_heads = 1;\n \n+\tif (reader.line_peeked)\n+\t\tBUG(\"buffer must be empty at the end of handshake()\");\n+\n \treturn refs;\n }\n \n+static struct ref *get_refs_via_connect(struct transport *transport, int for_push,\n+\t\t\t\t\tconst struct argv_array *ref_prefixes)\n+{\n+\treturn handshake(transport, for_push, ref_prefixes, 1);\n+}\n+\n static int fetch_refs_via_pack(struct transport *transport,\n \t\t\t int nr_heads, struct ref **to_fetch)\n {\n-- \n2.19.0.605.g01d371f741-goog\n\n"},{"id":"359102","messageId":"61ad64245470e51cb97988e0f2f5ea76c9e2276c.1538075680.git.jonathantanmy@google.com","threadId":"49424","inReplyTo":"cover.1538075680.git.jonathantanmy@google.com","subject":"[RFC PATCH v2 2/4] transport: do not list refs if possible","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-27T19:24:05Z","receivedAt":"2018-09-27T19:24:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When all refs to be fetched are exact OIDs, it is possible to perform a\nfetch without requiring the remote to list refs if protocol v2 is used.\nTeach Git to do this.\n\nThis currently has an effect only for lazy fetches done from partial\nclones. The change necessary to likewise optimize \"git fetch \n\" will be done in a subsequent patch.\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 2 +-\n t/t5702-protocol-v2.sh | 5 +++++\n transport.c | 13 +++++++++++--\n 3 files changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 75047a4b2a..15652b4776 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1598,7 +1598,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \tif (nr_sought)\n \t\tnr_sought = remove_duplicates_in_refs(sought, nr_sought);\n \n-\tif (!ref) {\n+\tif (version != protocol_v2 && !ref) {\n \t\tpacket_flush(fd[1]);\n \t\tdie(_(\"no matching remote head\"));\n \t}\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 3beeed4546..a316bb9bf4 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -286,6 +286,11 @@ test_expect_success 'dynamically fetch missing object' '\n \tgrep \"version 2\" trace\n '\n \n+test_expect_success 'when dynamically fetching missing object, do not list refs' '\n+\tcat trace &&\n+\t! grep \"git> command=ls-refs\" trace\n+'\n+\n test_expect_success 'partial fetch' '\n \trm -rf client \"$(pwd)/trace\" &&\n \tgit init client &&\ndiff --git a/transport.c b/transport.c\nindex 5fb9ff6b56..4329cca8e5 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -341,8 +341,17 @@ static int fetch_refs_via_pack(struct transport *transport,\n \targs.server_options = transport->server_options;\n \targs.negotiation_tips = data->options.negotiation_tips;\n \n-\tif (!data->got_remote_heads)\n-\t\trefs_tmp = get_refs_via_connect(transport, 0, NULL);\n+\tif (!data->got_remote_heads) {\n+\t\tint i;\n+\t\tint must_list_refs = 0;\n+\t\tfor (i = 0; i < nr_heads; i++) {\n+\t\t\tif (!to_fetch[i]->exact_oid) {\n+\t\t\t\tmust_list_refs = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\trefs_tmp = handshake(transport, 0, NULL, must_list_refs);\n+\t}\n \n \tswitch (data->version) {\n \tcase protocol_v2:\n-- \n2.19.0.605.g01d371f741-goog\n\n"},{"id":"359103","messageId":"33b22cf96ee27053a6de22fdfb48f58cc8012644.1538075680.git.jonathantanmy@google.com","threadId":"49424","inReplyTo":"cover.1538075680.git.jonathantanmy@google.com","subject":"[RFC PATCH v2 3/4] transport: list refs before fetch if necessary","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-27T19:24:06Z","receivedAt":"2018-09-27T19:24:37Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The built-in bundle transport and the transport helper interface do not\nwork when transport_fetch_refs() is called immediately after transport\ncreation. This will be needed in a subsequent patch, so fix this.\n\nEvidence: fetch_refs_from_bundle() relies on data->header being\ninitialized in get_refs_from_bundle(), and fetch() in transport-helper.c\nrelies on either data->fetch or data->import being set by get_helper(),\nbut neither transport_helper_init() nor fetch() calls get_helper().\n\nUp until the introduction of the partial clone feature, this has not\nbeen a problem, because transport_fetch_refs() is always called after\ntransport_get_remote_refs(). With the introduction of the partial clone\nfeature, which involves calling transport_fetch_refs() (to fetch objects\nby their OIDs) without transport_get_remote_refs(), this is still not a\nproblem, but only coincidentally - we do not support partially cloning a\nbundle, and as for cloning using a transport-helper-using protocol, it\nso happens that before transport_fetch_refs() is called, fetch_refs() in\nfetch-object.c calls transport_set_option(), which means that the\naforementioned get_helper() is invoked through set_helper_option() in\ntransport-helper.c.\n\nThis could be fixed by fixing the transports themselves, but it doesn't\nseem like a good idea to me to open up previously untested code paths;\nalso, there may be transport helpers in the wild that assume that \"list\"\nis always called before \"fetch\". Instead, fix this by having\ntransport_fetch_refs() call transport_get_remote_refs() to ensure that\nthe latter is always called at least once, unless the transport\nexplicitly states that it supports fetching without listing refs.\n\nSigned-off-by: Jonathan Tan \n---\n transport-helper.c | 1 +\n transport-internal.h | 6 ++++++\n transport.c | 12 ++++++++++++\n 3 files changed, 19 insertions(+)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 143ca008c8..7213fa0d32 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1105,6 +1105,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push,\n }\n \n static struct transport_vtable vtable = {\n+\t0,\n \tset_helper_option,\n \tget_refs_list,\n \tfetch,\ndiff --git a/transport-internal.h b/transport-internal.h\nindex 1cde6258a7..004bee5e36 100644\n--- a/transport-internal.h\n+++ b/transport-internal.h\n@@ -6,6 +6,12 @@ struct transport;\n struct argv_array;\n \n struct transport_vtable {\n+\t/**\n+\t * This transport supports the fetch() function being called\n+\t * without get_refs_list() first being called.\n+\t */\n+\tunsigned fetch_without_list : 1;\n+\n \t/**\n \t * Returns 0 if successful, positive if the option is not\n \t * recognized or is inapplicable, and negative if the option\ndiff --git a/transport.c b/transport.c\nindex 4329cca8e5..ea72fff6a6 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -733,6 +733,7 @@ static int disconnect_git(struct transport *transport)\n }\n \n static struct transport_vtable taken_over_vtable = {\n+\t1,\n \tNULL,\n \tget_refs_via_connect,\n \tfetch_refs_via_pack,\n@@ -882,6 +883,7 @@ void transport_check_allowed(const char *type)\n }\n \n static struct transport_vtable bundle_vtable = {\n+\t0,\n \tNULL,\n \tget_refs_from_bundle,\n \tfetch_refs_from_bundle,\n@@ -891,6 +893,7 @@ static struct transport_vtable bundle_vtable = {\n };\n \n static struct transport_vtable builtin_smart_vtable = {\n+\t1,\n \tNULL,\n \tget_refs_via_connect,\n \tfetch_refs_via_pack,\n@@ -1254,6 +1257,15 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)\n \tstruct ref **heads = NULL;\n \tstruct ref *rm;\n \n+\tif (!transport->vtable->fetch_without_list)\n+\t\t/*\n+\t\t * Some transports (e.g. the built-in bundle transport and the\n+\t\t * transport helper interface) do not work when fetching is\n+\t\t * done immediately after transport creation. List the remote\n+\t\t * refs anyway (if not already listed) as a workaround.\n+\t\t */\n+\t\ttransport_get_remote_refs(transport, NULL);\n+\n \tfor (rm = refs; rm; rm = rm->next) {\n \t\tnr_refs++;\n \t\tif (rm->peer_ref &&\n-- \n2.19.0.605.g01d371f741-goog\n\n"},{"id":"359104","messageId":"1ae00ea1fdd1118b92ac90d67f27a988750b60f2.1538075680.git.jonathantanmy@google.com","threadId":"49424","inReplyTo":"cover.1538075680.git.jonathantanmy@google.com","subject":"[RFC PATCH v2 4/4] fetch: do not list refs if fetching only hashes","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-27T19:24:07Z","receivedAt":"2018-09-27T19:24:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"If only hash literals are given on a \"git fetch\" command-line, tag\nfollowing is not requested, and the fetch is done using protocol v2, a\nlist of refs is not required from the remote. Therefore, optimize by\ninvoking transport_get_remote_refs() only if we need the refs.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/fetch.c | 32 ++++++++++++++++++++++++++------\n t/t5551-http-fetch-smart.sh | 15 +++++++++++++++\n t/t5702-protocol-v2.sh | 13 +++++++++++++\n 3 files changed, 54 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 0696abfc2a..4c4f8fa194 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1175,6 +1175,7 @@ static int do_fetch(struct transport *transport,\n \tint retcode = 0;\n \tconst struct ref *remote_refs;\n \tstruct argv_array ref_prefixes = ARGV_ARRAY_INIT;\n+\tint must_list_refs = 1;\n \n \tif (tags == TAGS_DEFAULT) {\n \t\tif (transport->remote->fetch_tags == 2)\n@@ -1190,17 +1191,36 @@ static int do_fetch(struct transport *transport,\n \t\t\tgoto cleanup;\n \t}\n \n-\tif (rs->nr)\n+\tif (rs->nr) {\n+\t\tint i;\n+\n \t\trefspec_ref_prefixes(rs, &ref_prefixes);\n-\telse if (transport->remote && transport->remote->fetch.nr)\n+\n+\t\t/*\n+\t\t * We can avoid listing refs if all of them are exact\n+\t\t * OIDs\n+\t\t */\n+\t\tmust_list_refs = 0;\n+\t\tfor (i = 0; i < rs->nr; i++) {\n+\t\t\tif (!rs->items[i].exact_sha1) {\n+\t\t\t\tmust_list_refs = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t} else if (transport->remote && transport->remote->fetch.nr)\n \t\trefspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);\n \n-\tif (ref_prefixes.argc &&\n-\t (tags == TAGS_SET || (tags == TAGS_DEFAULT))) {\n-\t\targv_array_push(&ref_prefixes, \"refs/tags/\");\n+\tif (tags == TAGS_SET || tags == TAGS_DEFAULT) {\n+\t\tmust_list_refs = 1;\n+\t\tif (ref_prefixes.argc)\n+\t\t\targv_array_push(&ref_prefixes, \"refs/tags/\");\n \t}\n \n-\tremote_refs = transport_get_remote_refs(transport, &ref_prefixes);\n+\tif (must_list_refs)\n+\t\tremote_refs = transport_get_remote_refs(transport, &ref_prefixes);\n+\telse\n+\t\tremote_refs = NULL;\n+\n \targv_array_clear(&ref_prefixes);\n \n \tref_map = get_ref_map(transport->remote, remote_refs, rs,\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 771f36f9ff..12b339d239 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -381,6 +381,21 @@ test_expect_success 'using fetch command in remote-curl updates refs' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'fetch by SHA-1 without tag following' '\n+\tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n+\trm -rf \"$SERVER\" client &&\n+\n+\tgit init \"$SERVER\" &&\n+\ttest_commit -C \"$SERVER\" foo &&\n+\n+\tgit clone $HTTPD_URL/smart/server client &&\n+\n+\ttest_commit -C \"$SERVER\" bar &&\n+\tgit -C \"$SERVER\" rev-parse bar >bar_hash &&\n+\tgit -C client -c protocol.version=0 fetch \\\n+\t\t--no-tags origin $(cat bar_hash)\n+'\n+\n test_expect_success 'GIT_REDACT_COOKIES redacts cookies' '\n \trm -rf clone &&\n \techo \"Set-Cookie: Foo=1\" >cookies &&\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex a316bb9bf4..1a97331648 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -79,6 +79,19 @@ test_expect_success 'fetch with git:// using protocol v2' '\n \tgrep \"fetch< version 2\" log\n '\n \n+test_expect_success 'fetch by hash without tag following with protocol v2 does not list refs' '\n+\ttest_when_finished \"rm -f log\" &&\n+\n+\ttest_commit -C \"$daemon_parent\" two_a &&\n+\tgit -C \"$daemon_parent\" rev-parse two_a >two_a_hash &&\n+\n+\tGIT_TRACE_PACKET=\"$(pwd)/log\" git -C daemon_child -c protocol.version=2 \\\n+\t\tfetch --no-tags origin $(cat two_a_hash) &&\n+\n+\tgrep \"fetch< version 2\" log &&\n+\t! grep \"fetch> command=ls-refs\" log\n+'\n+\n test_expect_success 'pull with git:// using protocol v2' '\n \ttest_when_finished \"rm -f log\" &&\n \n-- \n2.19.0.605.g01d371f741-goog\n\n"},{"id":"359118","messageId":"xmqqva6qfyry.fsf@gitster-ct.c.googlers.com","threadId":"49424","inReplyTo":"cover.1538075680.git.jonathantanmy@google.com","subject":"Re: [RFC PATCH v2 0/4] Avoid ls-refs when possible in protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-27T20:53:05Z","receivedAt":"2018-09-27T20:53:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> To answer Junio's questions in [1], I think it's best to include the\n> full patch set that I'm developing, so here it is. The original patch is\n> now patch 3 of this set.\n\nYeah, taking it out of context did make its purpose fuzzy. Without\nthe other patches in the series that set the overall direction of\nreducing ls-refs, it was unclear why some transports are marked to\nspecifically want to automatically call transport_get_remote_refs().\nWith these surrounding changes, \"we call it as needed, because an\nearlier step may not have called it\" starts to sound quite sensible.\n\n"},{"id":"359124","messageId":"CAGZ79kbN9Yu3sSrE+jxKFQyqRVM1eEvqoHCn3Y283biu-R6smw@mail.gmail.com","threadId":"49424","inReplyTo":"61ad64245470e51cb97988e0f2f5ea76c9e2276c.1538075680.git.jonathantanmy@google.com","subject":"Re: [RFC PATCH v2 2/4] transport: do not list refs if possible","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-27T21:38:42Z","receivedAt":"2018-09-27T21:38:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 27, 2018 at 12:24 PM Jonathan Tan wrote:\n\n>\n> +test_expect_success 'when dynamically fetching missing object, do not list refs' '\n> + cat trace &&\n\nleftover debug cat?\n"},{"id":"359126","messageId":"CAGZ79kZOecyJzXoD0ghZFKFWuWf-V0Sb0bYbBangTcbLGr_xVw@mail.gmail.com","threadId":"49424","inReplyTo":"1ae00ea1fdd1118b92ac90d67f27a988750b60f2.1538075680.git.jonathantanmy@google.com","subject":"Re: [RFC PATCH v2 4/4] fetch: do not list refs if fetching only hashes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-27T21:43:04Z","receivedAt":"2018-09-27T21:43:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 27, 2018 at 12:24 PM Jonathan Tan wrote:\n>\n> If only hash literals are given on a \"git fetch\" command-line, tag\n> following is not requested, and the fetch is done using protocol v2, a\n> list of refs is not required from the remote. Therefore, optimize by\n> invoking transport_get_remote_refs() only if we need the refs.\n>\n\nMakes sense\n\n> +\n> + /*\n> + * We can avoid listing refs if all of them are exact\n> + * OIDs\n> + */\n> + must_list_refs = 0;\n> + for (i = 0; i < rs->nr; i++) {\n> + if (!rs->items[i].exact_sha1) {\n> + must_list_refs = 1;\n> + break;\n> + }\n> + }\n\nThis seems to be a repeat pattern, Is it worth it to encapsulate it\nas a function in transport or refs?\n\n int must_list_refs(struct ref **to_fetch)\n {\n // body as the loop above\n }\n"},{"id":"359226","messageId":"20180928175027.181442-1-jonathantanmy@google.com","threadId":"49424","inReplyTo":"CAGZ79kZOecyJzXoD0ghZFKFWuWf-V0Sb0bYbBangTcbLGr_xVw@mail.gmail.com","subject":"Re: [RFC PATCH v2 4/4] fetch: do not list refs if fetching only hashes","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-28T17:50:27Z","receivedAt":"2018-09-28T17:50:33Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > + /*\n> > + * We can avoid listing refs if all of them are exact\n> > + * OIDs\n> > + */\n> > + must_list_refs = 0;\n> > + for (i = 0; i < rs->nr; i++) {\n> > + if (!rs->items[i].exact_sha1) {\n> > + must_list_refs = 1;\n> > + break;\n> > + }\n> > + }\n> \n> This seems to be a repeat pattern, Is it worth it to encapsulate it\n> as a function in transport or refs?\n> \n> int must_list_refs(struct ref **to_fetch)\n> {\n> // body as the loop above\n> }\n\nThe repetition is unfortunate - I tried to think of a better way to do\nit but couldn't. We can't do what you suggest because this one loops\nover refspecs but the other one loops over refs.\n"},{"id":"359784","messageId":"xmqq8t3avaq3.fsf@gitster-ct.c.googlers.com","threadId":"49424","inReplyTo":"CAGZ79kbN9Yu3sSrE+jxKFQyqRVM1eEvqoHCn3Y283biu-R6smw@mail.gmail.com","subject":"Re: [RFC PATCH v2 2/4] transport: do not list refs if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-07T00:53:08Z","receivedAt":"2018-10-07T00:53:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Thu, Sep 27, 2018 at 12:24 PM Jonathan Tan wrote:\n>\n>>\n>> +test_expect_success 'when dynamically fetching missing object, do not list refs' '\n>> + cat trace &&\n>\n> leftover debug cat?\n\nIt does look that way.\n"}]}