git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
@ 2013-03-28  4:58 John Koleszar
  2013-03-28  6:02 ` Junio C Hamano
  2013-03-28 14:43 ` Junio C Hamano
  0 siblings, 2 replies; 26+ messages in thread
From: John Koleszar @ 2013-03-28  4:58 UTC (permalink / raw)
  To: git; +Cc: Shawn Pearce, John Koleszar

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(-)

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

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-03-28  4:58 [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients John Koleszar
@ 2013-03-28  6:02 ` Junio C Hamano
  2013-03-28 15:49   ` Josh Triplett
  2013-03-28 14:43 ` Junio C Hamano
  1 sibling, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2013-03-28  6:02 UTC (permalink / raw)
  To: Josh Triplett, John Koleszar; +Cc: git, Shawn Pearce

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?

>  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)
> +'

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-03-28  4:58 [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients John Koleszar
  2013-03-28  6:02 ` Junio C Hamano
@ 2013-03-28 14:43 ` Junio C Hamano
       [not found]   ` <CAAvHm8NAqVHLz1wjNN-3ocpYzWfO-PDo0PAJ6pZO7KrMkhJ6Jw@mail.gmail.com>
  1 sibling, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2013-03-28 14:43 UTC (permalink / raw)
  To: John Koleszar; +Cc: git, Shawn Pearce

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.

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-03-28  6:02 ` Junio C Hamano
@ 2013-03-28 15:49   ` Josh Triplett
  0 siblings, 0 replies; 26+ messages in thread
From: Josh Triplett @ 2013-03-28 15:49 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: John Koleszar, git, Shawn Pearce

On Wed, Mar 27, 2013 at 11:02:28PM -0700, Junio C Hamano wrote:
> 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.

> >  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)
> > +'

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
       [not found]   ` <CAAvHm8NAqVHLz1wjNN-3ocpYzWfO-PDo0PAJ6pZO7KrMkhJ6Jw@mail.gmail.com>
@ 2013-03-28 15:54     ` John Koleszar
  2013-03-28 16:46       ` Junio C Hamano
  2013-04-03 15:52       ` John Koleszar
  0 siblings, 2 replies; 26+ messages in thread
From: John Koleszar @ 2013-03-28 15:54 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Shawn Pearce

On Thu, Mar 28, 2013 at 8:52 AM, John Koleszar <jkoleszar@google.com> wrote:
> 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.
>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-03-28 15:54     ` John Koleszar
@ 2013-03-28 16:46       ` Junio C Hamano
  2013-04-03 15:52       ` John Koleszar
  1 sibling, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2013-03-28 16:46 UTC (permalink / raw)
  To: John Koleszar; +Cc: git, Shawn Pearce

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.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-03-28 15:54     ` John Koleszar
  2013-03-28 16:46       ` Junio C Hamano
@ 2013-04-03 15:52       ` John Koleszar
  2013-04-03 16:10         ` Jeff King
  2013-04-03 18:04         ` Junio C Hamano
  1 sibling, 2 replies; 26+ messages in thread
From: John Koleszar @ 2013-04-03 15:52 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Shawn Pearce, Josh Triplett, John Koleszar

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(-)

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

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-03 15:52       ` John Koleszar
@ 2013-04-03 16:10         ` Jeff King
  2013-04-03 16:16           ` Jeff King
  2013-04-03 18:04         ` Junio C Hamano
  1 sibling, 1 reply; 26+ messages in thread
From: Jeff King @ 2013-04-03 16:10 UTC (permalink / raw)
  To: John Koleszar; +Cc: Junio C Hamano, git, Shawn Pearce, Josh Triplett

On Wed, Apr 03, 2013 at 08:52:09AM -0700, John Koleszar wrote:

> +	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:

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

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-03 16:10         ` Jeff King
@ 2013-04-03 16:16           ` Jeff King
  2013-04-03 18:05             ` Junio C Hamano
  0 siblings, 1 reply; 26+ messages in thread
From: Jeff King @ 2013-04-03 16:16 UTC (permalink / raw)
  To: John Koleszar; +Cc: Junio C Hamano, git, Shawn Pearce, Josh Triplett

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:

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

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-03 15:52       ` John Koleszar
  2013-04-03 16:10         ` Jeff King
@ 2013-04-03 18:04         ` Junio C Hamano
  1 sibling, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2013-04-03 18:04 UTC (permalink / raw)
  To: John Koleszar; +Cc: git, Shawn Pearce, Josh Triplett, Jeff King

