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

John Koleszar [off-list ref] writes:

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?

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.

On Thu, Apr 4, 2013 at 10:25 AM, Junio C Hamano [off-list ref] wrote:

John Koleszar [off-list ref] writes:

quoted

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

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 [off-list ref] wrote:

quoted

John Koleszar [off-list ref] writes:

quoted

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

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

quoted

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