threads / patch / 33315

patchhttp-backend: respect GIT_NAMESPACE with dumb clients

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

## tl;dr

26 messages between Mar 28, 2013 and Apr 10, 2013. Diffs are folded; open one to read it.

replies: 25people: 5as markdown or json

John Koleszar· Mar 28, 2013, 04:58 UTC · lore

Filter the list of refs returned via the dumb HTTP protocol according to the active namespace, consistent with other clients of the upload-pack service.

Signed-off-by: John Koleszar <jkoleszar@google.com>
---
 http-backend.c          |  8 +++++---
 t/lib-httpd/apache.conf |  5 +++++
 t/t5561-http-backend.sh |  4 ++++
 t/t556x_common          | 16 ++++++++++++++++
 4 files changed, 30 insertions(+), 3 deletions(-)
Show changes to 4 files +30 −3

http-backend.c, t/lib-httpd/apache.conf, t/t5561-http-backend.sh, t/t556x_common

diff --git a/http-backend.c b/http-backend.c
index f50e77f..b9896b0 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -361,17 +361,19 @@ static void run_service(const char **argv)
 static int show_text_ref(const char *name, const unsigned char *sha1,
 	int flag, void *cb_data)
 {
+	const char *name_nons = strip_namespace(name);
 	struct strbuf *buf = cb_data;
 	struct object *o = parse_object(sha1);
 	if (!o)
 		return 0;
 
-	strbuf_addf(buf, "%s\t%s\n", sha1_to_hex(sha1), name);
+	strbuf_addf(buf, "%s\t%s\n", sha1_to_hex(sha1), name_nons);
 	if (o->type == OBJ_TAG) {
 		o = deref_tag(o, name, 0);
 		if (!o)
 			return 0;
-		strbuf_addf(buf, "%s\t%s^{}\n", sha1_to_hex(o->sha1), name);
+		strbuf_addf(buf, "%s\t%s^{}\n", sha1_to_hex(o->sha1),
+		            name_nons);
 	}
 	return 0;
 }
@@ -402,7 +404,7 @@ static void get_info_refs(char *arg)
 
 	} else {
 		select_getanyfile();
-		for_each_ref(show_text_ref, &buf);
+		for_each_namespaced_ref(show_text_ref, &buf);
 		send_strbuf("text/plain", &buf);
 	}
 	strbuf_release(&buf);
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 938b4cf..ad85537 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -61,6 +61,11 @@ Alias /auth/dumb/ www/auth/dumb/
 	SetEnv GIT_COMMITTER_NAME "Custom User"
 	SetEnv GIT_COMMITTER_EMAIL custom@example.com
 </LocationMatch>
+<LocationMatch /smart_namespace/>
+	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+	SetEnv GIT_HTTP_EXPORT_ALL
+	SetEnv GIT_NAMESPACE ns
+</LocationMatch>
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
 <Directory ${GIT_EXEC_PATH}>
diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
index b5d7fbc..5a19d61 100755
--- a/t/t5561-http-backend.sh
+++ b/t/t5561-http-backend.sh
@@ -134,6 +134,10 @@ 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_namespace/repo.git/info/refs 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..cb9eb9d 100755
--- a/t/t556x_common
+++ b/t/t556x_common
@@ -120,3 +120,19 @@ 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' '
+	log_div "namespace test"
+	config http.uploadpack true &&
+	config http.getanyfile true &&
+
+	GIT_NAMESPACE=ns && export GIT_NAMESPACE &&
+	git push public master:master &&
+	(cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+		git for-each-ref | grep /$GIT_NAMESPACE/ >/dev/null
+	) &&
+
+	git ls-remote public >exp &&  
+	curl "$HTTPD_URL/smart_namespace/repo.git/info/refs" >act &&
+	test_cmp exp act &&
+	(grep /ns/ exp && false || true)
+'
-- 
1.8.1.3
Junio C Hamano· Mar 28, 2013, 06:02 UTC · re: John Koleszar · lore

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

John Koleszar <jkoleszar@google.com> writes:
Show 6 quoted lines
> Filter the list of refs returned via the dumb HTTP protocol according
> to the active namespace, consistent with other clients of the
> upload-pack service.
>
> Signed-off-by: John Koleszar <jkoleszar@google.com>
> ---
Looks sane from a cursory read---thanks.
Josh, any comments?
Show 96 quoted lines
>  http-backend.c          |  8 +++++---
>  t/lib-httpd/apache.conf |  5 +++++
>  t/t5561-http-backend.sh |  4 ++++
>  t/t556x_common          | 16 ++++++++++++++++
>  4 files changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/http-backend.c b/http-backend.c
> index f50e77f..b9896b0 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -361,17 +361,19 @@ static void run_service(const char **argv)
>  static int show_text_ref(const char *name, const unsigned char *sha1,
>  	int flag, void *cb_data)
>  {
> +	const char *name_nons = strip_namespace(name);
>  	struct strbuf *buf = cb_data;
>  	struct object *o = parse_object(sha1);
>  	if (!o)
>  		return 0;
>  
> -	strbuf_addf(buf, "%s\t%s\n", sha1_to_hex(sha1), name);
> +	strbuf_addf(buf, "%s\t%s\n", sha1_to_hex(sha1), name_nons);
>  	if (o->type == OBJ_TAG) {
>  		o = deref_tag(o, name, 0);
>  		if (!o)
>  			return 0;
> -		strbuf_addf(buf, "%s\t%s^{}\n", sha1_to_hex(o->sha1), name);
> +		strbuf_addf(buf, "%s\t%s^{}\n", sha1_to_hex(o->sha1),
> +		            name_nons);
>  	}
>  	return 0;
>  }
> @@ -402,7 +404,7 @@ static void get_info_refs(char *arg)
>  
>  	} else {
>  		select_getanyfile();
> -		for_each_ref(show_text_ref, &buf);
> +		for_each_namespaced_ref(show_text_ref, &buf);
>  		send_strbuf("text/plain", &buf);
>  	}
>  	strbuf_release(&buf);
> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
> index 938b4cf..ad85537 100644
> --- a/t/lib-httpd/apache.conf
> +++ b/t/lib-httpd/apache.conf
> @@ -61,6 +61,11 @@ Alias /auth/dumb/ www/auth/dumb/
>  	SetEnv GIT_COMMITTER_NAME "Custom User"
>  	SetEnv GIT_COMMITTER_EMAIL custom@example.com
>  </LocationMatch>
> +<LocationMatch /smart_namespace/>
> +	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
> +	SetEnv GIT_HTTP_EXPORT_ALL
> +	SetEnv GIT_NAMESPACE ns
> +</LocationMatch>
>  ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
>  ScriptAlias /broken_smart/ broken-smart-http.sh/
>  <Directory ${GIT_EXEC_PATH}>
> diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
> index b5d7fbc..5a19d61 100755
> --- a/t/t5561-http-backend.sh
> +++ b/t/t5561-http-backend.sh
> @@ -134,6 +134,10 @@ 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_namespace/repo.git/info/refs 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..cb9eb9d 100755
> --- a/t/t556x_common
> +++ b/t/t556x_common
> @@ -120,3 +120,19 @@ 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' '
> +	log_div "namespace test"
> +	config http.uploadpack true &&
> +	config http.getanyfile true &&
> +
> +	GIT_NAMESPACE=ns && export GIT_NAMESPACE &&
> +	git push public master:master &&
> +	(cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> +		git for-each-ref | grep /$GIT_NAMESPACE/ >/dev/null
> +	) &&
> +
> +	git ls-remote public >exp &&  
> +	curl "$HTTPD_URL/smart_namespace/repo.git/info/refs" >act &&
> +	test_cmp exp act &&
> +	(grep /ns/ exp && false || true)
> +'
Josh Triplett· Mar 28, 2013, 15:49 UTC · re: Junio C Hamano · lore

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

