[PATCH 0/2] ls-remote and v2 ref prefixes

[PATCH 0/2] ls-remote and v2 ref prefixes

[Jeff King]

This series fixes a bug where ls-remote sends a ref-advertisement prefix
when it shouldn't, and then optimizes a spot where it doesn't send one
but could.

  [1/2]: ls-remote: do not send ref prefixes for patterns
  [2/2]: ls-remote: pass heads/tags prefixes to transport

 builtin/ls-remote.c  | 13 +++++--------
 t/t5512-ls-remote.sh | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+), 8 deletions(-)

-Peff

[PATCH 1/2] ls-remote: do not send ref prefixes for patterns

[Jeff King]

Since b4be74105f (ls-remote: pass ref prefixes when requesting a
remote's refs, 2018-03-15), "ls-remote foo" will pass "refs/heads/foo",
"refs/tags/foo", etc to the transport code in an attempt to let the
other side reduce the size of its advertisement.

Unfortunately this is not correct, as ls-remote patterns do not follow
the usual ref lookup rules, and are in fact tail-matched. So we could
find "refs/heads/foo" or "refs/heads/a/much/deeper/foo" or even
"refs/another/hierarchy/foo".

Since we can't pass a prefix and there's not yet a v2 extension for
matching wildcards, we must disable this feature to keep the same
behavior as v1.

Reported-by: Jon Simons <jon@jonsimons.org>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/ls-remote.c  | 8 --------
 t/t5512-ls-remote.sh | 9 +++++++++
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 6a0cdec30d..5faa8459d9 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -88,15 +88,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		int i;
 		pattern = xcalloc(argc, sizeof(const char *));
 		for (i = 1; i < argc; i++) {
-			const char *glob;
 			pattern[i - 1] = xstrfmt("*/%s", argv[i]);
-
-			glob = strchr(argv[i], '*');
-			if (glob)
-				argv_array_pushf(&ref_prefixes, "%.*s",
-						 (int)(glob - argv[i]), argv[i]);
-			else
-				expand_ref_prefix(&ref_prefixes, argv[i]);
 		}
 	}
 
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index bc5703ff9b..ca1b7e5bc1 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -302,4 +302,13 @@ test_expect_success 'ls-remote works outside repository' '
 	nongit git ls-remote dst.git
 '
 
+test_expect_success 'ls-remote patterns work with all protocol versions' '
+	git for-each-ref --format="%(objectname)	%(refname)" \
+		refs/heads/master refs/remotes/origin/master >expect &&
+	git -c protocol.version=1 ls-remote . master >actual.v1 &&
+	test_cmp expect actual.v1 &&
+	git -c protocol.version=2 ls-remote . master >actual.v2 &&
+	test_cmp expect actual.v2
+'
+
 test_done
-- 
2.19.1.1298.g19f18f2a22

[PATCH 2/2] ls-remote: pass heads/tags prefixes to transport

[Jeff King]

Unlike its arbitrary text patterns, the --heads and --tags
options to ls-remote are true prefixes. We can pass this
information to the transport code. If the v2 protocol is in
use, that will reduce the size of the ref advertisement.

Note that the test added here succeeds both before and after
the patch. This is an optimization, not a bug-fix; it's just
making sure we didn't break anything.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/ls-remote.c  | 5 +++++
 t/t5512-ls-remote.sh | 9 +++++++++
 2 files changed, 14 insertions(+)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 5faa8459d9..1d7f1f5ce2 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -92,6 +92,11 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		}
 	}
 
