Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 3, 2013, 18:04 UTC
Message-ID: <7vfvz75w4v.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1365004329-15264-1-git-send-email-jkoleszar@google.com>

John Koleszar <jkoleszar@google.com> writes:

Show 38 quoted lines

> diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
> index b5d7fbc..97f97a1 100755
> --- a/t/t5561-http-backend.sh
> +++ b/t/t5561-http-backend.sh
> @@ -23,6 +23,10 @@ GET() {
>  	test_cmp exp act
>  }
>  
> +GET_BODY() {
> +	curl "$HTTPD_URL/$SMART/repo.git/$1"
> +}
> +
>  POST() {
>  	curl --include --data "$2" \
>  	--header "Content-Type: application/x-$1-request" \
> @@ -134,6 +138,13 @@ POST /smart/repo.git/git-receive-pack HTTP/1.1 200 -
>  ###
>  GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403 -
>  POST /smart/repo.git/git-receive-pack HTTP/1.1 403 -
> +
> +###  namespace test
> +###
> +GET  /smart/repo.git/info/refs HTTP/1.1 200
> +GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
> +GET  /smart_namespace/repo.git/info/refs HTTP/1.1 200
> +GET  /smart_namespace/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
>  EOF
>  test_expect_success 'server request log matches test results' '
>  	sed -e "
> diff --git a/t/t556x_common b/t/t556x_common
> index 82926cf..6c34f33 100755
> --- a/t/t556x_common
> +++ b/t/t556x_common
> @@ -120,3 +120,28 @@ test_expect_success 'http.receivepack false' '
>  	GET info/refs?service=git-receive-pack "403 Forbidden" &&
>  	POST git-receive-pack 0000 "403 Forbidden"
>  '
> +test_expect_success 'backend respects namespaces' '(

A blank line before this new test would be easier to read.

Show 12 quoted lines

> +	log_div "namespace test"
> +	config http.uploadpack true &&
> +	config http.getanyfile true &&
> +
> +	NS=ns &&
> +	(cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> +		git update-ref refs/namespaces/$NS/refs/heads/master HEAD
> +	) &&
> +
> +	SMART=smart
> +	git ls-remote public >expected &&  
> +	grep /$NS/ expected >/dev/null &&

The standard output is not shown while running tests without "-v", and matches like these are useful while diagnosing what went wrong, so there is no upside in sending it to >/dev/null in general.

> +	GET_BODY "info/refs" >actual &&
> +	test_cmp expected actual &&
> +	GET_BODY "info/refs?service=git-upload-pack" | grep /$NS/ >/dev/null &&

Can GET_BODY fail for some reason with non-zero status (perhaps the backend by mistake barfs, refuses to serve that request and dies)? It does not sounds like a good idea to hide that status behind a pipe.

> +	SMART=smart_namespace &&
> +	GIT_NAMESPACE=$NS && export GIT_NAMESPACE &&
> +	git ls-remote public >expected &&  
> +	! grep /$NS/ expected>/dev/null &&

Is it sufficient to make sure that GIT_NAMESPACE hides /ns/ from the advertisement and not test that everything in that namespace is actually shown?

> +	GET_BODY "info/refs" >actual &&
> +	test_cmp expected actual &&
> +	! (GET_BODY "info/refs?service=git-upload-pack" | grep /$NS/ >/dev/null)

Likewise, also for the pipe. If GET_BODY died and failed to produce any output, we would certainly not see /ns/ in its output and the test will pass.

> +)'