On Wed, Mar 27, 2013 at 11:02:28PM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> John Koleszar <jkoleszar@google.com> writes:
> 
> > Filter the list of refs returned via the dumb HTTP protocol according
> > to the active namespace, consistent with other clients of the
> > upload-pack service.
> >
> > Signed-off-by: John Koleszar <jkoleszar@google.com>
> > ---
> 
> Looks sane from a cursory read---thanks.
> 
> Josh, any comments?
Looks reasonable; glad to see tests explicitly covering it, as well.

Ideally I'd like to see an additional test against that same namespaced repository, fetching from a different URL that doesn't set GIT_NAMESPACE, and verifying that info/refs contains the full set of namespace-qualified refs from all namespaces. That would make sure that particular behavior doesn't regress in the future, since one of the use cases for namespaced repositories involves fetching the whole combined repo, for purposes such as backups or replication.

Show 96 quoted lines
> >  http-backend.c          |  8 +++++---
> >  t/lib-httpd/apache.conf |  5 +++++
> >  t/t5561-http-backend.sh |  4 ++++
> >  t/t556x_common          | 16 ++++++++++++++++
> >  4 files changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/http-backend.c b/http-backend.c
> > index f50e77f..b9896b0 100644
> > --- a/http-backend.c
> > +++ b/http-backend.c
> > @@ -361,17 +361,19 @@ static void run_service(const char **argv)
> >  static int show_text_ref(const char *name, const unsigned char *sha1,
> >  	int flag, void *cb_data)
> >  {
> > +	const char *name_nons = strip_namespace(name);
> >  	struct strbuf *buf = cb_data;
> >  	struct object *o = parse_object(sha1);
> >  	if (!o)
> >  		return 0;
> >  
> > -	strbuf_addf(buf, "%s\t%s\n", sha1_to_hex(sha1), name);
> > +	strbuf_addf(buf, "%s\t%s\n", sha1_to_hex(sha1), name_nons);
> >  	if (o->type == OBJ_TAG) {
> >  		o = deref_tag(o, name, 0);
> >  		if (!o)
> >  			return 0;
> > -		strbuf_addf(buf, "%s\t%s^{}\n", sha1_to_hex(o->sha1), name);
> > +		strbuf_addf(buf, "%s\t%s^{}\n", sha1_to_hex(o->sha1),
> > +		            name_nons);
> >  	}
> >  	return 0;
> >  }
> > @@ -402,7 +404,7 @@ static void get_info_refs(char *arg)
> >  
> >  	} else {
> >  		select_getanyfile();
> > -		for_each_ref(show_text_ref, &buf);
> > +		for_each_namespaced_ref(show_text_ref, &buf);
> >  		send_strbuf("text/plain", &buf);
> >  	}
> >  	strbuf_release(&buf);
> > diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
> > index 938b4cf..ad85537 100644
> > --- a/t/lib-httpd/apache.conf
> > +++ b/t/lib-httpd/apache.conf
> > @@ -61,6 +61,11 @@ Alias /auth/dumb/ www/auth/dumb/
> >  	SetEnv GIT_COMMITTER_NAME "Custom User"
> >  	SetEnv GIT_COMMITTER_EMAIL custom@example.com
> >  </LocationMatch>
> > +<LocationMatch /smart_namespace/>
> > +	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
> > +	SetEnv GIT_HTTP_EXPORT_ALL
> > +	SetEnv GIT_NAMESPACE ns
> > +</LocationMatch>
> >  ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
> >  ScriptAlias /broken_smart/ broken-smart-http.sh/
> >  <Directory ${GIT_EXEC_PATH}>
> > diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
> > index b5d7fbc..5a19d61 100755
> > --- a/t/t5561-http-backend.sh
> > +++ b/t/t5561-http-backend.sh
> > @@ -134,6 +134,10 @@ 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_namespace/repo.git/info/refs 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..cb9eb9d 100755
> > --- a/t/t556x_common
> > +++ b/t/t556x_common
> > @@ -120,3 +120,19 @@ 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' '
> > +	log_div "namespace test"
> > +	config http.uploadpack true &&
> > +	config http.getanyfile true &&
> > +
> > +	GIT_NAMESPACE=ns && export GIT_NAMESPACE &&
> > +	git push public master:master &&
> > +	(cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> > +		git for-each-ref | grep /$GIT_NAMESPACE/ >/dev/null
> > +	) &&
> > +
> > +	git ls-remote public >exp &&  
> > +	curl "$HTTPD_URL/smart_namespace/repo.git/info/refs" >act &&
> > +	test_cmp exp act &&
> > +	(grep /ns/ exp && false || true)
> > +'
Junio C Hamano· Mar 28, 2013, 14:43 UTC · re: John Koleszar · lore

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

John Koleszar <jkoleszar@google.com> writes:
Show 29 quoted lines
> diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
> index b5d7fbc..5a19d61 100755
> --- a/t/t5561-http-backend.sh
> +++ b/t/t5561-http-backend.sh
> @@ -134,6 +134,10 @@ 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_namespace/repo.git/info/refs 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..cb9eb9d 100755
> --- a/t/t556x_common
> +++ b/t/t556x_common
> @@ -120,3 +120,19 @@ 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' '
> +	log_div "namespace test"
> +	config http.uploadpack true &&
> +	config http.getanyfile true &&
> +
> +	GIT_NAMESPACE=ns && export GIT_NAMESPACE &&

When other people want to enhance this test suite later, their tests may not want the namespace contaminated with the environment variable. You would need to enclose from here to the end inside a subshell or something.

Show 7 quoted lines
> +	git push public master:master &&
> +	(cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> +		git for-each-ref | grep /$GIT_NAMESPACE/ >/dev/null
> +	) &&
> +
> +	git ls-remote public >exp &&  
> +	curl "$HTTPD_URL/smart_namespace/repo.git/info/refs" >act &&
Spell out "expect" and "actual".

For some unknwon reason, I am getting an HTTPD_URL at this point, causing it to fail with:

    curl: (3) <url> malformed
> +	test_cmp exp act &&
> +	(grep /ns/ exp && false || true)
What does that last line even mean?  Both
	false && false || true
        true && false || true
will yield true.  Leftover from your debugging session?
John Koleszar· Mar 28, 2013, 15:54 UTC · lore

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

