threads / patch / 48926

patchclone: send ref-prefixes when using protocol v2

Subject: [PATCH] clone: send ref-prefixes when using protocol v2

## tl;dr

4 messages between Jul 20, 2018 and Jul 20, 2018. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Brandon Williams· Jul 20, 2018, 19:27 UTC · lore
Signed-off-by: Brandon Williams <bmwill@google.com>
---

Noticed we miss out on server side filtering of refs when cloning using protocol v2, this will enable that.

 builtin/clone.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)
Show changes to builtin/clone.c +17 −5
diff --git a/builtin/clone.c b/builtin/clone.c
index 99e73dae85..55cc10e93a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -895,7 +895,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	int err = 0, complete_refs_before_fetch = 1;
 	int submodule_progress;
 
-	struct refspec_item refspec;
+	struct refspec rs = REFSPEC_INIT_FETCH;
+	struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
 
 	fetch_if_missing = 0;
 
@@ -1077,7 +1078,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (option_required_reference.nr || option_optional_reference.nr)
 		setup_reference();
 
-	refspec_item_init(&refspec, value.buf, REFSPEC_FETCH);
+	refspec_append(&rs, value.buf);
 
 	strbuf_reset(&value);
 
@@ -1134,10 +1135,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (transport->smart_options && !deepen && !filter_options.choice)
 		transport->smart_options->check_self_contained_and_connected = 1;
 
