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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 17, 2019, 22:08 UTC
Message-ID: <xmqq8szjlyxi.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190117200207.81825-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 18 quoted lines

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

Show 57 quoted lines

> 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