On Thu, Mar 28, 2013 at 8:52 AM, John Koleszar <jkoleszar@google.com> wrote:
Show 83 quoted lines
> On Thu, Mar 28, 2013 at 7:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> John Koleszar <jkoleszar@google.com> writes:
>>
>> > diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
>> > index b5d7fbc..5a19d61 100755
>> > --- a/t/t5561-http-backend.sh
>> > +++ b/t/t5561-http-backend.sh
>> > @@ -134,6 +134,10 @@ 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_namespace/repo.git/info/refs 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..cb9eb9d 100755
>> > --- a/t/t556x_common
>> > +++ b/t/t556x_common
>> > @@ -120,3 +120,19 @@ 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' '
>> > +     log_div "namespace test"
>> > +     config http.uploadpack true &&
>> > +     config http.getanyfile true &&
>> > +
>> > +     GIT_NAMESPACE=ns && export GIT_NAMESPACE &&
>>
>> When other people want to enhance this test suite later, their tests
>> may not want the namespace contaminated with the environment
>> variable.  You would need to enclose from here to the end inside a
>> subshell or something.
>>
>
> Ok. I'm not familiar with the test infrastructure, I had guessed that these
> were already running inside a subshell. I'll make this explicit.
>
>>
>> > +     git push public master:master &&
>> > +     (cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
>> > +             git for-each-ref | grep /$GIT_NAMESPACE/ >/dev/null
>> > +     ) &&
>> > +
>> > +     git ls-remote public >exp &&
>> > +     curl "$HTTPD_URL/smart_namespace/repo.git/info/refs" >act &&
>>
>> Spell out "expect" and "actual".
>>
>
> The exiting tests are using exp and act for this. No objection if you want
> me to spell it out here, but having two different files for this may be
> confusing.
>
>>
>> For some unknwon reason, I am getting an HTTPD_URL at this point,
>> causing it to fail with:
>>
>>     curl: (3) <url> malformed
>>
>
> Ah, my fault. I only ran t5561-http-backend.sh. Will fix.
>
>> > +     test_cmp exp act &&
>> > +     (grep /ns/ exp && false || true)
>>
>> What does that last line even mean?  Both
>>
>>         false && false || true
>>         true && false || true
>>
>> will yield true.  Leftover from your debugging session?
>
>
> Facepalm. The intent here is to invert the grep, to make sure that the /ns/
> does not appear in the output. No idea why I wrote it that way. Will fix.
>
Junio C Hamano· Mar 28, 2013, 16:46 UTC · re: John Koleszar · lore

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

John Koleszar <jkoleszar@google.com> writes:
>> Facepalm. The intent here is to invert the grep, to make sure that the /ns/
>> does not appear in the output. No idea why I wrote it that way. Will fix.
OK, "! grep /ns/ exp" would do.
Thanks.
John Koleszar· Apr 3, 2013, 15:52 UTC · re: John Koleszar · lore

Filter the list of refs returned via the dumb HTTP protocol according to the active namespace, consistent with other clients of the upload-pack service.

Signed-off-by: John Koleszar <jkoleszar@google.com>
---

This should incorporate all of Junio's and Josh's comments. Also fixes a bug in the first patch where the HEAD wasn't included in the list of refs returned. PTAL.

 http-backend.c                   |  9 ++++++---
 t/lib-httpd/apache.conf          |  5 +++++
 t/t5560-http-backend-noserver.sh |  7 +++++++
 t/t5561-http-backend.sh          | 11 +++++++++++
 t/t556x_common                   | 25 +++++++++++++++++++++++++
 5 files changed, 54 insertions(+), 3 deletions(-)
Show changes to 5 files +54 −3

http-backend.c, t/lib-httpd/apache.conf, t/t5560-http-backend-noserver.sh, t/t5561-http-backend.sh, t/t556x_common

diff --git a/http-backend.c b/http-backend.c
index f50e77f..d32128f 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -361,17 +361,19 @@ static void run_service(const char **argv)
 static int show_text_ref(const char *name, const unsigned char *sha1,
 	int flag, void *cb_data)
 {
+	const char *name_nons = strip_namespace(name);
 	struct strbuf *buf = cb_data;
 	struct object *o = parse_object(sha1);
 	if (!o)
 		return 0;
 
-	strbuf_addf(buf, "%s\t%s\n", sha1_to_hex(sha1), name);
+	strbuf_addf(buf, "%s\t%s\n", sha1_to_hex(sha1), name_nons);
 	if (o->type == OBJ_TAG) {
 		o = deref_tag(o, name, 0);
 		if (!o)
 			return 0;
-		strbuf_addf(buf, "%s\t%s^{}\n", sha1_to_hex(o->sha1), name);
+		strbuf_addf(buf, "%s\t%s^{}\n", sha1_to_hex(o->sha1),
+		            name_nons);
 	}
 	return 0;
 }
@@ -402,7 +404,8 @@ static void get_info_refs(char *arg)
 
 	} else {
 		select_getanyfile();
-		for_each_ref(show_text_ref, &buf);
+		head_ref_namespaced(show_text_ref, &buf);
+		for_each_namespaced_ref(show_text_ref, &buf);
 		send_strbuf("text/plain", &buf);
 	}
 	strbuf_release(&buf);
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 938b4cf..ad85537 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -61,6 +61,11 @@ Alias /auth/dumb/ www/auth/dumb/
 	SetEnv GIT_COMMITTER_NAME "Custom User"
 	SetEnv GIT_COMMITTER_EMAIL custom@example.com
 </LocationMatch>
+<LocationMatch /smart_namespace/>
+	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+	SetEnv GIT_HTTP_EXPORT_ALL
+	SetEnv GIT_NAMESPACE ns
+</LocationMatch>
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
 <Directory ${GIT_EXEC_PATH}>
diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index ef98d95..85a5625 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh
@@ -26,6 +26,13 @@ GET() {
 	test_cmp exp act
 }
 
+GET_BODY() {
+	REQUEST_METHOD="GET" && export REQUEST_METHOD &&
+	run_backend "/repo.git/$1" &&
+	sane_unset REQUEST_METHOD &&
+	tr '\015' Q <act.out | sed '1,/^Q$/d'
+}
+
 POST() {
 	REQUEST_METHOD="POST" && export REQUEST_METHOD &&
 	CONTENT_TYPE="application/x-$1-request" && export CONTENT_TYPE &&
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' '(
+	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 &&
+	GET_BODY "info/refs" >actual &&
+	test_cmp expected actual &&
+	GET_BODY "info/refs?service=git-upload-pack" | grep /$NS/ >/dev/null &&
+
+	SMART=smart_namespace &&
+	GIT_NAMESPACE=$NS && export GIT_NAMESPACE &&
+	git ls-remote public >expected &&  
+	! grep /$NS/ expected>/dev/null &&
+	GET_BODY "info/refs" >actual &&
+	test_cmp expected actual &&
+	! (GET_BODY "info/refs?service=git-upload-pack" | grep /$NS/ >/dev/null)
+)'
-- 
1.8.1.3
Jeff King· Apr 3, 2013, 16:10 UTC · re: John Koleszar · lore

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

On Wed, Apr 03, 2013 at 08:52:09AM -0700, John Koleszar wrote:
Show 15 quoted lines
> +	SMART=smart
> +	git ls-remote public >expected &&  
> +	grep /$NS/ expected >/dev/null &&
> +	GET_BODY "info/refs" >actual &&
> +	test_cmp expected actual &&
> +	GET_BODY "info/refs?service=git-upload-pack" | grep /$NS/ >/dev/null &&
> +
> +	SMART=smart_namespace &&
> +	GIT_NAMESPACE=$NS && export GIT_NAMESPACE &&
> +	git ls-remote public >expected &&  
> +	! grep /$NS/ expected>/dev/null &&
> +	GET_BODY "info/refs" >actual &&
> +	test_cmp expected actual &&
> +	! (GET_BODY "info/refs?service=git-upload-pack" | grep /$NS/ >/dev/null)
> +)'