-	refs = transport_get_remote_refs(transport, NULL);
+
+	argv_array_push(&ref_prefixes, "HEAD");
+	refspec_ref_prefixes(&rs, &ref_prefixes);
+	if (option_branch) {
+		expand_ref_prefix(&ref_prefixes, option_branch);
+	}
+	if (!option_no_tags) {
+		argv_array_push(&ref_prefixes, "refs/tags/");
+	}
+
+	refs = transport_get_remote_refs(transport, &ref_prefixes);
 
 	if (refs) {
-		mapped_refs = wanted_peer_refs(refs, &refspec);
+		mapped_refs = wanted_peer_refs(refs, &rs.items[0]);
 		/*
 		 * transport_get_remote_refs() may return refs with null sha-1
 		 * in mapped_refs (see struct transport->get_refs_list
@@ -1231,6 +1242,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&value);
 	junk_mode = JUNK_LEAVE_ALL;
 
-	refspec_item_clear(&refspec);
+	refspec_clear(&rs);
+	argv_array_clear(&ref_prefixes);
 	return err;
 }

-- 
2.18.0.233.g985f88cf7e-goog
Junio C Hamano· Jul 20, 2018, 19:53 UTC · re: Brandon Williams · lore

Re: [PATCH] clone: send ref-prefixes when using protocol v2

Brandon Williams <bmwill@google.com> writes:

Is there an end-user visible effect, caused by the lack of "prefix" being fixed with this patch, that is worth describing here? "The server ended up showing refs that are irrelevant to the normal clone request which is only for heads and tags, wasting time and bandwidth", for example?

Show 5 quoted lines
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>
> Noticed we miss out on server side filtering of refs when cloning using
> protocol v2, this will enable that.
Show 59 quoted lines
>
>  builtin/clone.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 99e73dae85..55cc10e93a 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -895,7 +895,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	int err = 0, complete_refs_before_fetch = 1;
>  	int submodule_progress;
>  
> -	struct refspec_item refspec;
> +	struct refspec rs = REFSPEC_INIT_FETCH;
> +	struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
>  
>  	fetch_if_missing = 0;
>  
> @@ -1077,7 +1078,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	if (option_required_reference.nr || option_optional_reference.nr)
>  		setup_reference();
>  
> -	refspec_item_init(&refspec, value.buf, REFSPEC_FETCH);
> +	refspec_append(&rs, value.buf);
>  
>  	strbuf_reset(&value);
>  
> @@ -1134,10 +1135,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	if (transport->smart_options && !deepen && !filter_options.choice)
>  		transport->smart_options->check_self_contained_and_connected = 1;
>  
> -	refs = transport_get_remote_refs(transport, NULL);
> +
> +	argv_array_push(&ref_prefixes, "HEAD");
> +	refspec_ref_prefixes(&rs, &ref_prefixes);
> +	if (option_branch) {
> +		expand_ref_prefix(&ref_prefixes, option_branch);
> +	}
> +	if (!option_no_tags) {
> +		argv_array_push(&ref_prefixes, "refs/tags/");
> +	}
> +
> +	refs = transport_get_remote_refs(transport, &ref_prefixes);
>  
>  	if (refs) {
> -		mapped_refs = wanted_peer_refs(refs, &refspec);
> +		mapped_refs = wanted_peer_refs(refs, &rs.items[0]);
>  		/*
>  		 * transport_get_remote_refs() may return refs with null sha-1
>  		 * in mapped_refs (see struct transport->get_refs_list
> @@ -1231,6 +1242,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	strbuf_release(&value);
>  	junk_mode = JUNK_LEAVE_ALL;
>  
> -	refspec_item_clear(&refspec);
> +	refspec_clear(&rs);
> +	argv_array_clear(&ref_prefixes);
>  	return err;
>  }
Jonathan Nieder· Jul 20, 2018, 19:54 UTC · re: Brandon Williams · lore

Re: [PATCH] clone: send ref-prefixes when using protocol v2

Hi,
Brandon Williams wrote:
Show 7 quoted lines
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> Noticed we miss out on server side filtering of refs when cloning using
> protocol v2, this will enable that.
>
>  builtin/clone.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
Nice!  The implementation looks good.
Can you add a test to ensure this filtering doesn't regress later?
[...]
> +++ b/builtin/clone.c
[...]
Show 14 quoted lines
> @@ -1134,10 +1135,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	if (transport->smart_options && !deepen && !filter_options.choice)
>  		transport->smart_options->check_self_contained_and_connected = 1;
>  
> -	refs = transport_get_remote_refs(transport, NULL);
> +
> +	argv_array_push(&ref_prefixes, "HEAD");
> +	refspec_ref_prefixes(&rs, &ref_prefixes);
> +	if (option_branch) {
> +		expand_ref_prefix(&ref_prefixes, option_branch);
> +	}
> +	if (!option_no_tags) {
> +		argv_array_push(&ref_prefixes, "refs/tags/");
> +	}
nit: no need for braces around one-line "if" body

Thanks, Jonathan

Brandon Williams· Jul 20, 2018, 22:07 UTC · re: Brandon Williams · lore

[PATCH v2] clone: send ref-prefixes when using protocol v2

Teach clone to send a list of ref-prefixes, when using protocol v2, to allow the server to filter out irrelevant references from the ref-advertisement. This reduces wasted time and bandwidth when cloning repositories with a larger number of references.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/clone.c        | 20 +++++++++++++++-----
 t/t5702-protocol-v2.sh |  7 ++++++-
 2 files changed, 21 insertions(+), 6 deletions(-)
Show changes to 2 files +21 −6

builtin/clone.c, t/t5702-protocol-v2.sh

diff --git a/builtin/clone.c b/builtin/clone.c
index 99e73dae85..5c0adbd6d0 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -895,7 +895,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	int err = 0, complete_refs_before_fetch = 1;
 	int submodule_progress;
 
-	struct refspec_item refspec;
+	struct refspec rs = REFSPEC_INIT_FETCH;
+	struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
 
 	fetch_if_missing = 0;
 
@@ -1077,7 +1078,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (option_required_reference.nr || option_optional_reference.nr)
 		setup_reference();
 
-	refspec_item_init(&refspec, value.buf, REFSPEC_FETCH);
+	refspec_append(&rs, value.buf);
 
 	strbuf_reset(&value);
 
@@ -1134,10 +1135,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (transport->smart_options && !deepen && !filter_options.choice)
 		transport->smart_options->check_self_contained_and_connected = 1;
 
-	refs = transport_get_remote_refs(transport, NULL);
+
+	argv_array_push(&ref_prefixes, "HEAD");
+	refspec_ref_prefixes(&rs, &ref_prefixes);
+	if (option_branch)
+		expand_ref_prefix(&ref_prefixes, option_branch);
+	if (!option_no_tags)
+		argv_array_push(&ref_prefixes, "refs/tags/");
+
+	refs = transport_get_remote_refs(transport, &ref_prefixes);
 
 	if (refs) {
-		mapped_refs = wanted_peer_refs(refs, &refspec);
+		mapped_refs = wanted_peer_refs(refs, &rs.items[0]);
 		/*
 		 * transport_get_remote_refs() may return refs with null sha-1
 		 * in mapped_refs (see struct transport->get_refs_list
@@ -1231,6 +1240,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&value);
 	junk_mode = JUNK_LEAVE_ALL;
 
-	refspec_item_clear(&refspec);
+	refspec_clear(&rs);
+	argv_array_clear(&ref_prefixes);
 	return err;
 }
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index a4fe6508bd..9c6ea04a69 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -181,7 +181,12 @@ test_expect_success 'clone with file:// using protocol v2' '
 	test_cmp expect actual &&
 
 	# Server responded using protocol v2
-	grep "clone< version 2" log
+	grep "clone< version 2" log &&
+	
+	# Client sent ref-prefixes to filter the ref-advertisement 
+	grep "ref-prefix HEAD" log &&
+	grep "ref-prefix refs/heads/" log &&
+	grep "ref-prefix refs/tags/" log
 '
 
 test_expect_success 'fetch with file:// using protocol v2' '

-- 
2.18.0.233.g985f88cf7e-goog

← back to recent threads