[PATCH] ls-refs: filter refs based on non-namespace name

[PATCH] ls-refs: filter refs based on non-namespace name

[Jonathan Tan]

Protocol v2 allows for filtering of ref advertisement based on a
client-given name prefix, but inclusion/exclusion is determined based on
the non-namespace-stripped version (e.g. matching a ref prefix of
"refs/heads" against "refs/namespaces/my-namespace/refs/heads/master")
instead of the namespace-stripped version, which is the one that the
user actually sees.

Determine inclusion/exclusion based on the namespace-stripped version.

This bug was discovered through applying patches [1] that override
protocol.version to 2 in repositories when running tests, allowing us to
notice differences in behavior across different protocol versions.

[1] https://public-inbox.org/git/cover.1547677183.git.jonathantanmy@google.com/

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Another bug discovered through GIT_TEST_PROTOCOL_VERSION.

If the patches in [1] above are merged with this patch, a test that
previously failed on GIT_TEST_PROTOCOL_VERSION=2 now passes.

I'm not sure of the relevance of the last paragraph and the "[1]" in the
commit message - feel free to remove it. Since the relevant patches are
not merged yet, the e-mails are probably the best reference, and I have
tried to summarize what they do concisely.
---
 ls-refs.c            |  2 +-
 t/t5701-git-serve.sh | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/ls-refs.c b/ls-refs.c