Hmm. This is testing just the ref advertisement. It would be nice to see a complete transaction tested with namespaces turned on. Something like this (squashed into your patch) seems to work for me:

Show changes to t/t5551-http-fetch.sh +12 −1
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index 47eb769..9fd8bbf 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -162,6 +162,18 @@ test_expect_success 'invalid Content-Type rejected' '
 	grep "not valid:" actual
 '
 
+test_expect_success 'create namespaced refs' '
+	test_commit namespaced &&
+	git push public HEAD:refs/namespaces/ns/refs/heads/master
+'
+
+test_expect_success 'clone respects namespace' '
+	git clone --bare "$HTTPD_URL/smart_namespace/repo.git" ns.git &&
+	echo namespaced >expect &&
+	git --git-dir=ns.git log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+
 test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
 
 test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '

-Peff
Jeff King· Apr 3, 2013, 16:16 UTC · re: Jeff King · lore

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

On Wed, Apr 03, 2013 at 12:10:38PM -0400, Jeff King wrote:
> Hmm. This is testing just the ref advertisement. It would be nice to see
> a complete transaction tested with namespaces turned on. Something like
> this (squashed into your patch) seems to work for me:

Actually, I guess the point of your patch was to fix the dumb-via-http-backend transport. So this would be more complete:

Show changes to t/t5551-http-fetch.sh +22 −0
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index 47eb769..b5032bd 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -162,6 +162,28 @@ test_expect_success 'invalid Content-Type rejected' '
 	grep "not valid:" actual
 '
 
+test_expect_success 'create namespaced refs' '
+	test_commit namespaced &&
+	git push public HEAD:refs/namespaces/ns/refs/heads/master
+'
+
+test_expect_success 'smart clone respects namespace' '
+	git clone --bare "$HTTPD_URL/smart_namespace/repo.git" ns-smart.git &&
+	echo namespaced >expect &&
+	git --git-dir=ns-smart.git log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'dumb clone via http-backend respects namespace' '
+	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
+		config http.getanyfile true &&
+	GIT_SMART_HTTP=0 git clone --bare \
+		"$HTTPD_URL/smart_namespace/repo.git" ns-dumb.git &&
+	echo namespaced >expect &&
+	git --git-dir=ns-dumb.git log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+
 test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
 
 test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
Junio C Hamano· Apr 3, 2013, 18:05 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
> On Wed, Apr 03, 2013 at 12:10:38PM -0400, Jeff King wrote:
>
>> Hmm. This is testing just the ref advertisement. It would be nice to see
>> a complete transaction tested with namespaces turned on. Something like
>> this (squashed into your patch) seems to work for me:
>
> Actually, I guess the point of your patch was to fix the
> dumb-via-http-backend transport. So this would be more complete:
Yeah, sounds sensible to me.
Show 33 quoted lines
> diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
> index 47eb769..b5032bd 100755
> --- a/t/t5551-http-fetch.sh
> +++ b/t/t5551-http-fetch.sh
> @@ -162,6 +162,28 @@ test_expect_success 'invalid Content-Type rejected' '
>  	grep "not valid:" actual
>  '
>  
> +test_expect_success 'create namespaced refs' '
> +	test_commit namespaced &&
> +	git push public HEAD:refs/namespaces/ns/refs/heads/master
> +'
> +
> +test_expect_success 'smart clone respects namespace' '
> +	git clone --bare "$HTTPD_URL/smart_namespace/repo.git" ns-smart.git &&
> +	echo namespaced >expect &&
> +	git --git-dir=ns-smart.git log -1 --format=%s >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'dumb clone via http-backend respects namespace' '
> +	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
> +		config http.getanyfile true &&
> +	GIT_SMART_HTTP=0 git clone --bare \
> +		"$HTTPD_URL/smart_namespace/repo.git" ns-dumb.git &&
> +	echo namespaced >expect &&
> +	git --git-dir=ns-dumb.git log -1 --format=%s >actual &&
> +	test_cmp expect actual
> +'
> +
>  test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
>  
>  test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
John Koleszar· Apr 4, 2013, 15:34 UTC · re: Junio C Hamano · lore

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

On Wed, Apr 3, 2013 at 11:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
> Jeff King <peff@peff.net> writes:
>
>> On Wed, Apr 03, 2013 at 12:10:38PM -0400, Jeff King wrote:
>>
>>> Hmm. This is testing just the ref advertisement. It would be nice to see
>>> a complete transaction tested with namespaces turned on. Something like
>>> this (squashed into your patch) seems to work for me:
>>
>> Actually, I guess the point of your patch was to fix the
>> dumb-via-http-backend transport. So this would be more complete:
>
> Yeah, sounds sensible to me.
>
Agreed, this is much more thorough. Thanks!
I'll fold this in with Junio's comments and have another patch ready in a bit.
John
John Koleszar· Apr 4, 2013, 16:01 UTC · re: John Koleszar · lore

Filter the list of refs returned via the dumb HTTP protocol according to the active namespace, consistent with other clients of the upload-pack service.

Signed-off-by: John Koleszar <jkoleszar@google.com>
---
Incorporate peff's suggested test, and Junio's comments. With regard to
whether it's sufficient to test for the presence of /ns/ without
testing the contents further, I think it is. The test uses ls-remote
as the gold standard, and the test verifies that what it gets back from
the http-backend matches what ls-remote sees on the local filesystem
without the http-backend, so I think we're covered under whatever tests
exist for ls-remote and upload-pack, in addition to the new tests peff
suggested. If you want more though, let me know.
 
http-backend.c                   |  9 ++++++---
 t/lib-httpd/apache.conf          |  5 +++++
 t/t5551-http-fetch.sh            | 22 ++++++++++++++++++++++
 t/t5560-http-backend-noserver.sh |  7 +++++++
 t/t5561-http-backend.sh          | 11 +++++++++++
 t/t556x_common                   | 28 ++++++++++++++++++++++++++++
 6 files changed, 79 insertions(+), 3 deletions(-)
Show changes to 6 files +79 −3

http-backend.c, t/lib-httpd/apache.conf, t/t5551-http-fetch.sh, t/t5560-http-backend-noserver.sh, t/t5561-http-backend.sh, t/t556x_common

diff --git a/http-backend.c b/http-backend.c
index f50e77f..d32128f 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -361,17 +361,19 @@ static void run_service(const char **argv)
 static int show_text_ref(const char *name, const unsigned char *sha1,
 	int flag, void *cb_data)
 {
+	const char *name_nons = strip_namespace(name);
 	struct strbuf *buf = cb_data;
 	struct object *o = parse_object(sha1);
 	if (!o)
 		return 0;
 
-	strbuf_addf(buf, "%s\t%s\n", sha1_to_hex(sha1), name);
+	strbuf_addf(buf, "%s\t%s\n", sha1_to_hex(sha1), name_nons);
 	if (o->type == OBJ_TAG) {
 		o = deref_tag(o, name, 0);
 		if (!o)
 			return 0;
-		strbuf_addf(buf, "%s\t%s^{}\n", sha1_to_hex(o->sha1), name);
+		strbuf_addf(buf, "%s\t%s^{}\n", sha1_to_hex(o->sha1),
+		            name_nons);
 	}
 	return 0;
 }