John Koleszar <jkoleszar@google.com> writes:

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


> +	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.

> +)'

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-03 16:16           ` Jeff King
@ 2013-04-03 18:05             ` Junio C Hamano
  2013-04-04 15:34               ` John Koleszar
  0 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2013-04-03 18:05 UTC (permalink / raw)
  To: Jeff King; +Cc: John Koleszar, git, Shawn Pearce, Josh Triplett

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.

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-03 18:05             ` Junio C Hamano
@ 2013-04-04 15:34               ` John Koleszar
  2013-04-04 16:01                 ` John Koleszar
  0 siblings, 1 reply; 26+ messages in thread
From: John Koleszar @ 2013-04-04 15:34 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git, Shawn Pearce, Josh Triplett

On Wed, Apr 3, 2013 at 11:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-04 15:34               ` John Koleszar
@ 2013-04-04 16:01                 ` John Koleszar
  2013-04-04 17:25                   ` Junio C Hamano
  2013-04-08 21:25                   ` [PATCH] " Thomas Rast
  0 siblings, 2 replies; 26+ messages in thread
From: John Koleszar @ 2013-04-04 16:01 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git, Shawn Pearce, Josh Triplett, John Koleszar

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(-)

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

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-04 16:01                 ` John Koleszar
@ 2013-04-04 17:25                   ` Junio C Hamano
  2013-04-05  1:22                     ` John Koleszar
  2013-04-08 21:25                   ` [PATCH] " Thomas Rast
  1 sibling, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2013-04-04 17:25 UTC (permalink / raw)
  To: John Koleszar; +Cc: Jeff King, git, Shawn Pearce, Josh Triplett

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?

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-04 17:25                   ` Junio C Hamano
@ 2013-04-05  1:22                     ` John Koleszar
  2013-04-05  2:35                       ` Josh Triplett
  0 siblings, 1 reply; 26+ messages in thread
From: John Koleszar @ 2013-04-05  1:22 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git, Shawn Pearce, Josh Triplett

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?

$ 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.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-05  1:22                     ` John Koleszar
@ 2013-04-05  2:35                       ` Josh Triplett
  2013-04-05  2:56                         ` Jeff King
  0 siblings, 1 reply; 26+ messages in thread
From: Josh Triplett @ 2013-04-05  2:35 UTC (permalink / raw)
  To: John Koleszar; +Cc: Junio C Hamano, Jeff King, git, Shawn Pearce

On Thu, Apr 04, 2013 at 06:22:08PM -0700, John Koleszar wrote:
> 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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-05  2:35                       ` Josh Triplett
@ 2013-04-05  2:56                         ` Jeff King
  2013-04-05  5:34                           ` Junio C Hamano
  0 siblings, 1 reply; 26+ messages in thread
From: Jeff King @ 2013-04-05  2:56 UTC (permalink / raw)
  To: Josh Triplett; +Cc: John Koleszar, Junio C Hamano, git, Shawn Pearce

On Thu, Apr 04, 2013 at 07:35:16PM -0700, Josh Triplett wrote:

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

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},

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-05  2:56                         ` Jeff King
@ 2013-04-05  5:34                           ` Junio C Hamano
  2013-04-05  5:43                             ` Jeff King
  0 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2013-04-05  5:34 UTC (permalink / raw)
  To: Jeff King; +Cc: Josh Triplett, John Koleszar, git, Shawn Pearce

Jeff King <peff@peff.net> writes:

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-05  5:34                           ` Junio C Hamano
@ 2013-04-05  5:43                             ` Jeff King
  2013-04-06  0:54                               ` John Koleszar
  0 siblings, 1 reply; 26+ messages in thread
From: Jeff King @ 2013-04-05  5:43 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Josh Triplett, John Koleszar, git, Shawn Pearce

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.

-Peff

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-05  5:43                             ` Jeff King
@ 2013-04-06  0:54                               ` John Koleszar
  2013-04-10  0:55                                 ` [PATCH v4] " John Koleszar
  0 siblings, 1 reply; 26+ messages in thread
From: John Koleszar @ 2013-04-06  0:54 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Josh Triplett, git, Shawn Pearce

On Thu, Apr 4, 2013 at 10:43 PM, Jeff King <peff@peff.net> wrote:
>
> 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.

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());

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-04 16:01                 ` John Koleszar
  2013-04-04 17:25                   ` Junio C Hamano
