{"thread":{"id":"56796","subject":"[RFC PATCH 0/1] protocol-v2.txt: align delim-pkt spec with usage","startedAt":"2021-10-27T19:35:30Z","lastAt":"2021-11-11T22:53:06Z","messageCount":6,"participants":["Calvin Wan","Junio C Hamano","Nasser Grainawi"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"439817","messageId":"20211027193501.556540-1-calvinwan@google.com","threadId":"56796","inReplyTo":null,"subject":"[RFC PATCH 0/1] protocol-v2.txt: align delim-pkt spec with usage","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2021-10-27T19:35:00Z","receivedAt":"2021-10-27T19:35:30Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"In the grammar for a command the \"delim-pkt\" is optional, and a command could be followed by a \"flush-pkt\". On the other hand, JGit code is assuming that there is ALWAYS a DELIM package after command. I.e. messages valid following the grammar would fail in JGit. This is not causing troubles because all commands (ls-refs and fetch) have command-args. This problem arose when adding parsing for the \"capability-list\" So, bug here is align protocol grammar and code expectations. Given that a. most commands have args, and b. having delim-pkt between sections simplifies parsing, delim-pkt should be mandatory.\n\nCalvin Wan (1):\n\n Documentation/technical/protocol-v2.txt | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\n\nbase-commit: 0785eb769886ae81e346df10e88bc49ffc0ac64e\n-- \n2.33.0.664.g0785eb7698\n\n"},{"id":"439818","messageId":"20211027193501.556540-2-calvinwan@google.com","threadId":"56796","inReplyTo":"20211027193501.556540-1-calvinwan@google.com","subject":"[RFC PATCH 1/1] protocol-v2.txt: align delim-pkt spec with usage","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2021-10-27T19:35:01Z","receivedAt":"2021-10-27T19:35:41Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"move delim-pkt from command-args to command-request\n\nSigned-off-by: Calvin Wan \nReported-by: Ivan Frade \n---\n Documentation/technical/protocol-v2.txt | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex 21e8258ccf..1e75863680 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -125,11 +125,11 @@\n empty-request = flush-pkt\n command-request = command\n \t\t capability-list\n-\t\t [command-args]\n+\t\t\t delim-pkt\n+\t\t command-args\n \t\t flush-pkt\n command = PKT-LINE(\"command=\" key LF)\n- command-args = delim-pkt\n-\t\t *command-specific-arg\n+ command-args = *command-specific-arg\n \n command-specific-args are packet line framed arguments defined by\n each individual command.\n-- \n2.33.0.664.g0785eb7698\n\n"},{"id":"439837","messageId":"xmqq7ddyuori.fsf@gitster.g","threadId":"56796","inReplyTo":"20211027193501.556540-2-calvinwan@google.com","subject":"Re: [RFC PATCH 1/1] protocol-v2.txt: align delim-pkt spec with usage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-27T21:44:01Z","receivedAt":"2021-10-27T21:44:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calvin Wan writes:\n\n> move delim-pkt from command-args to command-request\n\nThat is something we can read from the patch text. Can we explain\nwhat it means to the human readers? Let's read on the patch.\n\n> Signed-off-by: Calvin Wan \n> Reported-by: Ivan Frade \n\nThese are reversed, aren't they? Chronologically what happened?\nIvan reported, you wrote a patch, and then you signed it off before\nsending, no? Please align the order of the events with the order of\nthe trailer lines.\n\n> ---\n> Documentation/technical/protocol-v2.txt | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\n> index 21e8258ccf..1e75863680 100644\n> --- a/Documentation/technical/protocol-v2.txt\n> +++ b/Documentation/technical/protocol-v2.txt\n> @@ -125,11 +125,11 @@\n> empty-request = flush-pkt\n> command-request = command\n> \t\t capability-list\n> -\t\t [command-args]\n> +\t\t\t delim-pkt\n> +\t\t command-args\n\nAre these meant to be unaligned like this? In this project, a HT\nalways jumps to the next multiple-of-8 column.\n\n> \t\t flush-pkt\n> command = PKT-LINE(\"command=\" key LF)\n> - command-args = delim-pkt\n> -\t\t *command-specific-arg\n> + command-args = *command-specific-arg\n\nOK, so we used to say, after the capability list, there may be\na command-args, or no command-args. And a command-args begins\nwith a delim-pkt followed by zero or more command specific args.\n\nWe now say after the capability list, there always is a delim-pkt\nand zero or more command specific args will follow.\n\nSo, the difference (fix) is that the original production allowed a\ncommand-request that ends with capability list (without a final\ndelim-pkt), but we require delim-pkt after capability-list, whether\nthere is any command-specific-args?\n\nIf so, perhaps the human-readable rewrite of the proposed log\nmessage would be\n\n The current protocol EBNF allows command-request to end with the\n capability list, if no command specific arguments follow, but\n the protocol requires that after the capability list, there must\n be a delim-pkt regardless of the number of command speficic\n arguments. Fix the EBNF to match.\n\nBy the way, what are we matching this document to with this change?\nOur over-the-wire protocol implementation? I am asking because it\nwould mean that we may be retroactigvely changing the rules under\nother implementations, making what used to be allowed now invalid.\n\n"},{"id":"440938","messageId":"20211111220048.1702896-1-calvinwan@google.com","threadId":"56796","inReplyTo":"20211027193501.556540-1-calvinwan@google.com","subject":"[PATCH v2] protocol-v2.txt: align delim-pkt spec with usage","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2021-11-11T22:00:48Z","receivedAt":"2021-11-11T22:00:55Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"The current protocol EBNF allows command-request to end with the\ncapability list, if no command specific arguments follow, but the\nprotocol requires that after the capability list, there must be a\ndelim-pkt regardless of the number of command specific arguments. Fixed\nthe EBNF to match. Both JGit and libgit2's implementation has the\ndelim-pkt as mandatory. JGit's code is not publicly linkable, but\nlibgit2 is linked below[1]. As for currently implemented commands on v2\n(ls-ref and fetch), the delim packet is already being passed through\n\n[1]: https://github.com/libgit2/libgit2/blob/main/src/transports/git.c\n\nReported-by: Ivan Frade \nSigned-off-by: Calvin Wan \n---\n Documentation/technical/protocol-v2.txt | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex 21e8258ccf..8a877d27e2 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -125,11 +125,11 @@ command can be requested at a time.\n empty-request = flush-pkt\n command-request = command\n \t\t capability-list\n-\t\t [command-args]\n+\t\t delim-pkt\n+\t\t command-args\n \t\t flush-pkt\n command = PKT-LINE(\"command=\" key LF)\n- command-args = delim-pkt\n-\t\t *command-specific-arg\n+ command-args = *command-specific-arg\n \n command-specific-args are packet line framed arguments defined by\n each individual command.\n\nbase-commit: e9e5ba39a78c8f5057262d49e261b42a8660d5b9\n-- \n2.33.0.664.g0785eb7698\n\n"},{"id":"440939","messageId":"CD28B0FE-B378-45C6-9DD9-FC79C84040B8@quicinc.com","threadId":"56796","inReplyTo":"20211111220048.1702896-1-calvinwan@google.com","subject":"Re: [PATCH v2] protocol-v2.txt: align delim-pkt spec with usage","fromName":"Nasser Grainawi","fromEmail":"quic_nasserg@quicinc.com","sentAt":"2021-11-11T22:09:39Z","receivedAt":"2021-11-11T22:09:44Z","isPatch":true,"sender":{"key":"quic_nasserg@quicinc.com","avatar":null},"body":"\n> On Nov 11, 2021, at 3:00 PM, Calvin Wan wrote:\n> \n> The current protocol EBNF allows command-request to end with the\n> capability list, if no command specific arguments follow, but the\n> protocol requires that after the capability list, there must be a\n> delim-pkt regardless of the number of command specific arguments. Fixed\n> the EBNF to match. Both JGit and libgit2's implementation has the\n> delim-pkt as mandatory. JGit's code is not publicly linkable, but\n\nWhy is JGit not linkable? Isn’t it https://git.eclipse.org/r/plugins/gitiles/jgit/jgit/+/refs/heads/master/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java ?"},{"id":"440944","messageId":"xmqqpmr6jodt.fsf@gitster.g","threadId":"56796","inReplyTo":"20211111220048.1702896-1-calvinwan@google.com","subject":"Re: [PATCH v2] protocol-v2.txt: align delim-pkt spec with usage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-11T22:53:02Z","receivedAt":"2021-11-11T22:53:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calvin Wan writes:\n\n> The current protocol EBNF allows command-request to end with the\n> capability list, if no command specific arguments follow, but the\n> protocol requires that after the capability list, there must be a\n> delim-pkt regardless of the number of command specific arguments. Fixed\n> the EBNF to match. Both JGit and libgit2's implementation has the\n> delim-pkt as mandatory. JGit's code is not publicly linkable, but\n> libgit2 is linked below[1]. As for currently implemented commands on v2\n> (ls-ref and fetch), the delim packet is already being passed through\n>\n> [1]: https://github.com/libgit2/libgit2/blob/main/src/transports/git.c\n\nThanks for an extra level of research. Very much appreciated.\n\nWill queue.\n\n>\n> Reported-by: Ivan Frade \n> Signed-off-by: Calvin Wan \n> ---\n> Documentation/technical/protocol-v2.txt | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\n> index 21e8258ccf..8a877d27e2 100644\n> --- a/Documentation/technical/protocol-v2.txt\n> +++ b/Documentation/technical/protocol-v2.txt\n> @@ -125,11 +125,11 @@ command can be requested at a time.\n> empty-request = flush-pkt\n> command-request = command\n> \t\t capability-list\n> -\t\t [command-args]\n> +\t\t delim-pkt\n> +\t\t command-args\n> \t\t flush-pkt\n> command = PKT-LINE(\"command=\" key LF)\n> - command-args = delim-pkt\n> -\t\t *command-specific-arg\n> + command-args = *command-specific-arg\n> \n> command-specific-args are packet line framed arguments defined by\n> each individual command.\n>\n> base-commit: e9e5ba39a78c8f5057262d49e261b42a8660d5b9\n"}]}