@@ -402,7 +404,8 @@ static void get_info_refs(char *arg)
 
 	} else {
 		select_getanyfile();
-		for_each_ref(show_text_ref, &buf);
+		head_ref_namespaced(show_text_ref, &buf);
+		for_each_namespaced_ref(show_text_ref, &buf);
 		send_strbuf("text/plain", &buf);
 	}
 	strbuf_release(&buf);
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 938b4cf..ad85537 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -61,6 +61,11 @@ Alias /auth/dumb/ www/auth/dumb/
 	SetEnv GIT_COMMITTER_NAME "Custom User"
 	SetEnv GIT_COMMITTER_EMAIL custom@example.com
 </LocationMatch>
+<LocationMatch /smart_namespace/>
+	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+	SetEnv GIT_HTTP_EXPORT_ALL
+	SetEnv GIT_NAMESPACE ns
+</LocationMatch>
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
 <Directory ${GIT_EXEC_PATH}>
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index 47eb769..b5032bd 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -162,6 +162,28 @@ test_expect_success 'invalid Content-Type rejected' '
 	grep "not valid:" actual
 '
 
+test_expect_success 'create namespaced refs' '
+	test_commit namespaced &&
+	git push public HEAD:refs/namespaces/ns/refs/heads/master
+'
+
+test_expect_success 'smart clone respects namespace' '
+	git clone --bare "$HTTPD_URL/smart_namespace/repo.git" ns-smart.git &&
+	echo namespaced >expect &&
+	git --git-dir=ns-smart.git log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'dumb clone via http-backend respects namespace' '
+	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
+		config http.getanyfile true &&
+	GIT_SMART_HTTP=0 git clone --bare \
+		"$HTTPD_URL/smart_namespace/repo.git" ns-dumb.git &&
+	echo namespaced >expect &&
+	git --git-dir=ns-dumb.git log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+
 test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
 
 test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index ef98d95..85a5625 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh
@@ -26,6 +26,13 @@ GET() {
 	test_cmp exp act
 }
 
+GET_BODY() {
+	REQUEST_METHOD="GET" && export REQUEST_METHOD &&
+	run_backend "/repo.git/$1" &&
+	sane_unset REQUEST_METHOD &&
+	tr '\015' Q <act.out | sed '1,/^Q$/d'
+}
+
 POST() {
 	REQUEST_METHOD="POST" && export REQUEST_METHOD &&
 	CONTENT_TYPE="application/x-$1-request" && export CONTENT_TYPE &&
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..1ce2429 100755
--- a/t/t556x_common
+++ b/t/t556x_common
@@ -120,3 +120,31 @@ 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' '(
+	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 >expect &&  
+	grep /$NS/ expect &&
+	GET_BODY "info/refs" >actual &&
+	test_cmp expect actual &&
+	GET_BODY "info/refs?service=git-upload-pack" >actual &&
+	grep /$NS/ actual &&
+
+	SMART=smart_namespace &&
+	GIT_NAMESPACE=$NS && export GIT_NAMESPACE &&
+	git ls-remote public >expect &&  
+	! grep /$NS/ expect &&
+	GET_BODY "info/refs" >actual &&
+	test_cmp expect actual &&
+	GET_BODY "info/refs?service=git-upload-pack" >actual &&
+	! grep /$NS/ actual
+)'
-- 
1.8.1.3
Junio C Hamano· Apr 4, 2013, 17:25 UTC · re: John Koleszar · lore

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

John Koleszar <jkoleszar@google.com> writes:
Show 9 quoted lines
> @@ -402,7 +404,8 @@ static void get_info_refs(char *arg)
>  
>  	} else {
>  		select_getanyfile();
> -		for_each_ref(show_text_ref, &buf);
> +		head_ref_namespaced(show_text_ref, &buf);
> +		for_each_namespaced_ref(show_text_ref, &buf);
>  		send_strbuf("text/plain", &buf);
>  	}

Whether we are namespaced or not, we used to do for_each_ref() here, not advertising the HEAD (outside refs/ hierarchy), but we now do, and as the first element in the output.

Am I reading the patch correctly?

Is that an unrelated but useful bugfix even for people who do not use server namespaces?

Show 15 quoted lines
> diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
> index ef98d95..85a5625 100755
> --- a/t/t5560-http-backend-noserver.sh
> +++ b/t/t5560-http-backend-noserver.sh
> @@ -26,6 +26,13 @@ GET() {
>  	test_cmp exp act
>  }
>  
> +GET_BODY() {
> +	REQUEST_METHOD="GET" && export REQUEST_METHOD &&
> +	run_backend "/repo.git/$1" &&
> +	sane_unset REQUEST_METHOD &&
> +	tr '\015' Q <act.out | sed '1,/^Q$/d'
> +}
> +

These "export/unset" in &&-chains will allow a failing test to affect the next test, but that is not a new problem (existing POST already has that problem). Just highlighting, so that interested people may notice and want to clean it up on top of this patch.

>  POST() {
>  	REQUEST_METHOD="POST" && export REQUEST_METHOD &&
>  	CONTENT_TYPE="application/x-$1-request" && export CONTENT_TYPE &&
Thanks, will queue.
John Koleszar· Apr 5, 2013, 01:22 UTC · re: Junio C Hamano · lore

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

On Thu, Apr 4, 2013 at 10:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 21 quoted lines
> John Koleszar <jkoleszar@google.com> writes:
>
>> @@ -402,7 +404,8 @@ static void get_info_refs(char *arg)
>>
>>       } else {
>>               select_getanyfile();
>> -             for_each_ref(show_text_ref, &buf);
>> +             head_ref_namespaced(show_text_ref, &buf);
>> +             for_each_namespaced_ref(show_text_ref, &buf);
>>               send_strbuf("text/plain", &buf);
>>       }
>
> Whether we are namespaced or not, we used to do for_each_ref() here,
> not advertising the HEAD (outside refs/ hierarchy), but we now do,
> and as the first element in the output.
>
> Am I reading the patch correctly?
>
> Is that an unrelated but useful bugfix even for people who do not
> use server namespaces?
>

Actually, I think this line may be buggy. Hold off submitting if you haven't already.

Including the HEAD ref in the advertisement from /info/refs ends up duplicating it, since the dumb client unconditionally fetches the file /HEAD to use as the that ref. I think the right thing to do is generate the correct /HEAD using head_ref_namespaced(), rather than returning the bare file $GIT_DIR/HEAD, but I'm not 100% sure how HEAD and namespaces interact, since I haven't been able to produce a repo with a different HEAD in a namespace. Can you verify this approach?

$ GIT_SMART_HTTP=0 ./git ls-remote http://localhost:8080/ | grep HEAD bd9cd9a1859aa464b3092f2023b3a4040166572d HEAD bd9cd9a1859aa464b3092f2023b3a4040166572d HEAD

Generates these requests (ignore the errors): 2013/04/04 18:18:49 /info/refs 2013/04/04 18:18:49 http: invalid Content-Length of "3285\r\n" sent 2013/04/04 18:18:49 /HEAD 2013/04/04 18:18:49 http: invalid Content-Length of "41\r\n" sent

