{"thread":{"id":"48926","subject":"[PATCH] clone: send ref-prefixes when using protocol v2","startedAt":"2018-07-20T19:27:56Z","lastAt":"2018-07-20T22:08:00Z","messageCount":4,"participants":["Brandon Williams","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"353176","messageId":"20180720192749.224284-1-bmwill@google.com","threadId":"48926","inReplyTo":null,"subject":"[PATCH] clone: send ref-prefixes when using protocol v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-07-20T19:27:49Z","receivedAt":"2018-07-20T19:27:56Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Signed-off-by: Brandon Williams <bmwill@google.com>\n---\n\nNoticed we miss out on server side filtering of refs when cloning using\nprotocol v2, this will enable that.\n\n builtin/clone.c | 22 +++++++++++++++++-----\n 1 file changed, 17 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 99e73dae85..55cc10e93a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -895,7 +895,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tint err = 0, complete_refs_before_fetch = 1;\n \tint submodule_progress;\n \n-\tstruct refspec_item refspec;\n+\tstruct refspec rs = REFSPEC_INIT_FETCH;\n+\tstruct argv_array ref_prefixes = ARGV_ARRAY_INIT;\n \n \tfetch_if_missing = 0;\n \n@@ -1077,7 +1078,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (option_required_reference.nr || option_optional_reference.nr)\n \t\tsetup_reference();\n \n-\trefspec_item_init(&refspec, value.buf, REFSPEC_FETCH);\n+\trefspec_append(&rs, value.buf);\n \n \tstrbuf_reset(&value);\n \n@@ -1134,10 +1135,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (transport->smart_options && !deepen && !filter_options.choice)\n \t\ttransport->smart_options->check_self_contained_and_connected = 1;\n \n-\trefs = transport_get_remote_refs(transport, NULL);\n+\n+\targv_array_push(&ref_prefixes, \"HEAD\");\n+\trefspec_ref_prefixes(&rs, &ref_prefixes);\n+\tif (option_branch) {\n+\t\texpand_ref_prefix(&ref_prefixes, option_branch);\n+\t}\n+\tif (!option_no_tags) {\n+\t\targv_array_push(&ref_prefixes, \"refs/tags/\");\n+\t}\n+\n+\trefs = transport_get_remote_refs(transport, &ref_prefixes);\n \n \tif (refs) {\n-\t\tmapped_refs = wanted_peer_refs(refs, &refspec);\n+\t\tmapped_refs = wanted_peer_refs(refs, &rs.items[0]);\n \t\t/*\n \t\t * transport_get_remote_refs() may return refs with null sha-1\n \t\t * in mapped_refs (see struct transport->get_refs_list\n@@ -1231,6 +1242,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&value);\n \tjunk_mode = JUNK_LEAVE_ALL;\n \n-\trefspec_item_clear(&refspec);\n+\trefspec_clear(&rs);\n+\targv_array_clear(&ref_prefixes);\n \treturn err;\n }\n-- \n2.18.0.233.g985f88cf7e-goog\n\n"},{"id":"353180","messageId":"xmqqwotpadlh.fsf@gitster-ct.c.googlers.com","threadId":"48926","inReplyTo":"20180720192749.224284-1-bmwill@google.com","subject":"Re: [PATCH] clone: send ref-prefixes when using protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-20T19:53:14Z","receivedAt":"2018-07-20T19:53:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\nIs there an end-user visible effect, caused by the lack of \"prefix\"\nbeing fixed with this patch, that is worth describing here?  \"The\nserver ended up showing refs that are irrelevant to the normal clone\nrequest which is only for heads and tags, wasting time and\nbandwidth\", for example?\n\n> Signed-off-by: Brandon Williams <bmwill@google.com>\n> ---\n>\n> Noticed we miss out on server side filtering of refs when cloning using\n> protocol v2, this will enable that.\n\n\n>\n>  builtin/clone.c | 22 +++++++++++++++++-----\n>  1 file changed, 17 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 99e73dae85..55cc10e93a 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -895,7 +895,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \tint err = 0, complete_refs_before_fetch = 1;\n>  \tint submodule_progress;\n>  \n> -\tstruct refspec_item refspec;\n> +\tstruct refspec rs = REFSPEC_INIT_FETCH;\n> +\tstruct argv_array ref_prefixes = ARGV_ARRAY_INIT;\n>  \n>  \tfetch_if_missing = 0;\n>  \n> @@ -1077,7 +1078,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \tif (option_required_reference.nr || option_optional_reference.nr)\n>  \t\tsetup_reference();\n>  \n> -\trefspec_item_init(&refspec, value.buf, REFSPEC_FETCH);\n> +\trefspec_append(&rs, value.buf);\n>  \n>  \tstrbuf_reset(&value);\n>  \n> @@ -1134,10 +1135,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \tif (transport->smart_options && !deepen && !filter_options.choice)\n>  \t\ttransport->smart_options->check_self_contained_and_connected = 1;\n>  \n> -\trefs = transport_get_remote_refs(transport, NULL);\n> +\n> +\targv_array_push(&ref_prefixes, \"HEAD\");\n> +\trefspec_ref_prefixes(&rs, &ref_prefixes);\n> +\tif (option_branch) {\n> +\t\texpand_ref_prefix(&ref_prefixes, option_branch);\n> +\t}\n> +\tif (!option_no_tags) {\n> +\t\targv_array_push(&ref_prefixes, \"refs/tags/\");\n> +\t}\n> +\n> +\trefs = transport_get_remote_refs(transport, &ref_prefixes);\n>  \n>  \tif (refs) {\n> -\t\tmapped_refs = wanted_peer_refs(refs, &refspec);\n> +\t\tmapped_refs = wanted_peer_refs(refs, &rs.items[0]);\n>  \t\t/*\n>  \t\t * transport_get_remote_refs() may return refs with null sha-1\n>  \t\t * in mapped_refs (see struct transport->get_refs_list\n> @@ -1231,6 +1242,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \tstrbuf_release(&value);\n>  \tjunk_mode = JUNK_LEAVE_ALL;\n>  \n> -\trefspec_item_clear(&refspec);\n> +\trefspec_clear(&rs);\n> +\targv_array_clear(&ref_prefixes);\n>  \treturn err;\n>  }\n"},{"id":"353181","messageId":"20180720195401.GA83654@aiede.svl.corp.google.com","threadId":"48926","inReplyTo":"20180720192749.224284-1-bmwill@google.com","subject":"Re: [PATCH] clone: send ref-prefixes when using protocol v2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-20T19:54:01Z","receivedAt":"2018-07-20T19:54:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nBrandon Williams wrote:\n\n> Signed-off-by: Brandon Williams <bmwill@google.com>\n> ---\n> Noticed we miss out on server side filtering of refs when cloning using\n> protocol v2, this will enable that.\n>\n>  builtin/clone.c | 22 +++++++++++++++++-----\n>  1 file changed, 17 insertions(+), 5 deletions(-)\n\nNice!  The implementation looks good.\n\nCan you add a test to ensure this filtering doesn't regress later?\n\n[...]\n> +++ b/builtin/clone.c\n[...]\n> @@ -1134,10 +1135,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \tif (transport->smart_options && !deepen && !filter_options.choice)\n>  \t\ttransport->smart_options->check_self_contained_and_connected = 1;\n>  \n> -\trefs = transport_get_remote_refs(transport, NULL);\n> +\n> +\targv_array_push(&ref_prefixes, \"HEAD\");\n> +\trefspec_ref_prefixes(&rs, &ref_prefixes);\n> +\tif (option_branch) {\n> +\t\texpand_ref_prefix(&ref_prefixes, option_branch);\n> +\t}\n> +\tif (!option_no_tags) {\n> +\t\targv_array_push(&ref_prefixes, \"refs/tags/\");\n> +\t}\n\nnit: no need for braces around one-line \"if\" body\n\nThanks,\nJonathan\n"},{"id":"353198","messageId":"20180720220754.257158-1-bmwill@google.com","threadId":"48926","inReplyTo":"20180720192749.224284-1-bmwill@google.com","subject":"[PATCH v2] clone: send ref-prefixes when using protocol v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-07-20T22:07:54Z","receivedAt":"2018-07-20T22:08:00Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Teach clone to send a list of ref-prefixes, when using protocol v2, to\nallow the server to filter out irrelevant references from the\nref-advertisement.  This reduces wasted time and bandwidth when cloning\nrepositories with a larger number of references.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n builtin/clone.c        | 20 +++++++++++++++-----\n t/t5702-protocol-v2.sh |  7 ++++++-\n 2 files changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 99e73dae85..5c0adbd6d0 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -895,7 +895,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tint err = 0, complete_refs_before_fetch = 1;\n \tint submodule_progress;\n \n-\tstruct refspec_item refspec;\n+\tstruct refspec rs = REFSPEC_INIT_FETCH;\n+\tstruct argv_array ref_prefixes = ARGV_ARRAY_INIT;\n \n \tfetch_if_missing = 0;\n \n@@ -1077,7 +1078,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (option_required_reference.nr || option_optional_reference.nr)\n \t\tsetup_reference();\n \n-\trefspec_item_init(&refspec, value.buf, REFSPEC_FETCH);\n+\trefspec_append(&rs, value.buf);\n \n \tstrbuf_reset(&value);\n \n@@ -1134,10 +1135,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (transport->smart_options && !deepen && !filter_options.choice)\n \t\ttransport->smart_options->check_self_contained_and_connected = 1;\n \n-\trefs = transport_get_remote_refs(transport, NULL);\n+\n+\targv_array_push(&ref_prefixes, \"HEAD\");\n+\trefspec_ref_prefixes(&rs, &ref_prefixes);\n+\tif (option_branch)\n+\t\texpand_ref_prefix(&ref_prefixes, option_branch);\n+\tif (!option_no_tags)\n+\t\targv_array_push(&ref_prefixes, \"refs/tags/\");\n+\n+\trefs = transport_get_remote_refs(transport, &ref_prefixes);\n \n \tif (refs) {\n-\t\tmapped_refs = wanted_peer_refs(refs, &refspec);\n+\t\tmapped_refs = wanted_peer_refs(refs, &rs.items[0]);\n \t\t/*\n \t\t * transport_get_remote_refs() may return refs with null sha-1\n \t\t * in mapped_refs (see struct transport->get_refs_list\n@@ -1231,6 +1240,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&value);\n \tjunk_mode = JUNK_LEAVE_ALL;\n \n-\trefspec_item_clear(&refspec);\n+\trefspec_clear(&rs);\n+\targv_array_clear(&ref_prefixes);\n \treturn err;\n }\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex a4fe6508bd..9c6ea04a69 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -181,7 +181,12 @@ test_expect_success 'clone with file:// using protocol v2' '\n \ttest_cmp expect actual &&\n \n \t# Server responded using protocol v2\n-\tgrep \"clone< version 2\" log\n+\tgrep \"clone< version 2\" log &&\n+\t\n+\t# Client sent ref-prefixes to filter the ref-advertisement \n+\tgrep \"ref-prefix HEAD\" log &&\n+\tgrep \"ref-prefix refs/heads/\" log &&\n+\tgrep \"ref-prefix refs/tags/\" log\n '\n \n test_expect_success 'fetch with file:// using protocol v2' '\n-- \n2.18.0.233.g985f88cf7e-goog\n\n"}]}