+	if (flags & REF_TAGS)
+		argv_array_push(&ref_prefixes, "refs/tags/");
+	if (flags & REF_HEADS)
+		argv_array_push(&ref_prefixes, "refs/heads/");
+
 	remote = remote_get(dest);
 	if (!remote) {
 		if (dest)
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index ca1b7e5bc1..91ee6841c1 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -311,4 +311,13 @@ test_expect_success 'ls-remote patterns work with all protocol versions' '
 	test_cmp expect actual.v2
 '
 
+test_expect_success 'ls-remote prefixes work with all protocol versions' '
+	git for-each-ref --format="%(objectname)	%(refname)" \
+		refs/heads/ refs/tags/ >expect &&
+	git -c protocol.version=1 ls-remote --heads --tags . >actual.v1 &&
+	test_cmp expect actual.v1 &&
+	git -c protocol.version=2 ls-remote --heads --tags . >actual.v2 &&
+	test_cmp expect actual.v2
+'
+
 test_done
-- 
2.19.1.1298.g19f18f2a22

Re: [PATCH 1/2] ls-remote: do not send ref prefixes for patterns

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Since b4be74105f (ls-remote: pass ref prefixes when requesting a
> remote's refs, 2018-03-15), "ls-remote foo" will pass "refs/heads/foo",
> "refs/tags/foo", etc to the transport code in an attempt to let the
> other side reduce the size of its advertisement.

Jonathan, seeing 2b554353 ("fetch: send "refs/tags/" prefix upon CLI
refspecs", 2018-06-05), I am guessing that you are doing the proto v2
work inherited from Brandon?  Having to undo this is unfortunate, but
I agree with this patch that we need to do this until ref prefix learns
to grok wildcards.

> Unfortunately this is not correct, as ls-remote patterns do not follow
> the usual ref lookup rules, and are in fact tail-matched. So we could
> find "refs/heads/foo" or "refs/heads/a/much/deeper/foo" or even
> "refs/another/hierarchy/foo".
>
> Since we can't pass a prefix and there's not yet a v2 extension for
> matching wildcards, we must disable this feature to keep the same
> behavior as v1.
>
> Reported-by: Jon Simons <jon@jonsimons.org>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/ls-remote.c  | 8 --------
>  t/t5512-ls-remote.sh | 9 +++++++++
>  2 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
> index 6a0cdec30d..5faa8459d9 100644
> --- a/builtin/ls-remote.c
> +++ b/builtin/ls-remote.c
> @@ -88,15 +88,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>  		int i;
>  		pattern = xcalloc(argc, sizeof(const char *));
>  		for (i = 1; i < argc; i++) {
> -			const char *glob;
>  			pattern[i - 1] = xstrfmt("*/%s", argv[i]);
> -
> -			glob = strchr(argv[i], '*');
> -			if (glob)
> -				argv_array_pushf(&ref_prefixes, "%.*s",
> -						 (int)(glob - argv[i]), argv[i]);
> -			else
> -				expand_ref_prefix(&ref_prefixes, argv[i]);
>  		}
>  	}
>  
> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index bc5703ff9b..ca1b7e5bc1 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -302,4 +302,13 @@ test_expect_success 'ls-remote works outside repository' '
>  	nongit git ls-remote dst.git
>  '
>  
> +test_expect_success 'ls-remote patterns work with all protocol versions' '
> +	git for-each-ref --format="%(objectname)	%(refname)" \
> +		refs/heads/master refs/remotes/origin/master >expect &&
> +	git -c protocol.version=1 ls-remote . master >actual.v1 &&
> +	test_cmp expect actual.v1 &&
> +	git -c protocol.version=2 ls-remote . master >actual.v2 &&
> +	test_cmp expect actual.v2
> +'
> +
>  test_done

Re: [PATCH 1/2] ls-remote: do not send ref prefixes for patterns

[Jonathan Tan]

> Jeff King <peff@peff.net> writes:
> 
> > Since b4be74105f (ls-remote: pass ref prefixes when requesting a
> > remote's refs, 2018-03-15), "ls-remote foo" will pass "refs/heads/foo",
> > "refs/tags/foo", etc to the transport code in an attempt to let the
> > other side reduce the size of its advertisement.
> 
> Jonathan, seeing 2b554353 ("fetch: send "refs/tags/" prefix upon CLI
> refspecs", 2018-06-05), I am guessing that you are doing the proto v2
> work inherited from Brandon?

Sorry for the late reply - I had some personal events, but I should be
able to look more at Git stuff from now on.

Well, it's true that I have been fixing some bugs related to protocol
v2.

> Having to undo this is unfortunate, but
> I agree with this patch that we need to do this until ref prefix learns
> to grok wildcards.

It is unfortunate, although as far as I can tell, the performance
improvements from "git fetch" (which I think is the more frequent
command called) remain, so it might not be so bad. I see from your
What's Cooking that these patches are to be merged to master, which I
agree with.