I didn't catch this before, since the smart protocol includes HEAD in its response, and I was trying to make the two match.

Josh Triplett· Apr 5, 2013, 02:35 UTC · re: John Koleszar · lore

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

On Thu, Apr 04, 2013 at 06:22:08PM -0700, John Koleszar wrote:
Show 33 quoted lines
> On Thu, Apr 4, 2013 at 10:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > John Koleszar <jkoleszar@google.com> writes:
> >
> >> @@ -402,7 +404,8 @@ static void get_info_refs(char *arg)
> >>
> >>       } else {
> >>               select_getanyfile();
> >> -             for_each_ref(show_text_ref, &buf);
> >> +             head_ref_namespaced(show_text_ref, &buf);
> >> +             for_each_namespaced_ref(show_text_ref, &buf);
> >>               send_strbuf("text/plain", &buf);
> >>       }
> >
> > Whether we are namespaced or not, we used to do for_each_ref() here,
> > not advertising the HEAD (outside refs/ hierarchy), but we now do,
> > and as the first element in the output.
> >
> > Am I reading the patch correctly?
> >
> > Is that an unrelated but useful bugfix even for people who do not
> > use server namespaces?
> >
> 
> Actually, I think this line may be buggy. Hold off submitting if you
> haven't already.
> 
> Including the HEAD ref in the advertisement from /info/refs ends up
> duplicating it, since the dumb client unconditionally fetches the file
> /HEAD to use as the that ref. I think the right thing to do is
> generate the correct /HEAD using head_ref_namespaced(), rather than
> returning the bare file $GIT_DIR/HEAD, but I'm not 100% sure how HEAD
> and namespaces interact, since I haven't been able to produce a repo
> with a different HEAD in a namespace. Can you verify this approach?

Semantically, every namespace should act like a completely independent repository, which includes having its own independent HEAD. A namespace should *not* see the HEAD of the entire repository, only its own namespaced HEAD.

Namespaces exist so that you can make a pile of repos share the same object store while acting as independent repositories. As long as you never expose the un-namespaced repository, a client should not be able to tell whether you use namespaces.

- Josh Triplett
Jeff King· Apr 5, 2013, 02:56 UTC · re: Josh Triplett · lore

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

On Thu, Apr 04, 2013 at 07:35:16PM -0700, Josh Triplett wrote:
Show 12 quoted lines
> > Including the HEAD ref in the advertisement from /info/refs ends up
> > duplicating it, since the dumb client unconditionally fetches the file
> > /HEAD to use as the that ref. I think the right thing to do is
> > generate the correct /HEAD using head_ref_namespaced(), rather than
> > returning the bare file $GIT_DIR/HEAD, but I'm not 100% sure how HEAD
> > and namespaces interact, since I haven't been able to produce a repo
> > with a different HEAD in a namespace. Can you verify this approach?
> 
> Semantically, every namespace should act like a completely independent
> repository, which includes having its own independent HEAD.  A namespace
> should *not* see the HEAD of the entire repository, only its own
> namespaced HEAD.

Yeah, that makes sense. I think we'd want something like the (totally untested) patch below. And the tests I provided for t5551 should be amended to set up a HEAD within the namespace, should make the resulting clone non-bare, and should confirm that we check out the correct HEAD.

Show changes to http-backend.c +9 −1
diff --git a/http-backend.c b/http-backend.c
index 8144f3a..84ba7f9 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -376,6 +376,14 @@ static int show_text_ref(const char *name, const unsigned char *sha1,
 	return 0;
 }
 
+static void get_head(char *arg)
+{
+	struct strbuf buf = STRBUF_INIT;
+	head_ref_namespaced(show_text_ref, &buf);
+	send_strbuf("text/plain", &buf);
+	strbuf_release(&buf);
+}
+
 static void get_info_refs(char *arg)
 {
 	const char *service_name = get_parameter("service");
@@ -520,7 +528,7 @@ static struct service_cmd {
 	const char *pattern;
 	void (*imp)(char *);
 } services[] = {
-	{"GET", "/HEAD$", get_text_file},
+	{"GET", "/HEAD$", get_head },
 	{"GET", "/info/refs$", get_info_refs},
 	{"GET", "/objects/info/alternates$", get_text_file},
 	{"GET", "/objects/info/http-alternates$", get_text_file},
Junio C Hamano· Apr 5, 2013, 05:34 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 20 quoted lines
> Yeah, that makes sense. I think we'd want something like the (totally
> untested) patch below. And the tests I provided for t5551 should be
> amended to set up a HEAD within the namespace, should make the resulting
> clone non-bare, and should confirm that we check out the correct HEAD.
>
> diff --git a/http-backend.c b/http-backend.c
> index 8144f3a..84ba7f9 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -376,6 +376,14 @@ static int show_text_ref(const char *name, const unsigned char *sha1,
>  	return 0;
>  }
>  
> +static void get_head(char *arg)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	head_ref_namespaced(show_text_ref, &buf);
> +	send_strbuf("text/plain", &buf);
> +	strbuf_release(&buf);
> +}

You identified the right place to patch, but I think we need a bit more than this.

The show_text_ref() function gives "SHA-1 <TAB> refname". It is likely that the dumb client will ignore the trailing part of that output, but let's avoid a hack that we would not want see other implementations imitate.

One advantage dumb clients has over smart ones is that they can read HEAD that is a textual symref from a dumb server and learn which branch is the default one (remote.c::guess_remote_head()) without guessing. I think this function should:

 - Turn "HEAD" into a namespaced equivalent;
 - Run resolve_ref() on the result of the above;
 - Is it a symbolic ref?
   . If it is, then format "ref: <target>\n" into a strbuf and send
     it (make sure <target> is without the namespace prefix);
   . Otherwise, HEAD is detached. Prepare "%s\n" % sha1_to_hex(sha1),
     and send it.
Jeff King· Apr 5, 2013, 05:43 UTC · re: Junio C Hamano · lore

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

On Thu, Apr 04, 2013 at 10:34:49PM -0700, Junio C Hamano wrote:
Show 15 quoted lines
> > +static void get_head(char *arg)
> > +{
> > +	struct strbuf buf = STRBUF_INIT;
> > +	head_ref_namespaced(show_text_ref, &buf);
> > +	send_strbuf("text/plain", &buf);
> > +	strbuf_release(&buf);
> > +}
> 
> You identified the right place to patch, but I think we need a bit
> more than this.
> 
> The show_text_ref() function gives "SHA-1 <TAB> refname". It is
> likely that the dumb client will ignore the trailing part of that
> output, but let's avoid a hack that we would not want see other
> implementations imitate.

Oh, right. I was thinking too much about normal clients which see HEAD in the ref advertisement; of course the dumb client is expecting to see the actual HEAD file.

Show 16 quoted lines
> One advantage dumb clients has over smart ones is that they can read
> HEAD that is a textual symref from a dumb server and learn which
> branch is the default one (remote.c::guess_remote_head()) without
> guessing.  I think this function should:
> 
>  - Turn "HEAD" into a namespaced equivalent;
> 
>  - Run resolve_ref() on the result of the above;
> 
>  - Is it a symbolic ref?
> 
>    . If it is, then format "ref: <target>\n" into a strbuf and send
>      it (make sure <target> is without the namespace prefix);
> 
>    . Otherwise, HEAD is detached. Prepare "%s\n" % sha1_to_hex(sha1),
>      and send it.

