{"thread":{"id":"49722","subject":"[PATCH 0/2] ls-remote and v2 ref prefixes","startedAt":"2018-10-31T04:23:21Z","lastAt":"2018-11-08T20:52:55Z","messageCount":5,"participants":["Jeff King","Junio C Hamano","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"362057","messageId":"20181031042318.GA5347@sigill.intra.peff.net","threadId":"49722","inReplyTo":null,"subject":"[PATCH 0/2] ls-remote and v2 ref prefixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-31T04:23:18Z","receivedAt":"2018-10-31T04:23:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series fixes a bug where ls-remote sends a ref-advertisement prefix\nwhen it shouldn't, and then optimizes a spot where it doesn't send one\nbut could.\n\n [1/2]: ls-remote: do not send ref prefixes for patterns\n [2/2]: ls-remote: pass heads/tags prefixes to transport\n\n builtin/ls-remote.c | 13 +++++--------\n t/t5512-ls-remote.sh | 18 ++++++++++++++++++\n 2 files changed, 23 insertions(+), 8 deletions(-)\n\n-Peff\n"},{"id":"362059","messageId":"20181031042405.GA5503@sigill.intra.peff.net","threadId":"49722","inReplyTo":"20181031042318.GA5347@sigill.intra.peff.net","subject":"[PATCH 1/2] ls-remote: do not send ref prefixes for patterns","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-31T04:24:05Z","receivedAt":"2018-10-31T04:24:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since b4be74105f (ls-remote: pass ref prefixes when requesting a\nremote's refs, 2018-03-15), \"ls-remote foo\" will pass \"refs/heads/foo\",\n\"refs/tags/foo\", etc to the transport code in an attempt to let the\nother side reduce the size of its advertisement.\n\nUnfortunately this is not correct, as ls-remote patterns do not follow\nthe usual ref lookup rules, and are in fact tail-matched. So we could\nfind \"refs/heads/foo\" or \"refs/heads/a/much/deeper/foo\" or even\n\"refs/another/hierarchy/foo\".\n\nSince we can't pass a prefix and there's not yet a v2 extension for\nmatching wildcards, we must disable this feature to keep the same\nbehavior as v1.\n\nReported-by: Jon Simons \nSigned-off-by: Jeff King \n---\n builtin/ls-remote.c | 8 --------\n t/t5512-ls-remote.sh | 9 +++++++++\n 2 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\nindex 6a0cdec30d..5faa8459d9 100644\n--- a/builtin/ls-remote.c\n+++ b/builtin/ls-remote.c\n@@ -88,15 +88,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \t\tint i;\n \t\tpattern = xcalloc(argc, sizeof(const char *));\n \t\tfor (i = 1; i < argc; i++) {\n-\t\t\tconst char *glob;\n \t\t\tpattern[i - 1] = xstrfmt(\"*/%s\", argv[i]);\n-\n-\t\t\tglob = strchr(argv[i], '*');\n-\t\t\tif (glob)\n-\t\t\t\targv_array_pushf(&ref_prefixes, \"%.*s\",\n-\t\t\t\t\t\t (int)(glob - argv[i]), argv[i]);\n-\t\t\telse\n-\t\t\t\texpand_ref_prefix(&ref_prefixes, argv[i]);\n \t\t}\n \t}\n \ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex bc5703ff9b..ca1b7e5bc1 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -302,4 +302,13 @@ test_expect_success 'ls-remote works outside repository' '\n \tnongit git ls-remote dst.git\n '\n \n+test_expect_success 'ls-remote patterns work with all protocol versions' '\n+\tgit for-each-ref --format=\"%(objectname)\t%(refname)\" \\\n+\t\trefs/heads/master refs/remotes/origin/master >expect &&\n+\tgit -c protocol.version=1 ls-remote . master >actual.v1 &&\n+\ttest_cmp expect actual.v1 &&\n+\tgit -c protocol.version=2 ls-remote . master >actual.v2 &&\n+\ttest_cmp expect actual.v2\n+'\n+\n test_done\n-- \n2.19.1.1298.g19f18f2a22\n\n"},{"id":"362060","messageId":"20181031042441.GB5503@sigill.intra.peff.net","threadId":"49722","inReplyTo":"20181031042318.GA5347@sigill.intra.peff.net","subject":"[PATCH 2/2] ls-remote: pass heads/tags prefixes to transport","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-31T04:24:42Z","receivedAt":"2018-10-31T04:24:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Unlike its arbitrary text patterns, the --heads and --tags\noptions to ls-remote are true prefixes. We can pass this\ninformation to the transport code. If the v2 protocol is in\nuse, that will reduce the size of the ref advertisement.\n\nNote that the test added here succeeds both before and after\nthe patch. This is an optimization, not a bug-fix; it's just\nmaking sure we didn't break anything.\n\nSigned-off-by: Jeff King \n---\n builtin/ls-remote.c | 5 +++++\n t/t5512-ls-remote.sh | 9 +++++++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\nindex 5faa8459d9..1d7f1f5ce2 100644\n--- a/builtin/ls-remote.c\n+++ b/builtin/ls-remote.c\n@@ -92,6 +92,11 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tif (flags & REF_TAGS)\n+\t\targv_array_push(&ref_prefixes, \"refs/tags/\");\n+\tif (flags & REF_HEADS)\n+\t\targv_array_push(&ref_prefixes, \"refs/heads/\");\n+\n \tremote = remote_get(dest);\n \tif (!remote) {\n \t\tif (dest)\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex ca1b7e5bc1..91ee6841c1 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -311,4 +311,13 @@ test_expect_success 'ls-remote patterns work with all protocol versions' '\n \ttest_cmp expect actual.v2\n '\n \n+test_expect_success 'ls-remote prefixes work with all protocol versions' '\n+\tgit for-each-ref --format=\"%(objectname)\t%(refname)\" \\\n+\t\trefs/heads/ refs/tags/ >expect &&\n+\tgit -c protocol.version=1 ls-remote --heads --tags . >actual.v1 &&\n+\ttest_cmp expect actual.v1 &&\n+\tgit -c protocol.version=2 ls-remote --heads --tags . >actual.v2 &&\n+\ttest_cmp expect actual.v2\n+'\n+\n test_done\n-- \n2.19.1.1298.g19f18f2a22\n"},{"id":"362063","messageId":"xmqq7ehyybnd.fsf@gitster-ct.c.googlers.com","threadId":"49722","inReplyTo":"20181031042405.GA5503@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] ls-remote: do not send ref prefixes for patterns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-31T04:37:26Z","receivedAt":"2018-10-31T04:37:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Since b4be74105f (ls-remote: pass ref prefixes when requesting a\n> remote's refs, 2018-03-15), \"ls-remote foo\" will pass \"refs/heads/foo\",\n> \"refs/tags/foo\", etc to the transport code in an attempt to let the\n> other side reduce the size of its advertisement.\n\nJonathan, seeing 2b554353 (\"fetch: send \"refs/tags/\" prefix upon CLI\nrefspecs\", 2018-06-05), I am guessing that you are doing the proto v2\nwork inherited from Brandon? Having to undo this is unfortunate, but\nI agree with this patch that we need to do this until ref prefix learns\nto grok wildcards.\n\n> Unfortunately this is not correct, as ls-remote patterns do not follow\n> the usual ref lookup rules, and are in fact tail-matched. So we could\n> find \"refs/heads/foo\" or \"refs/heads/a/much/deeper/foo\" or even\n> \"refs/another/hierarchy/foo\".\n>\n> Since we can't pass a prefix and there's not yet a v2 extension for\n> matching wildcards, we must disable this feature to keep the same\n> behavior as v1.\n>\n> Reported-by: Jon Simons \n> Signed-off-by: Jeff King \n> ---\n> builtin/ls-remote.c | 8 --------\n> t/t5512-ls-remote.sh | 9 +++++++++\n> 2 files changed, 9 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\n> index 6a0cdec30d..5faa8459d9 100644\n> --- a/builtin/ls-remote.c\n> +++ b/builtin/ls-remote.c\n> @@ -88,15 +88,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n> \t\tint i;\n> \t\tpattern = xcalloc(argc, sizeof(const char *));\n> \t\tfor (i = 1; i < argc; i++) {\n> -\t\t\tconst char *glob;\n> \t\t\tpattern[i - 1] = xstrfmt(\"*/%s\", argv[i]);\n> -\n> -\t\t\tglob = strchr(argv[i], '*');\n> -\t\t\tif (glob)\n> -\t\t\t\targv_array_pushf(&ref_prefixes, \"%.*s\",\n> -\t\t\t\t\t\t (int)(glob - argv[i]), argv[i]);\n> -\t\t\telse\n> -\t\t\t\texpand_ref_prefix(&ref_prefixes, argv[i]);\n> \t\t}\n> \t}\n> \n> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\n> index bc5703ff9b..ca1b7e5bc1 100755\n> --- a/t/t5512-ls-remote.sh\n> +++ b/t/t5512-ls-remote.sh\n> @@ -302,4 +302,13 @@ test_expect_success 'ls-remote works outside repository' '\n> \tnongit git ls-remote dst.git\n> '\n> \n> +test_expect_success 'ls-remote patterns work with all protocol versions' '\n> +\tgit for-each-ref --format=\"%(objectname)\t%(refname)\" \\\n> +\t\trefs/heads/master refs/remotes/origin/master >expect &&\n> +\tgit -c protocol.version=1 ls-remote . master >actual.v1 &&\n> +\ttest_cmp expect actual.v1 &&\n> +\tgit -c protocol.version=2 ls-remote . master >actual.v2 &&\n> +\ttest_cmp expect actual.v2\n> +'\n> +\n> test_done\n"},{"id":"362752","messageId":"20181108205249.195494-1-jonathantanmy@google.com","threadId":"49722","inReplyTo":"xmqq7ehyybnd.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] ls-remote: do not send ref prefixes for patterns","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-11-08T20:52:49Z","receivedAt":"2018-11-08T20:52:55Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jeff King writes:\n> \n> > Since b4be74105f (ls-remote: pass ref prefixes when requesting a\n> > remote's refs, 2018-03-15), \"ls-remote foo\" will pass \"refs/heads/foo\",\n> > \"refs/tags/foo\", etc to the transport code in an attempt to let the\n> > other side reduce the size of its advertisement.\n> \n> Jonathan, seeing 2b554353 (\"fetch: send \"refs/tags/\" prefix upon CLI\n> refspecs\", 2018-06-05), I am guessing that you are doing the proto v2\n> work inherited from Brandon?\n\nSorry for the late reply - I had some personal events, but I should be\nable to look more at Git stuff from now on.\n\nWell, it's true that I have been fixing some bugs related to protocol\nv2.\n\n> Having to undo this is unfortunate, but\n> I agree with this patch that we need to do this until ref prefix learns\n> to grok wildcards.\n\nIt is unfortunate, although as far as I can tell, the performance\nimprovements from \"git fetch\" (which I think is the more frequent\ncommand called) remain, so it might not be so bad. I see from your\nWhat's Cooking that these patches are to be merged to master, which I\nagree with.\n"}]}