index a06f12eca8..7782bb054b 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -40,7 +40,7 @@ static int send_ref(const char *refname, const struct object_id *oid,
 	const char *refname_nons = strip_namespace(refname);
 	struct strbuf refline = STRBUF_INIT;
 
-	if (!ref_match(&data->prefixes, refname))
+	if (!ref_match(&data->prefixes, refname_nons))
 		return 0;
 
 	strbuf_addf(&refline, "%s %s", oid_to_hex(oid), refname_nons);
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index ae79c6bbc0..ec13064ecd 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -112,6 +112,27 @@ test_expect_success 'basic ref-prefixes' '
 	test_cmp expect actual
 '
 
+test_expect_success 'ref prefix with namespaced repository' '
+	# Create a namespaced ref
+	git update-ref refs/namespaces/ns/refs/heads/master "$(git rev-parse refs/tags/one)" &&
+
+	test-tool pkt-line pack >in <<-EOF &&
+	command=ls-refs
+	0001
+	ref-prefix refs/heads/master
+	0000
+	EOF
+
+	cat >expect <<-EOF &&
+	$(git rev-parse refs/tags/one) refs/heads/master
+	0000
+	EOF
+
+	GIT_NAMESPACE=ns git serve --stateless-rpc <in >out &&
+	test-tool pkt-line unpack <out >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'refs/heads prefix' '
 	test-tool pkt-line pack >in <<-EOF &&
 	command=ls-refs
-- 
2.19.0.271.gfe8321ec05.dirty

Re: [PATCH] ls-refs: filter refs based on non-namespace name

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Protocol v2 allows for filtering of ref advertisement based on a
> client-given name prefix, but inclusion/exclusion is determined based on
> the non-namespace-stripped version (e.g. matching a ref prefix of
> "refs/heads" against "refs/namespaces/my-namespace/refs/heads/master")
> instead of the namespace-stripped version, which is the one that the
> user actually sees.
>
> Determine inclusion/exclusion based on the namespace-stripped version.
>
> This bug was discovered through applying patches [1] that override
> protocol.version to 2 in repositories when running tests, allowing us to
> notice differences in behavior across different protocol versions.
>
> [1] https://public-inbox.org/git/cover.1547677183.git.jonathantanmy@google.com/
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> Another bug discovered through GIT_TEST_PROTOCOL_VERSION.

OK, so "ls-refs: filter based on non-namespace name" (in the title) is a
means to the objective 'ls-refs: make sure it honors namespaces"
which is a bugfix?

The new test peeks at the protocol level, but wouldn't we be able to
see the breakage by running ls-remote or something and observing its
result as well, or is the bug only observable with test-tool and not
triggerable by end-user facing git commands?

> If the patches in [1] above are merged with this patch, a test that
> previously failed on GIT_TEST_PROTOCOL_VERSION=2 now passes.
>
> I'm not sure of the relevance of the last paragraph and the "[1]" in the
> commit message - feel free to remove it. Since the relevant patches are
> not merged yet, the e-mails are probably the best reference, and I have
> tried to summarize what they do concisely.
> ---
>  ls-refs.c            |  2 +-
>  t/t5701-git-serve.sh | 21 +++++++++++++++++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/ls-refs.c b/ls-refs.c
> index a06f12eca8..7782bb054b 100644
> --- a/ls-refs.c
> +++ b/ls-refs.c
> @@ -40,7 +40,7 @@ static int send_ref(const char *refname, const struct object_id *oid,
>  	const char *refname_nons = strip_namespace(refname);
>  	struct strbuf refline = STRBUF_INIT;
>  
> -	if (!ref_match(&data->prefixes, refname))
> +	if (!ref_match(&data->prefixes, refname_nons))
>  		return 0;
>  
>  	strbuf_addf(&refline, "%s %s", oid_to_hex(oid), refname_nons);
> diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
> index ae79c6bbc0..ec13064ecd 100755
> --- a/t/t5701-git-serve.sh
> +++ b/t/t5701-git-serve.sh
> @@ -112,6 +112,27 @@ test_expect_success 'basic ref-prefixes' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'ref prefix with namespaced repository' '
> +	# Create a namespaced ref
> +	git update-ref refs/namespaces/ns/refs/heads/master "$(git rev-parse refs/tags/one)" &&
> +
> +	test-tool pkt-line pack >in <<-EOF &&
> +	command=ls-refs
> +	0001
> +	ref-prefix refs/heads/master
> +	0000
> +	EOF
> +
> +	cat >expect <<-EOF &&
> +	$(git rev-parse refs/tags/one) refs/heads/master
> +	0000
> +	EOF
> +
> +	GIT_NAMESPACE=ns git serve --stateless-rpc <in >out &&
> +	test-tool pkt-line unpack <out >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'refs/heads prefix' '
>  	test-tool pkt-line pack >in <<-EOF &&
>  	command=ls-refs

[PATCH v2] ls-refs: filter refs using namespace-stripped name

[Jonathan Tan]

If a user fetches refs/heads/master from a repo with namespace "ns", the
remote is expected to (1) not send the real refs/heads/master, and (2)
send refs/namespaces/ns/refs/heads/master with the name
refs/heads/master. (1) indeed happens now, but not (2) - Git only sends
refs that have the user-given prefix, but it checks them against the
full name of the ref (the one starting with refs/namespaces), and not
the namespace-stripped one.

This is demonstrated by the patch in the test. Currently, it results in
"fatal: couldn't find remote ref refs/heads/master" despite both
unnamespaced and namespaced master being present. With the code change,
it produces the expected result.

Check the ref prefixes against the namespace-stripped name.

This bug was discovered through applying patches [1] that override
protocol.version to 2 in repositories when running tests, allowing us to
notice differences in behavior across different protocol versions.

[1] https://public-inbox.org/git/cover.1547677183.git.jonathantanmy@google.com/

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Changes from v1:
 - updated commit message to explain from the end user viewpoint
 - wrote the test using end-user-facing Git commands

Thanks, Junio, for your review.

> OK, so "ls-refs: filter based on non-namespace name" (in the title) is a
> means to the objective 'ls-refs: make sure it honors namespaces"
> which is a bugfix?

Yes and no. I've updated the commit message to explain that it honors
namespaces in that it doesn't send the wrong refs, but it doesn't honor
them in that it doesn't send the right refs.

> The new test peeks at the protocol level, but wouldn't we be able to
> see the breakage by running ls-remote or something and observing its
> result as well, or is the bug only observable with test-tool and not
> triggerable by end-user facing git commands?

ls-remote wouldn't work as its filtering is incompatible with ref-prefix
(see 631f0f8c4b ("ls-remote: do not send ref prefixes for patterns",
2018-10-31)), but it can be observed with fetch. I've replaced the test
with one that uses fetch instead.
---
 ls-refs.c              |  2 +-
 t/t5702-protocol-v2.sh | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/ls-refs.c b/ls-refs.c
index a06f12eca8..7782bb054b 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -40,7 +40,7 @@ static int send_ref(const char *refname, const struct object_id *oid,
 	const char *refname_nons = strip_namespace(refname);
 	struct strbuf refline = STRBUF_INIT;
 
-	if (!ref_match(&data->prefixes, refname))
+	if (!ref_match(&data->prefixes, refname_nons))
 		return 0;
 
 	strbuf_addf(&refline, "%s %s", oid_to_hex(oid), refname_nons);
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 0f2b09ebb8..3d802aa587 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -514,6 +514,27 @@ test_expect_success 'fetch with http:// using protocol v2' '
 	grep "git< version 2" log
 '
 
+test_expect_success 'fetch from namespaced repo respects namespaces' '
+	test_when_finished "rm -f log" &&
+
+	git init "$HTTPD_DOCUMENT_ROOT_PATH/nsrepo" &&
+	test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/nsrepo" one &&
+	test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/nsrepo" two &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/nsrepo" \
+		update-ref refs/namespaces/ns/refs/heads/master one &&
+
+	GIT_TRACE_PACKET="$(pwd)/log" git -C http_child -c protocol.version=2 \
+		fetch "$HTTPD_URL/smart_namespace/nsrepo" \
+		refs/heads/master:refs/heads/theirs &&
+
+	# Server responded using protocol v2
+	grep "fetch< version 2" log &&
+
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/nsrepo" rev-parse one >expect &&
+	git -C http_child rev-parse theirs >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'push with http:// and a config of v2 does not request v2' '
 	test_when_finished "rm -f log" &&
 	# Till v2 for push is designed, make sure that if a client has
-- 
2.19.0.271.gfe8321ec05.dirty