Yes, that sounds right; it is basically just reconstructing a HEAD file. What do the HEADs inside namespaces look like? Do they refer to full global refs, or do they refer to refs within the namespace?

If the latter, we could just send the HEAD file directly. But I suspect it is the former, so that they can function when non-namespaced commands are used.

-Peff
John Koleszar· Apr 6, 2013, 00:54 UTC · re: Jeff King · lore

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

On Thu, Apr 4, 2013 at 10:43 PM, Jeff King <peff@peff.net> wrote:
Show 48 quoted lines
>
> On Thu, Apr 04, 2013 at 10:34:49PM -0700, Junio C Hamano wrote:
>
> > > +static void get_head(char *arg)
> > > +{
> > > +   struct strbuf buf = STRBUF_INIT;
> > > +   head_ref_namespaced(show_text_ref, &buf);
> > > +   send_strbuf("text/plain", &buf);
> > > +   strbuf_release(&buf);
> > > +}
> >
> > You identified the right place to patch, but I think we need a bit
> > more than this.
> >
> > The show_text_ref() function gives "SHA-1 <TAB> refname". It is
> > likely that the dumb client will ignore the trailing part of that
> > output, but let's avoid a hack that we would not want see other
> > implementations imitate.
>
> Oh, right. I was thinking too much about normal clients which see HEAD
> in the ref advertisement; of course the dumb client is expecting to see
> the actual HEAD file.
>
> > One advantage dumb clients has over smart ones is that they can read
> > HEAD that is a textual symref from a dumb server and learn which
> > branch is the default one (remote.c::guess_remote_head()) without
> > guessing.  I think this function should:
> >
> >  - Turn "HEAD" into a namespaced equivalent;
> >
> >  - Run resolve_ref() on the result of the above;
> >
> >  - Is it a symbolic ref?
> >
> >    . If it is, then format "ref: <target>\n" into a strbuf and send
> >      it (make sure <target> is without the namespace prefix);
> >
> >    . Otherwise, HEAD is detached. Prepare "%s\n" % sha1_to_hex(sha1),
> >      and send it.
>
> Yes, that sounds right; it is basically just reconstructing a HEAD
> file. What do the HEADs inside namespaces look like? Do they refer to
> full global refs, or do they refer to refs within the namespace?
>
> If the latter, we could just send the HEAD file directly. But I suspect
> it is the former, so that they can function when non-namespaced commands
> are used.
>

Here's a quick cut at this. Seems to work ok in local testing, I haven't updated the test suite yet. If the namespaced HEAD is a symbolic ref, its target must have the namespace prefix applied, or the resolved ref will be from outside the namespace (eg refs/heads/master vs refs/namespace/ns/refs/heads/master). This seems to be handled at write time, not sure if we need to do more verification here or not.

Show changes to http-backend.c +28 −1
diff --git a/http-backend.c b/http-backend.c
index d32128f..da4482c 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -404,13 +404,40 @@ static void get_info_refs(char *arg)

        } else {
                select_getanyfile();
-               head_ref_namespaced(show_text_ref, &buf);
                for_each_namespaced_ref(show_text_ref, &buf);
                send_strbuf("text/plain", &buf);
        }
        strbuf_release(&buf);
 }

+static int show_head_ref(const char *name, const unsigned char *sha1,
+       int flag, void *cb_data)
+{
+       struct strbuf *buf = cb_data;
+
+       if (flag & REF_ISSYMREF) {
+               unsigned char sha1[20];
+               const char *target = resolve_ref_unsafe(name, sha1, 1, NULL);
+               const char *target_nons = strip_namespace(target);
+
+               strbuf_addf(buf, "ref: %s\n", target_nons);
+       } else {
+               strbuf_addf(buf, "%s\n", sha1_to_hex(sha1));
+       }
+
+       return 0;
+}
+
+static void get_head(char *arg)
+{
+       struct strbuf buf = STRBUF_INIT;
+
+       select_getanyfile();
+       head_ref_namespaced(show_head_ref, &buf);
+       send_strbuf("text/plain", &buf);
+       strbuf_release(&buf);
+}
+
 static void get_info_packs(char *arg)
 {
        size_t objdirlen = strlen(get_object_directory());
John Koleszar· Apr 10, 2013, 00:55 UTC · re: John Koleszar · lore

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

Filter the list of refs returned via the dumb HTTP protocol according to the active namespace, consistent with other clients of the upload-pack service.

Signed-off-by: John Koleszar <jkoleszar@google.com>
---
Updates to generate HEAD. Drops my original tests, since they were under the
flawed assumption that both the dumb and smart protocols produced the same
ref advertisement at /info/refs.
 http-backend.c          |   38 ++++++++++++++++++++++++++++++++++----
 t/lib-httpd/apache.conf |    5 +++++
 t/t5551-http-fetch.sh   |   24 ++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 4 deletions(-)
Show changes to 3 files +63 −4

http-backend.c, t/lib-httpd/apache.conf, t/t5551-http-fetch.sh

diff --git a/http-backend.c b/http-backend.c
index f50e77f..4f35a31 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -361,17 +361,19 @@ static void run_service(const char **argv)
 static int show_text_ref(const char *name, const unsigned char *sha1,
 	int flag, void *cb_data)
 {
+	const char *name_nons = strip_namespace(name);
 	struct strbuf *buf = cb_data;
 	struct object *o = parse_object(sha1);
 	if (!o)
 		return 0;
 
-	strbuf_addf(buf, "%s\t%s\n", sha1_to_hex(sha1), name);
+	strbuf_addf(buf, "%s\t%s\n", sha1_to_hex(sha1), name_nons);
 	if (o->type == OBJ_TAG) {
 		o = deref_tag(o, name, 0);
 		if (!o)
 			return 0;
-		strbuf_addf(buf, "%s\t%s^{}\n", sha1_to_hex(o->sha1), name);
+		strbuf_addf(buf, "%s\t%s^{}\n", sha1_to_hex(o->sha1),
+		            name_nons);
 	}
 	return 0;
 }
@@ -402,12 +404,40 @@ static void get_info_refs(char *arg)
 
 	} else {
 		select_getanyfile();
-		for_each_ref(show_text_ref, &buf);
+		for_each_namespaced_ref(show_text_ref, &buf);
 		send_strbuf("text/plain", &buf);
 	}
 	strbuf_release(&buf);
 }
 