@ 2013-04-08 21:25                   ` Thomas Rast
  2013-04-08 21:45                     ` Jeff King
  1 sibling, 1 reply; 26+ messages in thread
From: Thomas Rast @ 2013-04-08 21:25 UTC (permalink / raw)
  To: John Koleszar; +Cc: Junio C Hamano, Jeff King, git, Shawn Pearce, Josh Triplett

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>

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-08 21:25                   ` [PATCH] " Thomas Rast
@ 2013-04-08 21:45                     ` Jeff King
  2013-04-09 22:13                       ` John Koleszar
  0 siblings, 1 reply; 26+ messages in thread
From: Jeff King @ 2013-04-08 21:45 UTC (permalink / raw)
  To: Thomas Rast
  Cc: John Koleszar, Junio C Hamano, git, Shawn Pearce, Josh Triplett

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.

-Peff

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-08 21:45                     ` Jeff King
@ 2013-04-09 22:13                       ` John Koleszar
  0 siblings, 0 replies; 26+ messages in thread
From: John Koleszar @ 2013-04-09 22:13 UTC (permalink / raw)
  To: Jeff King; +Cc: Thomas Rast, Junio C Hamano, git, Shawn Pearce, Josh Triplett

On Mon, Apr 8, 2013 at 2:45 PM, Jeff King <peff@peff.net> wrote:
> 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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v4] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-06  0:54                               ` John Koleszar
@ 2013-04-10  0:55                                 ` John Koleszar
  2013-04-10  1:09                                   ` Junio C Hamano
  2013-04-10  4:18                                   ` Jeff King
  0 siblings, 2 replies; 26+ messages in thread
From: John Koleszar @ 2013-04-10  0:55 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Josh Triplett, git, Shawn Pearce, Jeff King, John Koleszar

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(-)

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)

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH v4] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-10  0:55                                 ` [PATCH v4] " John Koleszar
@ 2013-04-10  1:09                                   ` Junio C Hamano
  2013-04-10  4:18                                   ` Jeff King
  1 sibling, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2013-04-10  1:09 UTC (permalink / raw)
  To: John Koleszar; +Cc: Josh Triplett, git, Shawn Pearce, Jeff King

Thanks; will replace the previous one that has been in 'pu'.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v4] http-backend: respect GIT_NAMESPACE with dumb clients
  2013-04-10  0:55                                 ` [PATCH v4] " John Koleszar
  2013-04-10  1:09                                   ` Junio C Hamano
@ 2013-04-10  4:18                                   ` Jeff King
  1 sibling, 0 replies; 26+ messages in thread
From: Jeff King @ 2013-04-10  4:18 UTC (permalink / raw)
  To: John Koleszar; +Cc: Junio C Hamano, Josh Triplett, git, Shawn Pearce

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2013-04-10  4:19 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-28  4:58 [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients John Koleszar
2013-03-28  6:02 ` Junio C Hamano
2013-03-28 15:49   ` Josh Triplett
2013-03-28 14:43 ` Junio C Hamano
     [not found]   ` <CAAvHm8NAqVHLz1wjNN-3ocpYzWfO-PDo0PAJ6pZO7KrMkhJ6Jw@mail.gmail.com>
2013-03-28 15:54     ` John Koleszar
2013-03-28 16:46       ` Junio C Hamano
2013-04-03 15:52       ` John Koleszar
2013-04-03 16:10         ` Jeff King
2013-04-03 16:16           ` Jeff King
2013-04-03 18:05             ` Junio C Hamano
2013-04-04 15:34               ` John Koleszar
2013-04-04 16:01                 ` John Koleszar
2013-04-04 17:25                   ` Junio C Hamano
2013-04-05  1:22                     ` John Koleszar
2013-04-05  2:35                       ` Josh Triplett
2013-04-05  2:56                         ` Jeff King
2013-04-05  5:34                           ` Junio C Hamano
2013-04-05  5:43                             ` Jeff King
2013-04-06  0:54                               ` John Koleszar
2013-04-10  0:55                                 ` [PATCH v4] " John Koleszar
2013-04-10  1:09                                   ` Junio C Hamano
2013-04-10  4:18                                   ` Jeff King
2013-04-08 21:25                   ` [PATCH] " Thomas Rast
2013-04-08 21:45                     ` Jeff King
2013-04-09 22:13                       ` John Koleszar
2013-04-03 18:04         ` Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).