* [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 related [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
* [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 related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-11-08 20:52 UTC | newest] 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).