git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH 0/2] ls-remote and v2 ref prefixes
@ 2018-10-31  4:23 Jeff King
  2018-10-31  4:24 ` [PATCH 1/2] ls-remote: do not send ref prefixes for patterns Jeff King
  2018-10-31  4:24 ` [PATCH 2/2] ls-remote: pass heads/tags prefixes to transport Jeff King
  0 siblings, 2 replies; 5+ messages in thread
From: Jeff King @ 2018-10-31  4:23 UTC (permalink / raw)
  To: git; +Cc: Jon Simons

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/2] ls-remote: do not send ref prefixes for patterns
  2018-10-31  4:23 [PATCH 0/2] ls-remote and v2 ref prefixes Jeff King
@ 2018-10-31  4:24 ` Jeff King
  2018-10-31  4:37   ` Junio C Hamano
  2018-10-31  4:24 ` [PATCH 2/2] ls-remote: pass heads/tags prefixes to transport Jeff King
  1 sibling, 1 reply; 5+ messages in thread
From: Jeff King @ 2018-10-31  4:24 UTC (permalink / raw)
  To: git; +Cc: Jon Simons

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/2] ls-remote: pass heads/tags prefixes to transport
  2018-10-31  4:23 [PATCH 0/2] ls-remote and v2 ref prefixes Jeff King
  2018-10-31  4:24 ` [PATCH 1/2] ls-remote: do not send ref prefixes for patterns Jeff King
@ 2018-10-31  4:24 ` Jeff King
  1 sibling, 0 replies; 5+ messages in thread
From: Jeff King @ 2018-10-31  4:24 UTC (permalink / raw)
  To: git; +Cc: Jon Simons

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] ls-remote: do not send ref prefixes for patterns
  2018-10-31  4:24 ` [PATCH 1/2] ls-remote: do not send ref prefixes for patterns Jeff King
@ 2018-10-31  4:37   ` Junio C Hamano
  2018-11-08 20:52     ` Jonathan Tan
  0 siblings, 1 reply; 5+ messages in thread
From: Junio C Hamano @ 2018-10-31  4:37 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: Jeff King, git

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] ls-remote: do not send ref prefixes for patterns
  2018-10-31  4:37   ` Junio C Hamano
@ 2018-11-08 20:52     ` Jonathan Tan
  0 siblings, 0 replies; 5+ messages in thread
From: Jonathan Tan @ 2018-11-08 20:52 UTC (permalink / raw)
  To: gitster; +Cc: jonathantanmy, peff, git

> 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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-31  4:23 [PATCH 0/2] ls-remote and v2 ref prefixes Jeff King
2018-10-31  4:24 ` [PATCH 1/2] ls-remote: do not send ref prefixes for patterns Jeff King
2018-10-31  4:37   ` Junio C Hamano
2018-11-08 20:52     ` Jonathan Tan
2018-10-31  4:24 ` [PATCH 2/2] ls-remote: pass heads/tags prefixes to transport Jeff King

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox