{"thread":{"id":"39760","subject":"[PATCH 5/7] pack-protocol.txt: Be more precise about pusher-key relationship","startedAt":"2015-07-01T18:08:12Z","lastAt":"2015-07-06T18:23:30Z","messageCount":50,"participants":["Dave Borowitz","Stefan Beller","Junio C Hamano","Jonathan Nieder","Jeff King","Shawn Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"265343","messageId":"1435774099-21260-1-git-send-email-dborowitz@google.com","threadId":"39760","inReplyTo":null,"subject":"[PATCH 0/7] Clarify signed push protocol documentation","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-01T18:08:12Z","receivedAt":"2015-07-01T18:08:12Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"The signed push protocol documentation did not really match the reality of what\nsend-pack.c and receive-pack.c do, much to my chagrin as I attempted to\nimplement this protocol in JGit. This series covers most of the issues I found\non a first pass.\n\nDave Borowitz (7):\n pack-protocol.txt: Add warning about protocol inaccuracies\n pack-protocol.txt: Mark LF in command-list as optional\n pack-protocol.txt: Mark all LFs in push-cert as required\n pack-protocol.txt: Elaborate on pusher identity\n pack-protocol.txt: Be more precise about pusher-key relationship\n pack-protocol.txt: Mark pushee field as optional\n send-pack.c: Die if the nonce is empty\n\n Documentation/technical/pack-protocol.txt | 38 +++++++++++++++++++++++++------\n send-pack.c | 2 ++\n 2 files changed, 33 insertions(+), 7 deletions(-)\n\n-- \n2.4.3.573.g4eafbef\n"},{"id":"265342","messageId":"1435774099-21260-2-git-send-email-dborowitz@google.com","threadId":"39760","inReplyTo":"1435774099-21260-1-git-send-email-dborowitz@google.com","subject":"[PATCH 1/7] pack-protocol.txt: Add warning about protocol inaccuracies","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-01T18:08:13Z","receivedAt":"2015-07-01T18:08:13Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"We want to fix such inaccuracies, but there are a lot and there is no\nguarantee at any particular point in time that this document will be\nerror-free.\n\nSigned-off-by: Dave Borowitz \n---\n Documentation/technical/pack-protocol.txt | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex 4064fc7..66d2d95 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -14,6 +14,17 @@ 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+Notes to Implementors\n+---------------------\n+\n+WARNING: This document is a work in progress. Some of the protocol\n+specifications below may be incomplete or inaccurate. When in doubt,\n+refer to the C code.\n+\n+One particular example is that many of the LFs referenced in the\n+specifications are optional, but may (yet) not be marked as such. If not\n+explicitly marked one way or the other, double-check with the C code.\n+\n Transports\n ----------\n There are three transports over which the packfile protocol is\n-- \n2.4.3.573.g4eafbef\n"},{"id":"265338","messageId":"1435774099-21260-3-git-send-email-dborowitz@google.com","threadId":"39760","inReplyTo":"1435774099-21260-1-git-send-email-dborowitz@google.com","subject":"[PATCH 2/7] pack-protocol.txt: Mark LF in command-list as optional","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-01T18:08:14Z","receivedAt":"2015-07-01T18:08:14Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"Signed-off-by: Dave Borowitz \n---\n Documentation/technical/pack-protocol.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex 66d2d95..1386840 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -481,7 +481,7 @@ references.\n shallow = PKT-LINE(\"shallow\" SP obj-id LF)\n \n command-list = PKT-LINE(command NUL capability-list LF)\n-\t\t *PKT-LINE(command LF)\n+\t\t *PKT-LINE(command LF?)\n \t\t flush-pkt\n \n command = create / delete / update\n-- \n2.4.3.573.g4eafbef\n"},{"id":"265339","messageId":"1435774099-21260-4-git-send-email-dborowitz@google.com","threadId":"39760","inReplyTo":"1435774099-21260-1-git-send-email-dborowitz@google.com","subject":"[PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-01T18:08:15Z","receivedAt":"2015-07-01T18:08:15Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"Signed-off-by: Dave Borowitz \n---\n Documentation/technical/pack-protocol.txt | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex 1386840..2d8b1a1 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -534,6 +534,9 @@ A push certificate begins with a set of header lines. After the\n header and an empty line, the protocol commands follow, one per\n line.\n \n+Note that (unlike other portions of the protocol), all LFs in the\n+`push-cert` specification above MUST be present.\n+\n Currently, the following header fields are defined:\n \n `pusher` ident::\n-- \n2.4.3.573.g4eafbef\n"},{"id":"265337","messageId":"1435774099-21260-5-git-send-email-dborowitz@google.com","threadId":"39760","inReplyTo":"1435774099-21260-1-git-send-email-dborowitz@google.com","subject":"[PATCH 4/7] pack-protocol.txt: Elaborate on pusher identity","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-01T18:08:16Z","receivedAt":"2015-07-01T18:08:16Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"This is sort of like a standard identity, except that RFC 4880 section\n4.11 allows any UTF-8 text in the User ID packet. It is trivial to get\ngpg to pass arbitrary text when generating a push cert by setting\nuser.signingKey to that arbitrary value (assuming it is an actual user\nID associated with that key).\n\nSigned-off-by: Dave Borowitz \n---\n Documentation/technical/pack-protocol.txt | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex 2d8b1a1..de3c72c 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -494,7 +494,7 @@ references.\n \n push-cert = PKT-LINE(\"push-cert\" NUL capability-list LF)\n \t\t PKT-LINE(\"certificate version 0.1\" LF)\n-\t\t PKT-LINE(\"pusher\" SP ident LF)\n+\t\t PKT-LINE(\"pusher\" SP push-cert-ident LF)\n \t\t PKT-LINE(\"pushee\" SP url LF)\n \t\t PKT-LINE(\"nonce\" SP nonce LF)\n \t\t PKT-LINE(LF)\n@@ -502,6 +502,8 @@ references.\n \t\t *PKT-LINE(gpg-signature-lines LF)\n \t\t PKT-LINE(\"push-cert-end\" LF)\n \n+ push-cert-ident = 1*(UTF8) SP [\"-\"] 1*(DIGIT) SP [\"-\"|\"+\"] 1*(DIGIT)\n+\n packfile = \"PACK\" 28*(OCTET)\n ----\n \n@@ -540,8 +542,14 @@ Note that (unlike other portions of the protocol), all LFs in the\n Currently, the following header fields are defined:\n \n `pusher` ident::\n-\tIdentify the GPG key in \"Human Readable Name \"\n-\tformat.\n+\tIdentity of the GPG key. This is similar to the identify found\n+\telsewhere, such as the author/committer field in commit headers,\n+\tin that it consists of a name portion, a timestamp, and a\n+\ttimezone offset. However, unlike normal git identities, the name\n+\tfield may be any valid OpenPGP User ID, which is any valid UTF-8\n+\tstring. (By convention this matches the form:\n+\t\"Human Readable Name (optional comment) \"\n+\tbut this is only a convention.)\n \n `pushee` url::\n \tThe repository URL (anonymized, if the URL contains\n-- \n2.4.3.573.g4eafbef\n"},{"id":"265336","messageId":"1435774099-21260-6-git-send-email-dborowitz@google.com","threadId":"39760","inReplyTo":"1435774099-21260-1-git-send-email-dborowitz@google.com","subject":"[PATCH 5/7] pack-protocol.txt: Be more precise about pusher-key relationship","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-01T18:08:17Z","receivedAt":"2015-07-01T18:08:17Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"The use of \"must\" (albeit not in all caps) suggests that this is\nactually a requirement of the protocol. As no implementation exists\nthat actually does this verification, this is misleading at best.\n\nSigned-off-by: Dave Borowitz \n---\n Documentation/technical/pack-protocol.txt | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex de3c72c..f37dcf1 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -564,7 +564,8 @@ Currently, the following header fields are defined:\n The GPG signature lines are a detached signature for the contents\n recorded in the push certificate before the signature block begins.\n The detached signature is used to certify that the commands were\n-given by the pusher, who must be the signer.\n+given by the pusher, which verifier code SHOULD enforce is a valid User\n+ID associated with the signer.\n \n Report Status\n -------------\n-- \n2.4.3.573.g4eafbef\n"},{"id":"265341","messageId":"1435774099-21260-7-git-send-email-dborowitz@google.com","threadId":"39760","inReplyTo":"1435774099-21260-1-git-send-email-dborowitz@google.com","subject":"[PATCH 6/7] pack-protocol.txt: Mark pushee field as optional","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-01T18:08:18Z","receivedAt":"2015-07-01T18:08:18Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"send-pack.c omits this field when args->url is null or empty. Fix the\nprotocol specification to match reality.\n\nSigned-off-by: Dave Borowitz \n---\n Documentation/technical/pack-protocol.txt | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex f37dcf1..98e512d 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -495,7 +495,7 @@ references.\n push-cert = PKT-LINE(\"push-cert\" NUL capability-list LF)\n \t\t PKT-LINE(\"certificate version 0.1\" LF)\n \t\t PKT-LINE(\"pusher\" SP push-cert-ident LF)\n-\t\t PKT-LINE(\"pushee\" SP url LF)\n+\t\t [PKT-LINE(\"pushee\" SP url LF)]\n \t\t PKT-LINE(\"nonce\" SP nonce LF)\n \t\t PKT-LINE(LF)\n \t\t *PKT-LINE(command LF)\n@@ -554,7 +554,8 @@ Currently, the following header fields are defined:\n `pushee` url::\n \tThe repository URL (anonymized, if the URL contains\n \tauthentication material) the user who ran `git push`\n-\tintended to push into.\n+\tintended to push into. This field is optional, and included at\n+\tthe client's discretion.\n \n `nonce` nonce::\n \tThe 'nonce' string the receiving repository asked the\n-- \n2.4.3.573.g4eafbef\n"},{"id":"265340","messageId":"1435774099-21260-8-git-send-email-dborowitz@google.com","threadId":"39760","inReplyTo":"1435774099-21260-1-git-send-email-dborowitz@google.com","subject":"[PATCH 7/7] send-pack.c: Die if the nonce is empty","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-01T18:08:19Z","receivedAt":"2015-07-01T18:08:19Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"pack-protocol.txt does not list the nonce as optional. Fortunately, it\nshould be impossible to not have a nonce by this point in the code, as\nthe caller should have died on line 380 prior to generating a push\ncertificate with an empty nonce.\n\nNonetheless, having this explicit error handling in the code reduces\nconfusion for implementors trying to understand the signed push\nprotocol by looking at the reference implementation.\n\nSigned-off-by: Dave Borowitz \n---\n send-pack.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 2a64fec..77e2131 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -254,6 +254,8 @@ static int generate_push_cert(struct strbuf *req_buf,\n \t}\n \tif (push_cert_nonce[0])\n \t\tstrbuf_addf(&cert, \"nonce %s\\n\", push_cert_nonce);\n+\telse\n+\t\tdie(_(\"server did not provide a nonce\"));\n \tstrbuf_addstr(&cert, \"\\n\");\n \n \tfor (ref = remote_refs; ref; ref = ref->next) {\n-- \n2.4.3.573.g4eafbef\n"},{"id":"265345","messageId":"CAGZ79kY-T8k7GjCUxKh5p_bf_t1+M8jRoBPDFp0hpExYmE8y=g@mail.gmail.com","threadId":"39760","inReplyTo":"1435774099-21260-3-git-send-email-dborowitz@google.com","subject":"Re: [PATCH 2/7] pack-protocol.txt: Mark LF in command-list as optional","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-07-01T18:21:10Z","receivedAt":"2015-07-01T18:21:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jul 1, 2015 at 11:08 AM, Dave Borowitz wrote:\n> Signed-off-by: Dave Borowitz \n> ---\n> Documentation/technical/pack-protocol.txt | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\n> index 66d2d95..1386840 100644\n> --- a/Documentation/technical/pack-protocol.txt\n> +++ b/Documentation/technical/pack-protocol.txt\n> @@ -481,7 +481,7 @@ references.\n> shallow = PKT-LINE(\"shallow\" SP obj-id LF)\n>\n> command-list = PKT-LINE(command NUL capability-list LF)\n\nWe may also want to mark it in this line above as well as in the shallow line?\n\nI think the problem with this part of the documentation is its ambiguity on\nwhat exactly we want to document. The sender SHOULD put an LF, while\nthe receiver MUST NOT assume the LF is there always, so I guess it's best\nto mark it optional from a receivers point of view.\n\n> - *PKT-LINE(command LF)\n> + *PKT-LINE(command LF?)\n> flush-pkt\n>\n> command = create / delete / update\n> --\n> 2.4.3.573.g4eafbef\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"265347","messageId":"CAD0k6qRosOZqnmQ=HW6FWYV5pi-HYCK0wYgHnpQkD5R5eRSoug@mail.gmail.com","threadId":"39760","inReplyTo":"CAGZ79kY-T8k7GjCUxKh5p_bf_t1+M8jRoBPDFp0hpExYmE8y=g@mail.gmail.com","subject":"Re: [PATCH 2/7] pack-protocol.txt: Mark LF in command-list as optional","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-01T18:46:24Z","receivedAt":"2015-07-01T18:46:24Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Wed, Jul 1, 2015 at 11:21 AM, Stefan Beller wrote:\n> I think the problem with this part of the documentation is its ambiguity on\n> what exactly we want to document. The sender SHOULD put an LF, while\n> the receiver MUST NOT assume the LF is there always, so I guess it's best\n> to mark it optional from a receivers point of view.\n\nTo be clear, this patch is a partial fix to one particular spec in the\nfile. See 1/7 for the general warning not to trust these. Auditing the\nfile completely was not the goal of this series.\n"},{"id":"265349","messageId":"xmqqbnfvaeqk.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"1435774099-21260-7-git-send-email-dborowitz@google.com","subject":"Re: [PATCH 6/7] pack-protocol.txt: Mark pushee field as optional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-01T18:56:35Z","receivedAt":"2015-07-01T18:56:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz writes:\n\n> send-pack.c omits this field when args->url is null or empty. Fix the\n> protocol specification to match reality.\n\nDo some clients omit this in the real world?\n\nAs you say, send_pack() does omit it if args->url is null or empty,\nbut args is prepared in transport.c as a copy of transport->url when\nthe function is called, and that transport->url is how\nbuiltin/push.c reports where it is pushing with:\n\n if (verbosity > 0)\n fprintf(stderr, _(\"Pushing to %s\\n\"), transport->url);\n\nSo I am somewhat puzzled...\n\n>\n> Signed-off-by: Dave Borowitz \n> ---\n> Documentation/technical/pack-protocol.txt | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\n> index f37dcf1..98e512d 100644\n> --- a/Documentation/technical/pack-protocol.txt\n> +++ b/Documentation/technical/pack-protocol.txt\n> @@ -495,7 +495,7 @@ references.\n> push-cert = PKT-LINE(\"push-cert\" NUL capability-list LF)\n> \t\t PKT-LINE(\"certificate version 0.1\" LF)\n> \t\t PKT-LINE(\"pusher\" SP push-cert-ident LF)\n> -\t\t PKT-LINE(\"pushee\" SP url LF)\n> +\t\t [PKT-LINE(\"pushee\" SP url LF)]\n> \t\t PKT-LINE(\"nonce\" SP nonce LF)\n> \t\t PKT-LINE(LF)\n> \t\t *PKT-LINE(command LF)\n> @@ -554,7 +554,8 @@ Currently, the following header fields are defined:\n> `pushee` url::\n> \tThe repository URL (anonymized, if the URL contains\n> \tauthentication material) the user who ran `git push`\n> -\tintended to push into.\n> +\tintended to push into. This field is optional, and included at\n> +\tthe client's discretion.\n> \n> `nonce` nonce::\n> \tThe 'nonce' string the receiving repository asked the\n"},{"id":"265350","messageId":"xmqq7fqjaen2.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"1435774099-21260-5-git-send-email-dborowitz@google.com","subject":"Re: [PATCH 4/7] pack-protocol.txt: Elaborate on pusher identity","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-01T18:58:41Z","receivedAt":"2015-07-01T18:58:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz writes:\n\n> This is sort of like a standard identity, except that RFC 4880 section\n> 4.11 allows any UTF-8 text in the User ID packet. It is trivial to get\n> gpg to pass arbitrary text when generating a push cert by setting\n> user.signingKey to that arbitrary value (assuming it is an actual user\n> ID associated with that key).\n>\n> Signed-off-by: Dave Borowitz \n> ---\n\nI think this is a good idea. I notice that \"nonce\" used near-by\nalso lacks the definition, which we would want to document.\n\nThanks.\n\n> Documentation/technical/pack-protocol.txt | 14 +++++++++++---\n> 1 file changed, 11 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\n> index 2d8b1a1..de3c72c 100644\n> --- a/Documentation/technical/pack-protocol.txt\n> +++ b/Documentation/technical/pack-protocol.txt\n> @@ -494,7 +494,7 @@ references.\n> \n> push-cert = PKT-LINE(\"push-cert\" NUL capability-list LF)\n> \t\t PKT-LINE(\"certificate version 0.1\" LF)\n> -\t\t PKT-LINE(\"pusher\" SP ident LF)\n> +\t\t PKT-LINE(\"pusher\" SP push-cert-ident LF)\n> \t\t PKT-LINE(\"pushee\" SP url LF)\n> \t\t PKT-LINE(\"nonce\" SP nonce LF)\n> \t\t PKT-LINE(LF)\n> @@ -502,6 +502,8 @@ references.\n> \t\t *PKT-LINE(gpg-signature-lines LF)\n> \t\t PKT-LINE(\"push-cert-end\" LF)\n> \n> + push-cert-ident = 1*(UTF8) SP [\"-\"] 1*(DIGIT) SP [\"-\"|\"+\"] 1*(DIGIT)\n> +\n> packfile = \"PACK\" 28*(OCTET)\n> ----\n> \n> @@ -540,8 +542,14 @@ Note that (unlike other portions of the protocol), all LFs in the\n> Currently, the following header fields are defined:\n> \n> `pusher` ident::\n> -\tIdentify the GPG key in \"Human Readable Name \"\n> -\tformat.\n> +\tIdentity of the GPG key. This is similar to the identify found\n> +\telsewhere, such as the author/committer field in commit headers,\n> +\tin that it consists of a name portion, a timestamp, and a\n> +\ttimezone offset. However, unlike normal git identities, the name\n> +\tfield may be any valid OpenPGP User ID, which is any valid UTF-8\n> +\tstring. (By convention this matches the form:\n> +\t\"Human Readable Name (optional comment) \"\n> +\tbut this is only a convention.)\n> \n> `pushee` url::\n> \tThe repository URL (anonymized, if the URL contains\n"},{"id":"265353","messageId":"CAD0k6qQgL_nn2R2Lye3b4xqp-P7gwcMG6PGLvqQEdRAfV_64uA@mail.gmail.com","threadId":"39760","inReplyTo":"xmqqbnfvaeqk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 6/7] pack-protocol.txt: Mark pushee field as optional","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-01T19:06:24Z","receivedAt":"2015-07-01T19:06:24Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Wed, Jul 1, 2015 at 11:56 AM, Junio C Hamano wrote:\n> Do some clients omit this in the real world?\n\nI just sent you privately a trace where this happens using a recent\ngit client. With that in the wild, I think our server is going to have\nto handle these even if there is a bug and it is fixed promptly.\n"},{"id":"265354","messageId":"xmqqy4iz8zon.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"xmqqbnfvaeqk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 6/7] pack-protocol.txt: Mark pushee field as optional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-01T19:07:04Z","receivedAt":"2015-07-01T19:07:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Dave Borowitz writes:\n>\n>> send-pack.c omits this field when args->url is null or empty. Fix the\n>> protocol specification to match reality.\n>\n> Do some clients omit this in the real world?\n>\n> As you say, send_pack() does omit it if args->url is null or empty,\n> but args is prepared in transport.c as a copy of transport->url when\n> the function is called, and that transport->url is how\n> builtin/push.c reports where it is pushing with:\n>\n> if (verbosity > 0)\n> fprintf(stderr, _(\"Pushing to %s\\n\"), transport->url);\n>\n> So I am somewhat puzzled...\n\nAnswering myself, the most trivial example is \"git send-pack\" ;-)\nIt passes args that has a NULL in the .url field.\n"},{"id":"265355","messageId":"xmqqtwtn8zls.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"xmqqy4iz8zon.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 6/7] pack-protocol.txt: Mark pushee field as optional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-01T19:08:47Z","receivedAt":"2015-07-01T19:08:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> Dave Borowitz writes:\n>>\n>>> send-pack.c omits this field when args->url is null or empty. Fix the\n>>> protocol specification to match reality.\n>>\n>> Do some clients omit this in the real world?\n>>\n>> As you say, send_pack() does omit it if args->url is null or empty,\n>> but args is prepared in transport.c as a copy of transport->url when\n>> the function is called, and that transport->url is how\n>> builtin/push.c reports where it is pushing with:\n>>\n>> if (verbosity > 0)\n>> fprintf(stderr, _(\"Pushing to %s\\n\"), transport->url);\n>>\n>> So I am somewhat puzzled...\n>\n> Answering myself, the most trivial example is \"git send-pack\" ;-)\n> It passes args that has a NULL in the .url field.\n\n... which may be something we want to fix, but that does not mean\nthe field is mandatory, as we have implementations in the field that\ndo not send it ;-)\n\nThe patch looks good.\n\nThanks.\n"},{"id":"265358","messageId":"CAD0k6qSsNHTVktgQFvhHmOXMMd7xZ1=7=ukGVmGmWuaw0DXp6Q@mail.gmail.com","threadId":"39760","inReplyTo":"xmqqy4iz8zon.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 6/7] pack-protocol.txt: Mark pushee field as optional","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-01T19:31:07Z","receivedAt":"2015-07-01T19:31:07Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Wed, Jul 1, 2015 at 12:07 PM, Junio C Hamano wrote:\n> Answering myself, the most trivial example is \"git send-pack\" ;-)\n> It passes args that has a NULL in the .url field.\n\nWell, the example I have involves an actual \"git push\" command. The\nfact that .url is NULL in that case may be a separate bug.\n"},{"id":"265359","messageId":"20150701193949.GC4865@google.com","threadId":"39760","inReplyTo":"1435774099-21260-2-git-send-email-dborowitz@google.com","subject":"Re: [PATCH 1/7] pack-protocol.txt: Add warning about protocol inaccuracies","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2015-07-01T19:39:49Z","receivedAt":"2015-07-01T19:39:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nDave Borowitz wrote:\n\n> --- a/Documentation/technical/pack-protocol.txt\n> +++ b/Documentation/technical/pack-protocol.txt\n> @@ -14,6 +14,17 @@ 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> +Notes to Implementors\n> +---------------------\n> +\n> +WARNING: This document is a work in progress. Some of the protocol\n> +specifications below may be incomplete or inaccurate. When in doubt,\n> +refer to the C code.\n\nIf we include this warning, can it also say to contact\ngit@vger.kernel.org for inaccuracies?\n\nOtherwise it is easy to misread as \"Some of this document may be\ninaccurate, and we're working on fixing that. Don't bug me --- I\nalready know about the problems --- just be patient!\"\n\nI would rather that it would say something like\n\n\tCaveat\n\t------\n\tThis document contains some inaccuracies. Do not forget to also\n\tcheck against the C code, and please contact git@vger.kernel.org\n\tif you run into any unclear or inaccurate passages in this spec.\n\n> +\n> +One particular example is that many of the LFs referenced in the\n> +specifications are optional, but may (yet) not be marked as such. If not\n> +explicitly marked one way or the other, double-check with the C code.\n\nThe 'Reference Discovery' section says:\n\n\tServer SHOULD terminate each non-flush line using LF (\"\\n\") terminator;\n\tclient MUST NOT complain if there is no terminator.\n\nUnfortunately that's not explained in a section with broader scope.\n\nIsn't that actually the rule everywhere except for in push certs?\nThe documentation will be easier to use if it says so instead of\nasking implementers to check the source of all implementations\n(since interoperability with only one isn't enough).\n\nThanks,\nJonathan\n"},{"id":"265360","messageId":"xmqqk2uj8xlh.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"20150701193949.GC4865@google.com","subject":"Re: [PATCH 1/7] pack-protocol.txt: Add warning about protocol inaccuracies","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-01T19:52:10Z","receivedAt":"2015-07-01T19:52:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> The 'Reference Discovery' section says:\n>\n> \tServer SHOULD terminate each non-flush line using LF (\"\\n\") terminator;\n> \tclient MUST NOT complain if there is no terminator.\n\nI think these should be \"sender/receiver\", not \"server/client\".\n"},{"id":"265361","messageId":"CAD0k6qTtvfKgJX1LyaO3i6as1Av9T=cZrwput-1HnTJuek5HoA@mail.gmail.com","threadId":"39760","inReplyTo":"20150701193949.GC4865@google.com","subject":"Re: [PATCH 1/7] pack-protocol.txt: Add warning about protocol inaccuracies","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-01T19:56:47Z","receivedAt":"2015-07-01T19:56:47Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Wed, Jul 1, 2015 at 12:39 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> Dave Borowitz wrote:\n>\n>> --- a/Documentation/technical/pack-protocol.txt\n>> +++ b/Documentation/technical/pack-protocol.txt\n>> @@ -14,6 +14,17 @@ 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>> +Notes to Implementors\n>> +---------------------\n>> +\n>> +WARNING: This document is a work in progress. Some of the protocol\n>> +specifications below may be incomplete or inaccurate. When in doubt,\n>> +refer to the C code.\n>\n> If we include this warning, can it also say to contact\n> git@vger.kernel.org for inaccuracies?\n>\n> Otherwise it is easy to misread as \"Some of this document may be\n> inaccurate, and we're working on fixing that. Don't bug me --- I\n> already know about the problems --- just be patient!\"\n>\n> I would rather that it would say something like\n>\n> Caveat\n> ------\n> This document contains some inaccuracies. Do not forget to also\n> check against the C code, and please contact git@vger.kernel.org\n> if you run into any unclear or inaccurate passages in this spec.\n\nAgreed with your rationale and suggested wording, thanks.\n\n>> +\n>> +One particular example is that many of the LFs referenced in the\n>> +specifications are optional, but may (yet) not be marked as such. If not\n>> +explicitly marked one way or the other, double-check with the C code.\n>\n> The 'Reference Discovery' section says:\n>\n> Server SHOULD terminate each non-flush line using LF (\"\\n\") terminator;\n> client MUST NOT complain if there is no terminator.\n>\n> Unfortunately that's not explained in a section with broader scope.\n>\n> Isn't that actually the rule everywhere except for in push certs?\n\nIt's certainly the rule in more places. I personally have partially\naudited send-pack.c, but there are many places that are not yet\naudited. I don't feel comfortable making a broader claim without\nhaving done so, and I do not have the time to do so at the moment.\n\n> The documentation will be easier to use if it says so instead of\n> asking implementers to check the source of all implementations\n> (since interoperability with only one isn't enough).\n\nUnfortunately, the trust I have in this document at this point is less\nthan zero. A handful of spot fixes, while useful, does not serve as\nany sort of assurance that we've gotten all or even most of the\nproblems. Until such time as this document is actually reliable,\nimplementors must check the source of all implementations if they want\nto be accurate. That sucks for implementors (believe me, I know), but\nit's the truth.\n\n> Thanks,\n> Jonathan\n"},{"id":"265362","messageId":"xmqqfv578x87.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"1435774099-21260-4-git-send-email-dborowitz@google.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-01T20:00:08Z","receivedAt":"2015-07-01T20:00:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz writes:\n\n> Signed-off-by: Dave Borowitz \n> ---\n> Documentation/technical/pack-protocol.txt | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/Documentation/technical/pack-protocol.txt\n> b/Documentation/technical/pack-protocol.txt\n> index 1386840..2d8b1a1 100644\n> --- a/Documentation/technical/pack-protocol.txt\n> +++ b/Documentation/technical/pack-protocol.txt\n> @@ -534,6 +534,9 @@ A push certificate begins with a set of header\n> lines. After the\n> header and an empty line, the protocol commands follow, one per\n> line.\n> \n> +Note that (unlike other portions of the protocol), all LFs in the\n> +`push-cert` specification above MUST be present.\n> +\n> Currently, the following header fields are defined:\n> \n> `pusher` ident::\n\nI am moderately negative about this; wouldn't it make the end result\ncleaner to fix the implementation?\n"},{"id":"265363","messageId":"CAD0k6qSN9=afCe3RumJPfP9JERy1w+tAYdjq01MsQnsOjdKu3A@mail.gmail.com","threadId":"39760","inReplyTo":"xmqqfv578x87.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-01T20:07:11Z","receivedAt":"2015-07-01T20:07:11Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Wed, Jul 1, 2015 at 1:00 PM, Junio C Hamano wrote:\n> Dave Borowitz writes:\n>\n>> Signed-off-by: Dave Borowitz \n>> ---\n>> Documentation/technical/pack-protocol.txt | 3 +++\n>> 1 file changed, 3 insertions(+)\n>>\n>> diff --git a/Documentation/technical/pack-protocol.txt\n>> b/Documentation/technical/pack-protocol.txt\n>> index 1386840..2d8b1a1 100644\n>> --- a/Documentation/technical/pack-protocol.txt\n>> +++ b/Documentation/technical/pack-protocol.txt\n>> @@ -534,6 +534,9 @@ A push certificate begins with a set of header\n>> lines. After the\n>> header and an empty line, the protocol commands follow, one per\n>> line.\n>>\n>> +Note that (unlike other portions of the protocol), all LFs in the\n>> +`push-cert` specification above MUST be present.\n>> +\n>> Currently, the following header fields are defined:\n>>\n>> `pusher` ident::\n>\n> I am moderately negative about this; wouldn't it make the end result\n> cleaner to fix the implementation?\n\nI'm not sure I understand your suggestion. Are you saying, you would\nprefer to make LFs optional in the push cert, for consistency with LFs\nbeing optional elsewhere?\n\nC git servers in the wild already require LFs when extracting the\nnonce value from the certificate (see find_header). If we make the LFs\noptional, then a conforming client may not send LFs, which will cause\nnonce verification to fail when pushing to an unfixed server. That is\nwhy I think we are stuck with this.\n\n(Also, this is probably not insurmountable, but the cert processing\ncode in receive-pack.c would have to be substantially rewritten if we\nloosened this requirement. Currently it concatenates the cert contents\nwithout pkt-line framing into a buffer, and searches around for \"\\n\"\nand \"\\n\\n\".\n\nIf LF is optional, then with that approach you might end up with a\nsection of that buffer like:\n nonce 1234-abcd0000000000000000000000000000000000000000\ndeadbeefdeadbeefdeadbeefdeadbeefdeadbeef refs/heads/master\nwhere it is impossible to distinguish between the end of the nonce and\nthe start of the first command.)\n"},{"id":"265364","messageId":"xmqq8uaz8vjb.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"xmqqfv578x87.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-01T20:36:40Z","receivedAt":"2015-07-01T20:36:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Dave Borowitz writes:\n>\n>> Signed-off-by: Dave Borowitz \n>> ---\n>> Documentation/technical/pack-protocol.txt | 3 +++\n>> 1 file changed, 3 insertions(+)\n>>\n>> diff --git a/Documentation/technical/pack-protocol.txt\n>> b/Documentation/technical/pack-protocol.txt\n>> index 1386840..2d8b1a1 100644\n>> --- a/Documentation/technical/pack-protocol.txt\n>> +++ b/Documentation/technical/pack-protocol.txt\n>> @@ -534,6 +534,9 @@ A push certificate begins with a set of header\n>> lines. After the\n>> header and an empty line, the protocol commands follow, one per\n>> line.\n>> \n>> +Note that (unlike other portions of the protocol), all LFs in the\n>> +`push-cert` specification above MUST be present.\n>> +\n>> Currently, the following header fields are defined:\n>> \n>> `pusher` ident::\n>\n> I am moderately negative about this; wouldn't it make the end result\n> cleaner to fix the implementation?\n\nI think that something like this should be sufficient. As the\nreceiving end, we must not complain if there is no terminator.\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 94d0571..303a1dd 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1415,9 +1415,12 @@ static struct command *read_head_info(struct sha1_array *shallow)\n \t\t\t\t\ttrue_flush = 1;\n \t\t\t\t\tbreak;\n \t\t\t\t}\n-\t\t\t\tif (!strcmp(certbuf, \"push-cert-end\\n\"))\n+\t\t\t\tif (!strcmp(certbuf, \"push-cert-end\") ||\n+\t\t\t\t !strcmp(certbuf, \"push-cert-end\\n\"))\n \t\t\t\t\tbreak; /* end of cert */\n \t\t\t\tstrbuf_addstr(&push_cert, certbuf);\n+\t\t\t\tif (certbuf[len - 1] != '\\n')\n+\t\t\t\t\tstrbuf_addch(&push_cert, '\\n');\n \t\t\t}\n \n \t\t\tif (true_flush)\n"},{"id":"265365","messageId":"xmqq4mln8ve2.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"xmqq8uaz8vjb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-01T20:39:49Z","receivedAt":"2015-07-01T20:39:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> I am moderately negative about this; wouldn't it make the end result\n>> cleaner to fix the implementation?\n>\n> I think that something like this should be sufficient. As the\n> receiving end, we must not complain if there is no terminator.\n> ...\n\nAnd the change we are *not* going to make, but I made temporarily\nonly for testing, on the sending side to violate our \"sender SHOULD\nterminate with LF\" rule would look like this:\n\nThere is a slight complication on sending an empty line without any\ntermination, though ;-) The reader that calls packet_read() cannot\ntell such a payload from a flush packet, I think.\n\n*That* may be something we want to document.\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 2a64fec..1a743db 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -273,9 +273,11 @@ static int generate_push_cert(struct strbuf *req_buf,\n \n \tpacket_buf_write(req_buf, \"push-cert%c%s\", 0, cap_string);\n \tfor (cp = cert.buf; cp < cert.buf + cert.len; cp = np) {\n+\t\tint len;\n \t\tnp = next_line(cp, cert.buf + cert.len - cp);\n+\t\tlen = (np <= cp + 1) ? 1 : (np - cp - 1);\n \t\tpacket_buf_write(req_buf,\n-\t\t\t\t \"%.*s\", (int)(np - cp), cp);\n+\t\t\t\t \"%.*s\", len, cp);\n \t}\n \tpacket_buf_write(req_buf, \"push-cert-end\\n\");\n \n"},{"id":"265366","messageId":"xmqqzj3f7gde.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"CAD0k6qSN9=afCe3RumJPfP9JERy1w+tAYdjq01MsQnsOjdKu3A@mail.gmail.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-01T20:49:33Z","receivedAt":"2015-07-01T20:49:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz writes:\n\n>> I am moderately negative about this; wouldn't it make the end result\n>> cleaner to fix the implementation?\n>\n> I'm not sure I understand your suggestion. Are you saying, you would\n> prefer to make LFs optional in the push cert, for consistency with LFs\n> being optional elsewhere?\n\nAbsolutely. It is not \"make\" it optional, but \"even though it is\noptional, the receiver has not been following the spec, and it is\nnot too late to fix it\".\n\nThe earliest these documentation updates can hit the public is 2.6;\nby that time I'd expect the deployed receivers would be fixed with\n2.5.1 and 2.4.7 maintenance releases.\n\nIf some third-party reimplemented their client not to terminate\nwith LF, they wouldn't be working correctly with the deployed\nservers right now *anyway*. And with the more lenient receive-pack\nin 2.5.1 or 2.4.7, they will start working.\n\nAnd we will not change our client to drop LF termination. So\noverall I do not see that it is too much a price to pay for\nconsistency across the protocol.\n\n> If LF is optional, then with that approach you might end up with a\n> section of that buffer like:\n\nI think I touched on this in my previous message. You cannot send\nan empty line anywhere, and this is not limited to push-cert section\nof the protocol. Strictly speaking, the wire level allows it, but I\ndo not think the deployed client APIs easily lets you deal with it.\n\nSo you must follow the \"SHOULD terminate with LF\" for an empty line,\neven when you choose to ignore the \"SHOULD\" in most other places.\n\nI do not think it is such a big loss, as long as it is properly\ndocumented.\n"},{"id":"265396","messageId":"20150702135309.GA18286@peff.net","threadId":"39760","inReplyTo":"xmqq4mln8ve2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-02T13:53:10Z","receivedAt":"2015-07-02T13:53:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 01, 2015 at 01:39:49PM -0700, Junio C Hamano wrote:\n\n> There is a slight complication on sending an empty line without any\n> termination, though ;-) The reader that calls packet_read() cannot\n> tell such a payload from a flush packet, I think.\n> \n> *That* may be something we want to document.\n\nUsually flush packets are \"0000\", and an empty data packet\nis \"0004\". Or are you talking about some kind of flush inside the\npkt-data stream?\n\n-Peff\n"},{"id":"265468","messageId":"xmqq38155e3s.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"20150702135309.GA18286@peff.net","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-03T17:45:59Z","receivedAt":"2015-07-03T17:45:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jul 01, 2015 at 01:39:49PM -0700, Junio C Hamano wrote:\n>\n>> There is a slight complication on sending an empty line without any\n>> termination, though ;-) The reader that calls packet_read() cannot\n>> tell such a payload from a flush packet, I think.\n>> \n>> *That* may be something we want to document.\n>\n> Usually flush packets are \"0000\", and an empty data packet\n> is \"0004\". Or are you talking about some kind of flush inside the\n> pkt-data stream?\n\nNeither. At the wire level there is a difference, but the callers\nof most often used function in pkt-line API, packet_read(), says\n\n\twhile (1) {\n\t\tlen = packet_read();\n\t if (!len) {\n\t \t/* flush */\n\t break;\n\t\t}\n\t ... do things on the \"len\" bytes received ...\n\t\t... and then on to the next packet ...\n\t}\n\nI think the least intrusive change to the caller side would be\nto teach packet_read() to keep a static and let the callers do\nthis:\n\n\twhile (1) {\n\t\tlen = packet_read();\n\t if (!len && packet_last_was_flush()) {\n\t \t/* flush */\n\t break;\n\t\t}\n\t ... do things on the \"len\" bytes received ...\n\t\t... and then on to the next packet ...\n\t}\n\neven though that looks very ugly.\n\n\tlen = packet_read(..., &flag);\n if (!len && (flag & PKT_LAST_WAS_FLUSH)) {\n \t/* flush */\n ...\n\nmight be better.\n"},{"id":"265474","messageId":"20150703180718.GB9223@peff.net","threadId":"39760","inReplyTo":"xmqq38155e3s.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-03T18:07:19Z","receivedAt":"2015-07-03T18:07:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 03, 2015 at 10:45:59AM -0700, Junio C Hamano wrote:\n\n> > Usually flush packets are \"0000\", and an empty data packet\n> > is \"0004\". Or are you talking about some kind of flush inside the\n> > pkt-data stream?\n> \n> Neither. At the wire level there is a difference, but the callers\n> of most often used function in pkt-line API, packet_read(), says\n> \n> \twhile (1) {\n> \t\tlen = packet_read();\n> \t if (!len) {\n> \t \t/* flush */\n> \t break;\n> \t\t}\n> \t ... do things on the \"len\" bytes received ...\n> \t\t... and then on to the next packet ...\n> \t}\n\nAh, I see. Yeah, that is a problem. The solutions you proposed seem like\ngood workarounds to me, but we are unfortunately stuck with existing\nclients and servers which did not behave that way.\n\nI wondered briefly whether this impacted the keepalives we added to\n`upload-pack` in 05e9515; those are implemented as 0-byte data packets,\nwhich we send during the potentially long counting/delta-compression\nphase before we send out pack data. It works there because the packets\nactually contain a single sideband byte, so they are never mistaken for\na flush packet.\n\nRelated, I recently ran into a case where I think we should do the same\nfor pushes. After receiving the pack, `index-pack` may chew on the\nresult for literally minutes (try pushing up the entire linux.git\nhistory sometime). We say nothing at all on the wire until we've\nfinished that, run check_everything_connected, and run all hooks. Some\nclients (or intermediates on the connection) may give up after a few\nminutes of silence.\n\nI think we should have:\n\n 1. Some progress eye-candy from the server to tell us that something\n is happening, and how close we are to finishing (basically\n \"index-pack -v\").\n\n 2. When progress is disabled, similar keepalive packets saying \"nope,\n no output yet\".\n\nFor (2), hopefully we can implement it in the same way, and rely on\nempty sideband-0 packets. I haven't tested it in practice, though (I\nhave some very rough patches for (1) already).\n\n-Peff\n"},{"id":"265477","messageId":"CAJo=hJsoKSB23KNNF9Xr-NUAqUXvq+O-ZYOSyb4wAC1TZKZ2-g@mail.gmail.com","threadId":"39760","inReplyTo":"20150703180718.GB9223@peff.net","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2015-07-03T18:43:33Z","receivedAt":"2015-07-03T18:43:33Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Fri, Jul 3, 2015 at 11:07 AM, Jeff King wrote:\n> I wondered briefly whether this impacted the keepalives we added to\n> `upload-pack` in 05e9515; those are implemented as 0-byte data packets,\n> which we send during the potentially long counting/delta-compression\n> phase before we send out pack data. It works there because the packets\n> actually contain a single sideband byte, so they are never mistaken for\n> a flush packet.\n\nInteresting. I didn't know about 05e9515. This is a great hack. We\nhave similar issues in our server-server system at $DAY_JOB, they use\n--quiet as no human is watching. So we did a different hack for the\nsame reason.\n\n> Related, I recently ran into a case where I think we should do the same\n> for pushes. After receiving the pack, `index-pack` may chew on the\n> result for literally minutes (try pushing up the entire linux.git\n> history sometime). We say nothing at all on the wire until we've\n> finished that, run check_everything_connected, and run all hooks. Some\n> clients (or intermediates on the connection) may give up after a few\n> minutes of silence.\n>\n> I think we should have:\n>\n> 1. Some progress eye-candy from the server to tell us that something\n> is happening, and how close we are to finishing (basically\n> \"index-pack -v\").\n\nJGit receive-pack has done this for years. We output a progress\nmonitor for the resolving delta phase, and the counting during the\ngraph connectivity check, as JGit being in Java is slow as snot and\ncannot digest the linux kernel instantly.\n\n> 2. When progress is disabled, similar keepalive packets saying \"nope,\n> no output yet\".\n\nYea this is a problem so I think JGit ignores the client's request for\n\"quiet\" here and shovels progress messages anyway as a hack to force\nkeep-alive. Never considered the empty side-band message that 05e9515\nintroduces.\n\n> For (2), hopefully we can implement it in the same way, and rely on\n> empty sideband-0 packets. I haven't tested it in practice, though (I\n> have some very rough patches for (1) already).\n\nsideband-0 is not going to work for JGit clients.\n\nJGit clients are strict about the sideband stream being 1,2,3 and fail\nhard if they get any other stream from the server[1].\n\n[1] https://eclipse.googlesource.com/jgit/jgit/+/master/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java#169\n"},{"id":"265478","messageId":"20150703184608.GA11526@peff.net","threadId":"39760","inReplyTo":"CAJo=hJsoKSB23KNNF9Xr-NUAqUXvq+O-ZYOSyb4wAC1TZKZ2-g@mail.gmail.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-03T18:46:08Z","receivedAt":"2015-07-03T18:46:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 03, 2015 at 11:43:33AM -0700, Shawn Pearce wrote:\n\n> > For (2), hopefully we can implement it in the same way, and rely on\n> > empty sideband-0 packets. I haven't tested it in practice, though (I\n> > have some very rough patches for (1) already).\n> \n> sideband-0 is not going to work for JGit clients.\n\nEr, sorry, mental hiccup. It is 0-length sideband-1 packets that I\nmeant. The same as for upload-pack keepalives.\n\n> JGit clients are strict about the sideband stream being 1,2,3 and fail\n> hard if they get any other stream from the server[1].\n\nI think git does, too.\n\n-Peff\n"},{"id":"265552","messageId":"CAD0k6qTDpH0H-k9h+f3X8PjXpOZ7tRzv+8wvi8HALhg9DDm4Ew@mail.gmail.com","threadId":"39760","inReplyTo":"xmqqzj3f7gde.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-06T14:46:06Z","receivedAt":"2015-07-06T14:46:06Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Wed, Jul 1, 2015 at 4:49 PM, Junio C Hamano wrote:\n>\n> Dave Borowitz writes:\n>\n> >> I am moderately negative about this; wouldn't it make the end result\n> >> cleaner to fix the implementation?\n> >\n> > I'm not sure I understand your suggestion. Are you saying, you would\n> > prefer to make LFs optional in the push cert, for consistency with LFs\n> > being optional elsewhere?\n>\n> Absolutely. It is not \"make\" it optional, but \"even though it is\n> optional, the receiver has not been following the spec, and it is\n> not too late to fix it\".\n>\n> The earliest these documentation updates can hit the public is 2.6;\n> by that time I'd expect the deployed receivers would be fixed with\n> 2.5.1 and 2.4.7 maintenance releases.\n>\n> If some third-party reimplemented their client not to terminate\n> with LF, they wouldn't be working correctly with the deployed\n> servers right now *anyway*. And with the more lenient receive-pack\n> in 2.5.1 or 2.4.7, they will start working.\n>\n> And we will not change our client to drop LF termination. So\n> overall I do not see that it is too much a price to pay for\n> consistency across the protocol.\n\nOk, I understand your proposal now, thank you. I will drop this\ndocumentation patch from this series, and abandon\nhttps://git.eclipse.org/r/51071 in JGit. I am not volunteering to\nrewrite push cert handling in git-core though ;)\n\n> > If LF is optional, then with that approach you might end up with a\n> > section of that buffer like:\n>\n> I think I touched on this in my previous message. You cannot send\n> an empty line anywhere, and this is not limited to push-cert section\n> of the protocol. Strictly speaking, the wire level allows it, but I\n> do not think the deployed client APIs easily lets you deal with it.\n>\n> So you must follow the \"SHOULD terminate with LF\" for an empty line,\n> even when you choose to ignore the \"SHOULD\" in most other places.\n>\n> I do not think it is such a big loss, as long as it is properly\n> documented.\n"},{"id":"265553","messageId":"CAD0k6qSJeNBX=kmo4dn-=SqHGottXT2PJfpCD=y_SKNwEMDMyA@mail.gmail.com","threadId":"39760","inReplyTo":"CAD0k6qTDpH0H-k9h+f3X8PjXpOZ7tRzv+8wvi8HALhg9DDm4Ew@mail.gmail.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-06T15:22:02Z","receivedAt":"2015-07-06T15:22:02Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Jul 6, 2015 at 10:46 AM, Dave Borowitz wrote:\n> On Wed, Jul 1, 2015 at 4:49 PM, Junio C Hamano wrote:\n>>\n>> Dave Borowitz writes:\n>>\n>> >> I am moderately negative about this; wouldn't it make the end result\n>> >> cleaner to fix the implementation?\n>> >\n>> > I'm not sure I understand your suggestion. Are you saying, you would\n>> > prefer to make LFs optional in the push cert, for consistency with LFs\n>> > being optional elsewhere?\n>>\n>> Absolutely. It is not \"make\" it optional, but \"even though it is\n>> optional, the receiver has not been following the spec, and it is\n>> not too late to fix it\".\n>>\n>> The earliest these documentation updates can hit the public is 2.6;\n>> by that time I'd expect the deployed receivers would be fixed with\n>> 2.5.1 and 2.4.7 maintenance releases.\n>>\n>> If some third-party reimplemented their client not to terminate\n>> with LF, they wouldn't be working correctly with the deployed\n>> servers right now *anyway*. And with the more lenient receive-pack\n>> in 2.5.1 or 2.4.7, they will start working.\n>>\n>> And we will not change our client to drop LF termination. So\n>> overall I do not see that it is too much a price to pay for\n>> consistency across the protocol.\n>\n> Ok, I understand your proposal now, thank you. I will drop this\n> documentation patch from this series, and abandon\n> https://git.eclipse.org/r/51071 in JGit. I am not volunteering to\n> rewrite push cert handling in git-core though ;)\n\nUnfortunately, optional LFs still make the stored certs for later\nauditing and parsing a bit illegible. This is one way in which push\ncerts are fundamentally different from the rest of the wire protocol,\nwhich is not intended to be persisted.\n\nThe corner case I pointed out before where nonce runs into commands is\nnot the only one.\n\nConsider the following cert fragment:\n001fpushee git://localhost/repo\n0029nonce 1433954361-bde756572d665bba81d8\n\nA naive cert storage/auditing implementation would store the raw\npayload that needs to be verified, without the pkt-line framing. In\nthis case:\npushee git://localhost/repononce 1433954361-bde756572d665bba81d8\n\nA naive parser that wants to find the pushee would look for \"pushee\n\", which would be wrong in this case. (To say nothing of the\nfact that \"pushee\" might actually be \"-0700pushee\".)\n\nThe alternatives for someone writing a parser are:\na. Store the original pkt-line framing.\nb. Write a parser in some other clever way, e.g. parsing the entire\ncert in reverse might work.\n\nNeither of these is very satisfying, and both reduce human legibility\nof the stored payload.\n\n>> > If LF is optional, then with that approach you might end up with a\n>> > section of that buffer like:\n>>\n>> I think I touched on this in my previous message. You cannot send\n>> an empty line anywhere, and this is not limited to push-cert section\n>> of the protocol. Strictly speaking, the wire level allows it, but I\n>> do not think the deployed client APIs easily lets you deal with it.\n>>\n>> So you must follow the \"SHOULD terminate with LF\" for an empty line,\n>> even when you choose to ignore the \"SHOULD\" in most other places.\n>>\n>> I do not think it is such a big loss, as long as it is properly\n>> documented.\n"},{"id":"265554","messageId":"CAD0k6qQyhMKe7=gzuPt3QwDEvX1ovr72aHnGeAHnf1=LffqF-Q@mail.gmail.com","threadId":"39760","inReplyTo":"CAD0k6qSJeNBX=kmo4dn-=SqHGottXT2PJfpCD=y_SKNwEMDMyA@mail.gmail.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-06T15:27:13Z","receivedAt":"2015-07-06T15:27:13Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Jul 6, 2015 at 11:22 AM, Dave Borowitz wrote:\n> b. Write a parser in some other clever way, e.g. parsing the entire\n> cert in reverse might work.\n\n...as long as \" \" is illegal in nonce and pushee values, which it may\nbe but is not explicitly documented. I still have no desire to write\nsuch a parser.\n"},{"id":"265555","messageId":"CAD0k6qSfPEp9L2htDp7+JQ6jv=Enm7O1+j_0hiThWZdi-3PL-g@mail.gmail.com","threadId":"39760","inReplyTo":"CAD0k6qQyhMKe7=gzuPt3QwDEvX1ovr72aHnGeAHnf1=LffqF-Q@mail.gmail.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-06T15:29:28Z","receivedAt":"2015-07-06T15:29:28Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Jul 6, 2015 at 11:27 AM, Dave Borowitz wrote:\n> On Mon, Jul 6, 2015 at 11:22 AM, Dave Borowitz wrote:\n>> b. Write a parser in some other clever way, e.g. parsing the entire\n>> cert in reverse might work.\n>\n> ...as long as \" \" is illegal in nonce and pushee values, which it may\n> be but is not explicitly documented. I still have no desire to write\n> such a parser.\n\nTBQH at this point I would prefer, as a protocol implementor, to\nrestore the original proposal of this patch, which is to require \\n in\npush certificates.\n"},{"id":"265556","messageId":"CAD0k6qQnaR_yiWvUw4XL64VufHwZQiUGokH0CqyotCg=sgsceg@mail.gmail.com","threadId":"39760","inReplyTo":"CAD0k6qSJeNBX=kmo4dn-=SqHGottXT2PJfpCD=y_SKNwEMDMyA@mail.gmail.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-06T15:35:08Z","receivedAt":"2015-07-06T15:35:08Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Jul 6, 2015 at 11:22 AM, Dave Borowitz wrote:\n> The alternatives for someone writing a parser are:\n> a. Store the original pkt-line framing.\n\nOr obviously, a2. Frame in some other way, e.g. JSON array of strings\n(complete straw man, not seriously proposing this).\n"},{"id":"265559","messageId":"CAJo=hJvfYfgBthFMYXnXJ6e6PVM92SsWGVNt7qNTSQH9=psGtQ@mail.gmail.com","threadId":"39760","inReplyTo":"xmqqzj3f7gde.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2015-07-06T15:46:09Z","receivedAt":"2015-07-06T15:46:09Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Wed, Jul 1, 2015 at 1:49 PM, Junio C Hamano wrote:\n> Dave Borowitz writes:\n>\n>>> I am moderately negative about this; wouldn't it make the end result\n>>> cleaner to fix the implementation?\n>>\n>> I'm not sure I understand your suggestion. Are you saying, you would\n>> prefer to make LFs optional in the push cert, for consistency with LFs\n>> being optional elsewhere?\n>\n> Absolutely. It is not \"make\" it optional, but \"even though it is\n> optional, the receiver has not been following the spec, and it is\n> not too late to fix it\".\n\nThis is madness. For all the reasons Dave points out later in the\nthread. You can't store and make sense of the push cert without the LF\nrecord delimiters.\n\npush cert format is like commit or tag format. You need those LFs. We\ncan't just go declare them optional because of the way pkt-line read\nfunction is implemented in git-core.\n"},{"id":"265563","messageId":"xmqqk2ud2rk8.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"CAD0k6qSJeNBX=kmo4dn-=SqHGottXT2PJfpCD=y_SKNwEMDMyA@mail.gmail.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-06T16:12:39Z","receivedAt":"2015-07-06T16:12:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz writes:\n\n> Unfortunately, optional LFs still make the stored certs for later\n> auditing and parsing a bit illegible. This is one way in which push\n> certs are fundamentally different from the rest of the wire protocol,\n> which is not intended to be persisted.\n\nHmm, I am not sure I follow. \n\n> The corner case I pointed out before where nonce runs into commands is\n> not the only one.\n>\n> Consider the following cert fragment:\n> 001fpushee git://localhost/repo\n> 0029nonce 1433954361-bde756572d665bba81d8\n>\n> A naive cert storage/auditing implementation would store the raw\n> payload that needs to be verified, without the pkt-line framing. In\n> this case:\n> pushee git://localhost/repononce 1433954361-bde756572d665bba81d8\n\n\"Without the pkt-line framing\" is fine, but my understanding (or,\nthe intention of the original implementor) of this part of the\nprotocol is that \"packets between the push-cert packet and the\npush-cert-end packet carry the meat of the each line of the\ncertificate, one packet per line\".\n\nIf pkt-line is allowed to omit the terminating LFs, then it follows\nthat the receiving ends can simply do something like what I\nillustrated in $gmane/273196 (in java or whatever other\nimplementation platform they use) to collect packets between\n\"push-cert\" and \"push-cert-end\", knowing that the packets may or may\nnot have terminating LF and supplying the omitted LFs themselves\nwhen they receive the cert before verifying and storing.\n\nSo in order to reconstitute the \"raw payload without pkt-line framing\",\nthe omitted LF obviously needs to be added. Why is that a problem?\n\n Side note: think of it in a different way. The key word of the\n first paragraph above is \"the meat of\"; if your cert has two\n lines\n\n \t\"pushee $URLnonce 1234-5670\"\n\n the lines in it are \"pushee $URL\" and \"nonce 1234-5670\"\n but the meat of them are \"pushee $URL\" and \"nonce 1234-5670\".\n\n The protocol wants to carry an array with two elements, (\"pushee\n $URL\", \"nonce 1234-5670\"), as the hypothetical cert has two\n lines. And then \"\\n\".join(the cert array) . \"\\n\" would be how\n you reconstruct the original payload.\n\n The illustration in $gmane/273196 is slightly cheating in that\n sense. Instead of first creating an array of plain strings\n without LF termination and joining them together later, it knows\n that we will LF-join in the end, and abuses the LF in the\n original payload that came from the sender and supplies its own\n if the sender omitted it.\n\nIt is very similar to and in the opposite of how each ref advertisement\nis handled. Until the first flush, each packet is expected to carry\nthe object name and the ref name. A pkt-line framing may add\nterminating LF but that obviously is not part of the ref name.\n\n> A naive parser that wants to find the pushee would look for \"pushee\n> \", which would be wrong in this case. (To say nothing of the\n> fact that \"pushee\" might actually be \"-0700pushee\".)\n>\n> The alternatives for someone writing a parser are:\n> a. Store the original pkt-line framing.\n> b. Write a parser in some other clever way, e.g. parsing the entire\n> cert in reverse might work.\n>\n> Neither of these is very satisfying, and both reduce human legibility\n> of the stored payload.\n>\n>>> > If LF is optional, then with that approach you might end up with a\n>>> > section of that buffer like:\n>>>\n>>> I think I touched on this in my previous message. You cannot send\n>>> an empty line anywhere, and this is not limited to push-cert section\n>>> of the protocol. Strictly speaking, the wire level allows it, but I\n>>> do not think the deployed client APIs easily lets you deal with it.\n>>>\n>>> So you must follow the \"SHOULD terminate with LF\" for an empty line,\n>>> even when you choose to ignore the \"SHOULD\" in most other places.\n>>>\n>>> I do not think it is such a big loss, as long as it is properly\n>>> documented.\n"},{"id":"265566","messageId":"xmqqfv512qu9.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"CAJo=hJvfYfgBthFMYXnXJ6e6PVM92SsWGVNt7qNTSQH9=psGtQ@mail.gmail.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-06T16:28:14Z","receivedAt":"2015-07-06T16:28:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce writes:\n\n> push cert format is like commit or tag format. You need those LFs. We\n> can't just go declare them optional because of the way pkt-line read\n> function is implemented in git-core.\n\nAs I said, I view each of the packets between \"push-cert\" and\n\"push-cert-end\" packets representing the meat of each line in the\ncert. The sending end takes a cert as a long multi-line string,\nsplits them into an array, each of whose element represents a line\nin it (iow \"certlines = certstring.split('\\n')\"), and sends them\npacketised.\n\nThe receiver receives a sequence of packets, notices \"push-cert\"\npacket, collects packets until it sees \"push-cert-end\" packet and\ntreats them as elements of this array. pkt-line deframing process\nwould have to strip optional LFs to reconstruct the original array\nthe sender had (i.e. the above certlines array).\n\nThe receiver needs to join the array with LF to recover the long\nmulti-line string once it received the array. But this LF does not\nhave anything to do with the optional trailing LF in pkt-line. If\nyou sent the original \"certlines\" array via different RPC mechanism,\nyou need to join them together with your own LF to reconstruct the\nmulti-line srring.\n"},{"id":"265567","messageId":"xmqqegkl2qu2.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"CAJo=hJvfYfgBthFMYXnXJ6e6PVM92SsWGVNt7qNTSQH9=psGtQ@mail.gmail.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-06T16:28:21Z","receivedAt":"2015-07-06T16:28:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce writes:\n\n> push cert format is like commit or tag format. You need those LFs. We\n> can't just go declare them optional because of the way pkt-line read\n> function is implemented in git-core.\n\nAs I said, I view each of the packets between \"push-cert\" and\n\"push-cert-end\" packets representing the meat of each line in the\ncert. The sending end takes a cert as a long multi-line string,\nsplits them into an array, each of whose element represents a line\nin it (iow \"certlines = certstring.split('\\n')\"), and sends them\npacketised.\n\nThe receiver receives a sequence of packets, notices \"push-cert\"\npacket, collects packets until it sees \"push-cert-end\" packet and\ntreats them as elements of this array. pkt-line deframing process\nwould have to strip optional LFs to reconstruct the original array\nthe sender had (i.e. the above certlines array).\n\nThe receiver needs to join the array with LF to recover the long\nmulti-line string once it received the array. But this LF does not\nhave anything to do with the optional trailing LF in pkt-line. If\nyou sent the original \"certlines\" array via different RPC mechanism,\nyou need to join them together with your own LF to reconstruct the\nmulti-line string.\n"},{"id":"265570","messageId":"CAD0k6qRLu1d7Sa8aVrHtDCsJNtVXwzHBAyOmmUHmVAx7qHmOPg@mail.gmail.com","threadId":"39760","inReplyTo":"xmqqegkl2qu2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-06T16:38:20Z","receivedAt":"2015-07-06T16:38:20Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Jul 6, 2015 at 12:28 PM, Junio C Hamano wrote:\n> Shawn Pearce writes:\n>\n>> push cert format is like commit or tag format. You need those LFs. We\n>> can't just go declare them optional because of the way pkt-line read\n>> function is implemented in git-core.\n>\n> As I said, I view each of the packets between \"push-cert\" and\n> \"push-cert-end\" packets representing the meat of each line in the\n> cert. The sending end takes a cert as a long multi-line string,\n> splits them into an array, each of whose element represents a line\n> in it (iow \"certlines = certstring.split('\\n')\"), and sends them\n> packetised.\n\nRight now the sending end sends newlines.\n\n> The receiver receives a sequence of packets, notices \"push-cert\"\n> packet, collects packets until it sees \"push-cert-end\" packet and\n> treats them as elements of this array. pkt-line deframing process\n> would have to strip optional LFs to reconstruct the original array\n> the sender had (i.e. the above certlines array).\n\nThe problem is the signature. Today, the client computes the signature\nover the payload it actually sends (minus pkt-line headers)\n\nThe server can munge pkt-lines and reinsert LFs, but it _must_ have\nsome way of reconstructing the payload that the client signed in order\nto verify the signature. If we just naively insert LFs where missing,\nwe lose the ability to verify the signature.\n\nIf we say the payload the client signs MUST have LFs only in certain\nplaces, then that gives the server enough information to reconstruct\nthe payload and verify the signature.\n\nBut if we say the signed payload MUST have LFs and the wire payload\nMAY have LFs, then now we have two completely different formats, only\none of which is documented.\n\n> The receiver needs to join the array with LF to recover the long\n> multi-line string once it received the array. But this LF does not\n> have anything to do with the optional trailing LF in pkt-line. If\n> you sent the original \"certlines\" array via different RPC mechanism,\n> you need to join them together with your own LF to reconstruct the\n> multi-line string.\n>\n"},{"id":"265572","messageId":"xmqq615x2ph1.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"CAD0k6qRLu1d7Sa8aVrHtDCsJNtVXwzHBAyOmmUHmVAx7qHmOPg@mail.gmail.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-06T16:57:46Z","receivedAt":"2015-07-06T16:57:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz writes:\n\n> The server can munge pkt-lines and reinsert LFs, but it _must_ have\n> some way of reconstructing the payload that the client signed in order\n> to verify the signature. If we just naively insert LFs where missing,\n> we lose the ability to verify the signature.\n\nI still do not understand this part.\n\nThere is no way to \"naively\" insert, is there? You have an array of\nlines (each of which you have already stripped its terminating LF at\nits end). How else other than adding one LF at the end of each\nelement do you reconstruct the original multi-line string the client\nsigned? Are there other ways that makes the result ambiguous??\n\n> If we say the payload the client signs MUST have LFs only in certain\n> places, then that gives the server enough information to reconstruct\n> the payload and verify the signature.\n>\n> But if we say the signed payload MUST have LFs and the wire payload\n> MAY have LFs, then now we have two completely different formats, only\n> one of which is documented.\n\nI thought that was what I was saying. The wire protocol sends the\ncontents of each line (both what is signed and the signature) on a\nseparate packet. When I say \"contents of a line\", I do not include\nthe terminating LF as part of the line (iow, LF is not even\noptional; the terminating LF is not considered as part of \"the\ncontents of a line\"). It becomes irrelevant that a pkt-line may or\nmay not have a trailing optional LF. If there is LF at the end of a\npacket between \"push-cert\" and \"push-cert-end\" packets, that LF by\ndefinition cannot be part of the \"contents of a line\" in a\ncertificate.\n\nIt is just a pkt-line framing artifact you can and should remove if\nyou are doing a \"split to array, join with LF\" implementation to\nrecover the original string.\n\nAnd that is very much consistent with the way we send other things\nwith pkt-line protocol. Each packet up to the first flush is\nexpected to have and as ref advertisement.\nThe pkt-line framing may or may not add a trailing LF, but LF is not\npart of . It is not even part of the payload; merely an\nartifact of pkt-line framing.\n"},{"id":"265574","messageId":"CAD0k6qT8=xQb6MRcLkyvZBm0MRdQ0Z-8ojqghovdgeJQ2EBNEA@mail.gmail.com","threadId":"39760","inReplyTo":"xmqq615x2ph1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-06T17:11:45Z","receivedAt":"2015-07-06T17:11:45Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Jul 6, 2015 at 12:57 PM, Junio C Hamano wrote:\n> Dave Borowitz writes:\n>\n>> The server can munge pkt-lines and reinsert LFs, but it _must_ have\n>> some way of reconstructing the payload that the client signed in order\n>> to verify the signature. If we just naively insert LFs where missing,\n>> we lose the ability to verify the signature.\n>\n> I still do not understand this part.\n>\n> There is no way to \"naively\" insert, is there? You have an array of\n> lines (each of which you have already stripped its terminating LF at\n> its end). How else other than adding one LF at the end of each\n> element do you reconstruct the original multi-line string the client\n> signed? Are there other ways that makes the result ambiguous??\n\nI think I understand the confusion now. I think you and I are working\nfrom different assumptions about the client behavior.\n\nMy assumption was: the intended behavior for the client is to sign the\nexact payload that was sent over the wire, minus pkt-line headers.\n\nFor example, under my assumption, if the client sent:\n\n0008foo\\n\n0007bar\n0008baz\\n\n\nthen this indicates the client signed:\n\n\"foo\\nbarbaz\\n\"\n\nUnder this assumption, \"naively inserting LF\" means inserting an LF\nafter \"bar\". Thus the server would record the following in a\npersistent store:\n\n\"foo\\nbar\\nbaz\\n\"\n\nIf we only store this string, and do not remember the fact that the\nclient originally omitted one of those LFs, then when we go back to\nverify that signature later, it will fail.\n\nThat was my assumption.\n\nYour assumption, IIUC, is that the payload the client signed MUST have\ncontained LFs in between each line. When framing the content for the\nwire, the client MUST send one \"logical line\", which has no trailing\nLF, per pkt-line, and furthermore the pkt-line content MAY contain an\nadditional trailing LF.\n\nUnder your assumption, the server always has enough information to\nreconstruct the original signed payload.\n\n\nThe problem with the documentation, then, is that the documentation\ndoes not say anything to indicate that the signed payload is anything\nother than what is on the wire.\n\nSo maybe this series should include an explicit description of the\nsinged payload outside of the context of a push. Then, in the push\nsection, we can describe the set of transformations that the client\nMUST perform (splitting on LF; adding pkt-line headers) and MAY\nperform (adding LFs).\n\n>> If we say the payload the client signs MUST have LFs only in certain\n>> places, then that gives the server enough information to reconstruct\n>> the payload and verify the signature.\n>>\n>> But if we say the signed payload MUST have LFs and the wire payload\n>> MAY have LFs, then now we have two completely different formats, only\n>> one of which is documented.\n>\n> I thought that was what I was saying. The wire protocol sends the\n> contents of each line (both what is signed and the signature) on a\n> separate packet. When I say \"contents of a line\", I do not include\n> the terminating LF as part of the line (iow, LF is not even\n> optional; the terminating LF is not considered as part of \"the\n> contents of a line\"). It becomes irrelevant that a pkt-line may or\n> may not have a trailing optional LF. If there is LF at the end of a\n> packet between \"push-cert\" and \"push-cert-end\" packets, that LF by\n> definition cannot be part of the \"contents of a line\" in a\n> certificate.\n>\n> It is just a pkt-line framing artifact you can and should remove if\n> you are doing a \"split to array, join with LF\" implementation to\n> recover the original string.\n>\n> And that is very much consistent with the way we send other things\n> with pkt-line protocol. Each packet up to the first flush is\n> expected to have and as ref advertisement.\n> The pkt-line framing may or may not add a trailing LF, but LF is not\n> part of . It is not even part of the payload; merely an\n> artifact of pkt-line framing.\n"},{"id":"265577","messageId":"CAD0k6qQW3TbgXDsc2Wzid8RNyugumUbSu4KTzO21euO3y_OWGw@mail.gmail.com","threadId":"39760","inReplyTo":"CAD0k6qT8=xQb6MRcLkyvZBm0MRdQ0Z-8ojqghovdgeJQ2EBNEA@mail.gmail.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-06T17:18:02Z","receivedAt":"2015-07-06T17:18:02Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Jul 6, 2015 at 1:11 PM, Dave Borowitz wrote:\n> On Mon, Jul 6, 2015 at 12:57 PM, Junio C Hamano wrote:\n>> Dave Borowitz writes:\n>>\n>>> The server can munge pkt-lines and reinsert LFs, but it _must_ have\n>>> some way of reconstructing the payload that the client signed in order\n>>> to verify the signature. If we just naively insert LFs where missing,\n>>> we lose the ability to verify the signature.\n>>\n>> I still do not understand this part.\n>>\n>> There is no way to \"naively\" insert, is there? You have an array of\n>> lines (each of which you have already stripped its terminating LF at\n>> its end). How else other than adding one LF at the end of each\n>> element do you reconstruct the original multi-line string the client\n>> signed? Are there other ways that makes the result ambiguous??\n>\n> I think I understand the confusion now. I think you and I are working\n> from different assumptions about the client behavior.\n>\n> My assumption was: the intended behavior for the client is to sign the\n> exact payload that was sent over the wire, minus pkt-line headers.\n>\n> For example, under my assumption, if the client sent:\n>\n> 0008foo\\n\n> 0007bar\n> 0008baz\\n\n>\n> then this indicates the client signed:\n>\n> \"foo\\nbarbaz\\n\"\n>\n> Under this assumption, \"naively inserting LF\" means inserting an LF\n> after \"bar\". Thus the server would record the following in a\n> persistent store:\n>\n> \"foo\\nbar\\nbaz\\n\"\n>\n> If we only store this string, and do not remember the fact that the\n> client originally omitted one of those LFs, then when we go back to\n> verify that signature later, it will fail.\n>\n> That was my assumption.\n>\n> Your assumption, IIUC, is that the payload the client signed MUST have\n> contained LFs in between each line. When framing the content for the\n> wire, the client MUST send one \"logical line\", which has no trailing\n> LF, per pkt-line, and furthermore the pkt-line content MAY contain an\n> additional trailing LF.\n>\n> Under your assumption, the server always has enough information to\n> reconstruct the original signed payload.\n>\n>\n> The problem with the documentation, then, is that the documentation\n> does not say anything to indicate that the signed payload is anything\n> other than what is on the wire.\n\nAnother way of looking at the problem with my assumptions is, I was\nassuming \"pkt-line framing\" was the same thing as \"pkt-line header\".\nYou seem to be saying the definition of \"pkt-line framing\" is \"header,\nand optional trailing newline\".\n\nA quick scan of pack-protocol.txt did not turn up anything one way or\nthe other on this issue, so perhaps we could make it more explicit.\nThe additional upside here is that we could then potentially remove\nall or almost all LFs from this document.\n\n> So maybe this series should include an explicit description of the\n> singed payload outside of the context of a push. Then, in the push\n> section, we can describe the set of transformations that the client\n> MUST perform (splitting on LF; adding pkt-line headers) and MAY\n> perform (adding LFs).\n>\n>>> If we say the payload the client signs MUST have LFs only in certain\n>>> places, then that gives the server enough information to reconstruct\n>>> the payload and verify the signature.\n>>>\n>>> But if we say the signed payload MUST have LFs and the wire payload\n>>> MAY have LFs, then now we have two completely different formats, only\n>>> one of which is documented.\n>>\n>> I thought that was what I was saying. The wire protocol sends the\n>> contents of each line (both what is signed and the signature) on a\n>> separate packet. When I say \"contents of a line\", I do not include\n>> the terminating LF as part of the line (iow, LF is not even\n>> optional; the terminating LF is not considered as part of \"the\n>> contents of a line\"). It becomes irrelevant that a pkt-line may or\n>> may not have a trailing optional LF. If there is LF at the end of a\n>> packet between \"push-cert\" and \"push-cert-end\" packets, that LF by\n>> definition cannot be part of the \"contents of a line\" in a\n>> certificate.\n>>\n>> It is just a pkt-line framing artifact you can and should remove if\n>> you are doing a \"split to array, join with LF\" implementation to\n>> recover the original string.\n>>\n>> And that is very much consistent with the way we send other things\n>> with pkt-line protocol. Each packet up to the first flush is\n>> expected to have and as ref advertisement.\n>> The pkt-line framing may or may not add a trailing LF, but LF is not\n>> part of . It is not even part of the payload; merely an\n>> artifact of pkt-line framing.\n"},{"id":"265579","messageId":"xmqqwpyd19dy.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"CAD0k6qT8=xQb6MRcLkyvZBm0MRdQ0Z-8ojqghovdgeJQ2EBNEA@mail.gmail.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-06T17:30:33Z","receivedAt":"2015-07-06T17:30:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz writes:\n\n> I think I understand the confusion now. I think you and I are working\n> from different assumptions about the client behavior.\n\nI agree that we now both understand where we come from ;-)\n\nAnd sorry for not being clear when I did the \"push-cert\" originally\nin the documentation. As I already said, \"packets between push-cert\nand push-cert-end are contents of individual lines of the GPG signed\npush certificate\" was the design meant from day one, and a85b377d\n(push: the beginning of \"git push --signed\", 2014-09-12) could have\nmade it clearer.\n\n> The problem with the documentation, then, is that the documentation\n> does not say anything to indicate that the signed payload is anything\n> other than what is on the wire.\n\nYeah, that was untold assumption, as I considered \"what is on the\nwire\" to include pkt-line framing when I wrote a85b377d (push: the\nbeginning of \"git push --signed\", 2014-09-12).\n\n> So maybe this series should include an explicit description of the\n> singed payload outside of the context of a push. Then, in the push\n> section, we can describe the set of transformations that the client\n> MUST perform (splitting on LF; adding pkt-line headers) and MAY\n> perform (adding LFs).\n\nYes, and the latter is not limited to push-cert but anything sent on\npkt-line.\n\nThat incidentally is the only point I deeply care about. I just\nwant to minimize \"the protocol is this way in general, but only for\nthis one you must do it differently\".\n\nOne example of \"only for this one you must do it differently\" is\nanother caveat for protocol implementors for the sending side (again\nnot limited to \"push cert\"). \n\nThat existing implementations of the receivers treat an empty packet\n(i.e. \"0004\") as if it is the same as a flush packet (i.e. \"0000\"),\nso even if the sending side chooses to ignore the \"SHOULD terminate\neach non-flush line using LF\", it is strongly advised not to do so\nwhen it wants to send an empty payload. This needs to be documented.\n\nThe receiving end SHOULD NOT treat \"0004\" the same way as \"0000\".\nI think that must be documented and implementations (including our\nown) should be fixed.\n\nThanks.\n"},{"id":"265604","messageId":"xmqqsi91197o.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"CAD0k6qQW3TbgXDsc2Wzid8RNyugumUbSu4KTzO21euO3y_OWGw@mail.gmail.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-06T17:34:19Z","receivedAt":"2015-07-06T17:34:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz writes:\n\n> Another way of looking at the problem with my assumptions is, I was\n> assuming \"pkt-line framing\" was the same thing as \"pkt-line header\".\n> You seem to be saying the definition of \"pkt-line framing\" is \"header,\n> and optional trailing newline\".\n\nYes. I thought that was what \"Server SHOULD terminate with LF;\nclient MUST NOT require it\" in the existing text meant.\n\nAh, that reminds me of one thing I already said elsewhere. We need\nto correct the above with s/Server/Sender/; s/Client/Receiver/; I\nthink.\n"},{"id":"265605","messageId":"CAD0k6qQhER7cDsSG21CnnMxZE+B1BbQh1AkcAgiS3Jpm6WEMcQ@mail.gmail.com","threadId":"39760","inReplyTo":"xmqqwpyd19dy.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-06T17:35:39Z","receivedAt":"2015-07-06T17:35:39Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Jul 6, 2015 at 1:30 PM, Junio C Hamano wrote:\n> Dave Borowitz writes:\n>\n>> I think I understand the confusion now. I think you and I are working\n>> from different assumptions about the client behavior.\n>\n> I agree that we now both understand where we come from ;-)\n>\n> And sorry for not being clear when I did the \"push-cert\" originally\n> in the documentation. As I already said, \"packets between push-cert\n> and push-cert-end are contents of individual lines of the GPG signed\n> push certificate\"\n\nThis sentence makes sense to me now, but only because we now agree\nthat \"contents\" does not include the LF. Different people may have\ndifferent initial assumptions about whether the \"contents\" of a line\nincludes the trailing newline or not.\n\n> was the design meant from day one, and a85b377d\n> (push: the beginning of \"git push --signed\", 2014-09-12) could have\n> made it clearer.\n>\n>> The problem with the documentation, then, is that the documentation\n>> does not say anything to indicate that the signed payload is anything\n>> other than what is on the wire.\n>\n> Yeah, that was untold assumption, as I considered \"what is on the\n> wire\" to include pkt-line framing when I wrote a85b377d (push: the\n> beginning of \"git push --signed\", 2014-09-12).\n>\n>> So maybe this series should include an explicit description of the\n>> singed payload outside of the context of a push. Then, in the push\n>> section, we can describe the set of transformations that the client\n>> MUST perform (splitting on LF; adding pkt-line headers) and MAY\n>> perform (adding LFs).\n>\n> Yes, and the latter is not limited to push-cert but anything sent on\n> pkt-line.\n>\n> That incidentally is the only point I deeply care about. I just\n> want to minimize \"the protocol is this way in general, but only for\n> this one you must do it differently\".\n\nUnderstood, and I'm glad we have finally come to an arrangement that\nis both consistent and easy to implement on the server side.\n\n> One example of \"only for this one you must do it differently\" is\n> another caveat for protocol implementors for the sending side (again\n> not limited to \"push cert\").\n>\n> That existing implementations of the receivers treat an empty packet\n> (i.e. \"0004\")\n\nor \"0005\\n\" ;)\n\n> as if it is the same as a flush packet (i.e. \"0000\"),\n> so even if the sending side chooses to ignore the \"SHOULD terminate\n> each non-flush line using LF\", it is strongly advised not to do so\n> when it wants to send an empty payload. This needs to be documented.\n>\n> The receiving end SHOULD NOT treat \"0004\" the same way as \"0000\".\n> I think that must be documented and implementations (including our\n> own) should be fixed.\n\nAgreed.\n\n> Thanks.\n"},{"id":"265607","messageId":"CAD0k6qRGQyFxZ8+yqkzYff_k4ZjWPaegQbBphwXyfBtUOCCw6g@mail.gmail.com","threadId":"39760","inReplyTo":"xmqqsi91197o.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-06T17:38:32Z","receivedAt":"2015-07-06T17:38:32Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Jul 6, 2015 at 1:34 PM, Junio C Hamano wrote:\n> Dave Borowitz writes:\n>\n>> Another way of looking at the problem with my assumptions is, I was\n>> assuming \"pkt-line framing\" was the same thing as \"pkt-line header\".\n>> You seem to be saying the definition of \"pkt-line framing\" is \"header,\n>> and optional trailing newline\".\n>\n> Yes. I thought that was what \"Server SHOULD terminate with LF;\n> client MUST NOT require it\" in the existing text meant.\n\nUnfortunately, the existing text is littered with examples of\n\"PKT-LINE(foo SP bar LF)\". If we assume \"PKT-LINE(...)\" means \"apply\npkt-line framing to the [...]\", then this strongly implies that\n\"pkt-line framing\" does _not_ include the trailing LF. (Or the logical\nbut bizarre alternative reading that such an example might have _two_\ntrailing LFs :)\n\n> Ah, that reminds me of one thing I already said elsewhere. We need\n> to correct the above with s/Server/Sender/; s/Client/Receiver/; I\n> think.\n"},{"id":"265610","messageId":"xmqqbnfp180x.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"CAD0k6qQhER7cDsSG21CnnMxZE+B1BbQh1AkcAgiS3Jpm6WEMcQ@mail.gmail.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-06T17:59:58Z","receivedAt":"2015-07-06T17:59:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz writes:\n\n>> That existing implementations of the receivers treat an empty packet\n>> (i.e. \"0004\")\n>\n> or \"0005\\n\" ;)\n\nIs that true? I think\n\n\tlen = pkt_line();\n if (!len)\n \tbreak; /* flush */\n\nwould give you len == 1 and would not confuse it with a flush.\n"},{"id":"265611","messageId":"xmqq7fqd17qn.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"CAD0k6qRGQyFxZ8+yqkzYff_k4ZjWPaegQbBphwXyfBtUOCCw6g@mail.gmail.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-06T18:06:08Z","receivedAt":"2015-07-06T18:06:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz writes:\n\n> On Mon, Jul 6, 2015 at 1:34 PM, Junio C Hamano wrote:\n>> Dave Borowitz writes:\n>>\n>>> Another way of looking at the problem with my assumptions is, I was\n>>> assuming \"pkt-line framing\" was the same thing as \"pkt-line header\".\n>>> You seem to be saying the definition of \"pkt-line framing\" is \"header,\n>>> and optional trailing newline\".\n>>\n>> Yes. I thought that was what \"Server SHOULD terminate with LF;\n>> client MUST NOT require it\" in the existing text meant.\n>\n> Unfortunately, the existing text is littered with examples of\n> \"PKT-LINE(foo SP bar LF)\". If we assume \"PKT-LINE(...)\" means \"apply\n> pkt-line framing to the [...]\", then this strongly implies that\n> \"pkt-line framing\" does _not_ include the trailing LF. (Or the logical\n> but bizarre alternative reading that such an example might have _two_\n> trailing LFs :)\n\nYes, But I never viewed PKT-LINE() as an element that strictly\ndefines the grammar of the packet protocol ;-)\n\nBy clarifying that \"sender SHOULD terminate with LF, receiver MUST\nNOT require it\" is the rule (and fixing the existing implementations\nat places where they violate the \"MUST NOT\" part, which I think are\nvery small number of places), I think we can drop these LF (or LF?\nfor that matter) from all of the PKT-LINE() in the construction in\nthe pack-protocol.txt, which would be a very good thing to do.\n\nThe example in your sentence will become PKT-LINE(foo SP bar) and\nthe \"there may be an LF at the end\" would only be at one place, as a\npart of the definition of PKT-LINE().\n"},{"id":"265612","messageId":"CAD0k6qQH1t2QzjQydjqzse+caib4Z+yCtJd9eDy=hBukxLKMhQ@mail.gmail.com","threadId":"39760","inReplyTo":"xmqq7fqd17qn.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-07-06T18:08:21Z","receivedAt":"2015-07-06T18:08:21Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Jul 6, 2015 at 2:06 PM, Junio C Hamano wrote:\n> Dave Borowitz writes:\n>\n>> On Mon, Jul 6, 2015 at 1:34 PM, Junio C Hamano wrote:\n>>> Dave Borowitz writes:\n>>>\n>>>> Another way of looking at the problem with my assumptions is, I was\n>>>> assuming \"pkt-line framing\" was the same thing as \"pkt-line header\".\n>>>> You seem to be saying the definition of \"pkt-line framing\" is \"header,\n>>>> and optional trailing newline\".\n>>>\n>>> Yes. I thought that was what \"Server SHOULD terminate with LF;\n>>> client MUST NOT require it\" in the existing text meant.\n>>\n>> Unfortunately, the existing text is littered with examples of\n>> \"PKT-LINE(foo SP bar LF)\". If we assume \"PKT-LINE(...)\" means \"apply\n>> pkt-line framing to the [...]\", then this strongly implies that\n>> \"pkt-line framing\" does _not_ include the trailing LF. (Or the logical\n>> but bizarre alternative reading that such an example might have _two_\n>> trailing LFs :)\n>\n> Yes, But I never viewed PKT-LINE() as an element that strictly\n> defines the grammar of the packet protocol ;-)\n>\n> By clarifying that \"sender SHOULD terminate with LF, receiver MUST\n> NOT require it\" is the rule (and fixing the existing implementations\n> at places where they violate the \"MUST NOT\" part, which I think are\n> very small number of places), I think we can drop these LF (or LF?\n> for that matter) from all of the PKT-LINE() in the construction in\n> the pack-protocol.txt, which would be a very good thing to do.\n\nCompletely agree, and that is what I meant when I said \"The additional\nupside [to explicitly defining pkt-line framing in this way] is that\nwe could then potentially remove all or almost all LFs from this\ndocument.\"\n\n> The example in your sentence will become PKT-LINE(foo SP bar) and\n> the \"there may be an LF at the end\" would only be at one place, as a\n> part of the definition of PKT-LINE().\n"},{"id":"265613","messageId":"xmqq381116xp.fsf@gitster.dls.corp.google.com","threadId":"39760","inReplyTo":"xmqq7fqd17qn.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-06T18:23:30Z","receivedAt":"2015-07-06T18:23:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> By clarifying that \"sender SHOULD terminate with LF, receiver MUST\n> NOT require it\" is the rule (and fixing the existing implementations\n> at places where they violate the \"MUST NOT\" part, which I think are\n> very small number of places), I think we can drop these LF (or LF?\n> for that matter) from all of the PKT-LINE() in the construction in\n> the pack-protocol.txt, which would be a very good thing to do.\n>\n> The example in your sentence will become PKT-LINE(foo SP bar) and\n> the \"there may be an LF at the end\" would only be at one place, as a\n> part of the definition of PKT-LINE().\n\nI quickly scanned both the sources where we use packet_write() in\nthe code and say PKT-LINE in the doc; aside from the actual packfile\ntransfer that happens on the sideband, which technically _is_ a user\nof PKT-LINE, we do not send anything that does not end with a text\nin PKT-LINE. I just wanted to make sure that \"there may or may not\nbe an LF at the end; if there is, it is not part of the payload but\nis part of the framing\" does not invite new implementors to break\ntheir binary transfer by reading the definition of PKT-LINE too\nliterally to mean \"ok, so I stuffed this 998 byte binary gunk to the\npacket and insert an optional LF before sending the remainder in\nseparate packets\".\n\nThanks.\n"}]}