{"thread":{"id":"39615","subject":"[PATCH/RFC] upload-pack: Fail if cloning empty namespace","startedAt":"2015-06-12T20:15:15Z","lastAt":"2015-06-21T18:43:43Z","messageCount":11,"participants":["Johannes Löthberg","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"263693","messageId":"1434140115-3139-1-git-send-email-johannes@kyriasis.com","threadId":"39615","inReplyTo":null,"subject":"[PATCH/RFC] upload-pack: Fail if cloning empty namespace","fromName":"Johannes Löthberg","fromEmail":"johannes@kyriasis.com","sentAt":"2015-06-12T20:15:15Z","receivedAt":"2015-06-12T20:15:15Z","isPatch":true,"sender":{"key":"johannes@kyriasis.com","avatar":"https://gravatar.com/avatar/af2dea1b1759403329a1b7eaa28a07b5c5d1901e9f55723103d6324ffb7737ae?d=mp&s=160"},"body":"Git should fail to clone if trying to clone from an non-existing\nref namespace, since it's the same as a non-existing repository\n\nSigned-off-by: Johannes Löthberg \n---\n\nIn version 4 of the ArchLinux User Repository, which is a hosting \nplatform for recepies for building Arch packages, we use Git to store \nthe recepies.\n\nTo save space we are using ref namespaces, which so far has saved quite \na bit of space. There is one issue though, when cloning a non-existing \nrepository we don't want the clone to work. This patch fixes this issue \nby failing the clone if a namespace was specified but it doesn't exist.\n\nMaking this conditional on a namespace being specified makes sure that \ncloning regular empty repos still works.\n\n upload-pack.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 89e832b..21f8891 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -778,6 +778,10 @@ static void upload_pack(void)\n \n \thead_ref_namespaced(find_symref, &symref);\n \n+\tif (get_git_namespace() && !symref.items) {\n+\t\tdie(\"git upload-pack: tried to clone from empty namespace\");\n+\t}\n+\n \tif (advertise_refs || !stateless_rpc) {\n \t\treset_timeout();\n \t\thead_ref_namespaced(send_ref, &symref);\n-- \n2.4.2\n"},{"id":"263698","messageId":"xmqq616sd4k6.fsf@gitster.dls.corp.google.com","threadId":"39615","inReplyTo":"1434140115-3139-1-git-send-email-johannes@kyriasis.com","subject":"Re: [PATCH/RFC] upload-pack: Fail if cloning empty namespace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-12T21:01:29Z","receivedAt":"2015-06-12T21:01:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Löthberg writes:\n\n> +\tif (get_git_namespace() && !symref.items) {\n> +\t\tdie(\"git upload-pack: tried to clone from empty namespace\");\n> +\t}\n\nIs this sufficient?\n\nget_git_namespace() returns environment.c::namespace, which is set\nup in setup_git_env() by calling expand_namespace() and strlen()\nis run on that value, so I would presume the function will *ALWAYS*\nreturn true. Even when not namespaced, you would get an empty\nstring \"\" whose address is not NULL, no?\n"},{"id":"263712","messageId":"1434147436-21272-1-git-send-email-johannes@kyriasis.com","threadId":"39615","inReplyTo":"1434140115-3139-1-git-send-email-johannes@kyriasis.com","subject":"[PATCH v2] upload-pack: Fail if cloning empty namespace","fromName":"Johannes Löthberg","fromEmail":"johannes@kyriasis.com","sentAt":"2015-06-12T22:17:16Z","receivedAt":"2015-06-12T22:17:16Z","isPatch":true,"sender":{"key":"johannes@kyriasis.com","avatar":"https://gravatar.com/avatar/af2dea1b1759403329a1b7eaa28a07b5c5d1901e9f55723103d6324ffb7737ae?d=mp&s=160"},"body":"Git should fail to clone if trying to clone from an non-existing\nref namespace, since it's the same as a non-existing repository\n\nSigned-off-by: Johannes Löthberg \n---\n\nChanges since v1:\n\n* Fixed the namespace check, since I apparently forgot to check with a\n bare repo in my last test. D'oh.\n\nTwo other options for this would be to either add a \nget_git_namespace_len() function and use that, or a is_namespaced() \nfuncton. But since it's only used here for now at least it feels simpler \nto not bloat the codabase with another function which has no other use.\n\n upload-pack.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 89e832b..99fb271 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -778,6 +778,10 @@ static void upload_pack(void)\n \n \thead_ref_namespaced(find_symref, &symref);\n \n+\tif (strcmp(get_git_namespace(), \"\") && !symref.items) {\n+\t\tdie(\"git upload-pack: tried to clone from empty namespace\");\n+\t}\n+\n \tif (advertise_refs || !stateless_rpc) {\n \t\treset_timeout();\n \t\thead_ref_namespaced(send_ref, &symref);\n-- \n2.4.2\n"},{"id":"263716","messageId":"20150612223223.GA22677@leeloo.kyriasis.com","threadId":"39615","inReplyTo":"1434147436-21272-1-git-send-email-johannes@kyriasis.com","subject":"Re: [PATCH v2] upload-pack: Fail if cloning empty namespace","fromName":"Johannes Löthberg","fromEmail":"johannes@kyriasis.com","sentAt":"2015-06-12T22:32:23Z","receivedAt":"2015-06-12T22:32:23Z","isPatch":true,"sender":{"key":"johannes@kyriasis.com","avatar":"https://gravatar.com/avatar/af2dea1b1759403329a1b7eaa28a07b5c5d1901e9f55723103d6324ffb7737ae?d=mp&s=160"},"body":"On 13/06, Johannes Löthberg wrote:\n>Git should fail to clone if trying to clone from an non-existing\n>ref namespace, since it's the same as a non-existing repository\n>\n>Signed-off-by: Johannes Löthberg \n>---\n>\n>Changes since v1:\n>\n>* Fixed the namespace check, since I apparently forgot to check with a\n> bare repo in my last test. D'oh.\n>\n>Two other options for this would be to either add a\n>get_git_namespace_len() function and use that, or a is_namespaced()\n>functon. But since it's only used here for now at least it feels simpler\n>to not bloat the codabase with another function which has no other use.\n>\n\nI should note that I have a small test script written now, ready to be \nconverted into a git test, though I want some opinions on whether the \npatch would be merged before bothering to convert it.\n\n-- \nSincerely,\n Johannes Löthberg\n PGP Key ID: 0x50FB9B273A9D0BB5\n https://theos.kyriasis.com/~kyrias/\n"},{"id":"263903","messageId":"xmqqh9q87l3x.fsf@gitster.dls.corp.google.com","threadId":"39615","inReplyTo":"20150612223223.GA22677@leeloo.kyriasis.com","subject":"Re: [PATCH v2] upload-pack: Fail if cloning empty namespace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-15T20:49:38Z","receivedAt":"2015-06-15T20:49:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Löthberg writes:\n\n> On 13/06, Johannes Löthberg wrote:\n>>Git should fail to clone if trying to clone from an non-existing\n>>ref namespace, since it's the same as a non-existing repository\n>>\n>>Signed-off-by: Johannes Löthberg \n>>---\n>>\n>>Changes since v1:\n>>\n>>* Fixed the namespace check, since I apparently forgot to check with a\n>> bare repo in my last test. D'oh.\n>>\n>>Two other options for this would be to either add a\n>>get_git_namespace_len() function and use that, or a is_namespaced()\n>>functon. But since it's only used here for now at least it feels simpler\n>>to not bloat the codabase with another function which has no other use.\n>>\n>\n> I should note that I have a small test script written now, ready to be\n> converted into a git test, though I want some opinions on whether the\n> patch would be merged before bothering to convert it.\n\nYou would probably want new tests, but more importantly did you make\nsure this passes existing tests? It seems to break 5509 (there\ncould be others) at least for me.\n"},{"id":"264402","messageId":"20150620142705.GA23163@leeloo.kyriasis.com","threadId":"39615","inReplyTo":"xmqqh9q87l3x.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] upload-pack: Fail if cloning empty namespace","fromName":"Johannes Löthberg","fromEmail":"johannes@kyriasis.com","sentAt":"2015-06-20T14:27:45Z","receivedAt":"2015-06-20T14:27:45Z","isPatch":true,"sender":{"key":"johannes@kyriasis.com","avatar":"https://gravatar.com/avatar/af2dea1b1759403329a1b7eaa28a07b5c5d1901e9f55723103d6324ffb7737ae?d=mp&s=160"},"body":"On 15/06, Junio C Hamano wrote:\n>You would probably want new tests, but more importantly did you make\n>sure this passes existing tests? It seems to break 5509 (there\n>could be others) at least for me.\n>\n\nIt breaks 5509 currently yeah, though I've already fixed it locally.\n\nAnyway,\n\nYou create a namespace by pushing to it, and without a HEAD the \nnamespace is not a proper repo, and the code won't deal with them \nproperly. (Which is why without that patch this patch is useless.)\n\nSo, if you don't want HEAD to be created when pushing to a ref \nnamespace, (which seems to be what you're leaning toward, considering \nyour last reply to the other thread,) then ref namespaces won't really \nwork in most cases, and it seems pointless to support them at all.\n\n-- \nSincerely,\n Johannes Löthberg\n PGP Key ID: 0x50FB9B273A9D0BB5\n https://theos.kyriasis.com/~kyrias/\n"},{"id":"264410","messageId":"xmqqr3p6jltx.fsf@gitster.dls.corp.google.com","threadId":"39615","inReplyTo":"20150620142705.GA23163@leeloo.kyriasis.com","subject":"Re: [PATCH v2] upload-pack: Fail if cloning empty namespace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-20T18:06:50Z","receivedAt":"2015-06-20T18:06:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Löthberg writes:\n\n> On 15/06, Junio C Hamano wrote:\n>>You would probably want new tests, but more importantly did you make\n>>sure this passes existing tests? It seems to break 5509 (there\n>>could be others) at least for me.\n>>\n>\n> It breaks 5509 currently yeah, though I've already fixed it locally.\n>\n> Anyway,\n>\n> You create a namespace by pushing to it,...\n\nYou keep repeating that, but I do not think we agreed that it is the\nsupported or correct procedure to set up a new namespace in the\nfirst place. Doesn't the server side need a lot more than just\nsetting up HEAD symref (like running upload/receive-pack with the\npath to the hierarchy exported as GIT_NAMESPACE environment)?\n"},{"id":"264411","messageId":"20150620181348.GA28776@leeloo.kyriasis.com","threadId":"39615","inReplyTo":"xmqqr3p6jltx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] upload-pack: Fail if cloning empty namespace","fromName":"Johannes Löthberg","fromEmail":"johannes@kyriasis.com","sentAt":"2015-06-20T18:13:48Z","receivedAt":"2015-06-20T18:13:48Z","isPatch":true,"sender":{"key":"johannes@kyriasis.com","avatar":"https://gravatar.com/avatar/af2dea1b1759403329a1b7eaa28a07b5c5d1901e9f55723103d6324ffb7737ae?d=mp&s=160"},"body":"On 20/06, Junio C Hamano wrote:\n>Johannes Löthberg writes:\n>\n>> On 15/06, Junio C Hamano wrote:\n>>>You would probably want new tests, but more importantly did you make\n>>>sure this passes existing tests? It seems to break 5509 (there\n>>>could be others) at least for me.\n>>>\n>>\n>> It breaks 5509 currently yeah, though I've already fixed it locally.\n>>\n>> Anyway,\n>>\n>> You create a namespace by pushing to it,...\n>\n>You keep repeating that, but I do not think we agreed that it is the\n>supported or correct procedure to set up a new namespace in the\n>first place.\n\nWhether it's 'correct' I can't say, but it's the only currently \nsupported way.\n\n>Doesn't the server side need a lot more than just\n>setting up HEAD symref (like running upload/receive-pack with the\n>path to the hierarchy exported as GIT_NAMESPACE environment)?\n\nAll it needs to do is use the refs under the ref namespace, and that it \nhas already done for a long time. (Since 2011 in fact.)\n\n-- \nSincerely,\n Johannes Löthberg\n PGP Key ID: 0x50FB9B273A9D0BB5\n https://theos.kyriasis.com/~kyrias/\n"},{"id":"264413","messageId":"xmqqmvzujcvb.fsf@gitster.dls.corp.google.com","threadId":"39615","inReplyTo":"xmqqr3p6jltx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] upload-pack: Fail if cloning empty namespace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-20T21:20:24Z","receivedAt":"2015-06-20T21:20:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> You create a namespace by pushing to it,...\n>\n> You keep repeating that, but I do not think we agreed that it is the\n> supported or correct procedure to set up a new namespace in the\n> first place. Doesn't the server side need a lot more than just\n> setting up HEAD symref (like running upload/receive-pack with the\n> path to the hierarchy exported as GIT_NAMESPACE environment)?\n\nLet me step back and try again, as I think I am missing some\nexisting feature you are using, and that missing piece is preventing\nme from seeing why this is a good idea (by the way, I do not think\nwe are doing this exchange in the right thread---the upload-pack\npatch is not about auto-vivifying a new namespace by creating HEAD).\n\nI'll do a brief braindump which hopefully shows the basis of why I\nkeep saying that creating HEAD at random place is not sufficient.\nHelp me, by telling me what I am missing, understand why it is\nsufficient.\n\nSuppose we have a server at URL git://site.xz/r; it serves from the\nfilesystem, say /usr/share/git/r.git/ (bare repository). In that\ndirectory, we have the usual HEAD, objects, refs/{heads,tags}, etc.\nFurther suppose we have there refs/namespaces/a/ under which HEAD,\nrefs/{heads,tags}, etc.\n\nThen we run the server (say, git-daemon) with GIT_NAMESPACE\nenvironment variable set to 'a'. The users can now run:\n\n $ git clone git://site.xz/r\n\nand they will see what's really under refs/namespaces/a/. The HEAD\nthey see is actually not /usr/share/git/r.git/HEAD but it is\n/usr/share/git/r.git/refs/namespaces/a/HEAD. Everything outside\n/usr/share/git/r.git/refs/namespaces/a is not visible to them.\n\nIn order to add a new namespace 'b' next to 'a', you would somehow\nneed to populate /usr/share/git/r.git/refs/namespaces/b. Presumably\nyou can prepare a source repository locally and then push everything\nthere manually, by doing something like this:\n\n $ git push ssh://site.xz/usr/share/git/r.git refs/*:refs/namespaces/b/*\n\nbut we agree that this does not create refs/namespaces/b/HEAD.\n\nBut if we made some change to the transfer to push-to-receive-pack\nso that this can also create HEAD that points at something, what\nwould the user say, instead of that earlier\n\n $ git clone git://site.xz/r\n\nto access this new namespace? Don't you have to be running another\ninstance of a server with GIT_NAMESPACE set to 'b'? How do you\narrange that to automagically happen?\n\nOr do your users that clone/fetch do things differnetly from the\nabove, and if so what do they do?\n\nHow is your server configured to support the access the existing\nnamespace 'a', and the new namespace 'b' that is automatically\ncreated by pointing /usr/share/git/r.git/refs/namespaces/b/HEAD\nto one ref inside that namespace?\n"},{"id":"264414","messageId":"20150620214901.GA5520@leeloo.kyriasis.com","threadId":"39615","inReplyTo":"xmqqmvzujcvb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] upload-pack: Fail if cloning empty namespace","fromName":"Johannes Löthberg","fromEmail":"johannes@kyriasis.com","sentAt":"2015-06-20T21:49:01Z","receivedAt":"2015-06-20T21:49:01Z","isPatch":true,"sender":{"key":"johannes@kyriasis.com","avatar":"https://gravatar.com/avatar/af2dea1b1759403329a1b7eaa28a07b5c5d1901e9f55723103d6324ffb7737ae?d=mp&s=160"},"body":"On 20/06, Junio C Hamano wrote:\n>Junio C Hamano writes:\n>Let me step back and try again, as I think I am missing some\n>existing feature you are using, and that missing piece is preventing\n>me from seeing why this is a good idea (by the way, I do not think\n>we are doing this exchange in the right thread---the upload-pack\n>patch is not about auto-vivifying a new namespace by creating HEAD).\n>\n\nYeah, that's true.\n\n>But if we made some change to the transfer to push-to-receive-pack\n>so that this can also create HEAD that points at something, what\n>would the user say, instead of that earlier\n>\n> $ git clone git://site.xz/r\n>\n>to access this new namespace? Don't you have to be running another\n>instance of a server with GIT_NAMESPACE set to 'b'? How do you\n>arrange that to automagically happen?\n>\n>Or do your users that clone/fetch do things differnetly from the\n>above, and if so what do they do?\n>\n>How is your server configured to support the access the existing\n>namespace 'a', and the new namespace 'b' that is automatically\n>created by pointing /usr/share/git/r.git/refs/namespaces/b/HEAD\n>to one ref inside that namespace?\n\nIf you want to hide the fact that you're using namespaces you'll \nprobably have to either run another daemon or use a wrapper script, yes, \nbut only if you want to hide it.\n\nHowever, if you don't want to hide the namespace behind some script the \nusers just have to use --namespace or GIT_NAMESPACE, possibly having a \nscript that does some access-control magic so that you eg can only push \nto specific namespaces.\n\nHaving it be created is especially important when someone might be \nworking on the same machine as the repo, where the push won't go through \neg SSH.\n\n-- \nSincerely,\n Johannes Löthberg\n PGP Key ID: 0x50FB9B273A9D0BB5\n https://theos.kyriasis.com/~kyrias/\n"},{"id":"264452","messageId":"xmqqa8vskilc.fsf@gitster.dls.corp.google.com","threadId":"39615","inReplyTo":"20150620214901.GA5520@leeloo.kyriasis.com","subject":"Re: [PATCH v2] upload-pack: Fail if cloning empty namespace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-21T18:43:43Z","receivedAt":"2015-06-21T18:43:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Löthberg writes:\n\n>>But if we made some change to the transfer to push-to-receive-pack\n>>so that this can also create HEAD that points at something, what\n>>would the user say, instead of that earlier\n>>\n>> $ git clone git://site.xz/r\n>>\n>>to access this new namespace? Don't you have to be running another\n>>instance of a server with GIT_NAMESPACE set to 'b'? How do you\n>>arrange that to automagically happen?\n>>\n>>Or do your users that clone/fetch do things differnetly from the\n>>above, and if so what do they do?\n>>\n>>How is your server configured to support the access the existing\n>>namespace 'a', and the new namespace 'b' that is automatically\n>>created by pointing /usr/share/git/r.git/refs/namespaces/b/HEAD\n>>to one ref inside that namespace?\n\nYour answer to the above question marks are?\n\n> If you want to hide the fact that you're using namespaces you'll\n> probably have to either run another daemon or use a wrapper script,\n> yes, but only if you want to hide it.\n\nI would view it as segregating (not mixing refs and objects\nreachable by them for 'a' and 'b') more than \"hiding\". Either way,\nyour users' clone/fetch need a way to say which one of 'a' or 'b'\nthey want. How are you arranging that to happen when you add a new\nnamespace 'b'?\n\nI asked you if my understanding of your workflow to start populating\na new namespace 'b' matches what you actually do in reality, but you\nomitted it from your quote in your response, which was this:\n\n>> In order to add a new namespace 'b' next to 'a', you would somehow\n>> need to populate /usr/share/git/r.git/refs/namespaces/b. Presumably\n>> you can prepare a source repository locally and then push everything\n>> there manually, by doing something like this:\n>> \n>> $ git push ssh://site.xz/usr/share/git/r.git refs/*:refs/namespaces/b/*\n>> \n>> but we agree that this does not create refs/namespaces/b/HEAD.\n\nI'd assume that the answer is \"yes, Junio got the 'push' to populate\ncorrectly\" and continue.\n\nPopulating refs/namespaces/b/ and the objects reachable by them by\npushing into that hierarchy is one step of doing so, and setting\nHEAD there is not done by \"git push\", but is that the only thing\nmissing?\n\nI do not view \"git fetch --namespace=b\" or \"git clone --namespace=b\ngit://site.xz/r\" as a usable way to do so (is there an option like\nthat in the first place?), and that is what I have been asking.\n\nNote that I am not opposed to an idea to make it possible to point a\nsymbolic ref .git/$something/HEAD at a ref that is different from\nwhat it is pointing at currently (or creating it if it does not\nexist yet) from outside the server, i.e. without ssh-ing in and\nrunning \"git symbolic ref\".\n\nThat $something may be \"refs/namespaces/b\" in your case, but it does\nnot have to be. We do not have a good way to support more common\ncase of repointing \".git/HEAD\" to say \"now the primary branch of\nthis repository is not 'master' but it is called 'trunk'\".\n\nThat is one reason why I do not want a patch to receive-pack that\nrandomly picks one ref among all refs pushed and point HEAD at it,\nwhich is the following part from your patch in the other thread\n(where we should be having this discussion ;-).\n\n@@ -981,6 +983,14 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\treturn NULL; /* good */\n \t}\n \telse {\n+\t\tnamespace = get_git_namespace();\n+\t\tif (strcmp(namespace, \"refs/namespaces/\")) {\n+\t\t\tstrbuf_addf(&namespaced_head_buf, \"%s%s\", namespace, \"HEAD\");\n+\t\t\tnamespaced_head_path = strbuf_detach(&namespaced_head_buf, NULL);\n+\n+\t\t\tcreate_symref(namespaced_head_path, namespaced_name, NULL);\n+\t\t}\n+\n \t\tstruct strbuf err = STRBUF_INIT;\n \t\tif (shallow_update && si->shallow_ref[cmd->index] &&\n \t\t update_shallow_ref(cmd, si))\n\nThis is part of update() function in receive-pack, which is called\nfor each and every ref that is pushed. If you did the \"initial\npopulation push\" I cited from earlier part of the discussion:\n\n $ git push ssh://site.xz/usr/share/git/r.git refs/*:refs/namespaces/b/*\n\nthe above would see updates to refs/namespaces/b/refs/heads/master,\nrefs/namespaces/b/refs/heads/next, refs/namespaces/b/refs/tags/v1.0,\netc., i.e. all the refs you are pushing in an unspecified order, and\nthe added code would run once for each and every one of them. Where\nwould the final HEAD points at is not something you can control, and\nonce you create one, you cannot repoint to a more reasonable ref.\n\nIf we introduce a way to point any symbolic ref at a ref that is\ndifferent from what it is pointing at currently (or creating it if\nit does not exist yet) from outside the server, i.e. without ssh-ing\nin and running \"git symbolic ref\", then that would work equally well\nfor your use case, and also will solve longstanding \"how do I change\nthe primary branch the server's HEAD points at?\" problem. I do not\nthink your patch is a good first step to introduce that solution.\n"}]}