+static int show_head_ref(const char *name, const unsigned char *sha1,
+	int flag, void *cb_data)
+{
+	struct strbuf *buf = cb_data;
+
+	if (flag & REF_ISSYMREF) {
+		unsigned char sha1[20];
+		const char *target = resolve_ref_unsafe(name, sha1, 1, NULL);
+		const char *target_nons = strip_namespace(target);
+
+		strbuf_addf(buf, "ref: %s\n", target_nons);
+	} else {
+		strbuf_addf(buf, "%s\n", sha1_to_hex(sha1));
+	}
+
+	return 0;
+}
+
+static void get_head(char *arg)
+{
+	struct strbuf buf = STRBUF_INIT;
+
+	select_getanyfile();
+	head_ref_namespaced(show_head_ref, &buf);
+	send_strbuf("text/plain", &buf);
+	strbuf_release(&buf);
+}
+
 static void get_info_packs(char *arg)
 {
 	size_t objdirlen = strlen(get_object_directory());
@@ -520,7 +550,7 @@ static struct service_cmd {
 	const char *pattern;
 	void (*imp)(char *);
 } services[] = {
-	{"GET", "/HEAD$", get_text_file},
+	{"GET", "/HEAD$", get_head},
 	{"GET", "/info/refs$", get_info_refs},
 	{"GET", "/objects/info/alternates$", get_text_file},
 	{"GET", "/objects/info/http-alternates$", get_text_file},
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 938b4cf..ad85537 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -61,6 +61,11 @@ Alias /auth/dumb/ www/auth/dumb/
 	SetEnv GIT_COMMITTER_NAME "Custom User"
 	SetEnv GIT_COMMITTER_EMAIL custom@example.com
 </LocationMatch>
+<LocationMatch /smart_namespace/>
+	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+	SetEnv GIT_HTTP_EXPORT_ALL
+	SetEnv GIT_NAMESPACE ns
+</LocationMatch>
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
 <Directory ${GIT_EXEC_PATH}>
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index 47eb769..b31019f 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -162,6 +162,30 @@ test_expect_success 'invalid Content-Type rejected' '
 	grep "not valid:" actual
 '
 
+test_expect_success 'create namespaced refs' '
+	test_commit namespaced &&
+	git push public HEAD:refs/namespaces/ns/refs/heads/master &&
+	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
+		symbolic-ref refs/namespaces/ns/HEAD refs/namespaces/ns/refs/heads/master
+'
+
+test_expect_success 'smart clone respects namespace' '
+	git clone "$HTTPD_URL/smart_namespace/repo.git" ns-smart &&
+	echo namespaced >expect &&
+	git --git-dir=ns-smart/.git log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'dumb clone via http-backend respects namespace' '
+	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
+		config http.getanyfile true &&
+	GIT_SMART_HTTP=0 git clone \
+		"$HTTPD_URL/smart_namespace/repo.git" ns-dumb &&
+	echo namespaced >expect &&
+	git --git-dir=ns-dumb/.git log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+
 test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
 
 test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
-- 
1.7.9.6 (Apple Git-31.1)
Jeff King· Apr 10, 2013, 04:18 UTC · re: John Koleszar · lore

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

On Tue, Apr 09, 2013 at 05:55:08PM -0700, John Koleszar wrote:
> Filter the list of refs returned via the dumb HTTP protocol according
> to the active namespace, consistent with other clients of the
> upload-pack service.
Thanks, this version looks good to me.
> Updates to generate HEAD. Drops my original tests, since they were under the
> flawed assumption that both the dumb and smart protocols produced the same
> ref advertisement at /info/refs.
Yeah, your new tests look good, and I think exercise the feature well.
-Peff
Thomas Rast· Apr 8, 2013, 21:25 UTC · re: John Koleszar · lore

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

John Koleszar <jkoleszar@google.com> writes:
Show 5 quoted lines
> Filter the list of refs returned via the dumb HTTP protocol according
> to the active namespace, consistent with other clients of the
> upload-pack service.
>
> Signed-off-by: John Koleszar <jkoleszar@google.com>

At the risk of repeating something that's been said already -- I only skimmed the thread -- this test breaks in today's pu on my machine. I get:

expecting success: (
	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 >expect &&
	grep /$NS/ expect &&
	GET_BODY "info/refs" >actual &&
	test_cmp expect actual &&
	GET_BODY "info/refs?service=git-upload-pack" >actual &&
	grep /$NS/ actual &&
	SMART=smart_namespace &&
	GIT_NAMESPACE=$NS && export GIT_NAMESPACE &&
	git ls-remote public >expect &&
	! grep /$NS/ expect &&
	GET_BODY "info/refs" >actual &&
	test_cmp expect actual &&
	GET_BODY "info/refs?service=git-upload-pack" >actual &&
	! grep /$NS/ actual
)
453190505bf07f7513bed9839da875eb3610f807	refs/namespaces/ns/refs/heads/master
--- expect	2013-04-08 21:24:36.571874540 +0000
+++ actual	2013-04-08 21:24:36.579874619 +0000
Show changes to diff +0 −1
@@ -1,3 +1,2 @@
-453190505bf07f7513bed9839da875eb3610f807	HEAD
 453190505bf07f7513bed9839da875eb3610f807	refs/heads/master
 453190505bf07f7513bed9839da875eb3610f807	refs/namespaces/ns/refs/heads/master
not ok 14 - backend respects namespaces
-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Jeff King· Apr 8, 2013, 21:45 UTC · re: Thomas Rast · lore

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

On Mon, Apr 08, 2013 at 11:25:39PM +0200, Thomas Rast wrote:
Show 11 quoted lines
> At the risk of repeating something that's been said already -- I only
> skimmed the thread -- this test breaks in today's pu on my machine.  I
> get:
> [...]
> --- expect	2013-04-08 21:24:36.571874540 +0000
> +++ actual	2013-04-08 21:24:36.579874619 +0000
> @@ -1,3 +1,2 @@
> -453190505bf07f7513bed9839da875eb3610f807	HEAD
>  453190505bf07f7513bed9839da875eb3610f807	refs/heads/master
>  453190505bf07f7513bed9839da875eb3610f807	refs/namespaces/ns/refs/heads/master
> not ok 14 - backend respects namespaces

I think what is in pu is not yet reflecting the latest discussion. HEAD should not be included in the simulated info/refs, but should be generated, respecting namespaces, whenever a client retrieves the HEAD file directly.

It looks like the thread was left here;
  http://article.gmane.org/gmane.comp.version-control.git/220237
and we are just waiting for John's re-roll.
-Peff
John Koleszar· Apr 9, 2013, 22:13 UTC · re: Jeff King · lore

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

On Mon, Apr 8, 2013 at 2:45 PM, Jeff King <peff@peff.net> wrote:
Show 25 quoted lines
> On Mon, Apr 08, 2013 at 11:25:39PM +0200, Thomas Rast wrote:
>
>> At the risk of repeating something that's been said already -- I only
>> skimmed the thread -- this test breaks in today's pu on my machine.  I
>> get:
>> [...]
>> --- expect    2013-04-08 21:24:36.571874540 +0000
>> +++ actual    2013-04-08 21:24:36.579874619 +0000
>> @@ -1,3 +1,2 @@
>> -453190505bf07f7513bed9839da875eb3610f807     HEAD
>>  453190505bf07f7513bed9839da875eb3610f807     refs/heads/master
>>  453190505bf07f7513bed9839da875eb3610f807     refs/namespaces/ns/refs/heads/master
>> not ok 14 - backend respects namespaces
>
> I think what is in pu is not yet reflecting the latest discussion. HEAD
> should not be included in the simulated info/refs, but should be
> generated, respecting namespaces, whenever a client retrieves the HEAD
> file directly.
>
> It looks like the thread was left here;
>
>   http://article.gmane.org/gmane.comp.version-control.git/220237
>
> and we are just waiting for John's re-roll.
>

I should be able to get this done either later today or later this week. Life/$DAYJOB has been taking up the time slot I was spending on this the past few days.

John
Junio C Hamano· Apr 3, 2013, 18:04 UTC · re: John Koleszar · lore

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

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.

> +)'

← back to recent threads