[PATCH/RFC] upload-pack: Fail if cloning empty namespace

[PATCH/RFC] upload-pack: Fail if cloning empty namespace

[Johannes Löthberg]

Git should fail to clone if trying to clone from an non-existing
ref namespace, since it's the same as a non-existing repository

Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
---

In version 4 of the ArchLinux User Repository, which is a hosting 
platform for recepies for building Arch packages, we use Git to store 
the recepies.

To save space we are using ref namespaces, which so far has saved quite 
a bit of space. There is one issue though, when cloning a non-existing 
repository we don't want the clone to work. This patch fixes this issue 
by failing the clone if a namespace was specified but it doesn't exist.

Making this conditional on a namespace being specified makes sure that 
cloning regular empty repos still works.

 upload-pack.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/upload-pack.c b/upload-pack.c
index 89e832b..21f8891 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -778,6 +778,10 @@ static void upload_pack(void)
 
 	head_ref_namespaced(find_symref, &symref);
 
+	if (get_git_namespace() && !symref.items) {
+		die("git upload-pack: tried to clone from empty namespace");
+	}
+
 	if (advertise_refs || !stateless_rpc) {
 		reset_timeout();
 		head_ref_namespaced(send_ref, &symref);
-- 
2.4.2

Re: [PATCH/RFC] upload-pack: Fail if cloning empty namespace

[Junio C Hamano]

Johannes Löthberg <johannes@kyriasis.com> writes:

> +	if (get_git_namespace() && !symref.items) {
> +		die("git upload-pack: tried to clone from empty namespace");
> +	}

Is this sufficient?

get_git_namespace() returns environment.c::namespace, which is set
up in setup_git_env() by calling expand_namespace() and strlen()
is run on that value, so I would presume the function will *ALWAYS*
return true.  Even when not namespaced, you would get an empty
string "" whose address is not NULL, no?

[PATCH v2] upload-pack: Fail if cloning empty namespace

[Johannes Löthberg]

Git should fail to clone if trying to clone from an non-existing
ref namespace, since it's the same as a non-existing repository

Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
---

Changes since v1:

* Fixed the namespace check, since I apparently forgot to check with a
  bare repo in my last test. D'oh.

Two other options for this would be to either add a 
get_git_namespace_len() function and use that, or a is_namespaced() 
functon. But since it's only used here for now at least it feels simpler 
to not bloat the codabase with another function which has no other use.

 upload-pack.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/upload-pack.c b/upload-pack.c
index 89e832b..99fb271 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -778,6 +778,10 @@ static void upload_pack(void)
 
 	head_ref_namespaced(find_symref, &symref);
 
+	if (strcmp(get_git_namespace(), "") && !symref.items) {
+		die("git upload-pack: tried to clone from empty namespace");
+	}
+
 	if (advertise_refs || !stateless_rpc) {
 		reset_timeout();
 		head_ref_namespaced(send_ref, &symref);
-- 
2.4.2

Re: [PATCH v2] upload-pack: Fail if cloning empty namespace

[Johannes Löthberg]

[-- Attachment #1: Type: text/plain, Size: 970 bytes --]

On 13/06, Johannes Löthberg wrote:
>Git should fail to clone if trying to clone from an non-existing
>ref namespace, since it's the same as a non-existing repository
>
>Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
>---
>
>Changes since v1:
>
>* Fixed the namespace check, since I apparently forgot to check with a
>  bare repo in my last test. D'oh.
>
>Two other options for this would be to either add a
>get_git_namespace_len() function and use that, or a is_namespaced()
>functon. But since it's only used here for now at least it feels simpler
>to not bloat the codabase with another function which has no other use.
>

I should note that I have a small test script written now, ready to be 
converted into a git test, though I want some opinions on whether the 
patch would be merged before bothering to convert it.

-- 
Sincerely,
  Johannes Löthberg
  PGP Key ID: 0x50FB9B273A9D0BB5
  https://theos.kyriasis.com/~kyrias/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1495 bytes --]

Re: [PATCH v2] upload-pack: Fail if cloning empty namespace

[Junio C Hamano]

Johannes Löthberg <johannes@kyriasis.com> writes:

> On 13/06, Johannes Löthberg wrote:
>>Git should fail to clone if trying to clone from an non-existing
>>ref namespace, since it's the same as a non-existing repository
>>
>>Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
>>---
>>
>>Changes since v1:
>>
>>* Fixed the namespace check, since I apparently forgot to check with a
>>  bare repo in my last test. D'oh.
>>
>>Two other options for this would be to either add a
>>get_git_namespace_len() function and use that, or a is_namespaced()
>>functon. But since it's only used here for now at least it feels simpler
>>to not bloat the codabase with another function which has no other use.
>>
>
> I should note that I have a small test script written now, ready to be
> converted into a git test, though I want some opinions on whether the
> patch would be merged before bothering to convert it.

You would probably want new tests, but more importantly did you make
sure this passes existing tests?  It seems to break 5509 (there
could be others) at least for me.

Re: [PATCH v2] upload-pack: Fail if cloning empty namespace

[Johannes Löthberg]

[-- Attachment #1: Type: text/plain, Size: 891 bytes --]

On 15/06, Junio C Hamano wrote:
>You would probably want new tests, but more importantly did you make
>sure this passes existing tests?  It seems to break 5509 (there
>could be others) at least for me.
>

It breaks 5509 currently yeah, though I've already fixed it locally.

Anyway,

You create a namespace by pushing to it, and without a HEAD the 
namespace is not a proper repo, and the code won't deal with them 
properly.  (Which is why without that patch this patch is useless.)

So, if you don't want HEAD to be created when pushing to a ref 
namespace, (which seems to be what you're leaning toward, considering 
your last reply to the other thread,) then ref namespaces won't really 
work in most cases, and it seems pointless to support them at all.

-- 
Sincerely,
  Johannes Löthberg
  PGP Key ID: 0x50FB9B273A9D0BB5
  https://theos.kyriasis.com/~kyrias/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1495 bytes --]

Re: [PATCH v2] upload-pack: Fail if cloning empty namespace

[Junio C Hamano]

Johannes Löthberg <johannes@kyriasis.com> writes:

> On 15/06, Junio C Hamano wrote:
>>You would probably want new tests, but more importantly did you make
>>sure this passes existing tests?  It seems to break 5509 (there
>>could be others) at least for me.
>>
>
> It breaks 5509 currently yeah, though I've already fixed it locally.
>
> Anyway,
>
> You create a namespace by pushing to it,...

You keep repeating that, but I do not think we agreed that it is the
supported or correct procedure to set up a new namespace in the
first place.  Doesn't the server side need a lot more than just
setting up HEAD symref (like running upload/receive-pack with the
path to the hierarchy exported as GIT_NAMESPACE environment)?

Re: [PATCH v2] upload-pack: Fail if cloning empty namespace

[Johannes Löthberg]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]

On 20/06, Junio C Hamano wrote:
>Johannes Löthberg <johannes@kyriasis.com> writes:
>
>> On 15/06, Junio C Hamano wrote:
>>>You would probably want new tests, but more importantly did you make
>>>sure this passes existing tests?  It seems to break 5509 (there
>>>could be others) at least for me.
>>>
>>
>> It breaks 5509 currently yeah, though I've already fixed it locally.
>>
>> Anyway,
>>
>> You create a namespace by pushing to it,...
>
>You keep repeating that, but I do not think we agreed that it is the
>supported or correct procedure to set up a new namespace in the
>first place.

Whether it's 'correct' I can't say, but it's the only currently 
supported way.

>Doesn't the server side need a lot more than just
>setting up HEAD symref (like running upload/receive-pack with the
>path to the hierarchy exported as GIT_NAMESPACE environment)?

All it needs to do is use the refs under the ref namespace, and that it 
has already done for a long time. (Since 2011 in fact.)

-- 
Sincerely,
  Johannes Löthberg
  PGP Key ID: 0x50FB9B273A9D0BB5
  https://theos.kyriasis.com/~kyrias/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1495 bytes --]

Re: [PATCH v2] upload-pack: Fail if cloning empty namespace

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> You create a namespace by pushing to it,...
>
> You keep repeating that, but I do not think we agreed that it is the
> supported or correct procedure to set up a new namespace in the
> first place.  Doesn't the server side need a lot more than just
> setting up HEAD symref (like running upload/receive-pack with the
> path to the hierarchy exported as GIT_NAMESPACE environment)?

Let me step back and try again, as I think I am missing some
existing feature you are using, and that missing piece is preventing
me from seeing why this is a good idea (by the way, I do not think
we are doing this exchange in the right thread---the upload-pack
patch is not about auto-vivifying a new namespace by creating HEAD).

I'll do a brief braindump which hopefully shows the basis of why I
keep saying that creating HEAD at random place is not sufficient.
Help me, by telling me what I am missing, understand why it is
sufficient.

Suppose we have a server at URL git://site.xz/r; it serves from the
filesystem, say /usr/share/git/r.git/ (bare repository).  In that
directory, we have the usual HEAD, objects, refs/{heads,tags}, etc.
Further suppose we have there refs/namespaces/a/ under which HEAD,
refs/{heads,tags}, etc.

Then we run the server (say, git-daemon) with GIT_NAMESPACE
environment variable set to 'a'.  The users can now run:

 $ git clone git://site.xz/r

and they will see what's really under refs/namespaces/a/.  The HEAD
they see is actually not /usr/share/git/r.git/HEAD but it is
/usr/share/git/r.git/refs/namespaces/a/HEAD.  Everything outside
/usr/share/git/r.git/refs/namespaces/a is not visible to them.

In order to add a new namespace 'b' next to 'a', you would somehow
need to populate /usr/share/git/r.git/refs/namespaces/b.  Presumably
you can prepare a source repository locally and then push everything
there manually, by doing something like this:

 $ git push ssh://site.xz/usr/share/git/r.git refs/*:refs/namespaces/b/*

but we agree that this does not create refs/namespaces/b/HEAD.

But if we made some change to the transfer to push-to-receive-pack
so that this can also create HEAD that points at something, what
would the user say, instead of that earlier

 $ git clone git://site.xz/r

to access this new namespace?  Don't you have to be running another
instance of a server with GIT_NAMESPACE set to 'b'?  How do you
arrange that to automagically happen?

Or do your users that clone/fetch do things differnetly from the
above, and if so what do they do?

How is your server configured to support the access the existing
namespace 'a', and the new namespace 'b' that is automatically
created by pointing /usr/share/git/r.git/refs/namespaces/b/HEAD
to one ref inside that namespace?

Re: [PATCH v2] upload-pack: Fail if cloning empty namespace

[Johannes Löthberg]

[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]

On 20/06, Junio C Hamano wrote:
>Junio C Hamano <gitster@pobox.com> writes:
>Let me step back and try again, as I think I am missing some
>existing feature you are using, and that missing piece is preventing
>me from seeing why this is a good idea (by the way, I do not think
>we are doing this exchange in the right thread---the upload-pack
>patch is not about auto-vivifying a new namespace by creating HEAD).
>

Yeah, that's true.

>But if we made some change to the transfer to push-to-receive-pack
>so that this can also create HEAD that points at something, what
>would the user say, instead of that earlier
>
> $ git clone git://site.xz/r
>
>to access this new namespace?  Don't you have to be running another
>instance of a server with GIT_NAMESPACE set to 'b'?  How do you
>arrange that to automagically happen?
>
>Or do your users that clone/fetch do things differnetly from the
>above, and if so what do they do?
>
>How is your server configured to support the access the existing
>namespace 'a', and the new namespace 'b' that is automatically
>created by pointing /usr/share/git/r.git/refs/namespaces/b/HEAD
>to one ref inside that namespace?

If you want to hide the fact that you're using namespaces you'll 
probably have to either run another daemon or use a wrapper script, yes, 
but only if you want to hide it.

However, if you don't want to hide the namespace behind some script the 
users just have to use --namespace or GIT_NAMESPACE, possibly having a 
script that does some access-control magic so that you eg can only push 
to specific namespaces.

Having it be created is especially important when someone might be 
working on the same machine as the repo, where the push won't go through 
eg SSH.

-- 
Sincerely,
  Johannes Löthberg
  PGP Key ID: 0x50FB9B273A9D0BB5
  https://theos.kyriasis.com/~kyrias/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1495 bytes --]

Re: [PATCH v2] upload-pack: Fail if cloning empty namespace

[Junio C Hamano]

Johannes Löthberg <johannes@kyriasis.com> writes:

>>But if we made some change to the transfer to push-to-receive-pack
>>so that this can also create HEAD that points at something, what
>>would the user say, instead of that earlier
>>
>> $ git clone git://site.xz/r
>>
>>to access this new namespace?  Don't you have to be running another
>>instance of a server with GIT_NAMESPACE set to 'b'?  How do you
>>arrange that to automagically happen?
>>
>>Or do your users that clone/fetch do things differnetly from the
>>above, and if so what do they do?
>>
>>How is your server configured to support the access the existing
>>namespace 'a', and the new namespace 'b' that is automatically
>>created by pointing /usr/share/git/r.git/refs/namespaces/b/HEAD
>>to one ref inside that namespace?

Your answer to the above question marks are?

> If you want to hide the fact that you're using namespaces you'll
> probably have to either run another daemon or use a wrapper script,
> yes, but only if you want to hide it.

I would view it as segregating (not mixing refs and objects
reachable by them for 'a' and 'b') more than "hiding".  Either way,
your users' clone/fetch need a way to say which one of 'a' or 'b'
they want.  How are you arranging that to happen when you add a new
namespace 'b'?

I asked you if my understanding of your workflow to start populating
a new namespace 'b' matches what you actually do in reality, but you
omitted it from your quote in your response, which was this:

>> In order to add a new namespace 'b' next to 'a', you would somehow
>> need to populate /usr/share/git/r.git/refs/namespaces/b.  Presumably
>> you can prepare a source repository locally and then push everything
>> there manually, by doing something like this:
>> 
>>  $ git push ssh://site.xz/usr/share/git/r.git refs/*:refs/namespaces/b/*
>> 
>> but we agree that this does not create refs/namespaces/b/HEAD.

I'd assume that the answer is "yes, Junio got the 'push' to populate
correctly" and continue.

Populating refs/namespaces/b/ and the objects reachable by them by
pushing into that hierarchy is one step of doing so, and setting
HEAD there is not done by "git push", but is that the only thing
missing?

I do not view "git fetch --namespace=b" or "git clone --namespace=b
git://site.xz/r" as a usable way to do so (is there an option like
that in the first place?), and that is what I have been asking.

Note that I am not opposed to an idea to make it possible to point a
symbolic ref .git/$something/HEAD at a ref that is different from
what it is pointing at currently (or creating it if it does not
exist yet) from outside the server, i.e. without ssh-ing in and
running "git symbolic ref".

That $something may be "refs/namespaces/b" in your case, but it does
not have to be.  We do not have a good way to support more common
case of repointing ".git/HEAD" to say "now the primary branch of
this repository is not 'master' but it is called 'trunk'".

That is one reason why I do not want a patch to receive-pack that
randomly picks one ref among all refs pushed and point HEAD at it,
which is the following part from your patch in the other thread
(where we should be having this discussion ;-).

@@ -981,6 +983,14 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 		return NULL; /* good */
 	}
 	else {
+		namespace = get_git_namespace();
+		if (strcmp(namespace, "refs/namespaces/")) {
+			strbuf_addf(&namespaced_head_buf, "%s%s", namespace, "HEAD");
+			namespaced_head_path = strbuf_detach(&namespaced_head_buf, NULL);
+
+			create_symref(namespaced_head_path, namespaced_name, NULL);
+		}
+
 		struct strbuf err = STRBUF_INIT;
 		if (shallow_update && si->shallow_ref[cmd->index] &&
 		    update_shallow_ref(cmd, si))

This is part of update() function in receive-pack, which is called
for each and every ref that is pushed.  If you did the "initial
population push" I cited from earlier part of the discussion:

 $ git push ssh://site.xz/usr/share/git/r.git refs/*:refs/namespaces/b/*

the above would see updates to refs/namespaces/b/refs/heads/master,
refs/namespaces/b/refs/heads/next, refs/namespaces/b/refs/tags/v1.0,
etc., i.e. all the refs you are pushing in an unspecified order, and
the added code would run once for each and every one of them.  Where
would the final HEAD points at is not something you can control, and
once you create one, you cannot repoint to a more reasonable ref.

If we introduce a way to point any symbolic ref at a ref that is
different from what it is pointing at currently (or creating it if
it does not exist yet) from outside the server, i.e. without ssh-ing
in and running "git symbolic ref", then that would work equally well
for your use case, and also will solve longstanding "how do I change
the primary branch the server's HEAD points at?" problem.  I do not
think your patch is a good first step to introduce that solution.