{"thread":{"id":"50251","subject":"[PATCH] ls-refs: filter refs based on non-namespace name","startedAt":"2019-01-17T20:02:14Z","lastAt":"2019-01-17T23:33:17Z","messageCount":3,"participants":["Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"367016","messageId":"20190117200207.81825-1-jonathantanmy@google.com","threadId":"50251","inReplyTo":null,"subject":"[PATCH] ls-refs: filter refs based on non-namespace name","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-17T20:02:07Z","receivedAt":"2019-01-17T20:02:14Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Protocol v2 allows for filtering of ref advertisement based on a\nclient-given name prefix, but inclusion/exclusion is determined based on\nthe non-namespace-stripped version (e.g. matching a ref prefix of\n\"refs/heads\" against \"refs/namespaces/my-namespace/refs/heads/master\")\ninstead of the namespace-stripped version, which is the one that the\nuser actually sees.\n\nDetermine inclusion/exclusion based on the namespace-stripped version.\n\nThis bug was discovered through applying patches [1] that override\nprotocol.version to 2 in repositories when running tests, allowing us to\nnotice differences in behavior across different protocol versions.\n\n[1] https://public-inbox.org/git/cover.1547677183.git.jonathantanmy@google.com/\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nAnother bug discovered through GIT_TEST_PROTOCOL_VERSION.\n\nIf the patches in [1] above are merged with this patch, a test that\npreviously failed on GIT_TEST_PROTOCOL_VERSION=2 now passes.\n\nI'm not sure of the relevance of the last paragraph and the \"[1]\" in the\ncommit message - feel free to remove it. Since the relevant patches are\nnot merged yet, the e-mails are probably the best reference, and I have\ntried to summarize what they do concisely.\n---\n ls-refs.c            |  2 +-\n t/t5701-git-serve.sh | 21 +++++++++++++++++++++\n 2 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex a06f12eca8..7782bb054b 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -40,7 +40,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \tconst char *refname_nons = strip_namespace(refname);\n \tstruct strbuf refline = STRBUF_INIT;\n \n-\tif (!ref_match(&data->prefixes, refname))\n+\tif (!ref_match(&data->prefixes, refname_nons))\n \t\treturn 0;\n \n \tstrbuf_addf(&refline, \"%s %s\", oid_to_hex(oid), refname_nons);\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex ae79c6bbc0..ec13064ecd 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -112,6 +112,27 @@ test_expect_success 'basic ref-prefixes' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'ref prefix with namespaced repository' '\n+\t# Create a namespaced ref\n+\tgit update-ref refs/namespaces/ns/refs/heads/master \"$(git rev-parse refs/tags/one)\" &&\n+\n+\ttest-tool pkt-line pack >in <<-EOF &&\n+\tcommand=ls-refs\n+\t0001\n+\tref-prefix refs/heads/master\n+\t0000\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse refs/tags/one) refs/heads/master\n+\t0000\n+\tEOF\n+\n+\tGIT_NAMESPACE=ns git serve --stateless-rpc <in >out &&\n+\ttest-tool pkt-line unpack <out >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'refs/heads prefix' '\n \ttest-tool pkt-line pack >in <<-EOF &&\n \tcommand=ls-refs\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"367036","messageId":"xmqq8szjlyxi.fsf@gitster-ct.c.googlers.com","threadId":"50251","inReplyTo":"20190117200207.81825-1-jonathantanmy@google.com","subject":"Re: [PATCH] ls-refs: filter refs based on non-namespace name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-17T22:08:41Z","receivedAt":"2019-01-17T22:08:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> Protocol v2 allows for filtering of ref advertisement based on a\n> client-given name prefix, but inclusion/exclusion is determined based on\n> the non-namespace-stripped version (e.g. matching a ref prefix of\n> \"refs/heads\" against \"refs/namespaces/my-namespace/refs/heads/master\")\n> instead of the namespace-stripped version, which is the one that the\n> user actually sees.\n>\n> Determine inclusion/exclusion based on the namespace-stripped version.\n>\n> This bug was discovered through applying patches [1] that override\n> protocol.version to 2 in repositories when running tests, allowing us to\n> notice differences in behavior across different protocol versions.\n>\n> [1] https://public-inbox.org/git/cover.1547677183.git.jonathantanmy@google.com/\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> Another bug discovered through GIT_TEST_PROTOCOL_VERSION.\n\nOK, so \"ls-refs: filter based on non-namespace name\" (in the title) is a\nmeans to the objective 'ls-refs: make sure it honors namespaces\"\nwhich is a bugfix?\n\nThe new test peeks at the protocol level, but wouldn't we be able to\nsee the breakage by running ls-remote or something and observing its\nresult as well, or is the bug only observable with test-tool and not\ntriggerable by end-user facing git commands?\n\n> If the patches in [1] above are merged with this patch, a test that\n> previously failed on GIT_TEST_PROTOCOL_VERSION=2 now passes.\n>\n> I'm not sure of the relevance of the last paragraph and the \"[1]\" in the\n> commit message - feel free to remove it. Since the relevant patches are\n> not merged yet, the e-mails are probably the best reference, and I have\n> tried to summarize what they do concisely.\n> ---\n>  ls-refs.c            |  2 +-\n>  t/t5701-git-serve.sh | 21 +++++++++++++++++++++\n>  2 files changed, 22 insertions(+), 1 deletion(-)\n>\n> diff --git a/ls-refs.c b/ls-refs.c\n> index a06f12eca8..7782bb054b 100644\n> --- a/ls-refs.c\n> +++ b/ls-refs.c\n> @@ -40,7 +40,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n>  \tconst char *refname_nons = strip_namespace(refname);\n>  \tstruct strbuf refline = STRBUF_INIT;\n>  \n> -\tif (!ref_match(&data->prefixes, refname))\n> +\tif (!ref_match(&data->prefixes, refname_nons))\n>  \t\treturn 0;\n>  \n>  \tstrbuf_addf(&refline, \"%s %s\", oid_to_hex(oid), refname_nons);\n> diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\n> index ae79c6bbc0..ec13064ecd 100755\n> --- a/t/t5701-git-serve.sh\n> +++ b/t/t5701-git-serve.sh\n> @@ -112,6 +112,27 @@ test_expect_success 'basic ref-prefixes' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'ref prefix with namespaced repository' '\n> +\t# Create a namespaced ref\n> +\tgit update-ref refs/namespaces/ns/refs/heads/master \"$(git rev-parse refs/tags/one)\" &&\n> +\n> +\ttest-tool pkt-line pack >in <<-EOF &&\n> +\tcommand=ls-refs\n> +\t0001\n> +\tref-prefix refs/heads/master\n> +\t0000\n> +\tEOF\n> +\n> +\tcat >expect <<-EOF &&\n> +\t$(git rev-parse refs/tags/one) refs/heads/master\n> +\t0000\n> +\tEOF\n> +\n> +\tGIT_NAMESPACE=ns git serve --stateless-rpc <in >out &&\n> +\ttest-tool pkt-line unpack <out >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_expect_success 'refs/heads prefix' '\n>  \ttest-tool pkt-line pack >in <<-EOF &&\n>  \tcommand=ls-refs\n"},{"id":"367038","messageId":"20190117233305.42679-1-jonathantanmy@google.com","threadId":"50251","inReplyTo":"20190117200207.81825-1-jonathantanmy@google.com","subject":"[PATCH v2] ls-refs: filter refs using namespace-stripped name","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-17T23:33:05Z","receivedAt":"2019-01-17T23:33:17Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"If a user fetches refs/heads/master from a repo with namespace \"ns\", the\nremote is expected to (1) not send the real refs/heads/master, and (2)\nsend refs/namespaces/ns/refs/heads/master with the name\nrefs/heads/master. (1) indeed happens now, but not (2) - Git only sends\nrefs that have the user-given prefix, but it checks them against the\nfull name of the ref (the one starting with refs/namespaces), and not\nthe namespace-stripped one.\n\nThis is demonstrated by the patch in the test. Currently, it results in\n\"fatal: couldn't find remote ref refs/heads/master\" despite both\nunnamespaced and namespaced master being present. With the code change,\nit produces the expected result.\n\nCheck the ref prefixes against the namespace-stripped name.\n\nThis bug was discovered through applying patches [1] that override\nprotocol.version to 2 in repositories when running tests, allowing us to\nnotice differences in behavior across different protocol versions.\n\n[1] https://public-inbox.org/git/cover.1547677183.git.jonathantanmy@google.com/\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nChanges from v1:\n - updated commit message to explain from the end user viewpoint\n - wrote the test using end-user-facing Git commands\n\nThanks, Junio, for your review.\n\n> OK, so \"ls-refs: filter based on non-namespace name\" (in the title) is a\n> means to the objective 'ls-refs: make sure it honors namespaces\"\n> which is a bugfix?\n\nYes and no. I've updated the commit message to explain that it honors\nnamespaces in that it doesn't send the wrong refs, but it doesn't honor\nthem in that it doesn't send the right refs.\n\n> The new test peeks at the protocol level, but wouldn't we be able to\n> see the breakage by running ls-remote or something and observing its\n> result as well, or is the bug only observable with test-tool and not\n> triggerable by end-user facing git commands?\n\nls-remote wouldn't work as its filtering is incompatible with ref-prefix\n(see 631f0f8c4b (\"ls-remote: do not send ref prefixes for patterns\",\n2018-10-31)), but it can be observed with fetch. I've replaced the test\nwith one that uses fetch instead.\n---\n ls-refs.c              |  2 +-\n t/t5702-protocol-v2.sh | 21 +++++++++++++++++++++\n 2 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex a06f12eca8..7782bb054b 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -40,7 +40,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \tconst char *refname_nons = strip_namespace(refname);\n \tstruct strbuf refline = STRBUF_INIT;\n \n-\tif (!ref_match(&data->prefixes, refname))\n+\tif (!ref_match(&data->prefixes, refname_nons))\n \t\treturn 0;\n \n \tstrbuf_addf(&refline, \"%s %s\", oid_to_hex(oid), refname_nons);\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 0f2b09ebb8..3d802aa587 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -514,6 +514,27 @@ test_expect_success 'fetch with http:// using protocol v2' '\n \tgrep \"git< version 2\" log\n '\n \n+test_expect_success 'fetch from namespaced repo respects namespaces' '\n+\ttest_when_finished \"rm -f log\" &&\n+\n+\tgit init \"$HTTPD_DOCUMENT_ROOT_PATH/nsrepo\" &&\n+\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/nsrepo\" one &&\n+\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/nsrepo\" two &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/nsrepo\" \\\n+\t\tupdate-ref refs/namespaces/ns/refs/heads/master one &&\n+\n+\tGIT_TRACE_PACKET=\"$(pwd)/log\" git -C http_child -c protocol.version=2 \\\n+\t\tfetch \"$HTTPD_URL/smart_namespace/nsrepo\" \\\n+\t\trefs/heads/master:refs/heads/theirs &&\n+\n+\t# Server responded using protocol v2\n+\tgrep \"fetch< version 2\" log &&\n+\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/nsrepo\" rev-parse one >expect &&\n+\tgit -C http_child rev-parse theirs >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'push with http:// and a config of v2 does not request v2' '\n \ttest_when_finished \"rm -f log\" &&\n \t# Till v2 for push is designed, make sure that if a client has\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"}]}