{"thread":{"id":"38638","subject":"[RFC/PATCH 0/3] protocol v2","startedAt":"2015-02-24T03:12:39Z","lastAt":"2015-04-02T23:14:52Z","messageCount":80,"participants":["Stefan Beller","Duy Nguyen","Junio C Hamano","Kyle J. McKay","Torsten Bögershausen","Johannes Sixt","Philip Oakley","David Lang","Shawn Pearce","Martin Fick"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"256553","messageId":"1424747562-5446-1-git-send-email-sbeller@google.com","threadId":"38638","inReplyTo":null,"subject":"[RFC/PATCH 0/3] protocol v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-24T03:12:39Z","receivedAt":"2015-02-24T03:12:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Inspired by a discusson on the scaling of git in the last days,\nI thought about starting the adventure to teach git a new transport\nprotocol.\n\nOne of the biggest problems of a new protocol would be deployment\nas the users probably would not care too deeply. It should just \nwork in the sense that the user should not even sense that the\nprotocol changed. To do so we need to make sure the protocol\nis backwards compatible and works if and old client talks to \na new server as well as the other way round.\n\nA later incarnation of the patch series will eventually add the \npossibility to add new versions of the transport protocols easily\nwithout harming the user. For now in the first revision of the \nseries it just documents an approach of how I'd start this problem \nof compatibility issues.\n\nI realize this will be a bigger change to git, so I'd rather\njust make a small step now. The actual discussion on how to\ndo the next protocol(s) may be started on the gitmerge\nconference? (bloomfilter! client speaks first!, rsyncing\nthe refs changes!)\n\nAny thoughts on how to make it easy to teach git new protocols\nare very welcome.\n\nThanks,\nStefan\n\nStefan Beller (3):\n Document protocol capabilities extension\n receive-pack: add advertisement of different protocol options\n receive-pack: enable protocol v2\n\n Documentation/technical/protocol-capabilities.txt | 11 +++++++++++\n builtin/receive-pack.c | 7 +++++++\n 2 files changed, 18 insertions(+)\n\n-- \n2.3.0.81.gc37f363\n"},{"id":"256556","messageId":"1424747562-5446-2-git-send-email-sbeller@google.com","threadId":"38638","inReplyTo":"1424747562-5446-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/3] Document protocol capabilities extension","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-24T03:12:40Z","receivedAt":"2015-02-24T03:12:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In the future we may want to have multiple different versions for\nthe transport protocol each optimized for a certain aspect of the\ntransport such as latency, bandwidth, CPU load etc. To make this\nfuture proof document a possible way how to advertise the versions\nsuch that the client can store it and use try to use such advertised\nprotocols later if the clients supports them.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/technical/protocol-capabilities.txt | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex 4f8a7bf..6829668 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -268,3 +268,14 @@ to accept a signed push certificate, and asks the to be\n included in the push certificate. A send-pack client MUST NOT\n send a push-cert packet unless the receive-pack server advertises\n this capability.\n+\n+versions\n+--------\n+\n+If the server supports more than the standard protocol, this capability\n+advertises the additional versions. It is a comma separated list of\n+the names of extensions for the binaries (i.e. \"v2\" would result in\n+the assumption that git-receive-pack-v2 as well as git-upload-pack-v2\n+is available). The items are desired in desceding order by the server,\n+i.e. the server would like the client to use the first advertised\n+version most.\n-- \n2.3.0.81.gc37f363\n"},{"id":"256555","messageId":"1424747562-5446-3-git-send-email-sbeller@google.com","threadId":"38638","inReplyTo":"1424747562-5446-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/3] receive-pack: add advertisement of different protocol options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-24T03:12:41Z","receivedAt":"2015-02-24T03:12:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n builtin/receive-pack.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e0ce78e..a077b1d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -71,6 +71,8 @@ static long nonce_stamp_slop;\n static unsigned long nonce_stamp_slop_limit;\n static struct ref_transaction *transaction;\n \n+static const char *advertised_versions = NULL;\n+\n static enum deny_action parse_deny_action(const char *var, const char *value)\n {\n \tif (value) {\n@@ -168,6 +170,9 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (strcmp(var, \"receive.advertisedversions\") == 0)\n+\t\treturn git_config_string(&advertised_versions, var, value);\n+\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -189,6 +194,8 @@ static void show_ref(const char *path, const unsigned char *sha1)\n \t\t\tstrbuf_addstr(&cap, \" ofs-delta\");\n \t\tif (push_cert_nonce)\n \t\t\tstrbuf_addf(&cap, \" push-cert=%s\", push_cert_nonce);\n+\t\tif (advertise_versions)\n+\t\t\tstrbuf_addstr(&cap, \" versions=%s\", advertise_versions);\n \t\tstrbuf_addf(&cap, \" agent=%s\", git_user_agent_sanitized());\n \t\tpacket_write(1, \"%s %s%c%s\\n\",\n \t\t\t sha1_to_hex(sha1), path, 0, cap.buf);\n-- \n2.3.0.81.gc37f363\n"},{"id":"256554","messageId":"1424747562-5446-4-git-send-email-sbeller@google.com","threadId":"38638","inReplyTo":"1424747562-5446-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/3] receive-pack: enable protocol v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-24T03:12:42Z","receivedAt":"2015-02-24T03:12:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This string would tell the client that git-receive-packv2 as well as\ngit-receive-pack would be supported.\n\nSigned-off-by: Stefan Beller \n---\n builtin/receive-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex a077b1d..dcb231b 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -71,7 +71,7 @@ static long nonce_stamp_slop;\n static unsigned long nonce_stamp_slop_limit;\n static struct ref_transaction *transaction;\n \n-static const char *advertised_versions = NULL;\n+static const char *advertised_versions = \"v2,\";\n \n static enum deny_action parse_deny_action(const char *var, const char *value)\n {\n-- \n2.3.0.81.gc37f363\n"},{"id":"256557","messageId":"CACsJy8BSf2h_xD-Q1tudAg_xCzffRQM+7xzUgprONxD7vM5RYw@mail.gmail.com","threadId":"38638","inReplyTo":"1424747562-5446-1-git-send-email-sbeller@google.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-02-24T04:02:05Z","receivedAt":"2015-02-24T04:02:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Feb 24, 2015 at 10:12 AM, Stefan Beller wrote:\n> One of the biggest problems of a new protocol would be deployment\n> as the users probably would not care too deeply. It should just\n> work in the sense that the user should not even sense that the\n> protocol changed.\n\nAgreed.\n\n> To do so we need to make sure the protocol\n> is backwards compatible and works if an old client talks to\n> a new server as well as the other way round.\n\nIt's very hard to keep backward compatibility if you want to stop the\ninitial ref adverstisement, costly when there are lots of refs. But we\ncan let both protocols run in parallel, with the old one advertise the\npresence of the new one. Then the client could switch to new protocol\ngradually. This way new protocol could forget about backward\ncompatibility. See\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/215054/focus=244325\n-- \nDuy\n"},{"id":"256558","messageId":"CAGZ79kYhUyRyvf6ou1QWM1j1L1X2YwCbZaNuVp6jJMyUcpwb+A@mail.gmail.com","threadId":"38638","inReplyTo":"CACsJy8BSf2h_xD-Q1tudAg_xCzffRQM+7xzUgprONxD7vM5RYw@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-24T05:40:43Z","receivedAt":"2015-02-24T05:40:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 23, 2015 at 8:02 PM, Duy Nguyen wrote:\n> On Tue, Feb 24, 2015 at 10:12 AM, Stefan Beller wrote:\n>> One of the biggest problems of a new protocol would be deployment\n>> as the users probably would not care too deeply. It should just\n>> work in the sense that the user should not even sense that the\n>> protocol changed.\n>\n> Agreed.\n>\n>> To do so we need to make sure the protocol\n>> is backwards compatible and works if an old client talks to\n>> a new server as well as the other way round.\n>\n> It's very hard to keep backward compatibility if you want to stop the\n> initial ref adverstisement, costly when there are lots of refs. But we\n> can let both protocols run in parallel, with the old one advertise the\n> presence of the new one.\n\nThat's what I actually meant, to have different versions out there,\nbut maybe having the version as of now as the least common denominator\nsuch that it always works (albeit inefficient for many refs).\n\n> Then the client could switch to new protocol\n> gradually. This way new protocol could forget about backward\n> compatibility. See\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/215054/focus=244325\n> --\n> Duy\n\n> I would add that upload-pack also advertises about the availability of\n> upload-pack2 and the client may set the remote.*.useUploadPack2 to\n> either yes or auto so next time upload-pack2 will be used.\n\nI had a similar thought, though I would not just restrict it to v2\nthis time, but\nI'd aim to make it possible to plug whatever protocol you want to.\n(Comparable to the SSL or ssh, it will always work, but as a proficient user\nyou can spend lot's of time tweaking what you actually want, looking\nat tradeoffs\nof efficiency, security, convenience).\n"},{"id":"256559","messageId":"CAPc5daVbrUaU6LFM65evru0+1tBT916+0AOyids=f7DZThTPGw@mail.gmail.com","threadId":"38638","inReplyTo":"CACsJy8BSf2h_xD-Q1tudAg_xCzffRQM+7xzUgprONxD7vM5RYw@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-24T06:15:09Z","receivedAt":"2015-02-24T06:15:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Mon, Feb 23, 2015 at 8:02 PM, Duy Nguyen wrote:\n>\n> It's very hard to keep backward compatibility if you want to stop the\n> initial ref adverstisement, costly when there are lots of refs. But we\n> can let both protocols run in parallel, with the old one advertise the\n> presence of the new one. Then the client could switch to new protocol\n> gradually. This way new protocol could forget about backward\n> compatibility. See\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/215054/focus=244325\n\nYes, the whole thread is worth a read, but the approach suggested by\nthat article $gmane/244325 is very good for its simplicity. The server\nend programs, upload-pack and receive-pack, need to only learn to\nadvertise the availability of upload-pack-v2 and receive-pack-v2\nservices and the client side programs, fetch-pack and push-pack,\nneed to only notice the advertisement and record the availability of\nv2 counterparts for the current remote *and* continue the exchange\nin v1 protocol. That way, there is very little risk for breaking anything.\n\nAnd the programs for new protocol exchange do not have to worry\nabout having to talk with older counterparts and downgrading the\nprotocol inline at all. As long as we learn from our past mistakes\nand make sure that the very initial exchange will be kept short (one\nof the items in the list of limitations, $gmane/264000), future servers\nand clients can upgrade the protocol they talk inline by probing\ncapabilities, just like the current protocol allows them to choose\nextensions. The biggest issue in the current protocol is not \"who\nspeaks first\" (that is merely one aspect) but \"what is spoken first\",\niow, \"one side blinly gives a large message as the first thing\", which\ncannot be squelched by capability exchange.\n\nSo if we are going to discuss a new protocol, I'd prefer to see the\ndiscussion without worrying too much about how to inter-operate\nwith the current vintage of Git. It is no longer an interesting problem,\nas we know how to solve it with minimum risk. Instead, I'd like to\nsee us design the new protocol in such a way that it is in-line\nupgradable without repeating our past mistakes.\n\nI am *not* convinced that we want multiple suite of protocols that\nmust be chosen from to suit the use pattern, as mentioned somewhere\nupthread, by the way.\n\nThanks.\n"},{"id":"256607","messageId":"CAGZ79kbZHtZuPrb6rEP41vbdnZqJmsMwq+8pNer-_D4U5B1xZw@mail.gmail.com","threadId":"38638","inReplyTo":"CAPc5daVbrUaU6LFM65evru0+1tBT916+0AOyids=f7DZThTPGw@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-24T23:37:27Z","receivedAt":"2015-02-24T23:37:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 23, 2015 at 10:15 PM, Junio C Hamano wrote:\n> On Mon, Feb 23, 2015 at 8:02 PM, Duy Nguyen wrote:\n>>\n>> It's very hard to keep backward compatibility if you want to stop the\n>> initial ref adverstisement, costly when there are lots of refs. But we\n>> can let both protocols run in parallel, with the old one advertise the\n>> presence of the new one. Then the client could switch to new protocol\n>> gradually. This way new protocol could forget about backward\n>> compatibility. See\n>>\n>> http://thread.gmane.org/gmane.comp.version-control.git/215054/focus=244325\n>\n> Yes, the whole thread is worth a read, but the approach suggested by\n> that article $gmane/244325 is very good for its simplicity. The server\n> end programs, upload-pack and receive-pack, need to only learn to\n> advertise the availability of upload-pack-v2 and receive-pack-v2\n> services and the client side programs, fetch-pack and push-pack,\n> need to only notice the advertisement and record the availability of\n> v2 counterparts for the current remote *and* continue the exchange\n> in v1 protocol. That way, there is very little risk for breaking anything.\n\nRight, I want to add this \"learn about v2 on the fly, continue as always\"\nto the protocol.\n\n>\n> So if we are going to discuss a new protocol, I'd prefer to see the\n> discussion without worrying too much about how to inter-operate\n> with the current vintage of Git. It is no longer an interesting problem,\n> as we know how to solve it with minimum risk. Instead, I'd like to\n> see us design the new protocol in such a way that it is in-line\n> upgradable without repeating our past mistakes.\n>\n> I am *not* convinced that we want multiple suite of protocols that\n> must be chosen from to suit the use pattern, as mentioned somewhere\n> upthread, by the way.\n\nI do think it makes sense to have different protocols or different tunings\nof one protocol, because there are many different situations in which different\nmetrics are the key metric.\n\nIf you are on mobile, you'd possibly be billed by the bytes on the wire, so\nyou want to have a protocol with as actual transport as possible and would\nmaybe trade off transported bytes to lots of computational overhead.\n\nIf you are in Australia (sorry downunder ;) or on satellite internet,\nyou may care a lot about latency and roundtrip times.\n\nIf you are in a corporate environment and just cloning from next door,\nyou may want to have the overall process (compute+network+\nlocal reconstruction) just be fast overall.\n\n\nI can understand, that we maybe want to just provide one generic\n\"version 2\" of the protocol which is an allrounder not doing bad in\nall of these aspects, but I can see usecases of having the desire to\nreplace the wire protocol by your own implementation. To do so\nwe could try to offer an API which makes implementing a new\nprotocol somewhat easy. The current state of affairs is not providing\nthis flexibility.\n\nI think it would be not much overhead to have such\nflexibility when writing the actual code for the \"very little risk\" v2\nupdate. So instead of advertising a boolean flag meaning\n\"This server/client speaks version2\", we would rather send a list\n\"This server speaks v2,v1 and v-custom-optimized-for-high-latency\".\n\nI started looking for academic literature if there are generic solutions\nto finding graph differences, but no real luck for adapting to our\nproblem yet.\n\nThanks for your input,\nStefan\n"},{"id":"256637","messageId":"CACsJy8BN2imGCW0cueh-jGKfN_nRg3=J-GTX2P5h2z0Tu=id6A@mail.gmail.com","threadId":"38638","inReplyTo":"CAGZ79kbZHtZuPrb6rEP41vbdnZqJmsMwq+8pNer-_D4U5B1xZw@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-02-25T12:44:41Z","receivedAt":"2015-02-25T12:44:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 25, 2015 at 6:37 AM, Stefan Beller wrote:\n> I can understand, that we maybe want to just provide one generic\n> \"version 2\" of the protocol which is an allrounder not doing bad in\n> all of these aspects, but I can see usecases of having the desire to\n> replace the wire protocol by your own implementation. To do so\n> we could try to offer an API which makes implementing a new\n> protocol somewhat easy. The current state of affairs is not providing\n> this flexibility.\n\nI think we are quite flexible after initial ref advertisement. After\nthat point the client tells the server its capabilities and the server\ndoes the same for the client. Only shared features can be used. So if\nyou want to add a new micro protocol for mobile, just add \"mobile\"\ncapability to both client and server. A new implementation can support\nno capabililities and it should work fine with C Git (less efficient\nthough, of course). And we have freedom to mix capabilities any way we\nwant (it's harder to do when you have to follow v2, v2.1, v2.2...)\n-- \nDuy\n"},{"id":"256643","messageId":"xmqqsidtoojh.fsf@gitster.dls.corp.google.com","threadId":"38638","inReplyTo":"CACsJy8BN2imGCW0cueh-jGKfN_nRg3=J-GTX2P5h2z0Tu=id6A@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-25T18:04:50Z","receivedAt":"2015-02-25T18:04:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Wed, Feb 25, 2015 at 6:37 AM, Stefan Beller wrote:\n>> I can understand, that we maybe want to just provide one generic\n>> \"version 2\" of the protocol which is an allrounder not doing bad in\n>> all of these aspects, but I can see usecases of having the desire to\n>> replace the wire protocol by your own implementation. To do so\n>> we could try to offer an API which makes implementing a new\n>> protocol somewhat easy. The current state of affairs is not providing\n>> this flexibility.\n>\n> I think we are quite flexible after initial ref advertisement.\n\nYes, that is exactly where my \"I am not convinced\" comes from.\n\n> After\n> that point the client tells the server its capabilities and the server\n> does the same for the client. Only shared features can be used. So if\n> you want to add a new micro protocol for mobile, just add \"mobile\"\n> capability to both client and server. A new implementation can support\n> no capabililities and it should work fine with C Git (less efficient\n> though, of course). And we have freedom to mix capabilities any way we\n> want (it's harder to do when you have to follow v2, v2.1, v2.2...)\n"},{"id":"256670","messageId":"CAGZ79kZE2+tCZgDzeTrQBn6JQv1OWJ7t_8j4kYMQgVaAbsnnxw@mail.gmail.com","threadId":"38638","inReplyTo":"xmqqsidtoojh.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-26T07:31:15Z","receivedAt":"2015-02-26T07:31:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Feb 25, 2015 at 10:04 AM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n>\n>> On Wed, Feb 25, 2015 at 6:37 AM, Stefan Beller wrote:\n>>> I can understand, that we maybe want to just provide one generic\n>>> \"version 2\" of the protocol which is an allrounder not doing bad in\n>>> all of these aspects, but I can see usecases of having the desire to\n>>> replace the wire protocol by your own implementation. To do so\n>>> we could try to offer an API which makes implementing a new\n>>> protocol somewhat easy. The current state of affairs is not providing\n>>> this flexibility.\n>>\n>> I think we are quite flexible after initial ref advertisement.\n>\n> Yes, that is exactly where my \"I am not convinced\" comes from.\n>\n\nWe are not. (not really at least). We can tune some parameters or\nchange the behavior slightly,\nbut we cannot fix core assumptions made when creating v2 protocol.\nThis you can see when when talking about v1 as well: we cannot fix any\nwrongdoings of v1 now by adding another capability. So from my point\nof view we don't waste resources when having an advertisement of\npossible protocols instead of a boolean flag indicating v2 is\nsupported. There is really not much overhead in coding nor bytes\nexchanged on the wire, so why not accept stuff that comes for free\n(nearly) ?\n\nI mean how do we know all the core assumptions made for v2 hold in the\nfuture? We don't. That's why I'd propose a plain and easy exchange at\nfirst stating the version to talk.\n\nAnyway what is the cost of a round trip time compared to the bytes on\nthe wire? Usually the cost of bytes on the wire correlate with the\nlatency anyway. (think mobile metered compared to corporate setting\nwith low latency). That's why I'd rather optimize for used bandwidth\nthan round trip times, but that may be just my personal perception of\nthe internet. That's why I'd propose different protocols.\n\nThanks,\nStefan\n"},{"id":"256673","messageId":"CACsJy8ASR-O-7tozw=p1Ek0ugct5EVZyWtxY_YA2nqcUV_+ECw@mail.gmail.com","threadId":"38638","inReplyTo":"CAGZ79kZE2+tCZgDzeTrQBn6JQv1OWJ7t_8j4kYMQgVaAbsnnxw@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-02-26T10:15:54Z","receivedAt":"2015-02-26T10:15:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Feb 26, 2015 at 2:31 PM, Stefan Beller wrote:\n> On Wed, Feb 25, 2015 at 10:04 AM, Junio C Hamano wrote:\n>> Duy Nguyen writes:\n>>\n>>> On Wed, Feb 25, 2015 at 6:37 AM, Stefan Beller wrote:\n>>>> I can understand, that we maybe want to just provide one generic\n>>>> \"version 2\" of the protocol which is an allrounder not doing bad in\n>>>> all of these aspects, but I can see usecases of having the desire to\n>>>> replace the wire protocol by your own implementation. To do so\n>>>> we could try to offer an API which makes implementing a new\n>>>> protocol somewhat easy. The current state of affairs is not providing\n>>>> this flexibility.\n>>>\n>>> I think we are quite flexible after initial ref advertisement.\n>>\n>> Yes, that is exactly where my \"I am not convinced\" comes from.\n>>\n>\n> We are not. (not really at least). We can tune some parameters or\n> change the behavior slightly,\n> but we cannot fix core assumptions made when creating v2 protocol.\n> This you can see when when talking about v1 as well: we cannot fix any\n> wrongdoings of v1 now by adding another capability.\n\nStep 1 then should be identifying these wrongdoings and assumptions.\n\nWe can really go wild with these capabilities. The only thing that\ncan't be changed is perhaps sending the first ref. I don't know\nwhether we can accept a dummy first ref... After that point, you can\nturn the protocol upside down because both client and server know what\nit would be.\n\n> So from my point\n> of view we don't waste resources when having an advertisement of\n> possible protocols instead of a boolean flag indicating v2 is\n> supported. There is really not much overhead in coding nor bytes\n> exchanged on the wire, so why not accept stuff that comes for free\n> (nearly) ?\n\nYou realize you're advertising v2 as a new capability, right? Instead\nof defining v2 feature set then advertise v2, people could simply add\nnew features directly. I don't see v2 (at least with these patches)\nadds any value.\n\n> I mean how do we know all the core assumptions made for v2 hold in the\n> future? We don't. That's why I'd propose a plain and easy exchange at\n> first stating the version to talk.\n\nAnd we already does that, except that we don't state what version (as\na number) exactly, but what feature that version supports. The focus\nshould be the new protocol at daemon.c and maybe remote-curl.c where\nwe do know the current protocol is not flexible enough.\n-- \nDuy\n"},{"id":"256699","messageId":"CAGZ79kay+EXKaZ0UibCB=t4s5cVb28zgvUXeVRbd9d+XywhAsw@mail.gmail.com","threadId":"38638","inReplyTo":"CACsJy8ASR-O-7tozw=p1Ek0ugct5EVZyWtxY_YA2nqcUV_+ECw@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-26T20:08:37Z","receivedAt":"2015-02-26T20:08:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 26, 2015 at 2:15 AM, Duy Nguyen wrote:\n> On Thu, Feb 26, 2015 at 2:31 PM, Stefan Beller wrote:\n>> On Wed, Feb 25, 2015 at 10:04 AM, Junio C Hamano wrote:\n>>> Duy Nguyen writes:\n>>>\n>>>> On Wed, Feb 25, 2015 at 6:37 AM, Stefan Beller wrote:\n>>>>> I can understand, that we maybe want to just provide one generic\n>>>>> \"version 2\" of the protocol which is an allrounder not doing bad in\n>>>>> all of these aspects, but I can see usecases of having the desire to\n>>>>> replace the wire protocol by your own implementation. To do so\n>>>>> we could try to offer an API which makes implementing a new\n>>>>> protocol somewhat easy. The current state of affairs is not providing\n>>>>> this flexibility.\n>>>>\n>>>> I think we are quite flexible after initial ref advertisement.\n>>>\n>>> Yes, that is exactly where my \"I am not convinced\" comes from.\n>>>\n>>\n>> We are not. (not really at least). We can tune some parameters or\n>> change the behavior slightly,\n>> but we cannot fix core assumptions made when creating v2 protocol.\n>> This you can see when when talking about v1 as well: we cannot fix any\n>> wrongdoings of v1 now by adding another capability.\n>\n> Step 1 then should be identifying these wrongdoings and assumptions.\n\nSo I think one of the key assumption was to not have many refs to advertise,\nand advertising the refs is fine under that assumption.\nSo from my point of view it is hard to change the general\n\n>\n> We can really go wild with these capabilities. The only thing that\n> can't be changed is perhaps sending the first ref. I don't know\n> whether we can accept a dummy first ref... After that point, you can\n> turn the protocol upside down because both client and server know what\n> it would be.\n\nSo the way I currently envision (the transition to and) version 2 of\nthe protocol:\n\nFirst connection (using the protocol as of now):\n\nServer: Here are all the refs and capabilities I can offer. The capabilities\n include \"not-send-refs-first\" (aka version2)\nClient: Ok, I'll store not-send-refs-first for next time. Now we will\ncontinue with\n these options: <...>. For now we continue using the current\nprotocol and I want\n to update the \"master\" branch.\nServer: Ok here is a pack file, and then master advances $SHA1..$SHA1\nClient: ok, thanks, bye\n\nFor the next connection I have different ideas:\n\nClient thinks v2 is supported, so it talks first: Last time we talked\nyour capabilities\nhashed to \"$SHA1\", is that still correct?\nServer: yes it is\n# In the above roundtrip we would have a new key assumption that the\ncapabilities\n# don't change often. Having push-certs enabled, this is invalid of\ntoday. Hover this\n# could be implemented with very low bandwidth usage\n# The alternative path would be:\n# Server: No my new capabilities are: <....>\nClient: Ok I want to update all of refs/heads/{master,next,pu}. My\nlast fetch was\n yesterday at noon.\nServer: Let me check the ref logs for these refs. Here is a packfile of length\n 1000 bytes: \n {master, next} did not update since yesterday noon, pu updates from A..B\nClient: ok, thanks, bye\n\n\nAnother approach would be this:\nClient thinks v2 is supported, so it talks first: Last time we talked\nyou sent me\n refs advertisement including capabilities which hash to $SHA1.\nServer: I see, I have stored that. Now that time has advanced there are a few\n differences, here is a diff of the refs advertisement:\n* b3a551adf53c224b04c40f05b72a8790807b3138 HEAD\\0 \n* b3a551adf53c224b04c40f05b72a8790807b3138 refs/heads/master\n- 24ca137a384aa1ac5a776eddaf35bb820fc6f6e6 refs/heads/tmp-fix\n+ 1da8335ad5d0e46062a929ba6481bbbe35c8eef0 refs/pull/123/head\n\n Note that I do not include changed lines as +one line and - one\nline as you know\n what the line was by your given $SHA1, so changed lines are marked\nwith *, while\n lines starting with '-' indicate deleted refs and '+' indicate new refs.\nClient: I see, I can reconstruct the refs advertisement. Now we can\ncontinue talking\nas we always talked using v1 protocol.\n\n>\n>> So from my point\n>> of view we don't waste resources when having an advertisement of\n>> possible protocols instead of a boolean flag indicating v2 is\n>> supported. There is really not much overhead in coding nor bytes\n>> exchanged on the wire, so why not accept stuff that comes for free\n>> (nearly) ?\n>\n> You realize you're advertising v2 as a new capability, right? Instead\n> of defining v2 feature set then advertise v2, people could simply add\n> new features directly. I don't see v2 (at least with these patches)\n> adds any value.\n\nYes, we can go wild after the refs advertisement, but that is not the\ncritical problem as it works ok-ish? The problem I see for now is the huge\nrefs advertisement even before the capabilities are exchanged. So maybe\nI do not want to talk about v2 but about changing the current protocol to first\ntalk about the capabilities in the first round trip, not sure if we\never want to attach\ndata into the first RT as it may explode as soon as that information grows in\nthe future. The disadvantage would be one added round trip time, though I'd\nguess that's ok for most compared to the huge refs advertisement.\n\n>\n>> I mean how do we know all the core assumptions made for v2 hold in the\n>> future? We don't. That's why I'd propose a plain and easy exchange at\n>> first stating the version to talk.\n>\n> And we already does that, except that we don't state what version (as\n> a number) exactly, but what feature that version supports. The focus\n> should be the new protocol at daemon.c and maybe remote-curl.c where\n> we do know the current protocol is not flexible enough.\n\nMost features are pretty non-invasive and at a few points in the code, you\ncan add\n if (featureX)\n doFeatureXRelatedStuff();\n\nFor changing the initial talking we'd need to make a more drastic change\nto the code, which is why I thought to call it v2.\n\nThanks for your input,\nStefan\n\n> --\n> Duy\n"},{"id":"256701","messageId":"xmqqzj80l9c7.fsf@gitster.dls.corp.google.com","threadId":"38638","inReplyTo":"CACsJy8ASR-O-7tozw=p1Ek0ugct5EVZyWtxY_YA2nqcUV_+ECw@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-26T20:13:44Z","receivedAt":"2015-02-26T20:13:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> Step 1 then should be identifying these wrongdoings and assumptions.\n>\n> We can really go wild with these capabilities. The only thing that\n> can't be changed is perhaps sending the first ref. I don't know\n> whether we can accept a dummy first ref... After that point, you can\n> turn the protocol upside down because both client and server know what\n> it would be.\n\nYes, exactly. To up/down/side-grade from v1 is technically\npossible, but being technically possible is different from being\nsensible. The capability-based sidegrade does not solve the problem\nwhen the problem to be solved is that the server side needs to spend\na lot of cycles and the network needs to carry megabytes of data\nbefore capability exchange happens. Yes, the newer server and the\nnewer client can notice that the counterparty is new and start\ntalking in new protocol (which may or may not benefit from already\nknowing the result of ref advertisement), but by the time that\nhappens, the resource has already been spent and wasted.\n\nI do not think v1 can be fixed by \"send one ref with capability,\nnewer client may respond immediately so we can stop enumerating\nremaining refs and older one will get stuck so we can have a timeout\nto see if the connection is from the newer one, and send the rest\nfor the older client\", because anything that involves such a timeout\nwould not reliably work over WAN.\n\n> You realize you're advertising v2 as a new capability, right? Instead\n> of defining v2 feature set then advertise v2, people could simply add\n> new features directly. I don't see v2 (at least with these patches)\n> adds any value.\n\nI agree with the value assessment of these patches 98%, but these\nbits can be taken as the \"we have v2 server availble for you on the\nside, by the way\" hint you mentioned in the older thread, I think.\n\n> And we already does that, except that we don't state what version (as\n> a number) exactly, but what feature that version supports. The focus\n> should be the new protocol at daemon.c and maybe remote-curl.c where\n> we do know the current protocol is not flexible enough.\n\nThe \"first\" thing the client tells the server is what service it\nrequests. A request over git:// protocol is read by \"git daemon\" to\nchoose which service to run, and it is read directly by the login\nshell if it comes over ssh:// protocol.\n\nThere is nothing that prevents us from defining that service to be a\ngeneric \"git service\", not \"upload-pack\", \"archive\", \"receive-pack\".\nAnd the early protocol exchange, once \"git service\" is spawned, with\nthe client can be \"what real services does the server end support?\"\ncapability list responded with \"wow, you are new enough to support\nthe 'trickle-pack' service---please connect me to it\" request.\n"},{"id":"256731","messageId":"CAGZ79kayaD792tvfySQFXsmk==p=V4+aw+c79RM1eCZED-yzsQ@mail.gmail.com","threadId":"38638","inReplyTo":"xmqqzj80l9c7.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-27T01:26:40Z","receivedAt":"2015-02-27T01:26:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 26, 2015 at 12:13 PM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n>\n>> Step 1 then should be identifying these wrongdoings and assumptions.\n>>\n>> We can really go wild with these capabilities. The only thing that\n>> can't be changed is perhaps sending the first ref. I don't know\n>> whether we can accept a dummy first ref... After that point, you can\n>> turn the protocol upside down because both client and server know what\n>> it would be.\n>\n> Yes, exactly. To up/down/side-grade from v1 is technically\n> possible, but being technically possible is different from being\n> sensible. The capability-based sidegrade does not solve the problem\n> when the problem to be solved is that the server side needs to spend\n> a lot of cycles and the network needs to carry megabytes of data\n> before capability exchange happens. Yes, the newer server and the\n> newer client can notice that the counterparty is new and start\n> talking in new protocol (which may or may not benefit from already\n> knowing the result of ref advertisement), but by the time that\n> happens, the resource has already been spent and wasted.\n>\n> I do not think v1 can be fixed by \"send one ref with capability,\n> newer client may respond immediately so we can stop enumerating\n> remaining refs and older one will get stuck so we can have a timeout\n> to see if the connection is from the newer one, and send the rest\n> for the older client\", because anything that involves such a timeout\n> would not reliably work over WAN.\n>\n>> You realize you're advertising v2 as a new capability, right? Instead\n>> of defining v2 feature set then advertise v2, people could simply add\n>> new features directly. I don't see v2 (at least with these patches)\n>> adds any value.\n>\n> I agree with the value assessment of these patches 98%, but these\n> bits can be taken as the \"we have v2 server availble for you on the\n> side, by the way\" hint you mentioned in the older thread, I think.\n>\n>> And we already does that, except that we don't state what version (as\n>> a number) exactly, but what feature that version supports. The focus\n>> should be the new protocol at daemon.c and maybe remote-curl.c where\n>> we do know the current protocol is not flexible enough.\n>\n> The \"first\" thing the client tells the server is what service it\n> requests. A request over git:// protocol is read by \"git daemon\" to\n> choose which service to run, and it is read directly by the login\n> shell if it comes over ssh:// protocol.\n>\n> There is nothing that prevents us from defining that service to be a\n> generic \"git service\", not \"upload-pack\", \"archive\", \"receive-pack\".\n> And the early protocol exchange, once \"git service\" is spawned, with\n> the client can be \"what real services does the server end support?\"\n> capability list responded with \"wow, you are new enough to support\n> the 'trickle-pack' service---please connect me to it\" request.\n>\n\nSo I am not quite sure how to understand this input.\n\nI wonder if a high level test could look like the following,\nwhich just tests the workflow with git fetch, but not the\ninternals.\n\n(Note: patch formatting may be broken as it's send via gmail web ui)\n---8<---\nFrom: Stefan Beller \nDate: Thu, 26 Feb 2015 17:19:30 -0800\nSubject: [PATCH] Propose new tests for transitioning to the new option\ntransport.capabilitiesfirst\n\nSigned-off-by: Stefan Beller \n---\n t/t5544-capability-handshake.sh | 81 +++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 81 insertions(+)\n create mode 100755 t/t5544-capability-handshake.sh\n\ndiff --git a/t/t5544-capability-handshake.sh b/t/t5544-capability-handshake.sh\nnew file mode 100755\nindex 0000000..aa2b52d\n--- /dev/null\n+++ b/t/t5544-capability-handshake.sh\n@@ -0,0 +1,81 @@\n+#!/bin/sh\n+\n+test_description='fetching from a repository using the \"capabilities\nfirst\" push option'\n+\n+. ./test-lib.sh\n+\n+mk_repo_pair () {\n+ rm -rf workbench upstream &&\n+ test_create_repo upstream &&\n+ test_create_repo workbench &&\n+ (\n+ cd upstream &&\n+ git config receive.denyCurrentBranch warn\n+ ) &&\n+ (\n+ cd workbench &&\n+ git remote add origin ../upstream\n+ )\n+}\n+\n+generate_commits_upstream () {\n+ (\n+ cd upstream &&\n+ echo \"more content\" >>file &&\n+ git add file &&\n+ git commit -a -m \"create a commit\"\n+ )\n+}\n+\n+# Compare the ref ($1) in upstream with a ref value from workbench ($2)\n+# i.e. test_refs second HEAD@{2}\n+test_refs () {\n+ test $# = 2 &&\n+ git -C upstream rev-parse --verify \"$1\" >expect &&\n+ git -C workbench rev-parse --verify \"$2\" >actual &&\n+ test_cmp expect actual\n+}\n+\n+test_expect_success 'transport.capabilitiesfirst is not overridden\nwhen set already' '\n+ mk_repo_pair &&\n+ (\n+ cd workbench &&\n+ git config transport.capabilitiesfirst 0\n+ git config --get transport.capabilitiesfirst 0 >expected\n+ )\n+ generate_commits_upstream &&\n+ (\n+ cd workbench &&\n+ git fetch --all\n+ git config --get transport.capabilitiesfirst >actual\n+ test_cmp expected actual\n+ )\n+'\n+\n+test_expect_success 'enable transport by fetching from new server' '\n+ mk_repo_pair &&\n+ (\n+ cd workbench &&\n+ git fetch origin\n+ ) &&\n+ generate_commits_upstream &&\n+ (\n+ cd workbench &&\n+ git config --get transport.capabilitiesfirst >actual &&\n+ echo \"1\" >expected &&\n+ test_cmp expected actual\n+ )\n+'\n+\n+test_expect_success 'test new transport still works' '\n+ # continue from last test\n+ generate_commits_upstream &&\n+ (\n+ cd workbench &&\n+ git fetch origin\n+ )\n+ test_refs HEAD refs/remotes/origin/HEAD\n+ test_refs master refs/remotes/origin/master\n+'\n+\n+test_done\n-- \n2.3.0.81.gc37f363\n"},{"id":"256735","messageId":"CAGZ79kbmdrZSXAwQAWDboBjA0Y8223WWOD_-M=wBkRHWpL6_hQ@mail.gmail.com","threadId":"38638","inReplyTo":"CAGZ79kayaD792tvfySQFXsmk==p=V4+aw+c79RM1eCZED-yzsQ@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-27T02:15:03Z","receivedAt":"2015-02-27T02:15:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":" On Thu, Feb 26, 2015 at 12:13 PM, Junio C Hamano wrote:\n>\n> I agree with the value assessment of these patches 98%, but these\n> bits can be taken as the \"we have v2 server availble for you on the\n> side, by the way\" hint you mentioned in the older thread, I think.\n\nThe patches are not well polished (In fact they don't even compile :/),\nbut I think they may demonstrate the ideas and though process. And\nas it turns out we'd not be following that spirit of ideas but rather want\nto have a dedicated v2.\n\nThat said I did not want to spend lots of time to polish the patch for\ninclusion but rather to demonstrate ideas, which can be done with\nsubstantial less quality IMHO. Correct me if I am wrong here!\n\nThanks,\nStefan\n"},{"id":"256756","messageId":"CAGZ79ka8zwnT+dn5AHNhvEbMTuaicZiZLZY85baFA=aYSp+OFg@mail.gmail.com","threadId":"38638","inReplyTo":"CACsJy8DOS_999ZgW7TqsH-dkrUFpjZf0TFQeFUt9s0bNhHY0Bw@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-27T22:20:09Z","receivedAt":"2015-02-27T22:20:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"+git@vger.kernel.org\n\nOn Thu, Feb 26, 2015 at 5:42 PM, Duy Nguyen wrote:\n> https://github.com/pclouds/git/commits/uploadpack2\n\nI rebased your branch, changed the order of commits slightly and\nstarted to add some.\nthey are found at https://github.com/stefanbeller/git/commits/uploadpack2\n\nI think the very first patch series which I try to polish now will\njust try to move the\ncapabilities negotiation into the beginning of the exchange.\n\nAny 'real' changes such as adding capabilities to the protocol to not\nhave all the refs\nadvertised will come in a later series.\n\nThanks for your help!\nStefan\n"},{"id":"256758","messageId":"xmqqioenhs4p.fsf@gitster.dls.corp.google.com","threadId":"38638","inReplyTo":"xmqqzj80l9c7.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-27T23:05:58Z","receivedAt":"2015-02-27T23:05:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I do not think v1 can be fixed by \"send one ref with capability,\n> newer client may respond immediately so we can stop enumerating\n> remaining refs and older one will get stuck so we can have a timeout\n> to see if the connection is from the newer one, and send the rest\n> for the older client\", because anything that involves such a timeout\n> would not reliably work over WAN.\n\nJust for fun, I was trying to see if there is a hole in the current\nprotocol that allows a new client to talk a valid v1 protocol\nexchange with existing, deployed servers without breaking, while\nletting it to know a new server that it is a new client and it does\nnot want to get blasted by megabytes of ref advertisement.\n\nThe idea is to find a request that can be sent as the first\nutterance by the client to an old server that is interpreted as a\nno-op and can be recognised by a new server as such a \"no-op probe\".\nIf there is such a request, then the exchange can go like this with\n(new client, old server) pair:\n\n - new client connects and sends that no-op.\n\n - old server starts blasting the ref advertisement\n\n - new client monitors and notices that the other side\n started speaking, and the ref advertisement lacks the\n capability bit for new protocol.\n\n - new client accepts the ref advertisement and does the v1\n protocol thing as a follow-up to what it already sent.\n\nAs long as the first one turns out to be no-op for old server, we\nwould be OK. On the other hand, (new client, new server) pair\nwould go like this:\n\n - new client connects and sends that no-op.\n\n - new server notices that there is already a data from the\n client, and recognises the \"no-op probe\".\n\n - new server gives the first v2 protocol message with\n capability.\n\n - new client notices thqat the other side started speaking, and\n it is the first v2 protocol message.\n\n - both sides happily speak v2.\n\nand (old client, new server) pair would go like this:\n\n - old client connects and waits.\n\n - new server notices that there is *no* data sent from the\n client and decides that the other side is a v1 client. It\n starts blasting the ref advertisement.\n\n - both sides happily speak v1 from here on.\n\nA misdetected case between (new client, new server) pair might go\nlike this:\n\n - new client connects and sends that no-op.\n\n - new server accepts the connection, but that no-op probe has\n not arrived yet.\". It misdetects the other side as a v1\n client and it starts blasting the ref advertisement.\n\n - new client notices that the ref advertisement has the\n capability bit and the server is capable of v2 protocol. it\n waits until the server sends \"sorry, I misdetected\" message.\n\n - new server eventually notices the \"no-op probe\" while blasting\n the ref advertisement and it can stop in the middle.\n hopefully this can happen after only sending a few kilobytes\n among megabytes of ref advertisement data ;-). The server\n sends \"sorry, I misdetected\" message to synchronise.\n\n - both sides happily speak v2 from here on.\n\nSo the topic of this exercise (\"just for fun\") is to see if there is\nsuch a no-op request the client side can send as the first thing for\nprobing.\n\nOn the fetch side, the first response upload-pack expects are one\nof:\n\n - \"want \" followed by an object name.\n - \"shallow \" followed by an object name.\n - \"deepen \" followed by a positive integer.\n\nAnd there _is_ a hole ;-). The parsing of \"shallow \" object name is\ndone in such a way that an object name that passes get_sha1_hex()\nthat results in a NULL return from parse_object() is _ignored_. So\na new client can use \"shallow 0{40}\" as a no-op probe.\n\nIt appears that on the push side, there is a similar hole that can\nbe used. receive-pack expects either \"shallow \", \"push-cert\" or the\nrefname updates (i.e. two \"[0-9a-f]{40}\" followed by a refname); the\nparsing of \"shallow \" is not as loose as the fetch side in that\nusing a \"shallow 0{40}\" as a no-op probe will end up causing\nprepare_shallow_info() sift the \"0{40}\" object name into \"theirs\",\nbut I think it will be ignored at the end as \"unreachable cruft\"\nwithout causing harm.\n\nI am _not_ proposing that we should go this route, at least not yet.\nI am merely pointing out that an in-place sidegrade from v1 to a\nprotocol that avoids the megabyte-advertisement-at-the-beginning\nseems to be possible, as a food for thought.\n"},{"id":"256759","messageId":"CAGZ79kY6B4BLvLVS-J50SqCz+t9uGd93WHxCYKmRU1Ey3qVg+A@mail.gmail.com","threadId":"38638","inReplyTo":"xmqqioenhs4p.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-27T23:44:45Z","receivedAt":"2015-02-27T23:44:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Feb 27, 2015 at 3:05 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> I do not think v1 can be fixed by \"send one ref with capability,\n>> newer client may respond immediately so we can stop enumerating\n>> remaining refs and older one will get stuck so we can have a timeout\n>> to see if the connection is from the newer one, and send the rest\n>> for the older client\", because anything that involves such a timeout\n>> would not reliably work over WAN.\n>\n> Just for fun, I was trying to see if there is a hole in the current\n> protocol that allows a new client to talk a valid v1 protocol\n> exchange with existing, deployed servers without breaking, while\n> letting it to know a new server that it is a new client and it does\n> not want to get blasted by megabytes of ref advertisement.\n>\n> The idea is to find a request that can be sent as the first\n> utterance by the client to an old server that is interpreted as a\n> no-op and can be recognised by a new server as such a \"no-op probe\".\n> If there is such a request, then the exchange can go like this with\n> (new client, old server) pair:\n>\n> - new client connects and sends that no-op.\n>\n> - old server starts blasting the ref advertisement\n>\n> - new client monitors and notices that the other side\n> started speaking, and the ref advertisement lacks the\n> capability bit for new protocol.\n>\n> - new client accepts the ref advertisement and does the v1\n> protocol thing as a follow-up to what it already sent.\n>\n> As long as the first one turns out to be no-op for old server, we\n> would be OK. On the other hand, (new client, new server) pair\n> would go like this:\n>\n> - new client connects and sends that no-op.\n>\n> - new server notices that there is already a data from the\n> client, and recognises the \"no-op probe\".\n>\n> - new server gives the first v2 protocol message with\n> capability.\n>\n> - new client notices thqat the other side started speaking, and\n> it is the first v2 protocol message.\n>\n> - both sides happily speak v2.\n>\n> and (old client, new server) pair would go like this:\n>\n> - old client connects and waits.\n>\n> - new server notices that there is *no* data sent from the\n> client and decides that the other side is a v1 client. It\n> starts blasting the ref advertisement.\n>\n> - both sides happily speak v1 from here on.\n>\n> A misdetected case between (new client, new server) pair might go\n> like this:\n>\n> - new client connects and sends that no-op.\n>\n> - new server accepts the connection, but that no-op probe has\n> not arrived yet.\". It misdetects the other side as a v1\n> client and it starts blasting the ref advertisement.\n>\n> - new client notices that the ref advertisement has the\n> capability bit and the server is capable of v2 protocol. it\n> waits until the server sends \"sorry, I misdetected\" message.\n>\n> - new server eventually notices the \"no-op probe\" while blasting\n> the ref advertisement and it can stop in the middle.\n> hopefully this can happen after only sending a few kilobytes\n> among megabytes of ref advertisement data ;-). The server\n> sends \"sorry, I misdetected\" message to synchronise.\n>\n> - both sides happily speak v2 from here on.\n>\n> So the topic of this exercise (\"just for fun\") is to see if there is\n> such a no-op request the client side can send as the first thing for\n> probing.\n>\n> On the fetch side, the first response upload-pack expects are one\n> of:\n>\n> - \"want \" followed by an object name.\n> - \"shallow \" followed by an object name.\n> - \"deepen \" followed by a positive integer.\n>\n> And there _is_ a hole ;-). The parsing of \"shallow \" object name is\n> done in such a way that an object name that passes get_sha1_hex()\n> that results in a NULL return from parse_object() is _ignored_. So\n> a new client can use \"shallow 0{40}\" as a no-op probe.\n>\n> It appears that on the push side, there is a similar hole that can\n> be used. receive-pack expects either \"shallow \", \"push-cert\" or the\n> refname updates (i.e. two \"[0-9a-f]{40}\" followed by a refname); the\n> parsing of \"shallow \" is not as loose as the fetch side in that\n> using a \"shallow 0{40}\" as a no-op probe will end up causing\n> prepare_shallow_info() sift the \"0{40}\" object name into \"theirs\",\n> but I think it will be ignored at the end as \"unreachable cruft\"\n> without causing harm.\n>\n> I am _not_ proposing that we should go this route, at least not yet.\n> I am merely pointing out that an in-place sidegrade from v1 to a\n> protocol that avoids the megabyte-advertisement-at-the-beginning\n> seems to be possible, as a food for thought.\n>\n\n\nThis is a fun thing indeed, though I'd personally feel uneasy with\nsuch a probe as\na serious proposal. (Remember somebody 10 years from now wants to enjoy\nreading the source code). So let's keep the idea around if we don't find another\nsolution.\n\nAs far as I can tell we have\n* native git protocol (git daemon)\n* ssh\n* http(s)\n* ftp (deprecated!)\n* rsync(deprecated)\n\nFor both native git as well as ssh, Duy presented a solution at [1, 2]\na year ago,\nwhich essentially presents the desired client capabilites 'out of\nband' to the server\nvia an argument to the server. So we'd only need to examine the\nhttp(s) path how to\npass in arguments there.\n\nSince 5 years an additional argument in the git protocol would cause\nno harm, so it's\npretty safe to extend it with caution. We'd advertise the\nclient-may-ask-for-capabilities-out-of-band\nper server and the client would eventually learn about it and only use\nthis for servers\nwhich advertised they can do so. And nobody sane out there will\ndowngrade a version\nof today to a git of 5 years ago.\n\nI did rebase Duys' patches and I am trying to send out a patch series\ntoday going that route\nasking for comments.\n\nThanks a lot for the digging!\nStefan\n\n[1] https://github.com/pclouds/git/commit/e26fa77c4d9ace06b9f2c80091af9eb7b63a1c95\n[2] https://github.com/pclouds/git/commit/20d048e5fc650b20fdc7dd8bbe35cb8510ac9c50\n"},{"id":"256760","messageId":"CACsJy8AqRPm8ked+u0k83R2sFCE+XCGDEzeT_d4QsrKOOXUwLw@mail.gmail.com","threadId":"38638","inReplyTo":"xmqqioenhs4p.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-02-28T00:07:17Z","receivedAt":"2015-02-28T00:07:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 28, 2015 at 6:05 AM, Junio C Hamano wrote:\n> Just for fun, I was trying to see if there is a hole in the current\n> protocol that allows a new client to talk a valid v1 protocol\n> exchange with existing, deployed servers without breaking, while\n> letting it to know a new server that it is a new client and it does\n> not want to get blasted by megabytes of ref advertisement.\n> ...\n> The idea is to find a request that can be sent as the first\n> utterance by the client to an old server that is interpreted as a\n> no-op and can be recognised by a new server as such a \"no-op probe\".\n> ...\n> And there _is_ a hole ;-). The parsing of \"shallow \" object name is\n> done in such a way that an object name that passes get_sha1_hex()\n> that results in a NULL return from parse_object() is _ignored_. So\n> a new client can use \"shallow 0{40}\" as a no-op probe.\n> ...\n> I am _not_ proposing that we should go this route, at least not yet.\n> I am merely pointing out that an in-place sidegrade from v1 to a\n> protocol that avoids the megabyte-advertisement-at-the-beginning\n> seems to be possible, as a food for thought.\n\nThere may be another hole, if we send \"want \", it looks\nlike it will go through without causing errors. It's not exactly no-op\nbecause an empty tree object will be bundled in result pack. But that\nmakes no difference in pratice. I didn't verify this though.\n\nIn the spirit of fun, I looked at how jgit handles this shallow line\n(because this is more like an implementation hole than protocol hole).\nI don't think jgit would ignore 0{40} the way C Git does. This SHA-1\nwill end up in shallowCommits set in upload-pack, then will be parsed\nas a commit. But even if the parsing is through, a non-empty\nshallowCommits set would disable pack bitmap. Fun is usually short..\n\nPS. heh my \"want empty-tree\" hole is probably impl-specific too. Not\nsure if jgit also keeps empty tree available even if it does not\nexist.\n-- \nDuy\n"},{"id":"256761","messageId":"CAPc5daUhN5OcQRs6Xn2oHDioZVze0VaPZFJ4OQCNKFy642hjWA@mail.gmail.com","threadId":"38638","inReplyTo":"CACsJy8AqRPm8ked+u0k83R2sFCE+XCGDEzeT_d4QsrKOOXUwLw@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-28T00:26:58Z","receivedAt":"2015-02-28T00:26:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Fri, Feb 27, 2015 at 4:07 PM, Duy Nguyen wrote:\n>\n> There may be another hole, if we send \"want \", it looks\n> like it will go through without causing errors. It's not exactly no-op\n> because an empty tree object will be bundled in result pack. But that\n> makes no difference in pratice. I didn't verify this though.\n\nIn addition to \"that's not a no-op\" problem, unless the old server has a\nref that has an emtpy tree at its tip, such a fetch request will be rejected,\nunless the server is configured to serve any object, no?\n\nIf your new server does have a ref that points at an empty tree, a client\nmay request you to send that, but this is not a problem, because the\nnew server can tell if the client is sending it as a no-op probe or a serious\nrequest by looking at its capability request. A serious old client will not\ntell you that he is new, a probing new client does, and a serious new\nclient does. So your new server can tell and will not be confused.\n\n> as a commit. But even if the parsing is through, a non-empty\n> shallowCommits set would disable pack bitmap.\n\nPerformance penalty is fine. Over time we would upgrade and the\npoint of the exercise is not to cause the old-new or new-old pair to\ndie but keep talking the old protocol and getting correct results.\n"},{"id":"256762","messageId":"CAPc5daXJ6s2oNvqSmtp5d-Dgm-EX6Mb8kY2nOLQVxAT-3wjAmQ@mail.gmail.com","threadId":"38638","inReplyTo":"CAGZ79kY6B4BLvLVS-J50SqCz+t9uGd93WHxCYKmRU1Ey3qVg+A@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-28T00:33:00Z","receivedAt":"2015-02-28T00:33:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Fri, Feb 27, 2015 at 3:44 PM, Stefan Beller wrote:\n> On Fri, Feb 27, 2015 at 3:05 PM, Junio C Hamano wrote:\n>>\n>> I am _not_ proposing that we should go this route, at least not yet.\n>> I am merely pointing out that an in-place sidegrade from v1 to a\n>> protocol that avoids the megabyte-advertisement-at-the-beginning\n>> seems to be possible, as a food for thought.\n>\n> This is a fun thing indeed, though I'd personally feel uneasy with\n> such a probe as\n> a serious proposal. (Remember somebody 10 years from now wants to enjoy\n> reading the source code).\n\nThat cannot be a serious objection, once you realize that NUL + capability\nwas exactly the same kind of \"yes, we have a hole to allow up customize\nthe protocol\". The code to do so may not be pretty, but the code to implement\nended up being reasonably clean with parse_feature_request() and friends.\nAfter all we live in a real world ;-)\n"},{"id":"256763","messageId":"CAGZ79ka8Zg86qqvWByNiP3F6a9QggO-bNY3ZZ9g+A-MdKYQ7NQ@mail.gmail.com","threadId":"38638","inReplyTo":"CAPc5daXJ6s2oNvqSmtp5d-Dgm-EX6Mb8kY2nOLQVxAT-3wjAmQ@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-28T00:46:47Z","receivedAt":"2015-02-28T00:46:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Feb 27, 2015 at 4:33 PM, Junio C Hamano wrote:\n> On Fri, Feb 27, 2015 at 3:44 PM, Stefan Beller wrote:\n>> On Fri, Feb 27, 2015 at 3:05 PM, Junio C Hamano wrote:\n>>>\n>>> I am _not_ proposing that we should go this route, at least not yet.\n>>> I am merely pointing out that an in-place sidegrade from v1 to a\n>>> protocol that avoids the megabyte-advertisement-at-the-beginning\n>>> seems to be possible, as a food for thought.\n>>\n>> This is a fun thing indeed, though I'd personally feel uneasy with\n>> such a probe as\n>> a serious proposal. (Remember somebody 10 years from now wants to enjoy\n>> reading the source code).\n>\n> That cannot be a serious objection, once you realize that NUL + capability\n> was exactly the same kind of \"yes, we have a hole to allow up customize\n> the protocol\". The code to do so may not be pretty, but the code to implement\n> ended up being reasonably clean with parse_feature_request() and friends.\n> After all we live in a real world ;-)\n\n - new server accepts the connection, but that no-op probe has\n not arrived yet.\". It misdetects the other side as a v1\n client and it starts blasting the ref advertisement.\n\nA race condition may be a serious objection then? Once people believe the\nrefs can scale fairly well they will use it, which means blasting the ref\nadvertisement will become very worse over time.\nI'll try to present a 'client asks for options first out of band' instead of the\nway you describe.\n\nAlso we should not rely on having holes here and there. (We might run out of\nholes over time), so I'd rather have the capabilities presented at first\nwhich rather opens new holes instead of closing old ones.\n\n(assuming we'll never run into megabytes of capabilities\nover time to have the same trouble again ;)\n"},{"id":"256769","messageId":"1425085318-30537-1-git-send-email-sbeller@google.com","threadId":"38638","inReplyTo":"CAGZ79ka8Zg86qqvWByNiP3F6a9QggO-bNY3ZZ9g+A-MdKYQ7NQ@mail.gmail.com","subject":"[RFC/PATCH 0/5] protocol v2 for upload-pack","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-28T01:01:53Z","receivedAt":"2015-02-28T01:01:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Heavily inspired by the ideas of Duy, who wrote the first\npatches nearly a year ago.\n\nNguyễn Thái Ngọc Duy (2):\n upload-pack: only accept capabilities on the first \"want\" line\n upload-pack: support out of band client capability requests\n\nStefan Beller (3):\n connect.c: connect to a remote service with some flags\n daemon.c: accept extra service arguments\n WIP/Document the http protocol change\n\n Documentation/git-upload-pack.txt | 10 +++++-\n Documentation/technical/http-protocol.txt | 4 +--\n Documentation/technical/protocol-capabilities.txt | 4 +++\n builtin/fetch-pack.c | 2 +-\n builtin/send-pack.c | 2 +-\n connect.c | 21 ++++++------\n connect.h | 2 +-\n daemon.c | 37 +++++++++++++--------\n transport.c | 3 +-\n upload-pack.c | 39 +++++++++++++++++------\n 10 files changed, 86 insertions(+), 38 deletions(-)\n\n-- \n2.3.0.81.gc37f363\n"},{"id":"256764","messageId":"1425085318-30537-2-git-send-email-sbeller@google.com","threadId":"38638","inReplyTo":"1425085318-30537-1-git-send-email-sbeller@google.com","subject":"[RFC/PATCH 1/5] upload-pack: only accept capabilities on the first \"want\" line","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-28T01:01:54Z","receivedAt":"2015-02-28T01:01:54Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\npack-protocol.txt says so and fetch-pack also follows it even though\nupload-pack is a bit lax. Fix it.\n\nSigned-off-by: Stefan Beller \n---\n upload-pack.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex e0ce2bf..d9230ba 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -558,6 +558,7 @@ static void receive_needs(void)\n \tstruct object_array shallows = OBJECT_ARRAY_INIT;\n \tint depth = 0;\n \tint has_non_tip = 0;\n+\tint first_want = 1;\n \n \tshallow_nr = 0;\n \tfor (;;) {\n@@ -596,7 +597,11 @@ static void receive_needs(void)\n \t\t\tdie(\"git upload-pack: protocol error, \"\n \t\t\t \"expected to get sha, not '%s'\", line);\n \n-\t\tparse_features(line + 45);\n+\t\tif (first_want) {\n+\t\t\tparse_features(line + 45);\n+\t\t\tfirst_want = 0;\n+\t\t} else if (line[45])\n+\t\t\tdie(\"garbage at the end of 'want' line %s\", line + 45);\n \n \t\to = parse_object(sha1_buf);\n \t\tif (!o)\n-- \n2.3.0.81.gc37f363\n"},{"id":"256765","messageId":"1425085318-30537-3-git-send-email-sbeller@google.com","threadId":"38638","inReplyTo":"1425085318-30537-1-git-send-email-sbeller@google.com","subject":"[RFC/PATCH 2/5] upload-pack: support out of band client capability requests","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-28T01:01:55Z","receivedAt":"2015-02-28T01:01:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nThe only difference from the original protocol client capabilities are\nnegotiated before initial refs advertisment.\n\nClient capabilities are sent out of band (upload-pack receives it as\nthe second command line argument). The server sends one pkt-line back\nadvertising its capabilities.\n\nSigned-off-by: Stefan Beller \n---\n\nNotes:\n v1:\n I am still undecided if the client should then accept/resend\n the capabilities to confirm them, which would make the client the\n ultimate decider which capabilities are used.\n \n My gut feeling is to rather let the server make the final decision\n for the capabilities, as it will use some requested capabilities\n already to not send out all the refs.\n\n Documentation/git-upload-pack.txt | 10 +++++++++-\n upload-pack.c | 42 +++++++++++++++++++++++++++------------\n 2 files changed, 38 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt\nindex 0abc806..ad3a89d 100644\n--- a/Documentation/git-upload-pack.txt\n+++ b/Documentation/git-upload-pack.txt\n@@ -9,7 +9,7 @@ git-upload-pack - Send objects packed back to git-fetch-pack\n SYNOPSIS\n --------\n [verse]\n-'git-upload-pack' [--strict] [--timeout=] \n+'git-upload-pack' [--strict] [--timeout=] []\n \n DESCRIPTION\n -----------\n@@ -34,6 +34,14 @@ OPTIONS\n ::\n \tThe repository to sync from.\n \n+capabilities::\n+\tHistorically the capabilities were exchanged inside the protocol of\n+\t'git-upload-pack' talking to 'git-fetch-pack'. It turned out this was\n+\ttoo late as 'git-upload-pack' already did work, which may have been\n+\tavoided. This allows to pass in the capabilities the client wants to\n+\tuse as one argument. The capabilites are separated by space.\n+\tSee technical/protocol-capabilities.txt (TODO: how to make it a link?)\n+\n SEE ALSO\n --------\n linkgit:gitnamespaces[7]\ndiff --git a/upload-pack.c b/upload-pack.c\nindex d9230ba..2e62c3f 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -31,6 +31,11 @@ static const char upload_pack_usage[] = \"git upload-pack [--strict] [--timeout=<\n \n static unsigned long oldest_have;\n \n+/**\n+ * client capabilities presented as program arguments, dissallow further\n+ * capabilities sent by client\n+ */\n+static int capabilities_first;\n static int multi_ack;\n static int no_done;\n static int use_thin_pack, use_ofs_delta, use_include_tag;\n@@ -597,11 +602,14 @@ static void receive_needs(void)\n \t\t\tdie(\"git upload-pack: protocol error, \"\n \t\t\t \"expected to get sha, not '%s'\", line);\n \n-\t\tif (first_want) {\n-\t\t\tparse_features(line + 45);\n-\t\t\tfirst_want = 0;\n-\t\t} else if (line[45])\n-\t\t\tdie(\"garbage at the end of 'want' line %s\", line + 45);\n+\t\tif (!capabilities_first) {\n+\t\t\tif (first_want) {\n+\t\t\t\tparse_features(line + 45);\n+\t\t\t\tfirst_want = 0;\n+\t\t\t} else if (line[45]) {\n+\t\t\t\tdie(\"garbage at the end of 'want' line %s\", line + 45);\n+\t\t\t}\n+\t\t}\n \n \t\to = parse_object(sha1_buf);\n \t\tif (!o)\n@@ -840,17 +848,25 @@ int main(int argc, char **argv)\n \t\t}\n \t}\n \n-\tif (i != argc-1)\n-\t\tusage(upload_pack_usage);\n+\tswitch (argc - i) {\n+\t\tcase 2:\n+\t\t\tcapabilities_first = 1;\n+\t\t\tparse_features(argv[i + 1]);\n+\t\t\t/* fall through*/\n+\t\tcase 1:\n+\t\t\tsetup_path();\n \n-\tsetup_path();\n+\t\t\tdir = argv[i];\n \n-\tdir = argv[i];\n+\t\t\tif (!enter_repo(dir, strict))\n+\t\t\t\tdie(\"'%s' does not appear to be a git repository\", dir);\n \n-\tif (!enter_repo(dir, strict))\n-\t\tdie(\"'%s' does not appear to be a git repository\", dir);\n+\t\t\tgit_config(upload_pack_config, NULL);\n+\t\t\tupload_pack();\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tusage(upload_pack_usage);\n+\t}\n \n-\tgit_config(upload_pack_config, NULL);\n-\tupload_pack();\n \treturn 0;\n }\n-- \n2.3.0.81.gc37f363\n"},{"id":"256766","messageId":"1425085318-30537-4-git-send-email-sbeller@google.com","threadId":"38638","inReplyTo":"1425085318-30537-1-git-send-email-sbeller@google.com","subject":"[RFC/PATCH 3/5] connect.c: connect to a remote service with some flags","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-28T01:01:56Z","receivedAt":"2015-02-28T01:01:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"If this is over git protocol, the flags is appended as the next\nparameter after host=. If it's ssh, a new argument is appended to the\ncommand line.\n\nNone of the callers use this now though.\n\n[sb: originally by pclouds, rebased as jk implemented 1823bea10,\n(git_connect: use argv_array), so any error is mine]\n\nSigned-off-by: Stefan Beller \n---\n builtin/fetch-pack.c | 2 +-\n builtin/send-pack.c | 2 +-\n connect.c | 18 ++++++++++++------\n connect.h | 2 +-\n transport.c | 3 ++-\n 5 files changed, 17 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 4a6b340..7e5b5fd 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -171,7 +171,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\tif (args.diag_url)\n \t\t\tflags |= CONNECT_DIAG_URL;\n \t\tconn = git_connect(fd, dest, args.uploadpack,\n-\t\t\t\t flags);\n+\t\t\t\t NULL, flags);\n \t\tif (!conn)\n \t\t\treturn args.diag_url ? 0 : 1;\n \t}\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex b961e5a..c2a066a 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -261,7 +261,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \t\tfd[0] = 0;\n \t\tfd[1] = 1;\n \t} else {\n-\t\tconn = git_connect(fd, dest, receivepack,\n+\t\tconn = git_connect(fd, dest, receivepack, NULL,\n \t\t\targs.verbose ? CONNECT_VERBOSE : 0);\n \t}\n \ndiff --git a/connect.c b/connect.c\nindex 062e133..7b6b241 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -650,7 +650,9 @@ static struct child_process no_fork = CHILD_PROCESS_INIT;\n * the connection failed).\n */\n struct child_process *git_connect(int fd[2], const char *url,\n-\t\t\t\t const char *prog, int flags)\n+\t\t\t\t const char *prog,\n+\t\t\t\t const char *service_flags,\n+\t\t\t\t int flags)\n {\n \tchar *hostandport, *path;\n \tstruct child_process *conn = &no_fork;\n@@ -685,10 +687,13 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t * Note: Do not add any other headers here! Doing so\n \t\t * will cause older git-daemon servers to crash.\n \t\t */\n-\t\tpacket_write(fd[1],\n-\t\t\t \"%s %s%chost=%s%c\",\n-\t\t\t prog, path, 0,\n-\t\t\t target_host, 0);\n+\t\tif (!service_flags)\n+\t\t\tpacket_write(fd[1], \"%s %s%chost=%s%c\",\n+\t\t\t\t prog, path, 0, target_host, 0);\n+\t\telse\n+\t\t\tpacket_write(fd[1], \"%s %s%chost=%s%c%s%c\",\n+\t\t\t\t prog, path, 0, target_host, 0,\n+\t\t\t\t service_flags, 0);\n \t\tfree(target_host);\n \t} else {\n \t\tconn = xmalloc(sizeof(*conn));\n@@ -733,7 +738,8 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\tconn->use_shell = 1;\n \t\t}\n \t\targv_array_push(&conn->args, cmd.buf);\n-\n+\t\tif (service_flags)\n+\t\t\targv_array_push(&conn->args, service_flags);\n \t\tif (start_command(conn))\n \t\t\tdie(\"unable to fork\");\n \ndiff --git a/connect.h b/connect.h\nindex c41a685..c4fa8a1 100644\n--- a/connect.h\n+++ b/connect.h\n@@ -3,7 +3,7 @@\n \n #define CONNECT_VERBOSE (1u << 0)\n #define CONNECT_DIAG_URL (1u << 1)\n-extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);\n+extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, const char *service_flags, int flags);\n extern int finish_connect(struct child_process *conn);\n extern int git_connection_is_socket(struct child_process *conn);\n extern int server_supports(const char *feature);\ndiff --git a/transport.c b/transport.c\nindex 0694a7c..626fd92 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -495,6 +495,7 @@ static int connect_setup(struct transport *transport, int for_push, int verbose)\n \tdata->conn = git_connect(data->fd, transport->url,\n \t\t\t\t for_push ? data->options.receivepack :\n \t\t\t\t data->options.uploadpack,\n+\t\t\t\t NULL,\n \t\t\t\t verbose ? CONNECT_VERBOSE : 0);\n \n \treturn 0;\n@@ -850,7 +851,7 @@ static int connect_git(struct transport *transport, const char *name,\n {\n \tstruct git_transport_data *data = transport->data;\n \tdata->conn = git_connect(data->fd, transport->url,\n-\t\t\t\t executable, 0);\n+\t\t\t\t executable, NULL, 0);\n \tfd[0] = data->fd[0];\n \tfd[1] = data->fd[1];\n \treturn 0;\n-- \n2.3.0.81.gc37f363\n"},{"id":"256767","messageId":"1425085318-30537-5-git-send-email-sbeller@google.com","threadId":"38638","inReplyTo":"1425085318-30537-1-git-send-email-sbeller@google.com","subject":"[RFC/PATCH 4/5] daemon.c: accept extra service arguments","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-28T01:01:57Z","receivedAt":"2015-02-28T01:01:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Before 73bb33a (daemon: Strictly parse the \"extra arg\" part of the\ncommand - 2009-06-04) a client sending extra arguments could DoS\ngit-daemon. 73bb33a fixed it by forbidding extra arguments.\n\nAllow arguments other than \"host=\" again as a preparation step for\nupload-pack2. \"host=\" if present must be the first argument\nthough. The remaining arguments are concatenated by whitespace.\n\nSo far none of supported services support extra arguments. Attempting\nto do will abort the service, just like how it is before. We might\nwant to make them silently ignore extra arguments though.\n\nSigned-off-by: Stefan Beller \n---\n connect.c | 3 ---\n daemon.c | 37 ++++++++++++++++++++++++-------------\n 2 files changed, 24 insertions(+), 16 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 7b6b241..97dd732 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -683,9 +683,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t/*\n \t\t * Separate original protocol components prog and path\n \t\t * from extended host header with a NUL byte.\n-\t\t *\n-\t\t * Note: Do not add any other headers here! Doing so\n-\t\t * will cause older git-daemon servers to crash.\n \t\t */\n \t\tif (!service_flags)\n \t\t\tpacket_write(fd[1], \"%s %s%chost=%s%c\",\ndiff --git a/daemon.c b/daemon.c\nindex 54a03bd..c45d0d6 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -221,7 +221,7 @@ static const char *path_ok(const char *directory)\n \treturn NULL;\t\t/* Fallthrough. Deny by default */\n }\n \n-typedef int (*daemon_service_fn)(void);\n+typedef int (*daemon_service_fn)(const char *);\n struct daemon_service {\n \tconst char *name;\n \tconst char *config_name;\n@@ -302,7 +302,8 @@ error_return:\n \treturn -1;\n }\n \n-static int run_service(const char *dir, struct daemon_service *service)\n+static int run_service(const char *dir, struct daemon_service *service,\n+\t\t const char *args)\n {\n \tconst char *path;\n \tint enabled = service->enabled;\n@@ -361,7 +362,7 @@ static int run_service(const char *dir, struct daemon_service *service)\n \t */\n \tsignal(SIGTERM, SIG_IGN);\n \n-\treturn service->fn();\n+\treturn service->fn(args);\n }\n \n static void copy_to_log(int fd)\n@@ -403,27 +404,31 @@ static int run_service_command(const char **argv)\n \treturn finish_command(&cld);\n }\n \n-static int upload_pack(void)\n+static int upload_pack(const char *args)\n {\n \t/* Timeout as string */\n \tchar timeout_buf[64];\n-\tconst char *argv[] = { \"upload-pack\", \"--strict\", NULL, \".\", NULL };\n+\tconst char *argv[] = { \"upload-pack\", \"--strict\", NULL, \".\", NULL, NULL };\n \n \targv[2] = timeout_buf;\n+\targv[4] = args;\n \n \tsnprintf(timeout_buf, sizeof timeout_buf, \"--timeout=%u\", timeout);\n \treturn run_service_command(argv);\n }\n \n-static int upload_archive(void)\n+static int upload_archive(const char *args)\n {\n \tstatic const char *argv[] = { \"upload-archive\", \".\", NULL };\n+\tif (args)\n+\t\tdie(\"invalid request\");\n \treturn run_service_command(argv);\n }\n \n-static int receive_pack(void)\n+static int receive_pack(const char *args)\n {\n-\tstatic const char *argv[] = { \"receive-pack\", \".\", NULL };\n+\tstatic const char *argv[] = { \"receive-pack\", \".\", NULL, NULL };\n+\targv[2] = args;\n \treturn run_service_command(argv);\n }\n \n@@ -487,7 +492,7 @@ static void parse_host_and_port(char *hostport, char **host,\n /*\n * Read the host as supplied by the client connection.\n */\n-static void parse_host_arg(char *extra_args, int buflen)\n+static void parse_host_arg(char *extra_args, char **remaining_args, int buflen)\n {\n \tchar *val;\n \tint vallen;\n@@ -514,8 +519,13 @@ static void parse_host_arg(char *extra_args, int buflen)\n \t\t\t/* On to the next one */\n \t\t\textra_args = val + vallen;\n \t\t}\n-\t\tif (extra_args < end && *extra_args)\n-\t\t\tdie(\"Invalid request\");\n+\t}\n+\n+\tif (remaining_args) {\n+\t\tfor (val = extra_args; val < end; val++)\n+\t\t\tif (!*val)\n+\t\t\t\t*val = ' ';\n+\t\t*remaining_args = extra_args;\n \t}\n \n \t/*\n@@ -577,6 +587,7 @@ static int execute(void)\n {\n \tchar *line = packet_buffer;\n \tint pktlen, len, i;\n+\tchar *args = NULL;\n \tchar *addr = getenv(\"REMOTE_ADDR\"), *port = getenv(\"REMOTE_PORT\");\n \n \tif (addr)\n@@ -603,7 +614,7 @@ static int execute(void)\n \thostname = canon_hostname = ip_address = tcp_port = NULL;\n \n \tif (len != pktlen)\n-\t\tparse_host_arg(line + len + 1, pktlen - len - 1);\n+\t\tparse_host_arg(line + len + 1, &args, pktlen - len - 1);\n \n \tfor (i = 0; i < ARRAY_SIZE(daemon_service); i++) {\n \t\tstruct daemon_service *s = &(daemon_service[i]);\n@@ -616,7 +627,7 @@ static int execute(void)\n \t\t\t * Note: The directory here is probably context sensitive,\n \t\t\t * and might depend on the actual service being performed.\n \t\t\t */\n-\t\t\treturn run_service(arg, s);\n+\t\t\treturn run_service(arg, s, args);\n \t\t}\n \t}\n \n-- \n2.3.0.81.gc37f363\n"},{"id":"256768","messageId":"1425085318-30537-6-git-send-email-sbeller@google.com","threadId":"38638","inReplyTo":"1425085318-30537-1-git-send-email-sbeller@google.com","subject":"[RFC/PATCH 5/5] WIP/Document the http protocol change","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-28T01:01:58Z","receivedAt":"2015-02-28T01:01:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n Documentation/technical/http-protocol.txt | 4 ++--\n Documentation/technical/protocol-capabilities.txt | 4 ++++\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/technical/http-protocol.txt b/Documentation/technical/http-protocol.txt\nindex 229f845..638819d 100644\n--- a/Documentation/technical/http-protocol.txt\n+++ b/Documentation/technical/http-protocol.txt\n@@ -191,10 +191,10 @@ HTTP clients that support the \"smart\" protocol (or both the\n \"smart\" and \"dumb\" protocols) MUST discover references by making\n a parameterized request for the info/refs file of the repository.\n \n-The request MUST contain exactly one query parameter,\n+The request MAY contain parameters. Supported parameters includes\n `service=$servicename`, where `$servicename` MUST be the service\n name the client wishes to contact to complete the operation.\n-The request MUST NOT contain additional query parameters.\n+Further parameters are as described in protocol-capabilities.txt\n \n C: GET $GIT_URL/info/refs?service=git-upload-pack HTTP/1.0\n \ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex 4f8a7bf..40ddb37 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -3,6 +3,10 @@ Git Protocol Capabilities\n \n Servers SHOULD support all capabilities defined in this document.\n \n+The client MAY ask for capabilities first out of band to the server.\n+If so the server MUST NOT advertise any capabilities the client did\n+not ask for.\n+\n On the very first line of the initial server response of either\n receive-pack and upload-pack the first reference is followed by\n a NUL byte and then a list of space delimited server capabilities.\n-- \n2.3.0.81.gc37f363\n"},{"id":"256770","messageId":"AB630BA1-8D48-418D-B576-6495DBE67C52@gmail.com","threadId":"38638","inReplyTo":"1425085318-30537-3-git-send-email-sbeller@google.com","subject":"Re: [RFC/PATCH 2/5] upload-pack: support out of band client capability requests","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-02-28T07:47:38Z","receivedAt":"2015-02-28T07:47:38Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Feb 27, 2015, at 17:01, Stefan Beller wrote:\n> From: Nguyễn Thái Ngọc Duy \n>\n> The only difference from the original protocol client capabilities are\n> negotiated before initial refs advertisment.\n>\n> Client capabilities are sent out of band (upload-pack receives it as\n> the second command line argument). The server sends one pkt-line back\n> advertising its capabilities.\n>\n> Signed-off-by: Stefan Beller \n> ---\n>\n> Notes:\n> v1:\n> I am still undecided if the client should then accept/resend\n> the capabilities to confirm them, which would make the client the\n> ultimate decider which capabilities are used.\n>\n> My gut feeling is to rather let the server make the final decision\n> for the capabilities, as it will use some requested capabilities\n> already to not send out all the refs.\n>\n> Documentation/git-upload-pack.txt | 10 +++++++++-\n> upload-pack.c | 42 ++++++++++++++++++++++++++ \n> +------------\n> 2 files changed, 38 insertions(+), 14 deletions(-)\n>\n> diff --git a/Documentation/git-upload-pack.txt b/Documentation/git- \n> upload-pack.txt\n> index 0abc806..ad3a89d 100644\n> --- a/Documentation/git-upload-pack.txt\n> +++ b/Documentation/git-upload-pack.txt\n> @@ -9,7 +9,7 @@ git-upload-pack - Send objects packed back to git- \n> fetch-pack\n> SYNOPSIS\n> --------\n> [verse]\n> -'git-upload-pack' [--strict] [--timeout=] \n> +'git-upload-pack' [--strict] [--timeout=] \n> []\n\nIsn't the problem with this that passing the extra argument to ssh \nservers will cause them to fail?\n\nHaving just looked at the upload-pack.c source it looks to me like \ntrying to send \"git-upload-pack 'dir' 'capabilities'\" to an ssh git \nserver running a current version of the code will just end up \nfailing. I realize the extra argument is optional, so does that mean \nthere's no out-of-band support for ssh connections since the extra \nargument would have to be omitted to remain compatible?\n\nOn Feb 26, 2015, at 12:13, Junio C Hamano wrote:\n> The capability-based sidegrade does not solve the problem\n> when the problem to be solved is that the server side needs to spend\n> a lot of cycles and the network needs to carry megabytes of data\n> before capability exchange happens.\n\n\nOn Feb 27, 2015, at 16:46, Stefan Beller wrote:\n> I'll try to present a 'client asks for options first out of band' \n> instead of the\n> way you describe.\n\nOn Feb 27, 2015, at 15:44, Stefan Beller wrote:\n> For both native git as well as ssh, Duy presented a solution at [1, 2]\n> a year ago, which essentially presents the desired client capabilites\n> 'out of band' to the server via an argument to the server. So we'd\n> only need to examine the http(s) path how to pass in arguments there.\n\n\nI've looked at those links and it's unclear to me how they support an \nout-of-band option for ssh, they seem to be targeted at git-daemon. \nMaybe there's another reference?\n\nBut there does seem to be a way to pass the protocol information out- \nof-band for each of the HTTP, git and ssh connections to stop the \ninitial ref advertisement.\n\nAs already suggested [1, 2], for git: another \"Extended attribute\" can \nbe added after the host=...\\0 to become host=...\\0protocol=extended\\0 \nor similar:\n\nFor HTTP, just add a second parameter in the query string .../info/ \nrefs?service=git-upload-pack&protocol=extended. Alternatively an \"X- \nGit-Protocol: extended\" or similar header can be added by the client. \nIt looks to me like the current http-backend.c already ignores any \nextra parameters/headers.\n\nThat leaves ssh. A bit more problematic, but if the server side adds \n\"AcceptEnv GIT_PROTOCOL\" to its sshd_config and then the client does \nsetenv(\"GIT_PROTOCOL\",\"extended\") and adds a \"-o SendEnv=GIT_PROTOCOL\" \noption to the ssh command line the GIT_PROTOCOL variable will be \npassed along. This one might need a config option to always disable \nadding the \"-o ...\" option to the command line in case connect.c \nguesses wrongly about it being OpenSSH and such is not likely to be \nsupported other than with OpenSSH on both ends. I'm not seeing any \nother way to pass out-of-band information to an ssh server configured \nto run git-shell that is safely ignored by current versions of the code.\n\nWorst case with SSH is the initial ref advertisement is not suppressed \neven though the server does support protocol v2 and a capability-based \nsidegrade is required which is unfortunate.\n\n-Kyle\n\n[1] https://github.com/pclouds/git/commit/e26fa77c4d9ace06b9f2c80091af9eb7b63a1c95\n[2] https://github.com/pclouds/git/commit/20d048e5fc650b20fdc7dd8bbe35cb8510ac9c50\n"},{"id":"256771","messageId":"54F1A272.80401@web.de","threadId":"38638","inReplyTo":"1425085318-30537-4-git-send-email-sbeller@google.com","subject":"Re: [RFC/PATCH 3/5] connect.c: connect to a remote service with some flags","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-02-28T11:11:46Z","receivedAt":"2015-02-28T11:11:46Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2015-02-28 02.01, Stefan Beller wrote:\n\n> If this is over git protocol, the flags is appended as the next\n> parameter after host=. If it's ssh, a new argument is appended to the\n> command line.\n> \n> None of the callers use this now though.\n> \n> [sb: originally by pclouds, rebased as jk implemented 1823bea10,\n> (git_connect: use argv_array), so any error is mine]\n> \n> Signed-off-by: Stefan Beller \n> ---\n> builtin/fetch-pack.c | 2 +-\n> builtin/send-pack.c | 2 +-\n> connect.c | 18 ++++++++++++------\n> connect.h | 2 +-\n> transport.c | 3 ++-\n> 5 files changed, 17 insertions(+), 10 deletions(-)\n> \n[]\n\n> diff --git a/connect.c b/connect.c\n> index 062e133..7b6b241 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -650,7 +650,9 @@ static struct child_process no_fork = CHILD_PROCESS_INIT;\n> * the connection failed).\n> */\n> struct child_process *git_connect(int fd[2], const char *url,\n> -\t\t\t\t const char *prog, int flags)\n> +\t\t\t\t const char *prog,\n> +\t\t\t\t const char *service_flags,\nThe name \"service_flags\" reminds me on a service:\nWe connect to a service, but we are not a service.\n\nAnd \"flags\" is often used for different bits, collected in an int, but not a \"string\"\n\nOptions are used at the command line, or arguments.\nHow about \"extra_arg\", or \"extra_option\", or \"option_str\", or simply \"options\" (or \"option\") ? \n> +\t\t\t\t int flags)\n> {\n> \tchar *hostandport, *path;\n> \tstruct child_process *conn = &no_fork;\n> @@ -685,10 +687,13 @@ struct child_process *git_connect(int fd[2], const char *url,\n> \t\t * Note: Do not add any other headers here! Doing so\n> \t\t * will cause older git-daemon servers to crash.\n> \t\t */\n> -\t\tpacket_write(fd[1],\n> -\t\t\t \"%s %s%chost=%s%c\",\n> -\t\t\t prog, path, 0,\n> -\t\t\t target_host, 0);\n> +\t\tif (!service_flags)\n> +\t\t\tpacket_write(fd[1], \"%s %s%chost=%s%c\",\n> +\t\t\t\t prog, path, 0, target_host, 0);\n> +\t\telse\n> +\t\t\tpacket_write(fd[1], \"%s %s%chost=%s%c%s%c\",\n> +\t\t\t\t prog, path, 0, target_host, 0,\n> +\t\t\t\t service_flags, 0);\nWe don't need this \"big if\" here, a simple \"?\" will do:\n \n\t\t\tpacket_write(fd[1], \"%s %s%chost=%s%c%s%c\",\n\t\t\t\t prog, path, 0, target_host, 0,\n\t\t\t\t options ? options : \"\", /* old service_flasgs */ \n 0);\n"},{"id":"256772","messageId":"CACsJy8DA86yBoYxvEbgUzoxGOwsKw0mO50iQ0qvNrxaihRNe2g@mail.gmail.com","threadId":"38638","inReplyTo":"AB630BA1-8D48-418D-B576-6495DBE67C52@gmail.com","subject":"Re: [RFC/PATCH 2/5] upload-pack: support out of band client capability requests","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-02-28T11:22:59Z","receivedAt":"2015-02-28T11:22:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 28, 2015 at 2:47 PM, Kyle J. McKay wrote:\n> On Feb 27, 2015, at 17:01, Stefan Beller wrote:\n>>\n>> From: Nguyễn Thái Ngọc Duy \n>>\n>> The only difference from the original protocol client capabilities are\n>> negotiated before initial refs advertisment.\n>>\n>> Client capabilities are sent out of band (upload-pack receives it as\n>> the second command line argument). The server sends one pkt-line back\n>> advertising its capabilities.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>>\n>> Notes:\n>> v1:\n>> I am still undecided if the client should then accept/resend\n>> the capabilities to confirm them, which would make the client the\n>> ultimate decider which capabilities are used.\n>>\n>> My gut feeling is to rather let the server make the final decision\n>> for the capabilities, as it will use some requested capabilities\n>> already to not send out all the refs.\n>>\n>> Documentation/git-upload-pack.txt | 10 +++++++++-\n>> upload-pack.c | 42\n>> +++++++++++++++++++++++++++------------\n>> 2 files changed, 38 insertions(+), 14 deletions(-)\n>>\n>> diff --git a/Documentation/git-upload-pack.txt\n>> b/Documentation/git-upload-pack.txt\n>> index 0abc806..ad3a89d 100644\n>> --- a/Documentation/git-upload-pack.txt\n>> +++ b/Documentation/git-upload-pack.txt\n>> @@ -9,7 +9,7 @@ git-upload-pack - Send objects packed back to\n>> git-fetch-pack\n>> SYNOPSIS\n>> --------\n>> [verse]\n>> -'git-upload-pack' [--strict] [--timeout=] \n>> +'git-upload-pack' [--strict] [--timeout=] []\n>\n>\n> Isn't the problem with this that passing the extra argument to ssh servers\n> will cause them to fail?\n>\n> Having just looked at the upload-pack.c source it looks to me like trying to\n> send \"git-upload-pack 'dir' 'capabilities'\" to an ssh git server running a\n> current version of the code will just end up failing. I realize the extra\n> argument is optional, so does that mean there's no out-of-band support for\n> ssh connections since the extra argument would have to be omitted to remain\n> compatible?\n\nThe client should only trigger this behavior when it knows the server\ncan deal with it. And that is possible because in the last fetch, the\nserver has told the client that it's capable of receiving this\ncapabilities argument. Backward compatibility is a concern at client\nside, not server side.\n\n> I've looked at those links and it's unclear to me how they support an\n> out-of-band option for ssh, they seem to be targeted at git-daemon. Maybe\n> there's another reference?\n\nFor ssh, I think connect.c is the one that constructs and executes ssh command.\n-- \nDuy\n"},{"id":"256773","messageId":"CACsJy8D1xHh7tQkX=+4H_FvLdk53RmtNwVPNuo9Ng2x9oQFPjw@mail.gmail.com","threadId":"38638","inReplyTo":"1425085318-30537-3-git-send-email-sbeller@google.com","subject":"Re: [RFC/PATCH 2/5] upload-pack: support out of band client capability requests","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-02-28T11:36:23Z","receivedAt":"2015-02-28T11:36:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 28, 2015 at 8:01 AM, Stefan Beller wrote:\n> Notes:\n> v1:\n> I am still undecided if the client should then accept/resend\n> the capabilities to confirm them, which would make the client the\n> ultimate decider which capabilities are used.\n>\n> My gut feeling is to rather let the server make the final decision\n> for the capabilities, as it will use some requested capabilities\n> already to not send out all the refs.\n\npack-capabilities.txt says\n\n\"Client will then send a space separated list of capabilities it wants\nto be in effect. The client MUST NOT ask for capabilities the server\ndid not say it supports.\"\n\nWhat was sent out of band is what the client can support, not what it\nwants. So perhaps drop this patch and let the client decide exactly\nwhat it wants.\n-- \nDuy\n"},{"id":"256775","messageId":"CACsJy8DtZKAw0KibHhHAeCU3rRXCwmjWg2O68iuX-vFA=0MaCA@mail.gmail.com","threadId":"38638","inReplyTo":"1425085318-30537-6-git-send-email-sbeller@google.com","subject":"Re: [RFC/PATCH 5/5] WIP/Document the http protocol change","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-02-28T12:26:13Z","receivedAt":"2015-02-28T12:26:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 28, 2015 at 8:01 AM, Stefan Beller wrote:\n> diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\n> index 4f8a7bf..40ddb37 100644\n> --- a/Documentation/technical/protocol-capabilities.txt\n> +++ b/Documentation/technical/protocol-capabilities.txt\n> @@ -3,6 +3,10 @@ Git Protocol Capabilities\n>\n> Servers SHOULD support all capabilities defined in this document.\n>\n> +The client MAY ask for capabilities first out of band to the server.\n> +If so the server MUST NOT advertise any capabilities the client did\n> +not ask for.\n\nI think what the current patches do is \"For v2, the client must _tell_\nthe server its capabilities\". The unwritten statement is \"the server\nmust not use any capabilities not advertised by the client\". v2 will\nnot send initial refs by default, and the client will need to send\n\"wantrefs refs/heads/* refs/tags/*\" or similar to retrieve ref list.\nSo we don't have any client capability yet. But there are two caps\nthat come to mind:\n\n - ref list compression (for sending full ref list in clone),\ncompression ratio may be 50%\n - setting locale in upload-pack and receive-pack to support l10n.\n\n> On the very first line of the initial server response of either\n> receive-pack and upload-pack the first reference is followed by\n> a NUL byte and then a list of space delimited server capabilities.\n\nThis one should be prefixed with \"in version 1, \"\n-- \nDuy\n"},{"id":"256782","messageId":"BA483DD3-3AD5-4B52-8379-F9FBA9A9305E@gmail.com","threadId":"38638","inReplyTo":"CACsJy8DA86yBoYxvEbgUzoxGOwsKw0mO50iQ0qvNrxaihRNe2g@mail.gmail.com","subject":"Re: [RFC/PATCH 2/5] upload-pack: support out of band client capability requests","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-02-28T22:36:18Z","receivedAt":"2015-02-28T22:36:18Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"\nOn Feb 28, 2015, at 03:22, Duy Nguyen wrote:\n> The client should only trigger this behavior when it knows the server\n> can deal with it. And that is possible because in the last fetch, the\n> server has told the client that it's capable of receiving this\n> capabilities argument. Backward compatibility is a concern at client\n> side, not server side.\n>\n>> I've looked at those links and it's unclear to me how they support an\n>> out-of-band option for ssh, they seem to be targeted at git- \n>> daemon. Maybe\n>> there's another reference?\n>\n> For ssh, I think connect.c is the one that constructs and executes \n> ssh command.\n\nThis I assume you're referring to this change in connect.c from [1]:\n\n@@ -729,6 +734,8 @@ struct child_process *git_connect(int fd[2], const \nchar *url,\n conn->use_shell = 1;\n }\n argv_array_push(&argv, cmd.buf);\n+ if (service_flags)\n+ argv_array_push(&argv, service_flags);\n conn->argv = argv.argv;\n if (start_command(conn))\n die(\"unable to fork\");\n\nThat's not going to work for ssh servers running a stock git-shell and \nI haven't seen any updates to shell.c to match. git-shell does not \nallow anything other than one argument to be passed to git-upload-pack/ \ngit-receive-pack.\n\nWhen shell.c calls do_generic_cmd and it calls sq_dequote on its \nargument that contains \"'dir' 'service-flags'\" it's going to return \nNULL and shell.c will die(\"bad argument\"). So I don't see how this \nsupports ssh as-is even if you know in advance the server supports the \nnew protocol. I don't see any changes to shell.c in that uploadpack2 \nbranch nor in this patch series.\n\n-Kyle\n\n[1] https://github.com/pclouds/git/commit/20d048e5fc650b20fdc7dd8bbe35cb8510ac9c50\n"},{"id":"256783","messageId":"CACsJy8A2bMgTr4cg73dV_JPr3U_5PiLCr9iRGpK79sg3QR39dg@mail.gmail.com","threadId":"38638","inReplyTo":"BA483DD3-3AD5-4B52-8379-F9FBA9A9305E@gmail.com","subject":"Re: [RFC/PATCH 2/5] upload-pack: support out of band client capability requests","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-01T00:11:49Z","receivedAt":"2015-03-01T00:11:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Mar 1, 2015 at 5:36 AM, Kyle J. McKay wrote:\n>\n> On Feb 28, 2015, at 03:22, Duy Nguyen wrote:\n>>\n>> The client should only trigger this behavior when it knows the server\n>> can deal with it. And that is possible because in the last fetch, the\n>> server has told the client that it's capable of receiving this\n>> capabilities argument. Backward compatibility is a concern at client\n>> side, not server side.\n>>\n>>> I've looked at those links and it's unclear to me how they support an\n>>> out-of-band option for ssh, they seem to be targeted at git-daemon.\n>>> Maybe\n>>> there's another reference?\n>>\n>>\n>> For ssh, I think connect.c is the one that constructs and executes ssh\n>> command.\n>\n>\n> This I assume you're referring to this change in connect.c from [1]:\n>\n> @@ -729,6 +734,8 @@ struct child_process *git_connect(int fd[2], const char\n> *url,\n> conn->use_shell = 1;\n> }\n> argv_array_push(&argv, cmd.buf);\n> + if (service_flags)\n> + argv_array_push(&argv, service_flags);\n> conn->argv = argv.argv;\n> if (start_command(conn))\n> die(\"unable to fork\");\n>\n> That's not going to work for ssh servers running a stock git-shell and I\n> haven't seen any updates to shell.c to match. git-shell does not allow\n> anything other than one argument to be passed to\n> git-upload-pack/git-receive-pack.\n>\n> When shell.c calls do_generic_cmd and it calls sq_dequote on its argument\n> that contains \"'dir' 'service-flags'\" it's going to return NULL and shell.c\n> will die(\"bad argument\"). So I don't see how this supports ssh as-is even\n> if you know in advance the server supports the new protocol. I don't see\n> any changes to shell.c in that uploadpack2 branch nor in this patch series.\n\nYou're right. If we continue to pass client capabilities as an extra\nargument, then git-shell needs updates.\n-- \nDuy\n"},{"id":"256790","messageId":"xmqqmw3xh04x.fsf@gitster.dls.corp.google.com","threadId":"38638","inReplyTo":"1425085318-30537-4-git-send-email-sbeller@google.com","subject":"Re: [RFC/PATCH 3/5] connect.c: connect to a remote service with some flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-01T03:22:54Z","receivedAt":"2015-03-01T03:22:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> If this is over git protocol, the flags is appended as the next\n> parameter after host=. If it's ssh, a new argument is appended to the\n> command line.\n>\n> None of the callers use this now though.\n\nReplace \"some flags\" with something more meaningful, so that the\nreader can imagine what purpose this change is meant to serve.\n\nI unfortunately cannot offer a good suggestion by guessing what it\nis at this point, as none of the callers use this at this step in\nthe series.\n\n> +\t\tif (!service_flags)\n> +\t\t\tpacket_write(fd[1], \"%s %s%chost=%s%c\",\n> +\t\t\t\t prog, path, 0, target_host, 0);\n> +\t\telse\n> +\t\t\tpacket_write(fd[1], \"%s %s%chost=%s%c%s%c\",\n> +\t\t\t\t prog, path, 0, target_host, 0,\n> +\t\t\t\t service_flags, 0);\n\nIs this meant to be consumed by daemon.c:parse_host_arg()? Doesn't\nit detect extra cruft after \"host=\\0\" and die with an\n\"Invalid request\"?\n"},{"id":"256791","messageId":"xmqqioelgzcn.fsf@gitster.dls.corp.google.com","threadId":"38638","inReplyTo":"1425085318-30537-5-git-send-email-sbeller@google.com","subject":"Re: [RFC/PATCH 4/5] daemon.c: accept extra service arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-01T03:39:52Z","receivedAt":"2015-03-01T03:39:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Before 73bb33a (daemon: Strictly parse the \"extra arg\" part of the\n> command - 2009-06-04) a client sending extra arguments could DoS\n> git-daemon. 73bb33a fixed it by forbidding extra arguments.\n>\n> Allow arguments other than \"host=\" again as a preparation step for\n> upload-pack2. \"host=\" if present must be the first argument\n> though. The remaining arguments are concatenated by whitespace.\n>\n> So far none of supported services support extra arguments. Attempting\n> to do will abort the service, just like how it is before. We might\n> want to make them silently ignore extra arguments though.\n\nThe necessity of this means 3/5, hiding new stuff after\n\"host=\\0\", is not a viable strategy to extend the\ncommunication to \"git daemon\" in a backward compatible way so that\nnew client can transparently talk with older daemon.\n\nAnd if you accept that limitation and declare that new client MUST\ntalk with a new git daemon, then there is not much point in hiding\nthe extra arguments after \"host=\\0\". You can just declare\nthat the new git daemon will accept arguments, including\n\"host=\\0\", as a series of \"var=\\0\" in separate packets,\nnot as a part of the initial \" \" packet, for example.\n\nOne thing 3/5 and 4/5 will give you is to allow old clients to talk\nto the new git daemon if the way new programs spawned by git daemon\nmust use that \"hiding\" trick, but I am not convinced that it is\nnecessary to resort to that \"hiding\" trick in the first place.\n\nUnder the same \"new client must talk with a new daemon, they cannot\ntalk with existing daemon\" limitation, I think you can arrange\nthings this way by not touching daemon at all:\n\n * Keep the daemon protocol as \" \\0host=\\0\".\n\n * New client, as it has to know that it is talking to a host with\n new enough Git, can ask \"upload-pack-2\" as .\n\n * Design the protocol that the protocol backend programs such as\n upload-pack-2 (and other updated programs like receive-pack-2,\n trickle-pack, ...) talks in such a way that these \"extra\"\n parameters sent when client connects are sent as the first\n packet(s).\n\nThat way, the new daemon will take \" \\0host=\\0\"\nand cannot take extra arguments at the end of that packet, but the\nnew protocol backend programs do not need extra arguments given by\nthe daemon to them anyway. Old clients will not ask to be connected\nto new upload-pack-2 and friends, but that is to be expected.\n"},{"id":"256797","messageId":"xmqq1tl9gld9.fsf@gitster.dls.corp.google.com","threadId":"38638","inReplyTo":"CAPc5daVbrUaU6LFM65evru0+1tBT916+0AOyids=f7DZThTPGw@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-01T08:41:54Z","receivedAt":"2015-03-01T08:41:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I earlier said:\n\n> So if we are going to discuss a new protocol, I'd prefer to see the\n> discussion without worrying too much about how to inter-operate\n> with the current vintage of Git. It is no longer an interesting problem,\n> as we know how to solve it with minimum risk. Instead, I'd like to\n> see us design the new protocol in such a way that it is in-line\n> upgradable without repeating our past mistakes.\n\nAnd I am happy to see that people are interested in discussing the\ndesign of new protocols.\n\nBut after seeing the patches Stefan sent out, I think we are risking\nof losing sight of what we are trying to accomplish. We do not want\nsomething that is merely new.\n\nThat is why I wanted people to think about, discuss and agree on\nwhat limitation of the current protocol has that are problematic\n(limitations that are not problematic are not something we do not\nneed to address [*1*]), so that we can design the new thing without\nreintroducing the same limitation.\n\nTo remind people, here is a reprint of the draft I sent out earlier\nin $gmane/264000.\n\n> The current protocol has the following problems that limit us:\n> \n> - It is not easy to make it resumable, because we recompute every\n> time. This is especially problematic for the initial fetch aka\n> \"clone\" as we will be talking about a large transfer [*1*].\n> \n> - The protocol extension has a fairly low length limit [*2*].\n> \n> - Because the protocol exchange starts by the server side\n> advertising all its refs, even when the fetcher is interested in\n> a single ref, the initial overhead is nontrivial, especially when\n> you are doing a small incremental update. The worst case is an\n> auto-builder that polls every five minutes, even when there is no\n> new commits to be fetched [*3*].\n> \n> - Because we recompute every time, taking into account of what the\n> fetcher has, in addition to what the fetcher obtained earlier\n> from us in order to reduce the transferred bytes, the payload for\n> incremental updates become tailor-made for each fetch and cannot\n> be easily reused [*4*].\n> \n> I'd like to see a new protocol that lets us overcome the above\n> limitations (did I miss others? I am sure people can help here)\n> sometime this year.\n\nUnfortunately, nobody seems to want to help us by responding to \"did\nI miss others?\" RFH, here are a few more from me.\n\n - The semantics of the side-bands are unclear.\n\n - Is band #2 meant only for progress output (I think the current\n protocol handlers assume that and unconditionally squelch it\n under --quiet)? Do we rather want a dedicated \"progress\" and\n \"error message\" sidebands instead?\n\n - Is band #2 meant for human consumption, or do we expect the\n other end to interpret and act on it? If the former, would it\n make sense to send locale information from the client side and\n ask the server side to produce its output with _(\"message\")?\n\n - The semantics of packet_flush() is suboptimal, and this\n shortcoming seeps through to the protocol mapped to the\n smart-HTTP transport.\n\n Originally, packet_flush() was meant as \"Here is an end of one\n logical section of what I am going to speak.\", hinting that it\n might be a good idea for the underlying implementation to hold\n the packets up to that point in-core and then write(2) them all\n out (i.e. \"flush\") to the file descriptor only when we handle\n packet_flush(). It never meant \"Now I am finished speaking for\n now and it is your turn to speak.\"\n\n But because HTTP is inherently a ping-pong protocol where the\n requestor at one point stops talking and lets the responder\n speak, the code to map our protocol to the smart HTTP transport\n made the packet_flush() boundary as \"Now I am done talking, it is\n my turn to listen.\"\n\n We probably need two kinds of packet_flush(). When a requestor\n needs to say two or more logical groups of things before telling\n the other side \"Now I am done talking; it is your turn.\", we need\n some marker (i.e. the original meaning of packet_flush()) at the\n end of these logical groups. And in order to be able to say \"Now\n I am done saying everything I need to say at this point for you\n to respond to me. It is your turn.\", we need another kind of\n marker.\n\n\n[Footnote]\n\n*1* For example, if we were working off of \"what mistakes do we want\nto correct?\" list, I do not think we would have seen \"capabilities\nhave to be only on the first packet\" or \"lets allow new daemon to\nread extra cruft at the end of the first request\". I do not think I\nheard why it is a problem that the daemon cannot pass extra info to\ninvoked program in the first place. There might be a valid reason,\nbut then that needs to be explained, understood and agreed upon and\nshould be part of an updated \"what are we fixing?\" list.\n"},{"id":"256798","messageId":"54F2D7C2.4010700@kdbg.org","threadId":"38638","inReplyTo":"1425085318-30537-1-git-send-email-sbeller@google.com","subject":"Re: [RFC/PATCH 0/5] protocol v2 for upload-pack","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-03-01T09:11:30Z","receivedAt":"2015-03-01T09:11:30Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 28.02.2015 um 02:01 schrieb Stefan Beller:\n> Heavily inspired by the ideas of Duy, who wrote the first\n> patches nearly a year ago.\n>\n> Nguyễn Thái Ngọc Duy (2):\n> upload-pack: only accept capabilities on the first \"want\" line\n> upload-pack: support out of band client capability requests\n>\n> Stefan Beller (3):\n> connect.c: connect to a remote service with some flags\n> daemon.c: accept extra service arguments\n> WIP/Document the http protocol change\n>\n> Documentation/git-upload-pack.txt | 10 +++++-\n> Documentation/technical/http-protocol.txt | 4 +--\n> Documentation/technical/protocol-capabilities.txt | 4 +++\n> builtin/fetch-pack.c | 2 +-\n> builtin/send-pack.c | 2 +-\n> connect.c | 21 ++++++------\n> connect.h | 2 +-\n> daemon.c | 37 +++++++++++++--------\n> transport.c | 3 +-\n> upload-pack.c | 39 +++++++++++++++++------\n> 10 files changed, 86 insertions(+), 38 deletions(-)\n>\n\nYou may also consider an idea I proposed here:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/206886/focus=207342\n\nThe idea is that the exchange begins as usual, but when the v2 client \nsees that the server also supports v2, then it begins sending its \ndesired refs. When the server notices that the client spoke while it was \nstill sending out its megabytes of ref advertisments, it stops the v1 \nadvertisements and continues with v2 protocol.\n\nBut Shawn pointed out that this would be difficult to implement in a \nJGit server because there is no way to poll the incoming channel while \nit is sending out refs.\n\n-- Hannes\n"},{"id":"256800","messageId":"CACsJy8DKbfU7TBHhT5_qpL0QM3zbxkaF+B4x3hQDpomQ_9OSEg@mail.gmail.com","threadId":"38638","inReplyTo":"xmqq1tl9gld9.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-01T11:32:08Z","receivedAt":"2015-03-01T11:32:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Mar 1, 2015 at 3:41 PM, Junio C Hamano wrote:\n>> - Because the protocol exchange starts by the server side\n>> advertising all its refs, even when the fetcher is interested in\n>> a single ref, the initial overhead is nontrivial, especially when\n>> you are doing a small incremental update. The worst case is an\n>> auto-builder that polls every five minutes, even when there is no\n>> new commits to be fetched [*3*].\n\nMaybe you can elaborate about how to handle states X, Y... in your\nfootnote 3. I just don't see how it's actually implemented. Or is it\noptional feature that will be provided (via hooks, maybe) by admin? Do\nwe need to worry about load balancers? Is it meant to address the\nexcessive state transfer due to stateless nature of smart-http?\n\n>> I'd like to see a new protocol that lets us overcome the above\n>> limitations (did I miss others? I am sure people can help here)\n>> sometime this year.\n>\n> Unfortunately, nobody seems to want to help us by responding to \"did\n> I miss others?\" RFH, here are a few more from me.\n\nHeh.. I did think about it, but I didn't see anything worth mentioning..\n\n> - The semantics of the side-bands are unclear.\n>\n> - Is band #2 meant only for progress output (I think the current\n> protocol handlers assume that and unconditionally squelch it\n> under --quiet)? Do we rather want a dedicated \"progress\" and\n> \"error message\" sidebands instead?\n>\n> - Is band #2 meant for human consumption, or do we expect the\n> other end to interpret and act on it? If the former, would it\n> make sense to send locale information from the client side and\n> ask the server side to produce its output with _(\"message\")?\n\nNo producing _(...) is a bad idea. First the client has to verify\nplaceholders and stuff, we can't just feed data from server straight\nto printf(). Producing _() could complicate server code a lot. And I\ndon't like the idea of using client .po files to translate server\nstrings. There could be custom strings added by admin, which are not\navailable in client .po. String translation should happen at server\nside.\n\nIf we want error messages to be handled by machine as well, just add a\nresult code at the beginning, like ftp, http, ... do. Hmm.. this could\nbe the reason to separate progress and error messages.\n-- \nDuy\n"},{"id":"256808","messageId":"CAGZ79kaHGw=OyrkktWeo-MR0V1_bASB1cioP+c2Ngpt6fkRxBA@mail.gmail.com","threadId":"38638","inReplyTo":"CACsJy8DKbfU7TBHhT5_qpL0QM3zbxkaF+B4x3hQDpomQ_9OSEg@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-01T19:56:10Z","receivedAt":"2015-03-01T19:56:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Mar 1, 2015 at 3:32 AM, Duy Nguyen wrote:\n> On Sun, Mar 1, 2015 at 3:41 PM, Junio C Hamano wrote:\n>>> - Because the protocol exchange starts by the server side\n>>> advertising all its refs, even when the fetcher is interested in\n>>> a single ref, the initial overhead is nontrivial, especially when\n>>> you are doing a small incremental update. The worst case is an\n>>> auto-builder that polls every five minutes, even when there is no\n>>> new commits to be fetched [*3*].\n>\n> Maybe you can elaborate about how to handle states X, Y... in your\n> footnote 3. I just don't see how it's actually implemented. Or is it\n> optional feature that will be provided (via hooks, maybe) by admin? Do\n> we need to worry about load balancers? Is it meant to address the\n> excessive state transfer due to stateless nature of smart-http?\n\nThe way I understand Junio here is to have predefined points which\nmakes it easier to communicate. There are lots of clients and they usually\nwant to catch up a different amount of commits, so we need to recompute it\nall the time. The idea is then to compute a small pack from the original point\nto one of these predefined points.\nSo a conversion might look like:\nClient: My newest commit is dated 2014-11-17.\nServer: ok here is a pack from 2014-11-17 until 2014-12-01 and then\nI have prepared packs I sent out all the time of 2014-12 and 2015-01\nand 2015-02 and then there will be another custom pack for you describing\nchanges of 2015-02-01 until now.\n\nMind that I choose dates instead of arbitrary sha1 values as I feel\nthat explains the\npoint better, the packs in between are precomputed because many\nclients need them.\n\nPersonally I don't buy that idea, because it produces a lot of question, like\nhow large should these packs be? Depending on time or commit counts?\n\nThe idea I'd rather favor (I am repeating myself from another post,\nbut maybe a bit clearer now):\n\n Client: The last time I asked for \"refs/heads/*\" and I got a refs\n advertisement hashing to $SHA1\n Server: Ok, here is the diff from that old ref advertisement to the\n current refs advertisement.\n\nI realize that these two ideas are not contradicting each other, but\ncould rather\nhelp each other as they are orthogonal to each other. One is about\nrefs advertising\nwhile the other is about object transmission.\n\n>\n>>> I'd like to see a new protocol that lets us overcome the above\n>>> limitations (did I miss others? I am sure people can help here)\n>>> sometime this year.\n>>\n>> Unfortunately, nobody seems to want to help us by responding to \"did\n>> I miss others?\" RFH, here are a few more from me.\n>\n> Heh.. I did think about it, but I didn't see anything worth mentioning..\n>\n>> - The semantics of the side-bands are unclear.\n>>\n>> - Is band #2 meant only for progress output (I think the current\n>> protocol handlers assume that and unconditionally squelch it\n>> under --quiet)? Do we rather want a dedicated \"progress\" and\n>> \"error message\" sidebands instead?\n>>\n>> - Is band #2 meant for human consumption, or do we expect the\n>> other end to interpret and act on it? If the former, would it\n>> make sense to send locale information from the client side and\n>> ask the server side to produce its output with _(\"message\")?\n>\n> No producing _(...) is a bad idea. First the client has to verify\n> placeholders and stuff, we can't just feed data from server straight\n> to printf(). Producing _() could complicate server code a lot. And I\n> don't like the idea of using client .po files to translate server\n> strings. There could be custom strings added by admin, which are not\n> available in client .po. String translation should happen at server\n> side.\n>\n> If we want error messages to be handled by machine as well, just add a\n> result code at the beginning, like ftp, http, ... do. Hmm.. this could\n> be the reason to separate progress and error messages.\n> --\n> Duy\n"},{"id":"256809","messageId":"9EB6341F415E41F3A07F9046056ACE13@PhilipOakley","threadId":"38638","inReplyTo":"xmqq1tl9gld9.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2015-03-01T19:56:10Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Junio C Hamano\" \n>I earlier said:\n>\n>> So if we are going to discuss a new protocol, I'd prefer to see the\n>> discussion without worrying too much about how to inter-operate\n>> with the current vintage of Git. It is no longer an interesting \n>> problem,\n>> as we know how to solve it with minimum risk. Instead, I'd like to\n>> see us design the new protocol in such a way that it is in-line\n>> upgradable without repeating our past mistakes.\n>\n> And I am happy to see that people are interested in discussing the\n> design of new protocols.\n>\n> But after seeing the patches Stefan sent out, I think we are risking\n> of losing sight of what we are trying to accomplish. We do not want\n> something that is merely new.\n>\n> That is why I wanted people to think about, discuss and agree on\n> what limitation of the current protocol has that are problematic\n> (limitations that are not problematic are not something we do not\n> need to address [*1*]), so that we can design the new thing without\n> reintroducing the same limitation.\n>\n> To remind people, here is a reprint of the draft I sent out earlier\n> in $gmane/264000.\n>\n>> The current protocol has the following problems that limit us:\n>>\n>> - It is not easy to make it resumable, because we recompute every\n>> time. This is especially problematic for the initial fetch aka\n>> \"clone\" as we will be talking about a large transfer [*1*].\n>>\n>> - The protocol extension has a fairly low length limit [*2*].\n>>\n>> - Because the protocol exchange starts by the server side\n>> advertising all its refs, even when the fetcher is interested in\n>> a single ref, the initial overhead is nontrivial, especially when\n>> you are doing a small incremental update. The worst case is an\n>> auto-builder that polls every five minutes, even when there is no\n>> new commits to be fetched [*3*].\n>>\n>> - Because we recompute every time, taking into account of what the\n>> fetcher has, in addition to what the fetcher obtained earlier\n>> from us in order to reduce the transferred bytes, the payload for\n>> incremental updates become tailor-made for each fetch and cannot\n>> be easily reused [*4*].\n>>\n>> I'd like to see a new protocol that lets us overcome the above\n>> limitations (did I miss others? I am sure people can help here)\n>> sometime this year.\n>\n> Unfortunately, nobody seems to want to help us by responding to \"did\n> I miss others?\" RFH, here are a few more from me.\n\nOK, maybe not exactly about protocol, but a possible option would be the \nability to send the data as a bundle or multi-bundles; Or perhasps as an \narchive, zip, or tar.\n\nData can then be exchanged across an airgap or pigeon mail. The airgap \nscenario is likely a real case that's not directly prominent at the \nmoment, just because it's not tha direct.\n\nThere has been discussion about servers having bundles available for \nclones, but with a multi-bundle, one could package up a large bundle \n(months) and an increment (weeks, and then days), before an final easy \nto pack last few hours. That would be a server work trade-off, and \nsupport a CDN view if needed.\n\nIf such an approach was reasonable would the protocol support it? etc.\n\nJust a thought while reading...\n>\n> - The semantics of the side-bands are unclear.\n>\n> - Is band #2 meant only for progress output (I think the current\n> protocol handlers assume that and unconditionally squelch it\n> under --quiet)? Do we rather want a dedicated \"progress\" and\n> \"error message\" sidebands instead?\n>\n> - Is band #2 meant for human consumption, or do we expect the\n> other end to interpret and act on it? If the former, would it\n> make sense to send locale information from the client side and\n> ask the server side to produce its output with _(\"message\")?\n>\n> - The semantics of packet_flush() is suboptimal, and this\n> shortcoming seeps through to the protocol mapped to the\n> smart-HTTP transport.\n>\n> Originally, packet_flush() was meant as \"Here is an end of one\n> logical section of what I am going to speak.\", hinting that it\n> might be a good idea for the underlying implementation to hold\n> the packets up to that point in-core and then write(2) them all\n> out (i.e. \"flush\") to the file descriptor only when we handle\n> packet_flush(). It never meant \"Now I am finished speaking for\n> now and it is your turn to speak.\"\n>\n> But because HTTP is inherently a ping-pong protocol where the\n> requestor at one point stops talking and lets the responder\n> speak, the code to map our protocol to the smart HTTP transport\n> made the packet_flush() boundary as \"Now I am done talking, it is\n> my turn to listen.\"\n>\n> We probably need two kinds of packet_flush(). When a requestor\n> needs to say two or more logical groups of things before telling\n> the other side \"Now I am done talking; it is your turn.\", we need\n> some marker (i.e. the original meaning of packet_flush()) at the\n> end of these logical groups. And in order to be able to say \"Now\n> I am done saying everything I need to say at this point for you\n> to respond to me. It is your turn.\", we need another kind of\n> marker.\n>\n>\n> [Footnote]\n>\n> *1* For example, if we were working off of \"what mistakes do we want\n> to correct?\" list, I do not think we would have seen \"capabilities\n> have to be only on the first packet\" or \"lets allow new daemon to\n> read extra cruft at the end of the first request\". I do not think I\n> heard why it is a problem that the daemon cannot pass extra info to\n> invoked program in the first place. There might be a valid reason,\n> but then that needs to be explained, understood and agreed upon and\n> should be part of an updated \"what are we fixing?\" list.\n>\n--\nPhilip \n"},{"id":"256810","messageId":"xmqqwq30fhbg.fsf@gitster.dls.corp.google.com","threadId":"38638","inReplyTo":"CACsJy8DKbfU7TBHhT5_qpL0QM3zbxkaF+B4x3hQDpomQ_9OSEg@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-01T23:06:59Z","receivedAt":"2015-03-01T23:06:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Sun, Mar 1, 2015 at 3:41 PM, Junio C Hamano wrote:\n>>> - Because the protocol exchange starts by the server side\n>>> advertising all its refs, even when the fetcher is interested in\n>>> a single ref, the initial overhead is nontrivial, especially when\n>>> you are doing a small incremental update. The worst case is an\n>>> auto-builder that polls every five minutes, even when there is no\n>>> new commits to be fetched [*3*].\n>\n> Maybe you can elaborate about how to handle states X, Y... in your\n> footnote 3. I just don't see how it's actually implemented. Or is\n> it optional feature that will be provided (via hooks, maybe) by\n> admin?\n\nThese footnotes are not the important part (I wanted us to agree on\nthe problem, and the ideas outlined with the footnotes are only an\nexample that illustrates how a potential solution and the problem to\nbe solved are described in relation to each other), but I'll give a\nshot anyway ;-)\n\nI am actually torn on how the names X, Y, etc. should be defined.\n\nOne side of me wants to leave its computation entirely up to the\nserver side. The client says \"Last time I talked with you asking\nfor refs/heads/* and successfully updated from you, you told me to\ncall that state X\" without knowing how X is computed, and then the\nserver will update you and then tell you your state is now Y. That\nway, large hosting sites and server implementations can choose to\nimplement it any way they like.\n\nOn the other hand, we could rigidly define it, perhaps like this:\n\n - Imagine that you saved the output from ls-remote that is run\n against that server, limited to the refs hierarchy you are\n requesting, the last time you talked with it.\n\n - Concatenate the above to the list of patterns the client used to\n ask the refs. This step is optional.\n\n - E.g. if you are asking it for \"refs/heads/*\", then we are talking\n something like this (illustrated with optional pattern in front):\n\n refs/heads/*\n 8004647... refs/heads/maint\n 7f4ba4b... refs/heads/master\n\n - Run SHA-1 hash over that. And that is the state name.\n\nI.e. if you as a client are doing\n\n [remote \"origin\"]\n fetch = refs/heads/*:refs/remotes/origin/*\n\nand if the only time your refs/remotes/origin/* hierarchy changes is\nwhen you fetch from there (which should be the norm), you can look\ninto remote.origin.fetch refspec (to learn that \"refs/heads*\" is\nwhat you are asking) and your refs/remotes/origin/* refs (and\nreverse the mapping you make when you fetch to make them talk about\nrefs/heads/* hierarchy on the server side), you can compute it\nlocally.\n\nThe latter will have one benefit over \"opaque thing the client does\nnot know how to compute\". Because I want us avoid sending unchanged\nrefs over connection, but I do want to see the protocol has some\nvalidation mechanism built in, even if we go the latter \"client can\ncompute what the state name ought to be\" route, I want the servrer\nto tell the client what to call that state. That way, the client\nside can tell when it goes out of sync for any reason and attempt to\nrecover.\n\n> Do we need to worry about load balancers?\n\nUnless you are allowing multiple backend servers to serve a same\nrepository behind a set of load balancers in an inconsistent way\n(e.g. you push to one while I push to two and you fetch from one and\nyou temporarily see my push but then my push will be rejected as\nconflicting and you fetch from one and now you see your push), I do\nnot think there is anything you need to worry about them more than\nwhat you should be worrying about already.\n\nThere would be a point where all backend servers would agree \"This\nis the set of values of these refs\" at some point (e.g. a majority\nof surviving servers vote to decide, laggers that later join the\nparty will update to the concensus value before serving the end-user\ntraffic), and they would not be showing half-updated values that\nhaven't ratified by other servers to end users (otherwise they may\nend up showing reversion).\n\n>> - Is band #2 meant for human consumption, or do we expect the\n>> other end to interpret and act on it? If the former, would it\n>> make sense to send locale information from the client side and\n>> ask the server side to produce its output with _(\"message\")?\n>\n> No producing _(...) is a bad idea. First the client has to verify\n> placeholders and stuff, we can't just feed data from server straight\n> to printf(). Producing _() could complicate server code a lot. And I\n> don't like the idea of using client .po files to translate server\n> strings. There could be custom strings added by admin, which are not\n> available in client .po. String translation should happen at server\n> side.\n\nWhat I meant to say was (1) the client says \"I want the human\nreadable message in Vietnamese\" and (2) the server uses .po on\n_(\"message\") in its code and send the result to sideband #2. There\nis no parsing, interpolation, or anything of that sort necessary on\nthe server end.\n\nBecause potential problem you mention looks to me about a different\ndesign where the server talks \"message\" and client side applies\n_($msg) to it, I suspect that you misread what I meant to say.\n"},{"id":"256816","messageId":"alpine.DEB.2.02.1503011653000.12474@nftneq.ynat.uz","threadId":"38638","inReplyTo":"CAGZ79kaHGw=OyrkktWeo-MR0V1_bASB1cioP+c2Ngpt6fkRxBA@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"David Lang","fromEmail":"david@lang.hm","sentAt":"2015-03-02T01:05:30Z","receivedAt":"2015-03-02T01:05:30Z","isPatch":true,"sender":{"key":"david@lang.hm","avatar":null},"body":"On Sun, 1 Mar 2015, Stefan Beller wrote:\n\n> The way I understand Junio here is to have predefined points which\n> makes it easier to communicate. There are lots of clients and they usually\n> want to catch up a different amount of commits, so we need to recompute it\n> all the time. The idea is then to compute a small pack from the original point\n> to one of these predefined points.\n> So a conversion might look like:\n> Client: My newest commit is dated 2014-11-17.\n> Server: ok here is a pack from 2014-11-17 until 2014-12-01 and then\n> I have prepared packs I sent out all the time of 2014-12 and 2015-01\n> and 2015-02 and then there will be another custom pack for you describing\n> changes of 2015-02-01 until now.\n>\n> Mind that I choose dates instead of arbitrary sha1 values as I feel\n> that explains the\n> point better, the packs in between are precomputed because many\n> clients need them.\n>\n> Personally I don't buy that idea, because it produces a lot of question, like\n> how large should these packs be? Depending on time or commit counts?\n\nI think this is going to depend on the project in question. I think that doing \nthis based on public tags makes lots of sense. The precomputed packs should also \nchange over time.\n\nFor example, with the linux kernel, as each -rc is released, there will be a lot \nof people wanting to upgrade from a prior -rc, so having a pack for each of \nthese is probably worthwhile. You probably also want a precomputed pack to move \nfrom some of the -rc releases to the final release. And then a single pack to \nmove from the prior final release to the newest one. There may also me a resons \nto make a pack that jumps several releases to go from one LTS kernel to the \nnext.\n\nExactly what precomputed packs make sense, and how large the packs should be is \ngoing to be _very_ dependent on the update patterns of users. The only people \nwho can decide exactly what packs they should use are the admins of the systems, \nand should be based on their logs of what requests are being made. I can see the \ngit project creating scripts to analyze the logs of client connections to make \nrecommendations on what packs would be useful to have pre-generated, ideally \nordered by how much computation they would save (and the amount of disk space \nrequired to hold the packs, and then the admin of the site can indicate where \nthey want the cutoff to be. Some extremely busy sites may have a lot of disk \nspace compared to CPU and be willing to have lots of packs around, others are \nless busy and will only want to keep a few around.\n\nDavid Lang\n"},{"id":"256817","messageId":"alpine.DEB.2.02.1503011707040.12474@nftneq.ynat.uz","threadId":"38638","inReplyTo":"xmqqwq30fhbg.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"David Lang","fromEmail":"david@lang.hm","sentAt":"2015-03-02T01:09:12Z","receivedAt":"2015-03-02T01:09:12Z","isPatch":true,"sender":{"key":"david@lang.hm","avatar":null},"body":"On Sun, 1 Mar 2015, Junio C Hamano wrote:\n\n> and if the only time your refs/remotes/origin/* hierarchy changes is\n> when you fetch from there (which should be the norm), you can look\n> into remote.origin.fetch refspec (to learn that \"refs/heads*\" is\n> what you are asking) and your refs/remotes/origin/* refs (and\n> reverse the mapping you make when you fetch to make them talk about\n> refs/heads/* hierarchy on the server side), you can compute it\n> locally.\n>\n> The latter will have one benefit over \"opaque thing the client does\n> not know how to compute\". Because I want us avoid sending unchanged\n> refs over connection, but I do want to see the protocol has some\n> validation mechanism built in, even if we go the latter \"client can\n> compute what the state name ought to be\" route, I want the servrer\n> to tell the client what to call that state. That way, the client\n> side can tell when it goes out of sync for any reason and attempt to\n> recover.\n\nhow would these approaches be affected by a client that is pulling from \ndifferent remotes into one local repository? For example, pulling from the main \nkernel repo and from the -stable repo.\n\nDavid Lang\n"},{"id":"256818","messageId":"xmqqmw3wf6l3.fsf@gitster.dls.corp.google.com","threadId":"38638","inReplyTo":"54F2D7C2.4010700@kdbg.org","subject":"Re: [RFC/PATCH 0/5] protocol v2 for upload-pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-02T02:58:48Z","receivedAt":"2015-03-02T02:58:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> You may also consider an idea I proposed here:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/206886/focus=207342\n>\n> The idea is that the exchange begins as usual, but when the v2 client\n> sees that the server also supports v2, then it begins sending its\n> desired refs. When the server notices that the client spoke while it\n> was still sending out its megabytes of ref advertisments, it stops the\n> v1 advertisements and continues with v2 protocol.\n\nYes, it would be possible to design the protocol in such a way\nprovided if we can find a v1 protocol message that we can use so\nthat a new client will send it as \"I am a client capable of talking\nthe new protocol\" and the message appears as a no-op to older\nservers. And that would allow us to transparently upgrade the\nprotocol in-line. That would be a big plus.\n\nAnd there seems to be such a message for both fetch and push\nprotoocol, as discussed in $gmane/264512 a few days ago.\n\nBut I think the current thinking is that it is OK to avoid the\nugliness and complexity of having to say \"oops, sorry but I did not\nhear your 'I am new enough' soon enough and sent a lot of v1\nadvertisement, let's resync with v2 protocol\" and instead upgrade\nthe old client upon the _next_ exchange (see $gmane/264309).\n"},{"id":"256819","messageId":"xmqqioekf629.fsf@gitster.dls.corp.google.com","threadId":"38638","inReplyTo":"alpine.DEB.2.02.1503011707040.12474@nftneq.ynat.uz","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-02T03:10:06Z","receivedAt":"2015-03-02T03:10:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Lang writes:\n\n> how would these approaches be affected by a client that is pulling\n> from different remotes into one local repository? For example, pulling\n> from the main kernel repo and from the -stable repo.\n>\n> David Lang\n\nAs I said in $gmane/264000, which the above came from:\n\n Note that the above would work if and only if we accept that it\n is OK to send objects between the remote tracking branches the\n fetcher has (i.e. the objects it last fetched from the server)\n and the current tips of branches the server has, without\n optimizing by taking into account that some commits in that set\n may have already been obtained by the fetcher from a\n third-party.\n\nThe scheme tries go gain by reducing the ref advertisement cost by\nsacrificing the optimization opportunity when you fetch from updated\nLinus's tree after having fetched from a recent next tree, the\nlatter of which may have contained a lot of objects that went to the\nLinus's tree since you fetched from Linus's the last time. The\ncurrent protocol, by negotiating what you have (including the\nobjects you obtained from sideways via 'next') with the Linus's\ntree, allows the server to compute a minimum packfile customized\njust for you. By trading that off with \"everybody that follow this\nrepository will get the same set of packfiles in sequence trickled\ninto his repository\" model, it would instead allow the server to\nprepare these packfiles thousands of clients that follow Linus's\ntree will want only once.\n\nThe client-server pair may want to have a negociation mechanism\n(e.g. \"I may have many objects I fetched from sideways, give me\nminimum pack that is customized for me by spending cycles---I am\nwilling to wait until you finish computing it\" vs \"I am just\nfollowing along and not doing anything fancy, just give me the same\nthing as everybody else\") to select what optimization they want to\nuse.\n"},{"id":"256821","messageId":"xmqqioekawmb.fsf@gitster.dls.corp.google.com","threadId":"38638","inReplyTo":"CAGZ79ka8Zg86qqvWByNiP3F6a9QggO-bNY3ZZ9g+A-MdKYQ7NQ@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-02T03:47:40Z","receivedAt":"2015-03-02T03:47:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> A race condition may be a serious objection then? Once people believe the\n> refs can scale fairly well they will use it, which means blasting the ref\n> advertisement will become very worse over time.\n\nI think we are already in agreement about that case:\n\n A misdetected case between (new client, new server) pair might go\n like this:\n\n - new client connects and sends that no-op.\n\n - new server accepts the connection, but that no-op probe has\n not arrived yet.. It misdetects the other side as a v1\n client and it starts blasting the ref advertisement.\n\n - new client notices that the ref advertisement has the\n capability bit and the server is capable of v2 protocol. it\n waits until the server sends \"sorry, I misdetected\" message.\n\n - new server eventually notices the \"no-op probe\" while blasting\n the ref advertisement and it can stop in the middle.\n hopefully this can happen after only sending a few kilobytes\n among megabytes of ref advertisement data ;-). The server\n sends \"sorry, I misdetected\" message to synchronise.\n\n - both sides happily speak v2 from here on.\n\nHowever, I do not think it needs to become worse over time, because\nwe can change and adjust as the user population and their use\npatterns evolve. For example, you can introduce a small delay\nbefore the new versions of server starts the v1 advertisement, and\nmake that delay longer and longer over time, as the population of\nv1-only clients go down, for example.\n\nDifficulty (see J6t's comment) in other implementations may be a\nmore important roadblocks. It seems, however, that our current\nthinking is that it is OK to do the \"allow new v1 clients to notice\nthe availabilty of v2 servers, so that they can talk v2 the next\ntime\" thing, so my preference is to throw this \"client first and let\nserver notice\" into \"maybe doable but not our first choice\" bin, at\nleast for now.\n\nThanks.\n"},{"id":"256822","messageId":"20150302092136.GA30278@lanh","threadId":"38638","inReplyTo":"xmqqioekawmb.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-02T09:21:36Z","receivedAt":"2015-03-02T09:21:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Mar 01, 2015 at 07:47:40PM -0800, Junio C Hamano wrote:\n> It seems, however, that our current thinking is that it is OK to do\n> the \"allow new v1 clients to notice the availabilty of v2 servers,\n> so that they can talk v2 the next time\" thing, so my preference is\n> to throw this \"client first and let server notice\" into \"maybe\n> doable but not our first choice\" bin, at least for now.\n\nOK let's see if first choice like this could work. Very draft but it\nshould give some idea how to make a prototype to test it out. Note\nthat the server still speaks first in this proposal.\n\n-- 8< --\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex 462e206..32a1186 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -1,11 +1,11 @@\n Packfile transfer protocols\n ===========================\n \n-Git supports transferring data in packfiles over the ssh://, git:// and\n+Git supports transferring data in packfiles over the ssh://, git://, http:// and\n file:// transports. There exist two sets of protocols, one for pushing\n data from a client to a server and another for fetching data from a\n server to a client. All three transports (ssh, git, file) use the same\n-protocol to transfer data.\n+protocol to transfer data. http is documented in http-protocol.txt.\n \n The processes invoked in the canonical Git implementation are 'upload-pack'\n on the server side and 'fetch-pack' on the client side for fetching data;\n@@ -14,6 +14,12 @@ data. The protocol functions to have a server tell a client what is\n currently on the server, then for the two to negotiate the smallest amount\n of data to send in order to fully update one or the other.\n \n+\"upload-pack-2\" and \"receive-pack-2\" are the next generation of\n+\"upload-pack\" and \"receive-pack\" respectively. The first two are\n+referred as \"version 2\" in this document and pack-capabilities.txt\n+while the last two are \"version 1\". Unless stated otherwise, \"version 1\"\n+is implied.\n+\n Transports\n ----------\n There are three transports over which the packfile protocol is\n@@ -42,7 +48,8 @@ hostname parameter, terminated by a NUL byte.\n \n --\n git-proto-request = request-command SP pathname NUL [ host-parameter NUL ]\n- request-command = \"git-upload-pack\" / \"git-receive-pack\" /\n+ request-command = \"git-upload-pack\" / \"git-upload-pack-2\" /\n+\t\t \"git-receive-pack\" / \"git-receive-pack-2\" /\n \t\t \"git-upload-archive\" ; case sensitive\n pathname = *( %x01-ff ) ; exclude NUL\n host-parameter = \"host=\" hostname [ \":\" port ]\n@@ -67,7 +74,6 @@ gracefully with an error message.\n error-line = PKT-LINE(\"ERR\" SP explanation-text)\n ----\n \n-\n SSH Transport\n -------------\n \n@@ -124,9 +130,58 @@ has, the first can 'fetch' from the second. This operation determines\n what data the server has that the client does not then streams that\n data down to the client in packfile format.\n \n+Capability discovery (v2)\n+-------------------------\n \n-Reference Discovery\n--------------------\n+In version 1, capability discovery is part of reference discovery and\n+covered in reference discovery section.\n+\n+In versino 2, when the client initially connects, the server\n+immediately sends its capabilities to the client. Then the client must\n+send the list of server capabilities it wants to use to the server.\n+\n+ S: 00XXcapabilities multi_ack thin-pack ofs-delta lang\\n\n+ C: 00XXcapabilities thin-pack ofs-delta lang=en\\n\n+\n+----\n+ cap = PKT-LINE(\"capabilities\" SP capability-list LF)\n+ capability-list = capability *(SP capability)\n+ capability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n+ LC_ALPHA = %x61-7A\n+----\n+\n+The client MUST NOT ask for capabilities the server did not say it\n+supports.\n+\n+Server MUST diagnose and abort if capabilities it does not understand\n+was sent. Server MUST NOT ignore capabilities that client requested\n+and server advertised. As a consequence of these rules, server MUST\n+NOT advertise capabilities it does not understand.\n+\n+See protocol-capabilities.txt for a list of allowed server and client\n+capabilities and descriptions.\n+\n+XXX: this approach wastes one round trip in smart-http because the\n+client would speak first. Perhaps we could allow client speculation.\n+It can assume what server caps will send and send commands based on that\n+assumption. If it turns out true, we save one round trip. E.g. fast\n+path:\n+\n+ C: You are supposed to send caps A, B. I would respond with cap B.\n+ Then I would send \"want-refs refs/heads/foo\".\n+ S: (yes we are sending caps A and B), validate client caps,\n+ execute \"want-refs\" and return ref list\n+\n+and slow path:\n+\n+ C: You are supposed to send caps A, B. I would respond with cap B.\n+ Then I would send \"want-refs refs/heads/foo\".\n+ S: Send caps A, B and C. ignore the rest from client\n+ C: Want caps A and C. Send \"want-refs foo\"\n+ S: return ref foo\n+\n+Reference Discovery (v1)\n+------------------------\n \n When the client initially connects the server will immediately respond\n with a listing of each reference it has (all branches and tags) along\n@@ -178,16 +233,69 @@ MUST peel the ref if it's an annotated tag.\n shallow = PKT-LINE(\"shallow\" SP obj-id)\n \n capability-list = capability *(SP capability)\n- capability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\")\n+ capability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n LC_ALPHA = %x61-7A\n ----\n \n Server and client MUST use lowercase for obj-id, both MUST treat obj-id\n as case-insensitive.\n \n+On the very first line of the initial server response of either\n+receive-pack and upload-pack the first reference is followed by\n+a NUL byte and then a list of space delimited server capabilities.\n+These allow the server to declare what it can and cannot support\n+to the client.\n+\n+Client will then send a space separated list of capabilities it wants\n+to be in effect. The client MUST NOT ask for capabilities the server\n+did not say it supports.\n+\n+Server MUST diagnose and abort if capabilities it does not understand\n+was sent. Server MUST NOT ignore capabilities that client requested\n+and server advertised. As a consequence of these rules, server MUST\n+NOT advertise capabilities it does not understand.\n+\n See protocol-capabilities.txt for a list of allowed server capabilities\n and descriptions.\n \n+Reference Discovery (v2)\n+------------------------\n+\n+In version 2, reference discovery is initiated by the client with\n+\"want-refs\" line. The client may skip reference discovery phase\n+entirely by not sending \"want-refs\" (e.g. the client has another way\n+to retrieve ref list).\n+\n+----\n+ want-refs = PKT-LINE(\"want-refs\" SP mode *argument)\n+ mode = \"all\"\n+ argument = SP arg\n+ arg = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n+----\n+\n+Mode \"all\" sends all visible refs to the client like in version 1. No\n+arguments are accepted in this mode. More modes may be available based\n+on capabilities.\n+\n+----\n+ advertised-refs = (no-refs / list-of-refs)\n+\t\t *shallow\n+\t\t flush-pkt\n+\n+ no-refs = PKT-LINE(zero-id LF)\n+\n+ list-of-refs = *other-ref\n+ other-ref = PKT-LINE(other-tip / other-peeled)\n+ other-tip = obj-id SP refname LF\n+ other-peeled = obj-id SP refname \"^{}\" LF\n+\n+ shallow = PKT-LINE(\"shallow\" SP obj-id)\n+\n+ capability-list = capability *(SP capability)\n+ capability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n+ LC_ALPHA = %x61-7A\n+----\n+\n Packfile Negotiation\n --------------------\n After reference and capabilities discovery, the client can decide to\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex 4f8a7bf..ecb0efd 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -3,21 +3,6 @@ Git Protocol Capabilities\n \n Servers SHOULD support all capabilities defined in this document.\n \n-On the very first line of the initial server response of either\n-receive-pack and upload-pack the first reference is followed by\n-a NUL byte and then a list of space delimited server capabilities.\n-These allow the server to declare what it can and cannot support\n-to the client.\n-\n-Client will then send a space separated list of capabilities it wants\n-to be in effect. The client MUST NOT ask for capabilities the server\n-did not say it supports.\n-\n-Server MUST diagnose and abort if capabilities it does not understand\n-was sent. Server MUST NOT ignore capabilities that client requested\n-and server advertised. As a consequence of these rules, server MUST\n-NOT advertise capabilities it does not understand.\n-\n The 'atomic', 'report-status', 'delete-refs', 'quiet', and 'push-cert'\n capabilities are sent and recognized by the receive-pack (push to server)\n process.\n@@ -268,3 +253,10 @@ to accept a signed push certificate, and asks the to be\n included in the push certificate. A send-pack client MUST NOT\n send a push-cert packet unless the receive-pack server advertises\n this capability.\n+\n+v2\n+--\n+\n+'git-upload-pack' and 'git-receive-pack' may advertise this capability\n+if the server supports 'git-upload-pack-2' and 'git-receive-pack-2'\n+respectively.\n-- 8< --\n"},{"id":"256823","messageId":"20150302092430.GA30395@lanh","threadId":"38638","inReplyTo":"20150302092136.GA30278@lanh","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-02T09:24:30Z","receivedAt":"2015-03-02T09:24:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Mar 02, 2015 at 04:21:36PM +0700, Duy Nguyen wrote:\n> On Sun, Mar 01, 2015 at 07:47:40PM -0800, Junio C Hamano wrote:\n> > It seems, however, that our current thinking is that it is OK to do\n> > the \"allow new v1 clients to notice the availabilty of v2 servers,\n> > so that they can talk v2 the next time\" thing, so my preference is\n> > to throw this \"client first and let server notice\" into \"maybe\n> > doable but not our first choice\" bin, at least for now.\n> \n> OK let's see if first choice like this could work. Very draft but it\n> should give some idea how to make a prototype to test it out. Note\n> that the server still speaks first in this proposal.\n\nAnd ref discovery phase could be modified by new capabilities. For\nexample,\n\n-- 8< --\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex 56c11b4..56a8c2e 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -304,3 +304,36 @@ language code.\n \n The default language code is unspecified, even though it's usually\n English in ASCII encoding.\n+\n+compressed-refs\n+---------------\n+\n+This is applicable to upload-pack-2 and receive-pack-2 only. The\n+client expects ref list in reference discovery phase to be sent in\n+compressed format:\n+\n+ - Each PKT-LINE may contain more than one ref\n+ - SHA-1 is in binary encoding (i.e. 20 bytes instead of\n+ 40 bytes as hex string)\n+ - ref name is prefix compressed, see index-format.txt version 4.\n+ - Ref list ends with flush-pkt\n+\n+glob-refs\n+---------\n+\n+This is applicable to upload-pack-2 and receive-pack-2 only. In\n+reference discovery phase, a new mode \"glob\" is supported. Where the\n+arguments are wildmatch patterns. Negative patterns begin with '!'.\n+Only refs matching requested patterns are sent to the client.\n+\n+stateful-refs\n+-------------\n+\n+This is applicable to upload-pack-2 and receive-pack-2 only. In\n+reference discovery phase, a new mode \"stateful\" is supported. Where\n+the first argument is a string representing the ref list that was sent\n+by the same server last time. The remaining arguments are glob.\n+\n+The first ref line that the server sends should carry a new state\n+string after ref name. The server may send only updated refs it if\n+understands the state string sent by the client. Still under discussion.\n-- 8< --\n"},{"id":"256831","messageId":"20150302093259.GA30459@lanh","threadId":"38638","inReplyTo":"9EB6341F415E41F3A07F9046056ACE13@PhilipOakley","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-02T09:32:59Z","receivedAt":"2015-03-02T09:32:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Mar 01, 2015 at 11:06:21PM -0000, Philip Oakley wrote:\n> OK, maybe not exactly about protocol, but a possible option would be the \n> ability to send the data as a bundle or multi-bundles; Or perhasps as an \n> archive, zip, or tar.\n> \n> Data can then be exchanged across an airgap or pigeon mail. The airgap \n> scenario is likely a real case that's not directly prominent at the \n> moment, just because it's not tha direct.\n> \n> There has been discussion about servers having bundles available for \n> clones, but with a multi-bundle, one could package up a large bundle \n> (months) and an increment (weeks, and then days), before an final easy \n> to pack last few hours. That would be a server work trade-off, and \n> support a CDN view if needed.\n> \n> If such an approach was reasonable would the protocol support it? etc.\n\nIt came up several times. Many people are in favor of it. Some\nreferences..\n\n http://thread.gmane.org/gmane.comp.version-control.git/264305/focus=264565\n http://thread.gmane.org/gmane.comp.version-control.git/263898/focus=263928\n http://thread.gmane.org/gmane.comp.version-control.git/263898/focus=264000\n http://thread.gmane.org/gmane.comp.version-control.git/238472/focus=238844\n\nThis is what I got so far. I think the hard part is how to let\nprojects control this in a clean and flexible way. Not written in the\npatch, but I'm thinking maybe we can allow hooking a remote helper in\nstandard git://, ssh://, http://... That would give total control to\nprojects.\n\n-- 8< --\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex ecb0efd..2b99464 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -260,3 +260,34 @@ v2\n 'git-upload-pack' and 'git-receive-pack' may advertise this capability\n if the server supports 'git-upload-pack-2' and 'git-receive-pack-2'\n respectively.\n+\n+redirect\n+--------\n+\n+This capability is applicable for upload-pack and upload-pack-v2\n+only. When the client requests this capability it must specify\n+supported transport protocol separated by colon,\n+e.g. \"redirect=http:ftp:ssh:torrent\".\n+\n+Instead of sending a packfile data to the client, the server may send\n+4-byte signature { 'L', 'I', 'N', 'K' } followed by a NUL-terminated\n+URLs, each one pointing to a bundle. This fake pack ends with an empty\n+string.\n+\n+The bundle does not have to contain all refs requested by the\n+client. Different bundles from different URLs could have different\n+content. The client must follow one of the links to get a bundle.\n+The server must not send URL in a protocol that the client does not\n+support.\n+\n+FIXME: do we keep current connection alive until the bundle is\n+downloaded and get a normal pack, or let the client initiate a new\n+connection? Or perhaps if the client fails to get the bundle for\n+whatever reason, it could send \"NAK\" to the server and the server\n+sends normal packfile data.\n+\n+FIXME: how do we implement this exactly? The decision to redirect\n+should probably be delegated to some hook. Maybe sending all \"want\"\n+lines to the script is enough.. Sending \"have\" lines is more difficult\n+because the server decides when to stop receiving them. That decision\n+must be moved to the hook...\n-- 8< --\n"},{"id":"256888","messageId":"20150303103351.GA4922@lanh","threadId":"38638","inReplyTo":"20150302092136.GA30278@lanh","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-03T10:33:51Z","receivedAt":"2015-03-03T10:33:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Mar 02, 2015 at 04:21:36PM +0700, Duy Nguyen wrote:\n> On Sun, Mar 01, 2015 at 07:47:40PM -0800, Junio C Hamano wrote:\n> > It seems, however, that our current thinking is that it is OK to do\n> > the \"allow new v1 clients to notice the availabilty of v2 servers,\n> > so that they can talk v2 the next time\" thing, so my preference is\n> > to throw this \"client first and let server notice\" into \"maybe\n> > doable but not our first choice\" bin, at least for now.\n> \n> OK let's see if first choice like this could work. Very draft but it\n> should give some idea how to make a prototype to test it out. Note\n> that the server still speaks first in this proposal.\n\nJunio pointed out in private that I didn't address the packet length\nlimit (64k). I thought I could get away with a new capability\n(i.e. not worry about it now) but I finally admit that was a bad\nhack. So perhaps this on top.\n\nThe \"WARN\" line was originally supposed to be used when packet length\nis still 64k and the server has a ref longer than that. It could then\nskip that long ref and inform the user, so the user could re-request\nagain, this time asking for \"long packet length\" capability.\n\nThat's irrelevant now. But I think an option to say something without\nabort may still be a good idea, especially if we allow hooks to\nintercept the protocol.\n\n-- 8< --\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex 32a1186..e2003c0 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -37,6 +37,20 @@ communicates with that invoked process over the SSH connection.\n The file:// transport runs the 'upload-pack' or 'receive-pack'\n process locally and communicates with it over a pipe.\n \n+Pkt-line format\n+---------------\n+\n+In version 1, a packet line consists of four bytes containing the\n+length of the entire line plus four, in hexadecimal format. A flush\n+consists of four zero bytes.\n+\n+In version 2, the four-byte header format remains supported but the\n+maximum length is 0xfffe. If the length is 0xffff, the actual length\n+follows in variable encoding in hexadecimal.\n+\n+XXX: perhaps go with 2-byte length by default instead because we don't\n+usually need pkt-line longer than 256?? Maybe not worth saving a couple bytes\n+\n Git Transport\n -------------\n \n@@ -68,10 +82,12 @@ process on the server side over the Git protocol is this:\n nc -v example.com 9418\n \n If the server refuses the request for some reasons, it could abort\n-gracefully with an error message.\n+gracefully with an error message, or show a warning but and keep\n+moving.\n \n ----\n error-line = PKT-LINE(\"ERR\" SP explanation-text)\n+ warning-line = PKT-LINE(\"WARN\" SP explanation-text)\n ----\n \n SSH Transport\n-- 8< --\n"},{"id":"256913","messageId":"xmqqk2yy80mq.fsf@gitster.dls.corp.google.com","threadId":"38638","inReplyTo":"20150303103351.GA4922@lanh","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-03T17:13:49Z","receivedAt":"2015-03-03T17:13:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> Junio pointed out in private that I didn't address the packet length\n> limit (64k). I thought I could get away with a new capability\n> (i.e. not worry about it now) but I finally admit that was a bad\n> hack. So perhaps this on top.\n\nNo, I didn't ;-) but I tend to agree that \"perhaps 4GB huge packet?\"\nis a bad idea.\n\nThe problem I had with the version in your write-up was that it\nstill assumed that all capabilities must come on one packet-line.\n\nThe immediate issue that limitation in the current protocol we had\nwas that the usual \"we can help newer programs to operate better\nwhile getting ignored by existing programs by sending optional\ninformation as part of the capability advert\" would not work for\n\"upload-pack\" to enumerate symrefs and their targets to help\n\"clone\".\n\nThe lesson to draw from that experience is not \"we should have an\noption to use large packets\". 64kB is plenty but the senders and\nthe receivers have a lot lower limit in practice to avoid harming\nlatency (I think it is like 1000 bytes before both ends agree to\nswitch talking over the sideband multiplexer). It is not \"we should\nanticipate and design protocol better\", either. We are humans and\nit is hard to predict things, especially things in the future.\n\nThe lesson we should learn is that it is important to leave us\nenough wiggle room to allow us cope with such unanticipated\nlimitations ;-).\n\nMy recollection is that the consensus from the last time we\ndiscussed protocol revamping was to list one capability per packet\nso that packet length limit does not matter, but you may want to\ncheck with the list archive yourself.\n"},{"id":"256918","messageId":"xmqqd24pc185.fsf@gitster.dls.corp.google.com","threadId":"38638","inReplyTo":"xmqqk2yy80mq.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-03T19:47:22Z","receivedAt":"2015-03-03T19:47:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Duy Nguyen writes:\n>\n>> Junio pointed out in private that I didn't address the packet length\n>> limit (64k). I thought I could get away with a new capability\n>> (i.e. not worry about it now) but I finally admit that was a bad\n>> hack. So perhaps this on top.\n>\n> No, I didn't ;-) but I tend to agree that \"perhaps 4GB huge packet?\"\n> is a bad idea.\n> ...\n\nI realize that I responded with \"No, I did not complain about X, I\nhad trouble about Y and here is why\" and talked mostly about Y\nwithout talking much about X. So let's touch X a bit.\n\nAs to the packet length, I think it is a good idea to give us an\nescape hatch to bust 64k limit. Refs may not be the reason to do\nso, but as I said, we cannot forsee the future needs.\n\nHaving X behind us, now back to Y, and then I'll remind us of Z ;-)\n[*1*]\n\n> My recollection is that the consensus from the last time we\n> discussed protocol revamping was to list one capability per packet\n> ...\n\nAnd the above is \"the right thing\" from the protocol point of view.\nThe only reason the current protocol says \"capabilities go on a\nsingle line separated by SP\" is because the hole we found to add to\nthe protocol was to piggyback after the ref advertisement lines, and\nthere was no guarantee that we have more than one ref advertised, so\nwe needed to be able to stuff everything on a single line.\n\nStepping back and thinking about what a \"packet\" in pkt-line\nprotocol is, we realize that it is the smallest logical unit of\ntransferring information. The state of a single ref in a series of\nref advertisement. The fact that receiving end has all the history\nleading up to a single commit. The request to obtain all history\nleading up to a single commit.\n\nThat is why I say that one-cap-per-packet is the right thing.\n\nThese individual logical units are grouped into a larger logical\nunit by (1) being at a specific point in the protocol exchange, (2)\nbeing adjacent to each other and (3) terminated by a \"flush\npacket\". Examples:\n\n - A bunch of individual ref states that is at the beginning of the\n upload-pack to fetch-pack commniucation that ends with a flush\n constitutes a ref advertisement. \n\n - A series of \"want\" packets at the beginning of the fetch-pack to\n upload-pack communiucation that ends with a flush constitutes a\n fetch request.\n\nAnother thing I didn't find in the updated documentation was a\nproposal to define what a \"flush\" exactly means. \n\nIn my above writing, it should be clear that a \"flush\" is merely\n\"the end of a group\". It does not mean (and it never meant, until\nsmart HTTP) \"I am finished talking, now it is your turn.\" If a\nrequestor needs to give two groups of items before the responder can\nprocess the request, we would want to be able to say \"A1, A2, ...,\nnow I am done with As; B1, B2, B3, ..., now I am done with Bs; this\nconcludes my request, and it is your turn to process and respond to\nme\". But you cannot easily do so without affecting smart HTTP, as\nit is written in such a way that it assumes \"flush\" is \"I am done,\nit is your turn\".\n\nI am perfectly OK if v2 redefined \"flush\" to mean \"I am done, it is\nyoru turn.\" But then the protocol should have another way to allow\npackets into larger groups. A sequence of packets \"begin A\", \"A1\",\n\"A2\", ..., \"end\", \"begin B\", \"B1\", \"B2\", \"B3\", \"end\", \"flush\" may be\na way to do so, and if we continue to rely on the order of packets\nto help determine the semantics (aka \"being at a specific point in\nthe protocol exchange\" above), we may even be able to omit \"begin A\"\nand \"begin B\" packets (i.e. the \"end\" is the new \"end of a logical\ngroup\", which is what \"flush\" originally was).\n\n\n[Footnote]\n\n *1* For those who haven't been following the discussion:\n\n X: maximum packet length being 64kB might be problematic.\n\n Y: requiring capability advertisement and request in a single\n packet is wrong.\n\n Z: the meaning of \"flush\" needs to be clarified.\n"},{"id":"256955","messageId":"CACsJy8B_r_0nP9NyKFBnr9bXgwjx8dJkSVkHbZw+Mxin_YpZZw@mail.gmail.com","threadId":"38638","inReplyTo":"xmqqk2yy80mq.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-04T01:54:57Z","receivedAt":"2015-03-04T01:54:57Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 4, 2015 at 12:13 AM, Junio C Hamano wrote:\n> My recollection is that the consensus from the last time we\n> discussed protocol revamping was to list one capability per packet\n> so that packet length limit does not matter, but you may want to\n> check with the list archive yourself.\n\nI couldn't find that consensus mail, but this one [1] is good enough\nevidence that we can hit packet length limit in capability line\neasily.\n\nWith an escape hatch to allow maximum packet length up to uint_max, I\nthink we'll be fine for a long time even if we don't send one cap per\npkt-line. So I'm trying to see if we really want to go with one cap\nper pkt-line..\n\nPros:\n\n - better memory management, current pkt-line static buffer is probably fine\n - a capability can contain spaces after '='\n\nCons:\n\n - some refactoring needed to hide away differences between v1 and v2\n\nLooks like one cap per pkt-line is winning..\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/237929\n-- \nDuy\n"},{"id":"256960","messageId":"CAJo=hJtgda4ATtPeLWbhgDPiTm9h-gzBkpknxsOdRddueUgTbw@mail.gmail.com","threadId":"38638","inReplyTo":"CACsJy8B_r_0nP9NyKFBnr9bXgwjx8dJkSVkHbZw+Mxin_YpZZw@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2015-03-04T04:27:33Z","receivedAt":"2015-03-04T04:27:33Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Tue, Mar 3, 2015 at 5:54 PM, Duy Nguyen wrote:\n> On Wed, Mar 4, 2015 at 12:13 AM, Junio C Hamano wrote:\n>> My recollection is that the consensus from the last time we\n>> discussed protocol revamping was to list one capability per packet\n>> so that packet length limit does not matter, but you may want to\n>> check with the list archive yourself.\n>\n> I couldn't find that consensus mail, but this one [1] is good enough\n> evidence that we can hit packet length limit in capability line\n> easily.\n> With an escape hatch to allow maximum packet length up to uint_max, I\n\nThe symbolic ref thing was done badly. There isn't an escape hatch in\ncurrent v1 protocol sufficient to allow this but each ref should be\nits own pkt-line, or should be a small batch of refs per pkt-line, or\nthe ref advertisement should be a data stream in a side-band-64k sort\nof format inside the pkt-line framing.\n\nAt 64k per frame of side-band there is plenty of data to header ratio\nthat we don't need to escape to uint_max.\n\n> Looks like one cap per pkt-line is winning..\n\nYes.\n\n> [1] http://thread.gmane.org/gmane.comp.version-control.git/237929\n\n\nLet me go on a different tangent a bit from the current protocol.\n\nhttp://www.grpc.io/ was recently released and is built on the HTTP/2\nstandard. It uses protobuf as a proven extensibility mechanism.\nIncluding a full C based grpc stack just to speak the Git wire\nprotocol is quite likely overkill, but I think the embedding of a\nproven extensible format inside of a bi-directional framed streaming\nsystem like HTTP/2 offers some good guidance.\n\nNetwork protocol parsing is hard. Especially in languages like C where\nbuffer overflows are possible. Or where a client could trivially DoS a\nserver by sending a packet of size uint_max and the server naively\ntrying to malloc() that buffer. Defining the network protocol in an\nIDL like protobuf 3 and being machine generated from stable well\nmaintained code has its advantages.\n"},{"id":"256998","messageId":"CACsJy8B=MN412dZYYc=NwpXHVz=Pxv+8wWMT7JNurVgqKrWn8A@mail.gmail.com","threadId":"38638","inReplyTo":"CAJo=hJtgda4ATtPeLWbhgDPiTm9h-gzBkpknxsOdRddueUgTbw@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-04T12:05:08Z","receivedAt":"2015-03-04T12:05:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 4, 2015 at 11:27 AM, Shawn Pearce wrote:\n> Let me go on a different tangent a bit from the current protocol.\n>\n> http://www.grpc.io/ was recently released and is built on the HTTP/2\n> standard. It uses protobuf as a proven extensibility mechanism.\n> Including a full C based grpc stack just to speak the Git wire\n> protocol is quite likely overkill, but I think the embedding of a\n> proven extensible format inside of a bi-directional framed streaming\n> system like HTTP/2 offers some good guidance.\n\nI'll take this as \"learn from grpc, not just reuse grpc\"\n\n> Network protocol parsing is hard. Especially in languages like C where\n> buffer overflows are possible. Or where a client could trivially DoS a\n> server by sending a packet of size uint_max and the server naively\n> trying to malloc() that buffer. Defining the network protocol in an\n> IDL like protobuf 3 and being machine generated from stable well\n> maintained code has its advantages.\n\nI'm still studying the spec, so I can't comment if using IDL/protobuf3\nis a good idea yet.\n\nBut I think at least we can avoid DoS by changing the pkt-line (again)\na bit: the length 0xffff means that actual length is 0xfffe and the\nnext pkt-line is part of this pkt-line. Higher level (upload-pack or\nfetch-pack, for example) must set an upper limit for packet_read() so\nit won't try to concatenate pkt-lines forever.\n-- \nDuy\n"},{"id":"257008","messageId":"CAJo=hJupr=07+r6z5s9RiT_v2LYtdioiVS=yNfMag+mKfH9FhA@mail.gmail.com","threadId":"38638","inReplyTo":"CACsJy8B=MN412dZYYc=NwpXHVz=Pxv+8wWMT7JNurVgqKrWn8A@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2015-03-04T19:10:21Z","receivedAt":"2015-03-04T19:10:21Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Wed, Mar 4, 2015 at 4:05 AM, Duy Nguyen wrote:\n> On Wed, Mar 4, 2015 at 11:27 AM, Shawn Pearce wrote:\n>> Let me go on a different tangent a bit from the current protocol.\n>>\n>> http://www.grpc.io/ was recently released and is built on the HTTP/2\n>> standard. It uses protobuf as a proven extensibility mechanism.\n>> Including a full C based grpc stack just to speak the Git wire\n>> protocol is quite likely overkill, but I think the embedding of a\n>> proven extensible format inside of a bi-directional framed streaming\n>> system like HTTP/2 offers some good guidance.\n>\n> I'll take this as \"learn from grpc, not just reuse grpc\"\n\nCorrect, that was what I was trying to say and I just wrote it poorly.\n\nHTTP 1.x, HTTP/2 and protobuf have proven themselves to be fairly open\nto extension and working well in the wild for transports. There is\nuseful guidance there that we should draw from to try and leave doors\nopen for the future.\n\nHTTP/2, protobuf and grpc are fairly complex. I consider any one of\nthem too complicated for Git specific use. However HTTP/2 is probably\nthe future of HTTP stacks so we may see it show up in libcurl or\nsomething as popular as libcurl in another 10 years. Hg had some\nreasonably sane ideas about building the wire protocol to work well on\nHTTP 1.x upfront rather than Git tacking it on much later.\n\n>> Network protocol parsing is hard. Especially in languages like C where\n>> buffer overflows are possible. Or where a client could trivially DoS a\n>> server by sending a packet of size uint_max and the server naively\n>> trying to malloc() that buffer. Defining the network protocol in an\n>> IDL like protobuf 3 and being machine generated from stable well\n>> maintained code has its advantages.\n>\n> I'm still studying the spec, so I can't comment if using IDL/protobuf3\n> is a good idea yet.\n>\n> But I think at least we can avoid DoS by changing the pkt-line (again)\n> a bit: the length 0xffff means that actual length is 0xfffe and the\n> next pkt-line is part of this pkt-line. Higher level (upload-pack or\n> fetch-pack, for example) must set an upper limit for packet_read() so\n> it won't try to concatenate pkt-lines forever.\n\npkt-line is a reasonably simple and efficient framing system. A 64 KiB\npkt-line frame only costs ~0.0061% overhead; ~0.0076% overhead if you\nare a pack stream in a side-band-64k channel. That is probably more\nefficient than HTTP/2 or SSL framing.\n\nI see no reason to attempt to try and reduce that overhead further. 64\nKiB frame size is enough for anyone to move data efficiently with\nthese headers. In practice you are going to wrap that up into SSH or\nSSL/TLS and those overheads are so much higher it doesn't matter we\nhave a tiny loss here.\n\nI think a mistake in the wire protocol was making the pkt-line length\nhuman readable hex, but the sideband channel binary. _If_ we redo the\nframing the only change I would make is making the side band readable.\nThus far we have only used 0, 1, 2 for sideband channels. These could\neasily be moved into human readable channel ids:\n\n 'd': currently sideband 0; this is the application data, aka the pack data\n 'p': currently sideband 1; this is the progress stream for stderr\n 'e': currently sideband 2; there was an error, data in this packet\nis the message text, and the connection will shutdown after the\npacket.\n\nAnd then leave all other sideband values undefined and reserved for\nfuture use, just like they are all open today.\n\nI am not convinced framing changes are necessary. I would fine with\nleaving the sideband streams as 0,1,2... but if we want a text based\nprotocol for ease of debugging we should be text based across the\nboard and try very hard to avoid these binary values in the framing,\nor ever needing to use a magical NUL byte in the middle of a packet to\nfind a gap in older parsers for newer data.\n\n\nIf you want to build a larger stream like ref advertisement inside a\npkt-line framing without using a pkt-line per ref record, you should\nfollow the approach used by pack data streams where it uses the 5 byte\nside-band pkt-line framing and a side-band channel is allocated for\nthat data. Application code can then run a side-band demux to yank out\nthe inner stream and parse it.\n\nIt may be simpler to restrict ref names to be smaller than 64k in\nlength so you have room for framing and hash value to be transferred\ninside of a single pkt-line, then use the pkt-line framing to do the\ntransfer.\n\nToday's upload-pack ref advertisment has ~25% overhead. Most of that\nis in the duplicated tag name for the peeled refs/tags/v1.0^{} lines.\nIf you drop those names (but keep the pkt-line and SHA-1), its only\nabout 8% overhead above the packed-refs file.\n\nI think optimization efforts for ref advertisement need to focus on\nreducing the number of refs sent back and forth, not shrinking the\nindividual records down smaller.\n\n\nEarlier in this thread Junio raised a point that the flush-pkt is\nconfusing because it has way too many purposes. I agree. IIRC we have\n0001-0003 as invalid pkt-line headers that could be used for special\nmarkers in the stream.\n\nFor example we could use 0000 flush-pkt as \"end of section\" and define\n0001 as \"i am done speaking and now wait for you to reply\".\n\nI think we need to keep 0004 as \"empty packet\" in case we ever get\nourselves into a position where it makes sense to pass an empty string\nfrom one end of the connection to another.\n\n\nRunning pkt-line inside of side-band-64k is... weird to read. IIRC we\nonly do that in the smart HTTP protocol to the remote-curl helper, and\nthis is where Junio's remarks about flush-pkt being confusing come\nfrom. If we rewrote the protocol to have that explicit 0001 \"i am done\ntalking\" marker the helper wouldn't need the double framing to\nunderstand how to break up the originally bidirectional packet stream\ninto a sequence of bursts sent on unidirectional HTTP 1.x.\n"},{"id":"257044","messageId":"CAGZ79ka0TFO6ZUd-vcxoDNAOyYPYdkLicmObTGhHb_hcDUm4-A@mail.gmail.com","threadId":"38638","inReplyTo":"CAJo=hJupr=07+r6z5s9RiT_v2LYtdioiVS=yNfMag+mKfH9FhA@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-05T01:03:22Z","receivedAt":"2015-03-05T01:03:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 4, 2015 at 11:10 AM, Shawn Pearce wrote:\n> On Wed, Mar 4, 2015 at 4:05 AM, Duy Nguyen wrote:\n>> On Wed, Mar 4, 2015 at 11:27 AM, Shawn Pearce wrote:\n>>> Let me go on a different tangent a bit from the current protocol.\n>>>\n>>> http://www.grpc.io/ was recently released and is built on the HTTP/2\n>>> standard. It uses protobuf as a proven extensibility mechanism.\n>>> Including a full C based grpc stack just to speak the Git wire\n>>> protocol is quite likely overkill, but I think the embedding of a\n>>> proven extensible format inside of a bi-directional framed streaming\n>>> system like HTTP/2 offers some good guidance.\n>>\n>> I'll take this as \"learn from grpc, not just reuse grpc\"\n>\n> Correct, that was what I was trying to say and I just wrote it poorly.\n>\n> HTTP 1.x, HTTP/2 and protobuf have proven themselves to be fairly open\n> to extension and working well in the wild for transports. There is\n> useful guidance there that we should draw from to try and leave doors\n> open for the future.\n>\n> HTTP/2, protobuf and grpc are fairly complex. I consider any one of\n> them too complicated for Git specific use. However HTTP/2 is probably\n> the future of HTTP stacks so we may see it show up in libcurl or\n> something as popular as libcurl in another 10 years. Hg had some\n> reasonably sane ideas about building the wire protocol to work well on\n> HTTP 1.x upfront rather than Git tacking it on much later.\n>\n>>> Network protocol parsing is hard. Especially in languages like C where\n>>> buffer overflows are possible. Or where a client could trivially DoS a\n>>> server by sending a packet of size uint_max and the server naively\n>>> trying to malloc() that buffer. Defining the network protocol in an\n>>> IDL like protobuf 3 and being machine generated from stable well\n>>> maintained code has its advantages.\n>>\n>> I'm still studying the spec, so I can't comment if using IDL/protobuf3\n>> is a good idea yet.\n>>\n>> But I think at least we can avoid DoS by changing the pkt-line (again)\n>> a bit: the length 0xffff means that actual length is 0xfffe and the\n>> next pkt-line is part of this pkt-line. Higher level (upload-pack or\n>> fetch-pack, for example) must set an upper limit for packet_read() so\n>> it won't try to concatenate pkt-lines forever.\n>\n> pkt-line is a reasonably simple and efficient framing system. A 64 KiB\n> pkt-line frame only costs ~0.0061% overhead; ~0.0076% overhead if you\n> are a pack stream in a side-band-64k channel. That is probably more\n> efficient than HTTP/2 or SSL framing.\n>\n> I see no reason to attempt to try and reduce that overhead further. 64\n> KiB frame size is enough for anyone to move data efficiently with\n> these headers. In practice you are going to wrap that up into SSH or\n> SSL/TLS and those overheads are so much higher it doesn't matter we\n> have a tiny loss here.\n>\n> I think a mistake in the wire protocol was making the pkt-line length\n> human readable hex, but the sideband channel binary. _If_ we redo the\n> framing the only change I would make is making the side band readable.\n> Thus far we have only used 0, 1, 2 for sideband channels. These could\n> easily be moved into human readable channel ids:\n>\n> 'd': currently sideband 0; this is the application data, aka the pack data\n> 'p': currently sideband 1; this is the progress stream for stderr\n> 'e': currently sideband 2; there was an error, data in this packet\n> is the message text, and the connection will shutdown after the\n> packet.\n>\n> And then leave all other sideband values undefined and reserved for\n> future use, just like they are all open today.\n>\n> I am not convinced framing changes are necessary. I would fine with\n> leaving the sideband streams as 0,1,2... but if we want a text based\n> protocol for ease of debugging we should be text based across the\n> board and try very hard to avoid these binary values in the framing,\n> or ever needing to use a magical NUL byte in the middle of a packet to\n> find a gap in older parsers for newer data.\n>\n>\n> If you want to build a larger stream like ref advertisement inside a\n> pkt-line framing without using a pkt-line per ref record, you should\n> follow the approach used by pack data streams where it uses the 5 byte\n> side-band pkt-line framing and a side-band channel is allocated for\n> that data. Application code can then run a side-band demux to yank out\n> the inner stream and parse it.\n>\n> It may be simpler to restrict ref names to be smaller than 64k in\n> length so you have room for framing and hash value to be transferred\n> inside of a single pkt-line, then use the pkt-line framing to do the\n> transfer.\n>\n> Today's upload-pack ref advertisment has ~25% overhead. Most of that\n> is in the duplicated tag name for the peeled refs/tags/v1.0^{} lines.\n> If you drop those names (but keep the pkt-line and SHA-1), its only\n> about 8% overhead above the packed-refs file.\n>\n> I think optimization efforts for ref advertisement need to focus on\n> reducing the number of refs sent back and forth, not shrinking the\n> individual records down smaller.\n>\n>\n> Earlier in this thread Junio raised a point that the flush-pkt is\n> confusing because it has way too many purposes. I agree. IIRC we have\n> 0001-0003 as invalid pkt-line headers that could be used for special\n> markers in the stream.\n>\n> For example we could use 0000 flush-pkt as \"end of section\" and define\n> 0001 as \"i am done speaking and now wait for you to reply\".\n>\n> I think we need to keep 0004 as \"empty packet\" in case we ever get\n> ourselves into a position where it makes sense to pass an empty string\n> from one end of the connection to another.\n>\n>\n> Running pkt-line inside of side-band-64k is... weird to read. IIRC we\n> only do that in the smart HTTP protocol to the remote-curl helper, and\n> this is where Junio's remarks about flush-pkt being confusing come\n> from. If we rewrote the protocol to have that explicit 0001 \"i am done\n> talking\" marker the helper wouldn't need the double framing to\n> understand how to break up the originally bidirectional packet stream\n> into a sequence of bursts sent on unidirectional HTTP 1.x.\n\nSo I think my next steps are to look at grpc and the sideband channel\na bit more closely to have an opinion on my own.\n\nAt the beginning of the zear (Jan 8th) I started collecting data about\nthe refs advertisement. So I saved the output of `git ls-remote`\nfor\n https://github.com/gitster/git\n https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git\n https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git\n http://anongit.freedesktop.org/git/systemd/systemd.git\n\nIt's 57 days since I started the collection. For git.git the total size (of all\n57 outputs) combined is 7.0 MB.\n\nWhen compressing all into a zip file, we get down to 3.3 MB.\n(I figure the zip format is not the right way to measure entropy here)\n\nSo when using tar.xz instead, all of the outputs combined compress\nto 64.6 kB, while a single output compressed to 52.0 kB\n(uncompressed at 128kB, which makes sense as we have\nlots of redundancy with the common re-occurring pattern\n\"refs/heads/\" as well as \"refs/remotes/\". Also the tag lines\nshould not change over time, so we would not need to\nretransmit them if we know the client has it already.\n\nMaybe the transmission of the diff of the ls-remote would help us\nas well\n\n # the current size for each ls-remote in bytes:\n wc -c git_2015-03-03_01:41:01-08:00\n 127889 git_2015-03-03_01:41:01-08:00\n # The diff of one month:\n diff git_2015-03-03_01:41:01-08:00 git_2015-02-01_01\\:41\\:01-08\\:00 |wc -c\n 11257\n # Fetch after one week\n diff git_2015-03-03_01:41:01-08:00 git_2015-02-25_01:41:01-08:00 |wc -c\n 6308\n # just one day:\n diff git_2015-03-03_01:41:01-08:00 git_2015-03-01_01\\:41\\:02-08\\:00 |wc -c\n 1881\n\nIf anyone wants to experiment with the data I gathered, I can make them\navailable.\n\nThanks for all your different ideas!\nStefan\n"},{"id":"257082","messageId":"CAGZ79kaC2LPPDOjnn6_5Rjm=tpkP3R2NF9CpkUA98bAMxCZmEg@mail.gmail.com","threadId":"38638","inReplyTo":"CAGZ79ka0TFO6ZUd-vcxoDNAOyYPYdkLicmObTGhHb_hcDUm4-A@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-05T16:03:59Z","receivedAt":"2015-03-05T16:03:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 4, 2015 at 5:03 PM, Stefan Beller wrote:\n>\n> If anyone wants to experiment with the data I gathered, I can make them\n> available.\n>\n\nAll data of `ls-remote` including the gathering script is found at\n\n(112 kB .tar.xz)\nhttps://drive.google.com/file/d/0B7E93UKgFAfjcHRvM1N2YjBfTzA/view?usp=sharing\n(6.6MB in .zip)\nhttps://drive.google.com/file/d/0B7E93UKgFAfjRko3WHhtUWZtTEU/view?usp=sharing\n\nI also do have all the object files which are referenced in the\noutputs of ls-remote, though sharing\nthem is a bit tough as I cannot just git push them (forced pushes make\nsome of the objects\nunreachable, so my local gathering repo is explicitly configured to\nnot garbage collect), and these\nare huge compared to just the output of ls-remote.\n"},{"id":"257207","messageId":"1425685087-21633-1-git-send-email-sbeller@google.com","threadId":"38638","inReplyTo":"20150302092136.GA30278@lanh","subject":"[PATCH] protocol upload-pack-v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-06T23:38:07Z","receivedAt":"2015-03-06T23:38:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Duy Nguyen \n\nSigned-off-by: Stefan Beller \n---\n\nNotes:\n This is just aimed at untangling capabilities and refs\n advertisement, no new features.\n \n Hence this is missing the proposal from Duy to save one RTT.\n I have the impression that most of you are dreaming about new\n features which would solve the actual problem, but I'd first try\n to change the protocol which enables such game changers without\n adding new features at first. The optimisation to save a round trip\n or the optimisation how to save sending all the refs will come\n in a follow up series.\n \n XXX: this approach wastes one round trip in smart-http because the\n client would speak first. Perhaps we could allow client speculation.\n It can assume what server caps will send and send commands based on that\n assumption. If it turns out true, we save one round trip. E.g. fast\n path:\n C: You are supposed to send caps A, B. I would respond with cap B.\n Then I would send \"want-refs refs/heads/foo\".\n S: (yes we are sending caps A and B), validate client caps,\n execute \"want-refs\" and return ref list\n \n and slow path:\n \n C: You are supposed to send caps A, B. I would respond with cap B.\n Then I would send \"want-refs refs/heads/foo\".\n S: Send caps A, B and C. ignore the rest from client\n C: Want caps A and C. Send \"want-refs foo\"\n S: return ref foo\n\n Documentation/technical/pack-protocol.txt | 122 ++++++++++++++++++++--\n Documentation/technical/protocol-capabilities.txt | 22 ++--\n 2 files changed, 121 insertions(+), 23 deletions(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex 462e206..38bed2c 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -1,11 +1,11 @@\n Packfile transfer protocols\n ===========================\n \n-Git supports transferring data in packfiles over the ssh://, git:// and\n+Git supports transferring data in packfiles over the ssh://, git://, http:// and\n file:// transports. There exist two sets of protocols, one for pushing\n data from a client to a server and another for fetching data from a\n-server to a client. All three transports (ssh, git, file) use the same\n-protocol to transfer data.\n+server to a client. The three transports (ssh, git, file) use the same\n+protocol to transfer data. http is documented in http-protocol.txt.\n \n The processes invoked in the canonical Git implementation are 'upload-pack'\n on the server side and 'fetch-pack' on the client side for fetching data;\n@@ -14,6 +14,12 @@ data. The protocol functions to have a server tell a client what is\n currently on the server, then for the two to negotiate the smallest amount\n of data to send in order to fully update one or the other.\n \n+\"upload-pack-2\" and \"receive-pack-2\" are the next generation of\n+\"upload-pack\" and \"receive-pack\" respectively. The first two are\n+referred as \"version 2\" in this document and pack-capabilities.txt\n+while the last two are \"version 1\". Unless stated otherwise, \"version 1\"\n+is implied.\n+\n Transports\n ----------\n There are three transports over which the packfile protocol is\n@@ -42,7 +48,8 @@ hostname parameter, terminated by a NUL byte.\n \n --\n git-proto-request = request-command SP pathname NUL [ host-parameter NUL ]\n- request-command = \"git-upload-pack\" / \"git-receive-pack\" /\n+ request-command = \"git-upload-pack\" / \"git-upload-pack-2\" /\n+\t\t \"git-receive-pack\" / \"git-receive-pack-2\" /\n \t\t \"git-upload-archive\" ; case sensitive\n pathname = *( %x01-ff ) ; exclude NUL\n host-parameter = \"host=\" hostname [ \":\" port ]\n@@ -67,7 +74,6 @@ gracefully with an error message.\n error-line = PKT-LINE(\"ERR\" SP explanation-text)\n ----\n \n-\n SSH Transport\n -------------\n \n@@ -124,9 +130,56 @@ has, the first can 'fetch' from the second. This operation determines\n what data the server has that the client does not then streams that\n data down to the client in packfile format.\n \n+Capability discovery (v2)\n+-------------------------\n \n-Reference Discovery\n--------------------\n+In version 1, capability discovery is part of reference discovery and\n+covered in reference discovery section.\n+\n+In version 2, when the client initially connects, the server\n+immediately sends its capabilities to the client. Then the client must\n+send the list of server capabilities it wants to use to the server.\n+\n+ S: 00XXcapabilities 4\\n\n+ S: 00XXcap:lang\\n\n+ S: 00XXcap:thin-pack\\n\n+ S: 00XXcap:ofs-delta\\n\n+ S: 00XXagent:agent=git/2:3.4.5+custom-739-gb850f98\\n\n+\n+ C: 00XXcapabilities 3\n+ C: 00XXcap:thin-pack\\n\n+ C: 00XXcap:ofs-delta\\n\n+ C: 00XXcap:lang=en\\n\n+ C: 00XXagent:agent=git/custom_string\\n\n+\n+----\n+ cap = PKT-LINE(\"capabilities\" SP size LF list)\n+ size = *DIGIT\n+ capability-list = *(capability) [agent LF]\n+ capability = \"cap:\" keyvaluepair LF\n+ agent = keyvaluepair LF\n+ keyvaluepair = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n+ LC_ALPHA = %x61-7A\n+----\n+\n+The client MUST ignore any data before the pkt-line starting with \"capabilities\"\n+for future easy of extension.\n+\n+The server MUST advertise \"size\" as the decimal number of lines following\n+the \"capabilities\" line. This includes lines starting \"cap:\" and \"agent:\" for now.\n+The client MUST ignore lines which start with an unknown pattern.\n+\n+The client MUST NOT ask for capabilities the server did not say it\n+supports. The server MUST diagnose and abort if capabilities it does\n+not understand was requested. The server MUST NOT ignore capabilities\n+that client requested and server advertised. As a consequence of these\n+rules, server MUST NOT advertise capabilities it does not understand.\n+\n+See protocol-capabilities.txt for a list of allowed server and client\n+capabilities and descriptions.\n+\n+Reference Discovery (v1)\n+------------------------\n \n When the client initially connects the server will immediately respond\n with a listing of each reference it has (all branches and tags) along\n@@ -178,16 +231,69 @@ MUST peel the ref if it's an annotated tag.\n shallow = PKT-LINE(\"shallow\" SP obj-id)\n \n capability-list = capability *(SP capability)\n- capability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\")\n+ capability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n LC_ALPHA = %x61-7A\n ----\n \n Server and client MUST use lowercase for obj-id, both MUST treat obj-id\n as case-insensitive.\n \n+On the very first line of the initial server response of either\n+receive-pack and upload-pack the first reference is followed by\n+a NUL byte and then a list of space delimited server capabilities.\n+These allow the server to declare what it can and cannot support\n+to the client.\n+\n+Client will then send a space separated list of capabilities it wants\n+to be in effect. The client MUST NOT ask for capabilities the server\n+did not say it supports.\n+\n+Server MUST diagnose and abort if capabilities it does not understand\n+was sent. Server MUST NOT ignore capabilities that client requested\n+and server advertised. As a consequence of these rules, server MUST\n+NOT advertise capabilities it does not understand.\n+\n See protocol-capabilities.txt for a list of allowed server capabilities\n and descriptions.\n \n+Reference Discovery (v2)\n+------------------------\n+\n+In version 2, reference discovery is initiated by the client with\n+\"want-refs\" line. The client may skip reference discovery phase\n+entirely by not sending \"want-refs\" (e.g. the client has another way\n+to retrieve ref list).\n+\n+----\n+ want-refs = PKT-LINE(\"want-refs\" SP mode *argument)\n+ mode = \"all\"\n+ argument = SP arg\n+ arg = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n+----\n+\n+Mode \"all\" sends all visible refs to the client like in version 1. No\n+arguments are accepted in this mode. More modes may be available based\n+on capabilities.\n+\n+----\n+ advertised-refs = (no-refs / list-of-refs)\n+\t\t *shallow\n+\t\t flush-pkt\n+\n+ no-refs = PKT-LINE(zero-id LF)\n+\n+ list-of-refs = *other-ref\n+ other-ref = PKT-LINE(other-tip / other-peeled)\n+ other-tip = obj-id SP refname LF\n+ other-peeled = obj-id SP refname \"^{}\" LF\n+\n+ shallow = PKT-LINE(\"shallow\" SP obj-id)\n+\n+ capability-list = capability *(SP capability)\n+ capability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n+ LC_ALPHA = %x61-7A\n+----\n+\n Packfile Negotiation\n --------------------\n After reference and capabilities discovery, the client can decide to\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex 4f8a7bf..ecb0efd 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -3,21 +3,6 @@ Git Protocol Capabilities\n \n Servers SHOULD support all capabilities defined in this document.\n \n-On the very first line of the initial server response of either\n-receive-pack and upload-pack the first reference is followed by\n-a NUL byte and then a list of space delimited server capabilities.\n-These allow the server to declare what it can and cannot support\n-to the client.\n-\n-Client will then send a space separated list of capabilities it wants\n-to be in effect. The client MUST NOT ask for capabilities the server\n-did not say it supports.\n-\n-Server MUST diagnose and abort if capabilities it does not understand\n-was sent. Server MUST NOT ignore capabilities that client requested\n-and server advertised. As a consequence of these rules, server MUST\n-NOT advertise capabilities it does not understand.\n-\n The 'atomic', 'report-status', 'delete-refs', 'quiet', and 'push-cert'\n capabilities are sent and recognized by the receive-pack (push to server)\n process.\n@@ -268,3 +253,10 @@ to accept a signed push certificate, and asks the to be\n included in the push certificate. A send-pack client MUST NOT\n send a push-cert packet unless the receive-pack server advertises\n this capability.\n+\n+v2\n+--\n+\n+'git-upload-pack' and 'git-receive-pack' may advertise this capability\n+if the server supports 'git-upload-pack-2' and 'git-receive-pack-2'\n+respectively.\n-- \n2.3.0.81.gc37f363\n"},{"id":"257208","messageId":"CAGZ79kbXH+UZy2KoGyNdz9SkdRyxLRAUTeUK+-KsJzM+ZHTvCw@mail.gmail.com","threadId":"38638","inReplyTo":"1425685087-21633-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] protocol upload-pack-v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-06T23:40:48Z","receivedAt":"2015-03-06T23:40:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Mar 6, 2015 at 3:38 PM, Stefan Beller wrote:\n> From: Duy Nguyen \n\nOops. I edited the proposal from Duy heavily(?), such that it is\ndifferent from what he proposed 4 days ago.\n\nIn my impression this is what most of the participants would agree on.\n"},{"id":"257212","messageId":"CACsJy8ARDgjBLxYmfyrVcCb5WMnG=w6Ro_j+vVe=Sk6pEfR=Cw@mail.gmail.com","threadId":"38638","inReplyTo":"1425685087-21633-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] protocol upload-pack-v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-06T23:55:17Z","receivedAt":"2015-03-06T23:55:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"I'm still wondering if we should reserve more from the packet length.\nWe have used length 0000 for pkt-flush. Shawn pointed out that we\nstill have 0001, 0002 and 0003 but we may use some of them to avoid\nabuse of pkt-flush in some cases. Perhaps we could limit packet length\nto 0xfff0, so we have 0xfff1-0xffff to assign special meanings in\nfuture, if we have to.\n\nOn Sat, Mar 7, 2015 at 6:38 AM, Stefan Beller wrote:\n> +In version 2, when the client initially connects, the server\n> +immediately sends its capabilities to the client. Then the client must\n> +send the list of server capabilities it wants to use to the server.\n> +\n> + S: 00XXcapabilities 4\\n\n> + S: 00XXcap:lang\\n\n> + S: 00XXcap:thin-pack\\n\n> + S: 00XXcap:ofs-delta\\n\n> + S: 00XXagent:agent=git/2:3.4.5+custom-739-gb850f98\\n\n> +\n> + C: 00XXcapabilities 3\n> + C: 00XXcap:thin-pack\\n\n> + C: 00XXcap:ofs-delta\\n\n> + C: 00XXcap:lang=en\\n\n> + C: 00XXagent:agent=git/custom_string\\n\n> +\n> +----\n> + cap = PKT-LINE(\"capabilities\" SP size LF list)\n> + size = *DIGIT\n> + capability-list = *(capability) [agent LF]\n> + capability = \"cap:\" keyvaluepair LF\n> + agent = keyvaluepair LF\n> + keyvaluepair = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n\nIf we send one cap per pkt-line, cap can contain spaces. The question\nis, should we allow them?\n\nEnding cap lines with LF seems redudant because we already know the line length.\n\n> + LC_ALPHA = %x61-7A\n> +----\n> +\n> +The client MUST ignore any data before the pkt-line starting with \"capabilities\"\n> +for future easy of extension.\n> +\n> +The server MUST advertise \"size\" as the decimal number of lines following\n> +the \"capabilities\" line. This includes lines starting \"cap:\" and \"agent:\" for now.\n> +The client MUST ignore lines which start with an unknown pattern.\n\nI think the common pattern in our protocol is to end these with a\npkt-flush, instead of send the number of items upfront. If we do that\nwe don't have to specify \"cap:\" or \"agent:\" either. All pkt-lines\nuntil pkt-flush at the beginning of v2 are cap lines. And agent is\njust another \"capability\".\n-- \nDuy\n"},{"id":"257213","messageId":"CACsJy8B5jdKt0h8pTn8hVrbEw3o-gRLmoP+MBMMpm-reKtBOvQ@mail.gmail.com","threadId":"38638","inReplyTo":"1425685087-21633-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] protocol upload-pack-v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-07T00:00:14Z","receivedAt":"2015-03-07T00:00:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 7, 2015 at 6:38 AM, Stefan Beller wrote:\n> +Reference Discovery (v2)\n> +------------------------\n> +\n> +In version 2, reference discovery is initiated by the client with\n> +\"want-refs\" line. The client may skip reference discovery phase\n> +entirely by not sending \"want-refs\" (e.g. the client has another way\n> +to retrieve ref list).\n> +\n> +----\n> + want-refs = PKT-LINE(\"want-refs\" SP mode *argument)\n> + mode = \"all\"\n> + argument = SP arg\n> + arg = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n> +----\n\nOn the same line with capabilities, we would not want to run into a\nsituation where we have too many arguments to send and exceed pkt-line\nlimit. So perhaps do one argument per pkt-line too, ending with a\npkt-flush.\n-- \nDuy\n"},{"id":"257215","messageId":"xmqqr3t1vefz.fsf@gitster.dls.corp.google.com","threadId":"38638","inReplyTo":"1425685087-21633-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] protocol upload-pack-v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-07T00:28:16Z","receivedAt":"2015-03-07T00:28:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> @@ -67,7 +74,6 @@ gracefully with an error message.\n> error-line = PKT-LINE(\"ERR\" SP explanation-text)\n> ----\n> \n> -\n> SSH Transport\n\nNoise?\n\n> @@ -124,9 +130,56 @@ has, the first can 'fetch' from the second. This operation determines\n> what data the server has that the client does not then streams that\n> data down to the client in packfile format.\n> \n> +Capability discovery (v2)\n> +-------------------------\n> \n> +In version 1, capability discovery is part of reference discovery and\n> +covered in reference discovery section.\n> +\n> +In version 2, when the client initially connects, the server\n> +immediately sends its capabilities to the client. Then the client must\n> +send the list of server capabilities it wants to use to the server.\n> +\n> + S: 00XXcapabilities 4\\n\n> + S: 00XXcap:lang\\n\n> + S: 00XXcap:thin-pack\\n\n> + S: 00XXcap:ofs-delta\\n\n> + S: 00XXagent:agent=git/2:3.4.5+custom-739-gb850f98\\n\n> +\n> + C: 00XXcapabilities 3\n> + C: 00XXcap:thin-pack\\n\n> + C: 00XXcap:ofs-delta\\n\n> + C: 00XXcap:lang=en\\n\n> + C: 00XXagent:agent=git/custom_string\\n\n\nI do not see a good reason why we want \"I am sending N caps\"\nupfront, instead of \"this, that, and here is the end of the group\".\nIf you expect the recipient to benefit by being able to pre-allocate\nN slots, then that might make sense, but otherwise, I'd rather see\nus stick to a (weaker) flush that says \"group ends here\".\n\nBesides, I do not know how you counted 4 on the S: side and 3 on\nthe C: side in the above example and missing LF after 3 ;-).\n\n> +----\n> + cap = PKT-LINE(\"capabilities\" SP size LF list)\n\nIsn't a cap packet terminated by LF without any \"list\" following it?\nThe notation PKT-LINE() is \"wrap in a single packet\",\nand I do not think you meant to send the capability line and a series\nof cap:foo entries in a single packet.\n\n> + size = *DIGIT\n> + capability-list = *(capability) [agent LF]\n> + capability = \"cap:\" keyvaluepair LF\n> + agent = keyvaluepair LF\n\nI do not see a reason to make 'agent' as part of capability. That\nwas an implementation detail of v1 but v2 does not have an\nobligation to consider agent announcement as capability.\n\nserver-announcement = PKT-LINE(\"capabilities\" ...) capability-list [agent-announcement]\ncapability-list = as you have it w/o \"[agent LF]\"\nagent-announcement = PKT-LINE(\"agent=\" agent-token LF)\n\nor something, perhaps?\n\n> +The client MUST ignore any data before the pkt-line starting with \"capabilities\"\n> +for future easy of extension.\n\ns/easy/ease/; but I am not sure if this makes sense. Without\nknowing the extended preamble, you cannot even tell if a packet line\nthat happens to start with \"capabilities\" is a proper beginning of\n0-th iteration of v2 protocol, or an embedded data in the preamble,\nno?\n\n> +Reference Discovery (v2)\n> +------------------------\n> +\n> +In version 2, reference discovery is initiated by the client with\n> +\"want-refs\" line. The client may skip reference discovery phase\n> +entirely by not sending \"want-refs\" (e.g. the client has another way\n> +to retrieve ref list).\n> +\n> +----\n> + want-refs = PKT-LINE(\"want-refs\" SP mode *argument)\n> + mode = \"all\"\n> + argument = SP arg\n> + arg = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n> +----\n> +\n> +Mode \"all\" sends all visible refs to the client like in version 1. No\n> +arguments are accepted in this mode. More modes may be available based\n> +on capabilities.\n\nI tend to agree with Duy that the protocol must anticipate that args\ncan become longer.\n\n> +----\n> + advertised-refs = (no-refs / list-of-refs)\n> +\t\t *shallow\n> +\t\t flush-pkt\n\nI am not sure if defining \"shallow\" as part of \"refs advertisement\"\nis a good idea. The latter lives in the same place in the v1\nprotocol merely because that was how it was later bolted onto the\nprotocol. But this concern can easily be allayed by retitling\n\"advertised-refs\" to something else.\n"},{"id":"257223","messageId":"CAGZ79kZBYFSwR6E86BF6Dt7xdh0zs07tkGnQAKfEJpSduTK-aw@mail.gmail.com","threadId":"38638","inReplyTo":"xmqqr3t1vefz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] protocol upload-pack-v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-07T04:28:17Z","receivedAt":"2015-03-07T04:28:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Mar 6, 2015 at 4:28 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> @@ -67,7 +74,6 @@ gracefully with an error message.\n>> error-line = PKT-LINE(\"ERR\" SP explanation-text)\n>> ----\n>>\n>> -\n>> SSH Transport\n>\n> Noise?\n>\n>> @@ -124,9 +130,56 @@ has, the first can 'fetch' from the second. This operation determines\n>> what data the server has that the client does not then streams that\n>> data down to the client in packfile format.\n>>\n>> +Capability discovery (v2)\n>> +-------------------------\n>>\n>> +In version 1, capability discovery is part of reference discovery and\n>> +covered in reference discovery section.\n>> +\n>> +In version 2, when the client initially connects, the server\n>> +immediately sends its capabilities to the client. Then the client must\n>> +send the list of server capabilities it wants to use to the server.\n>> +\n>> + S: 00XXcapabilities 4\\n\n>> + S: 00XXcap:lang\\n\n>> + S: 00XXcap:thin-pack\\n\n>> + S: 00XXcap:ofs-delta\\n\n>> + S: 00XXagent:agent=git/2:3.4.5+custom-739-gb850f98\\n\n>> +\n>> + C: 00XXcapabilities 3\n>> + C: 00XXcap:thin-pack\\n\n>> + C: 00XXcap:ofs-delta\\n\n>> + C: 00XXcap:lang=en\\n\n>> + C: 00XXagent:agent=git/custom_string\\n\n>\n> I do not see a good reason why we want \"I am sending N caps\"\n> upfront, instead of \"this, that, and here is the end of the group\".\n\nI thought about having an end marker, so something like\ncapabilities start\nthin-pack\nlang\nofs-delta\ncapabilities done\n\n(Each line a pkt-line)\n\nThough all decisions I thought through I tried to put more weight on\nfuture expandability. Now that I think about it again, it makes no\ndifference, whether to use a counter or an end marker.\n\n> If you expect the recipient to benefit by being able to pre-allocate\n> N slots, then that might make sense, but otherwise, I'd rather see\n> us stick to a (weaker) flush that says \"group ends here\".\n\nI think it's not about pre allocating but counting down. Then you know\nat the beginning how much to expect which might become relevant if\nthat section grows large again. (\"The server really wants to send 1500\ncapability lines? Nope I'll just disconnect because I am on mobile!\")\n\nImplementation wise an end marker is easier though (you don't need\nto count down, so it feels more stateless to me).\n\n>\n> Besides, I do not know how you counted 4 on the S: side and 3 on\n> the C: side in the above example and missing LF after 3 ;-).\n>\n\nSorry about that, I added one answer late and forgot to increment the 3.\n\n>> +----\n>> + cap = PKT-LINE(\"capabilities\" SP size LF list)\n>\n> Isn't a cap packet terminated by LF without any \"list\" following it?\n> The notation PKT-LINE() is \"wrap in a single packet\",\n> and I do not think you meant to send the capability line and a series\n> of cap:foo entries in a single packet.\n\nYeah I meant to use one packet per line\nSo after considering your input, you'd want to have\nPKT-LINE(\"capabilities start\")\nPKT-LINE(\"no-prefix-for-capabilities\")\nPKT-LINE(\"ofs-delta\")\nPKT-LINE(\"agent-as-capability-2.6\")\nPKT-LINE(\"capabilities end\")\n\nAnd additionally to that a PKT-LINE should have the ability to grow larger than\n0xffff, which would be encoded with 0xffff being an escape character\nindicating the\nlength is encoded somehow different. (Maybe take the next 8 bytes\ninstead of just 4).\n\n\n>\n>> + size = *DIGIT\n>> + capability-list = *(capability) [agent LF]\n>> + capability = \"cap:\" keyvaluepair LF\n>> + agent = keyvaluepair LF\n>\n> I do not see a reason to make 'agent' as part of capability. That\n> was an implementation detail of v1 but v2 does not have an\n> obligation to consider agent announcement as capability.\n\nSo I think we don't want to drop the agent announcement as it may\nreveal useful information (\"How many outdated clients connect to our\n$HOSTING_SITE?\", \"I need to debug failures which happen only rarely,\nOh! it can be narrowed down to clients with agent xyz.\")\n\nSo then we need to decide where to put the agent. And as it is only small\nbut useful (meta?)-information I'd rather put it at the beginning of the\ndata exchange, so in case the other side seems to be missbehaving,\nit is easier to debug in the hope the agent transmission was still\nsuccessful.\n\n>\n> server-announcement = PKT-LINE(\"capabilities\" ...) capability-list [agent-announcement]\n> capability-list = as you have it w/o \"[agent LF]\"\n> agent-announcement = PKT-LINE(\"agent=\" agent-token LF)\n>\n> or something, perhaps?\n\nThis looks like me as if all capabilities are in one PKT-LINE, which\nyou seemed to oppose?\n\n>\n>> +The client MUST ignore any data before the pkt-line starting with \"capabilities\"\n>> +for future easy of extension.\n>\n> s/easy/ease/; but I am not sure if this makes sense. Without\n> knowing the extended preamble, you cannot even tell if a packet line\n> that happens to start with \"capabilities\" is a proper beginning of\n> 0-th iteration of v2 protocol, or an embedded data in the preamble,\n> no?\n\nI rather thought about the case where the implementation would\njust close the connection on sight of unknown preamble.\nIf we want to extend the protocol again and the string\n\"capabilites\" should be part before the actual capabilities start,\nwe will think about escaping it in the future as then we can still\ntalk to clients as of this design.\n\nIn case we'd close the connection we would have a similar problem as\nof now, it cannot be really extended.\n\n>\n>> +Reference Discovery (v2)\n>> +------------------------\n>> +\n>> +In version 2, reference discovery is initiated by the client with\n>> +\"want-refs\" line. The client may skip reference discovery phase\n>> +entirely by not sending \"want-refs\" (e.g. the client has another way\n>> +to retrieve ref list).\n>> +\n>> +----\n>> + want-refs = PKT-LINE(\"want-refs\" SP mode *argument)\n>> + mode = \"all\"\n>> + argument = SP arg\n>> + arg = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n>> +----\n>> +\n>> +Mode \"all\" sends all visible refs to the client like in version 1. No\n>> +arguments are accepted in this mode. More modes may be available based\n>> +on capabilities.\n>\n> I tend to agree with Duy that the protocol must anticipate that args\n> can become longer.\n\nok, so PKT-LINE needs to be able to deal with larger lines, I'll add that.\n\n>\n>> +----\n>> + advertised-refs = (no-refs / list-of-refs)\n>> + *shallow\n>> + flush-pkt\n>\n> I am not sure if defining \"shallow\" as part of \"refs advertisement\"\n> is a good idea. The latter lives in the same place in the v1\n> protocol merely because that was how it was later bolted onto the\n> protocol. But this concern can easily be allayed by retitling\n> \"advertised-refs\" to something else.\n\nI don't quite understand this. What are your concerns about shallow here?\n\nThanks on the feedback!\nStefan\n"},{"id":"257225","messageId":"CACsJy8DX6pg7XthfsdtKXstPAiT67C2Hf=vqJZz-199+fZko1w@mail.gmail.com","threadId":"38638","inReplyTo":"CAGZ79kZBYFSwR6E86BF6Dt7xdh0zs07tkGnQAKfEJpSduTK-aw@mail.gmail.com","subject":"Re: [PATCH] protocol upload-pack-v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-07T05:21:00Z","receivedAt":"2015-03-07T05:21:00Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 7, 2015 at 11:28 AM, Stefan Beller wrote:\n>>> +----\n>>> + advertised-refs = (no-refs / list-of-refs)\n>>> + *shallow\n>>> + flush-pkt\n>>\n>> I am not sure if defining \"shallow\" as part of \"refs advertisement\"\n>> is a good idea. The latter lives in the same place in the v1\n>> protocol merely because that was how it was later bolted onto the\n>> protocol. But this concern can easily be allayed by retitling\n>> \"advertised-refs\" to something else.\n>\n> I don't quite understand this. What are your concerns about shallow here?\n\nThis made me look for explanation about these shallow lines (commit\nad49136).They are sent when source repo is a shallow one. They tell\nthe receiver about the bottom the source repo. My experiment with \"git\nclone --since\" shows that such a shallow clone could end up with a few\nthousand shallow lines. Not as many as refs, but sending shallow lines\nthis way still cost more than necessary. If we want to do that, we\nneed more flexibility that just sending all shallow lines this way.\n\nOne of the approach is, because these shallow lines (on source repo)\nrarely change. We could simply exchange SHA-1 of the source repo's\n\"shallow\" file first. The client only requests shallow info when SHA-1\ndoes not match.\n-- \nDuy\n"},{"id":"257343","messageId":"xmqqr3szql9r.fsf@gitster.dls.corp.google.com","threadId":"38638","inReplyTo":"CAGZ79kZBYFSwR6E86BF6Dt7xdh0zs07tkGnQAKfEJpSduTK-aw@mail.gmail.com","subject":"Re: [PATCH] protocol upload-pack-v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-08T20:36:32Z","receivedAt":"2015-03-08T20:36:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> I do not see a good reason why we want \"I am sending N caps\"\n>> upfront, instead of \"this, that, and here is the end of the group\".\n>\n> I thought about having an end marker, so something like\n> capabilities start\n> thin-pack\n> lang\n> ofs-delta\n> capabilities done\n>\n> (Each line a pkt-line)\n>\n> Though all decisions I thought through I tried to put more weight on\n> future expandability. Now that I think about it again, it makes no\n> difference, whether to use a counter or an end marker.\n\nOne reason why I would suggest avoiding \"count upfront\" is to make\nsure we do not repeat the mistake of \"Content-Length\" which had to\nlater be corrected by introducing chunked transfer by HTTP folks.\nCloser to home, our \"type and then size upfront and then contents\nand hash the whole thing\" loose object format makes it quite hard\nto produce without having the whole thing in-core, or otherwise\nhaving a separate way to know the size upfront.\n\nFor the capability list, the number of the capabilities you support\nmay be limited, bounded and may even be known at the compile time,\nso count-upfront may not be a burden, but in other parts of the\nprotocol where you need to feed the result of computation to the\nother end, you would need \"the group ends here\" marker. It would\nbe easier for everybody if we can make all types of messages use\nthe same syntax, regardless of type.\n\n>>> + cap = PKT-LINE(\"capabilities\" SP size LF list)\n>>\n>> Isn't a cap packet terminated by LF without any \"list\" following it?\n>> The notation PKT-LINE() is \"wrap in a single packet\",\n>> and I do not think you meant to send the capability line and a series\n>> of cap:foo entries in a single packet.\n>\n> Yeah I meant to use one packet per line\n> So after considering your input, you'd want to have\n> PKT-LINE(\"capabilities start\")\n> PKT-LINE(\"no-prefix-for-capabilities\")\n> PKT-LINE(\"ofs-delta\")\n> PKT-LINE(\"agent-as-capability-2.6\")\n> PKT-LINE(\"capabilities end\")\n\nOK, so that \"list\" at the end is just a typo; there shouldn't be\n\"list at the end inside PKT-LINE().\n\n>>> + size = *DIGIT\n>>> + capability-list = *(capability) [agent LF]\n>>> + capability = \"cap:\" keyvaluepair LF\n>>> + agent = keyvaluepair LF\n>>\n>> I do not see a reason to make 'agent' as part of capability. That\n>> was an implementation detail of v1 but v2 does not have an\n>> obligation to consider agent announcement as capability.\n>\n> So I think we don't want to drop the agent announcement as it may\n> reveal useful information (\"How many outdated clients connect to our\n> $HOSTING_SITE?\", \"I need to debug failures which happen only rarely,\n> Oh! it can be narrowed down to clients with agent xyz.\")\n\nDon't be overly defensive and try not to misunderstand and see a\ncriticism where there is none. All I am saying is that agent\nannouncement is not annoucing capability. You may announce many\nthings, and server or client version may be something you would want\nto announce.\n\nI have a feeling that it is a bit too premature to specify the\ndetails at such a low level as \"capaiblities are announced by\nprefixing four-byte 'c', 'a', 'p', ':' in front\" and \"a multi-record\ngroup has its element count at the beginning (or an end marker at\nthe end, for that matter)\", and it may be a better idea to outline\nall the necessary elements at a bit higher level first---that would\navoid needs for useless exchanges like what we are having right now.\n\nIt's that when you write things in EBNF, you are writing something\nthat you would eventually want to cast in stone, and the\nnon-terminal names in EBNF matter (they convey the semantics, what\nthese named things are), and I was reacting to that because I wanted\nto make sure we avoid mislabaling things as something that are not.\n\nThe \"shallow\" vs \"reference advertisement\" is the same. I think the\nformer is _not_ part of reference announcement but is an optional\nphase of the protocol, but the level of the detail that would make\nthe difference matter would appear only when you start writing it in\nEBNF and call both \"reference advertisement\". If you keep the\ndiscussion at the level like \"fetch first asks capabilities it wants\nupload-pack-2 to enable, optionally gives the current shallow\nboundaries when the capaibilty says the other side supports it, and\nthen starts showing what it has\" while we are trying to achieve\nconcensus on what kind of protocol elements we would need, and what\ninformation each element would carry, the discussion will help us\nreach a shared understanding on what to write down in EBNF form\nexactly faster, I would imagine.\n"},{"id":"257438","messageId":"CACsJy8AuCAfHMbnV3N4ZyZCCqF6WAs_k2dS_Y1ZccWAzWm0yVA@mail.gmail.com","threadId":"38638","inReplyTo":"1425685087-21633-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] protocol upload-pack-v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-10T01:38:39Z","receivedAt":"2015-03-10T01:38:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"A minor point on capability negotiation. I remember why I passed\ncapabilities via command line instead of this proposal. With this\nproposal, daemon.c does not recognize \"i18n\" capability and cannot\nswitch to the correct language before it reports an error.\n\nBut perhaps I approached it the wrong way. Instead of letting the\ndaemon produce non-English language directly, if could sort of\nstandardize the messages and send an error code back in \"ERR\" line,\ntogether with an English message (current pack-protocol.txt says \"ERR\"\nfollowed by explanation-text). The client can use this error code to\nprint non-English messages if it wants to. Perhaps we can reuse http\n(or ftp) return codes or some other protocol then customize to fit\nGit's needs.\n-- \nDuy\n"},{"id":"257491","messageId":"96E9559D-9B15-464D-AC9E-8C17229A0A78@gmail.com","threadId":"38638","inReplyTo":"CACsJy8AuCAfHMbnV3N4ZyZCCqF6WAs_k2dS_Y1ZccWAzWm0yVA@mail.gmail.com","subject":"Re: [PATCH] protocol upload-pack-v2","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-03-10T19:36:40Z","receivedAt":"2015-03-10T19:36:40Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Mar 9, 2015, at 18:38, Duy Nguyen wrote:\n> A minor point on capability negotiation. I remember why I passed\n> capabilities via command line instead of this proposal. With this\n> proposal, daemon.c does not recognize \"i18n\" capability and cannot\n> switch to the correct language before it reports an error.\n>\n> But perhaps I approached it the wrong way. Instead of letting the\n> daemon produce non-English language directly, if could sort of\n> standardize the messages and send an error code back in \"ERR\" line,\n> together with an English message (current pack-protocol.txt says \"ERR\"\n> followed by explanation-text). The client can use this error code to\n> print non-English messages if it wants to. Perhaps we can reuse http\n> (or ftp) return codes or some other protocol then customize to fit\n> Git's needs.\n\nMay I suggest that you take a look at RFC 2034 [1]. It describes \nalmost exactly this same situation and how SMTP was enhanced to \nsupport error code numbers that could then be translated.\n\nNo reason this enhancement needs to be restricted to protocol v2 if \nit's just an \"enhancedstatuscodes\" capability the server sends back to \nindicate that its ERR text is in that format.\n\n-Kyle\n\n[1] http://tools.ietf.org/html/rfc2034\n"},{"id":"258432","messageId":"CAGZ79kZ22Fo5xGrk3x5+hV6WdbUg0A0h1fXgPBukenOxqyxVZw@mail.gmail.com","threadId":"38638","inReplyTo":"xmqqk2yy80mq.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-24T17:42:12Z","receivedAt":"2015-03-24T17:42:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 3, 2015 at 9:13 AM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n>\n>> Junio pointed out in private that I didn't address the packet length\n>> limit (64k). I thought I could get away with a new capability\n>> (i.e. not worry about it now) but I finally admit that was a bad\n>> hack. So perhaps this on top.\n>\n> No, I didn't ;-) but I tend to agree that \"perhaps 4GB huge packet?\"\n> is a bad idea.\n>\n> The problem I had with the version in your write-up was that it\n> still assumed that all capabilities must come on one packet-line.\n>\n\nSo I started looking into extending the buffer size as another 'first step'\ntowards the protocol version 2 again. But now I think the packed length\nlimit of 64k is actually a good and useful thing to have and should be\nextended/fixed if and only if we run into serious trouble with too small\npackets later.\n\nI mean we can add the possibility now by introducing these\nspecial length 0xFFFF or 0xFFFE to mean we'd want to extend it in the\nfuture. But when doing this we need to be extra careful with buffer allocation.\nAs it is easy to produce a denial of service attack if the receiving side\nblindly trusts the length and allocates as much memory. So having a 64k\nlimit actually helps preventing this attack a bit as it is a very small number.\n"},{"id":"258438","messageId":"xmqq619q6zsx.fsf@gitster.dls.corp.google.com","threadId":"38638","inReplyTo":"CAGZ79kZ22Fo5xGrk3x5+hV6WdbUg0A0h1fXgPBukenOxqyxVZw@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-24T18:00:30Z","receivedAt":"2015-03-24T18:00:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> So I started looking into extending the buffer size as another 'first step'\n> towards the protocol version 2 again. But now I think the packed length\n> limit of 64k is actually a good and useful thing to have and should be\n> extended/fixed if and only if we run into serious trouble with too small\n> packets later.\n\nI tend to agree. Too large a packet size would mean your latency\nwould also suck, as pkt-line interface will not give you anything\nuntil you read the entire packet. The new protocol should be\ndesigned around a reasonably sized packets, using multiple packets\nto carry larger payload as necessary.\n"},{"id":"258769","messageId":"xmqqd23pq6r3.fsf@gitster.dls.corp.google.com","threadId":"38638","inReplyTo":"xmqqr3szql9r.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] protocol upload-pack-v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-31T19:58:08Z","receivedAt":"2015-03-31T19:58:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I have a feeling that it is a bit too premature to specify the\n> details at such a low level as \"capaiblities are announced by\n> prefixing four-byte 'c', 'a', 'p', ':' in front\" and \"a multi-record\n> group has its element count at the beginning (or an end marker at\n> the end, for that matter)\", and it may be a better idea to outline\n> all the necessary elements at a bit higher level first---that would\n> avoid needs for useless exchanges like what we are having right now.\n>\n> .... If you keep the\n> discussion at the level like \"fetch first asks capabilities it wants\n> upload-pack-2 to enable, optionally gives the current shallow\n> boundaries when the capaibilty says the other side supports it, and\n> then starts showing what it has\" while we are trying to achieve\n> concensus on what kind of protocol elements we would need, and what\n> information each element would carry, the discussion will help us\n> reach a shared understanding on what to write down in EBNF form\n> exactly faster, I would imagine.\n\nAnd I see we went silent after this, so let's try to stir the pot\nagain to see if it simmers.\n\nThis is a follow-up on $gmane/264553, which is a continuation of\n$gmane/264000, but instead of giving two required readings to\nreaders, I'll start with reproduction of the two, and add a few more\nthings the current protocol lacks that I would want to see in the\nupdated protocol.\n\n\n\nThe current protocol has the following problems that limit us:\n\n - It is not easy to make it resumable, because we recompute every\n time. This is especially problematic for the initial fetch aka\n \"clone\" as we will be talking about a large transfer. Redirection\n to a bundle hosted on CDN might be something we could do\n transparently.\n\n - The protocol extension has a fairly low length limit.\n\n - Because the protocol exchange starts by the server side\n advertising all its refs, even when the fetcher is interested in\n a single ref, the initial overhead is nontrivial, especially when\n you are doing a small incremental update. The worst case is an\n auto-builder that polls every five minutes, even when there is no\n new commits to be fetched.\n\n - Because we recompute every time, taking into account of what the\n fetcher has, in addition to what the fetcher obtained earlier\n from us in order to reduce the transferred bytes, the payload for\n incremental updates become tailor-made for each fetch and cannot\n be easily reused.\n\n - The semantics of the side-bands are unclear.\n\n - Is band #2 meant only for progress output (I think the current\n protocol handlers assume that and unconditionally squelch it\n under --quiet)? Do we rather want a dedicated \"progress\" and\n \"error message\" sidebands instead?\n\n - Is band #2 meant for human consumption, or do we expect the\n other end to interpret and act on it? If the former, would it\n make sense to send locale information from the client side and\n ask the server side to produce its output with _(\"message\")?\n\n - The semantics of packet_flush() is suboptimal, and this\n shortcoming seeps through to the protocol mapped to the\n smart-HTTP transport.\n\n Originally, packet_flush() was meant as \"Here is an end of one\n logical section of what I am going to speak.\", hinting that it\n might be a good idea for the underlying implementation to hold\n the packets up to that point in-core and then write(2) them all\n out (i.e. \"flush\") to the file descriptor only when we handle\n packet_flush(). It never meant \"Now I am finished speaking for\n now and it is your turn to speak.\"\n\n But because HTTP is inherently a ping-pong protocol where the\n requestor at one point stops talking and lets the responder\n speak, the code to map our protocol to the smart HTTP transport\n made the packet_flush() boundary as \"Now I am done talking, it is\n my turn to listen.\"\n\n We probably need two kinds of packet_flush(). When a requestor\n needs to say two or more logical groups of things before telling\n the other side \"Now I am done talking; it is your turn.\", we need\n some marker (i.e. the original meaning of packet_flush()) at the\n end of these logical groups. And in order to be able to say \"Now\n I am done saying everything I need to say at this point for you\n to respond to me. It is your turn.\", we need another kind of\n marker.\n\n - The fetch-pack direction does the common-parent discovery but the\n push-pack direction does not. This is OK for the normal\n fast-forward push, in which case we will see a known commit on\n the tip of the branch we are pushing into, but makes forced push\n inefficient.\n\n - The existing common-parent discovery done on the fetch-pack side\n enumerates commits contiguously traversing the history to the\n past. We might want to go exponential or Fibonacci to quickly\n find an ancient common commit and bisect the history from there\n (or it might turn out not to be worth it).\n\n - We may want to revamp the builtin/receive-pack.c::report() that\n reports the final result of a push back to the pusher to tell the\n exact object name that sits at the updated tips of refs, not just\n refnames. It will allow the server side to accept a push of\n commit X to a branch, do some \"magic\" on X (e.g. rebase it on top\n of the current tip, merge it with the current tip, or let a hook\n to rewrite the commit in any way it deems appropriate) and put\n the resulting commit Y at the tip of the branch. Without such a\n revamp, it is currently not possible to sensibly allow the server\n side to rewrite what got pushed.\n\n - If we were to start allowing the receiver side to rewrite pushed\n commits, the updated send-pack protocol must be able to send the\n new objects created by that \"magic\" back to the pusher. The\n current protocol does not allow the receive-pack to send packdata\n back to send-pack.\n\nI'd like to see a new protocol that lets us overcome the above\nlimitations (did I miss others? I am sure people can help here)\nsometime this year.\n"},{"id":"258856","messageId":"CACsJy8Cj97NrvuAXjqRbd52ivaHZ_oqy+k-3_LeRSe2Jy6U33Q@mail.gmail.com","threadId":"38638","inReplyTo":"xmqqd23pq6r3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] protocol upload-pack-v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-04-02T12:37:20Z","receivedAt":"2015-04-02T12:37:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Apr 1, 2015 at 2:58 AM, Junio C Hamano wrote:\n> This is a follow-up on $gmane/264553, which is a continuation of\n> $gmane/264000, but instead of giving two required readings to\n> readers, I'll start with reproduction of the two, and add a few more\n> things the current protocol lacks that I would want to see in the\n> updated protocol.\n\nI think the important thing to get v2 started is making sure we do not\nneed v3 to get rid of any of these limitations. In other words v2\nshould be extensible enough to implement them later. I'm looking from\nthis perspective.\n\n> The current protocol has the following problems that limit us:\n>\n> - It is not easy to make it resumable, because we recompute every\n> time. This is especially problematic for the initial fetch aka\n> \"clone\" as we will be talking about a large transfer. Redirection\n> to a bundle hosted on CDN might be something we could do\n> transparently.\n\nSending multiple packs or some redirection instructions could be done\neven with v1. The only recompute part that is unavoidable in v1 is ref\nadvertisement, which I think is solved.\n\n> - The protocol extension has a fairly low length limit.\n\nOne pkt-line per protocol extension should do it.\n\n> - Because the protocol exchange starts by the server side\n> advertising all its refs, even when the fetcher is interested in\n> a single ref, the initial overhead is nontrivial, especially when\n> you are doing a small incremental update. The worst case is an\n> auto-builder that polls every five minutes, even when there is no\n> new commits to be fetched.\n\nOne of the reason v2 is started, should be ok with current v2 design.\n\n> - Because we recompute every time, taking into account of what the\n> fetcher has, in addition to what the fetcher obtained earlier\n> from us in order to reduce the transferred bytes, the payload for\n> incremental updates become tailor-made for each fetch and cannot\n> be easily reused.\n\nWell, we reuse at a lower level, pack-objects would try to copy\nexisting deltas instead of making new ones. We can cache new deltas in\nhope that they may be useful for the next fetch. But that has nothing\nto do with the protocol..\n\n> - The semantics of the side-bands are unclear.\n>\n> - Is band #2 meant only for progress output (I think the current\n> protocol handlers assume that and unconditionally squelch it\n> under --quiet)? Do we rather want a dedicated \"progress\" and\n> \"error message\" sidebands instead?\n>\n> - Is band #2 meant for human consumption, or do we expect the\n> other end to interpret and act on it? If the former, would it\n> make sense to send locale information from the client side and\n> ask the server side to produce its output with _(\"message\")?\n\nThe interpretation of side-band could be changed by introducing a new\nextension, couldn't it?\n\n> - The semantics of packet_flush() is suboptimal, and this\n> shortcoming seeps through to the protocol mapped to the\n> smart-HTTP transport.\n>\n> ...\n\nI don't have an answer to this one. So the reaction is, if it is not\n\"broken\" (in pratice, not in theory), don't touch it. I know I'm\nburying my head in the sand..\n\n> - The fetch-pack direction does the common-parent discovery but the\n> push-pack direction does not. This is OK for the normal\n> fast-forward push, in which case we will see a known commit on\n> the tip of the branch we are pushing into, but makes forced push\n> inefficient.\n\nIntroducing the ref exchange in push-pack could be done in an\nextension too, I think.\n\n> - The existing common-parent discovery done on the fetch-pack side\n> enumerates commits contiguously traversing the history to the\n> past. We might want to go exponential or Fibonacci to quickly\n> find an ancient common commit and bisect the history from there\n> (or it might turn out not to be worth it).\n\nHm.. i'm wondering if we can already do this with v1 if we have enough\nman power.\n\n> - We may want to revamp the builtin/receive-pack.c::report() that\n> reports the final result of a push back to the pusher to tell the\n> exact object name that sits at the updated tips of refs, not just\n> refnames. It will allow the server side to accept a push of\n> commit X to a branch, do some \"magic\" on X (e.g. rebase it on top\n> of the current tip, merge it with the current tip, or let a hook\n> to rewrite the commit in any way it deems appropriate) and put\n> the resulting commit Y at the tip of the branch. Without such a\n> revamp, it is currently not possible to sensibly allow the server\n> side to rewrite what got pushed.\n\nSounds more coding than changing the protocol, which should be\npossible with another extension.\n\n> - If we were to start allowing the receiver side to rewrite pushed\n> commits, the updated send-pack protocol must be able to send the\n> new objects created by that \"magic\" back to the pusher. The\n> current protocol does not allow the receive-pack to send packdata\n> back to send-pack.\n\nIs it cleaner to just initiate another fetch to get that pack? Maybe\nrace conditions make it not a good option?\n-- \nDuy\n"},{"id":"258858","messageId":"xmqqmw2qlhb1.fsf@gitster.dls.corp.google.com","threadId":"38638","inReplyTo":"CACsJy8Cj97NrvuAXjqRbd52ivaHZ_oqy+k-3_LeRSe2Jy6U33Q@mail.gmail.com","subject":"Re: [PATCH] protocol upload-pack-v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-02T14:45:54Z","receivedAt":"2015-04-02T14:45:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Wed, Apr 1, 2015 at 2:58 AM, Junio C Hamano wrote:\n>> This is a follow-up on $gmane/264553, which is a continuation of\n>> $gmane/264000, but instead of giving two required readings to\n>> readers, I'll start with reproduction of the two, and add a few more\n>> things the current protocol lacks that I would want to see in the\n>> updated protocol.\n>\n> I think the important thing to get v2 started is making sure we do not\n> need v3 to get rid of any of these limitations. In other words v2\n> should be extensible enough to implement them later.\n\nYes.\n\n>> ...\n>\n> I don't have an answer to this one. So the reaction is,...\n> \"broken\" (in pratice, not in theory), don't touch it. I know I'm\n> burying my head in the sand..\n> ...\n> Introducing the ref exchange in push-pack could be done in an\n> extension too, I think.\n\nThanks, but you do not have to \"solve\" these sample issues here. As\nyou said above, your primary goal at this stage is to make sure v2\nis extensible to cover future needs, and in order to do so, you need\nto make sure \"what's lacking\" list is fairly complete. You are not\nhelping from that point of view.\n\nI'd like to see a new protocol that lets us overcome the limitations\n(did I miss others? I am sure people can help here) sometime this\nyear.\n"},{"id":"258905","messageId":"1935847.zk75T7vt7M@mfick1-lnx","threadId":"38638","inReplyTo":"xmqqd23pq6r3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] protocol upload-pack-v2","fromName":"Martin Fick","fromEmail":"mfick@codeaurora.org","sentAt":"2015-04-02T22:18:22Z","receivedAt":"2015-04-02T22:18:22Z","isPatch":true,"sender":{"key":"mfick@codeaurora.org","avatar":null},"body":"> The current protocol has the following problems that limit\n> us:\n> \n> - It is not easy to make it resumable, because we\n> recompute every time. This is especially problematic for\n> the initial fetch aka \"clone\" as we will be talking about\n> a large transfer. Redirection to a bundle hosted on CDN\n> might be something we could do transparently.\n> \n> - The protocol extension has a fairly low length limit.\n> \n> - Because the protocol exchange starts by the server side\n> advertising all its refs, even when the fetcher is\n> interested in a single ref, the initial overhead is\n> nontrivial, especially when you are doing a small\n> incremental update. The worst case is an auto-builder\n> that polls every five minutes, even when there is no new\n> commits to be fetched.\n\nA lot of focus about the problems with ref advertisement is \nabout the obvious problem mentioned above (a bad problem \nindeed). I would like to add that there is another related \nproblem that all potential solutions to the above problem do \nnot neccessarily improve. When polling regularly there is \nalso no current efficient way to check on the current state of \nall refs. It would be nice to also be able to get an \nincremental update on large refs spaces.\n\nThanks,\n\n-Martin\n\n-- \nThe Qualcomm Innovation Center, Inc. is a member of Code \nAurora Forum, hosted by The Linux Foundation\n"},{"id":"258906","messageId":"xmqq7ftugms5.fsf@gitster.dls.corp.google.com","threadId":"38638","inReplyTo":"1935847.zk75T7vt7M@mfick1-lnx","subject":"Re: [PATCH] protocol upload-pack-v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-02T22:58:50Z","receivedAt":"2015-04-02T22:58:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Fick writes:\n\n>> The current protocol has the following problems that limit\n>> us:\n>> \n>> - It is not easy to make it resumable, because we\n>> recompute every time. This is especially problematic for\n>> the initial fetch aka \"clone\" as we will be talking about\n>> a large transfer. Redirection to a bundle hosted on CDN\n>> might be something we could do transparently.\n>> \n>> - The protocol extension has a fairly low length limit.\n>> \n>> - Because the protocol exchange starts by the server side\n>> advertising all its refs, even when the fetcher is\n>> interested in a single ref, the initial overhead is\n>> nontrivial, especially when you are doing a small\n>> incremental update. The worst case is an auto-builder\n>> that polls every five minutes, even when there is no new\n>> commits to be fetched.\n>\n> A lot of focus about the problems with ref advertisement is \n> about the obvious problem mentioned above (a bad problem \n> indeed). I would like to add that there is another related \n> problem that all potential solutions to the above problem do \n> not neccessarily improve. When polling regularly there is \n> also no current efficient way to check on the current state of \n> all refs. It would be nice to also be able to get an \n> incremental update on large refs spaces.\n\nYes, incremental ref update is an important problem to solve. I\nthink one potential solution was indeed mentioned to improve that\nexact issue, e.g. footnote #3 in $gmane/264000.\n"},{"id":"258907","messageId":"CAGZ79ka9ficRLEV6KrG5qeAac0ZAQrj2nT=QF=_tFys5pq=5=w@mail.gmail.com","threadId":"38638","inReplyTo":"1935847.zk75T7vt7M@mfick1-lnx","subject":"Re: [PATCH] protocol upload-pack-v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-04-02T23:00:11Z","receivedAt":"2015-04-02T23:00:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Apr 2, 2015 at 3:18 PM, Martin Fick wrote:\n>> The current protocol has the following problems that limit\n>> us:\n>>\n>> - It is not easy to make it resumable, because we\n>> recompute every time. This is especially problematic for\n>> the initial fetch aka \"clone\" as we will be talking about\n>> a large transfer. Redirection to a bundle hosted on CDN\n>> might be something we could do transparently.\n>>\n>> - The protocol extension has a fairly low length limit.\n>>\n>> - Because the protocol exchange starts by the server side\n>> advertising all its refs, even when the fetcher is\n>> interested in a single ref, the initial overhead is\n>> nontrivial, especially when you are doing a small\n>> incremental update. The worst case is an auto-builder\n>> that polls every five minutes, even when there is no new\n>> commits to be fetched.\n>\n> A lot of focus about the problems with ref advertisement is\n> about the obvious problem mentioned above (a bad problem\n> indeed). I would like to add that there is another related\n> problem that all potential solutions to the above problem do\n> not neccessarily improve. When polling regularly there is\n> also no current efficient way to check on the current state of\n> all refs. It would be nice to also be able to get an\n> incremental update on large refs spaces.\n\nI think once the new protocol is in place, the server could advertise\nthe capability to send a differential of refs.\n\nTo make sure that works the capability phase should be strictly separated\nfrom the rest, so you can think of any new fancy scheme to transmit\nrefs or objects, and once both client and server agree on that fancy scheme\nboth know when to expect the \"new changed\" protocol.\n\nSo from a high level perspective it should look like:\nPhase 1) negotiation of capabilities\nPhase 2) ref advertisement (i.e. changes in the DAG end points)\nPhase 3) transmitting the missing blobs.\n\nThe crucial point now is to make sure Phase 1) is not growing to large in\ntransmission size / required compute power (/ complexity).\n\nAnd as everybody out there wants to invent new schemes how to do 2) and 3)\nefficient, I wonder if we need to do Phase 1) as a differential as well, so I'd\npresume the optimum could look like\n\nClient: Last time we talked the capabilities you advertised hashed to $SHA\nServer: That's right, but additionally I have \"push_cert_nonce=$value\"\n\nIn the non-optimal case:\nClient: Last time we talked the capabilities you advertised hashed to $SHA\nServer: I don't know that value, here comes the list of all\ncapabilities I can do:\n ...\n ...\n\nI like that approach as it would really break down to transmitting the minimal\namount of information most of the time. The downside is to know which\ncapabilities are cache-able and then hash-able, such that the remote side\nonly needs to maintain only a very small set of advertised capability lists\nand their hash. For example the nonce for signed pushes will hopefully\nnever be the same, so it makes no sense to have them inside the capabilities\ncache.\n\nHaving such a capabilities cache would give us a long time until the\nphase to negotiate the capabilities will grow too large again (most of the\ncapabilities I'd assume are rather static per server)\n\nAnd the way I understand the current situation, it's all about talking this\nearly negotiation phase, which then allows us to model the refs\nadvertisement and\nthe blob transmission later on as a response to upcoming problems in the future.\n\n>\n> Thanks,\n>\n> -Martin\n>\n> --\n> The Qualcomm Innovation Center, Inc. is a member of Code\n> Aurora Forum, hosted by The Linux Foundation\n>\n"},{"id":"258908","messageId":"CAGZ79kZtqn-7eZ1bZPFXL75NKMQc4yLafO7=StgapDx9=-PCdA@mail.gmail.com","threadId":"38638","inReplyTo":"CAGZ79ka9ficRLEV6KrG5qeAac0ZAQrj2nT=QF=_tFys5pq=5=w@mail.gmail.com","subject":"Re: [PATCH] protocol upload-pack-v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-04-02T23:14:52Z","receivedAt":"2015-04-02T23:14:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"After looking at $gmane/264000 again, maybe the client should talk first\nstating all the relevant information it wants to get, though I realize this\nis not part of capabilities so maybe it could even before, such as:\n\nClient: All I want to do is an ls-remote, so only Phase 2, no\ntransmission of blobs phase 3\nServer: ok\nClient[as in the previous mail]: Last time we talked you advertised\nhashed to $SHA\nServer: that's correct!\n\nAs the server knows the client doesn't want to know about Phase3, it\ncan omit things\nrelevant to that phase such as the signed push nonce.\n\nSo from a high level perspective we'd maybe need 4 phases like\nPhase 0) declare the intent (fetch/push all or partial parts)\nPhase 1) negotiation of capabilities\nPhase 2) ref advertisement (i.e. changes in the DAG end points)\nPhase 3) transmitting the missing blobs.\n\nThe problem may be that phase 0 and 1 may require mixing, as you may want\nto declare new things to do in 0) which you would have needed to advertise as\na capability in 1). So maybe we need to swap that around:\n\nPhase 1a) negotiation of capabilities\nPhase 1b) negotiation of intent (fetch/push of all/few branches in\nfull/shallow/narrow fashion)\nPhase 2) ref advertisement (i.e. changes in the DAG end points)\nPhase 3) transmitting the missing blobs.\n"}]}