* [RFC PATCH 0/1] protocol-v2.txt: align delim-pkt spec with usage @ 2021-10-27 19:35 Calvin Wan 2021-10-27 19:35 ` [RFC PATCH 1/1] " Calvin Wan 2021-11-11 22:00 ` [PATCH v2] " Calvin Wan 0 siblings, 2 replies; 6+ messages in thread From: Calvin Wan @ 2021-10-27 19:35 UTC (permalink / raw) To: git; +Cc: Calvin Wan 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. Calvin Wan (1): Documentation/technical/protocol-v2.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) base-commit: 0785eb769886ae81e346df10e88bc49ffc0ac64e -- 2.33.0.664.g0785eb7698 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC PATCH 1/1] protocol-v2.txt: align delim-pkt spec with usage 2021-10-27 19:35 [RFC PATCH 0/1] protocol-v2.txt: align delim-pkt spec with usage Calvin Wan @ 2021-10-27 19:35 ` Calvin Wan 2021-10-27 21:44 ` Junio C Hamano 2021-11-11 22:00 ` [PATCH v2] " Calvin Wan 1 sibling, 1 reply; 6+ messages in thread From: Calvin Wan @ 2021-10-27 19:35 UTC (permalink / raw) To: git; +Cc: Calvin Wan, Ivan Frade move delim-pkt from command-args to command-request Signed-off-by: Calvin Wan <calvinwan@google.com> Reported-by: Ivan Frade <ifrade@google.com> --- Documentation/technical/protocol-v2.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt index 21e8258ccf..1e75863680 100644 --- a/Documentation/technical/protocol-v2.txt +++ b/Documentation/technical/protocol-v2.txt @@ -125,11 +125,11 @@ empty-request = flush-pkt command-request = command capability-list - [command-args] + delim-pkt + command-args flush-pkt command = PKT-LINE("command=" key LF) - command-args = delim-pkt - *command-specific-arg + command-args = *command-specific-arg command-specific-args are packet line framed arguments defined by each individual command. -- 2.33.0.664.g0785eb7698 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 1/1] protocol-v2.txt: align delim-pkt spec with usage 2021-10-27 19:35 ` [RFC PATCH 1/1] " Calvin Wan @ 2021-10-27 21:44 ` Junio C Hamano 0 siblings, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2021-10-27 21:44 UTC (permalink / raw) To: Calvin Wan; +Cc: git, Ivan Frade Calvin Wan <calvinwan@google.com> writes: > move delim-pkt from command-args to command-request That is something we can read from the patch text. Can we explain what it means to the human readers? Let's read on the patch. > Signed-off-by: Calvin Wan <calvinwan@google.com> > Reported-by: Ivan Frade <ifrade@google.com> These are reversed, aren't they? Chronologically what happened? Ivan reported, you wrote a patch, and then you signed it off before sending, no? Please align the order of the events with the order of the trailer lines. > --- > Documentation/technical/protocol-v2.txt | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt > index 21e8258ccf..1e75863680 100644 > --- a/Documentation/technical/protocol-v2.txt > +++ b/Documentation/technical/protocol-v2.txt > @@ -125,11 +125,11 @@ > empty-request = flush-pkt > command-request = command > capability-list > - [command-args] > + delim-pkt > + command-args Are these meant to be unaligned like this? In this project, a HT always jumps to the next multiple-of-8 column. > flush-pkt > command = PKT-LINE("command=" key LF) > - command-args = delim-pkt > - *command-specific-arg > + command-args = *command-specific-arg OK, so we used to say, after the capability list, there may be a command-args, or no command-args. And a command-args begins with a delim-pkt followed by zero or more command specific args. We now say after the capability list, there always is a delim-pkt and zero or more command specific args will follow. So, the difference (fix) is that the original production allowed a command-request that ends with capability list (without a final delim-pkt), but we require delim-pkt after capability-list, whether there is any command-specific-args? If so, perhaps the human-readable rewrite of the proposed log message would be The current protocol EBNF allows command-request to end with the capability list, if no command specific arguments follow, but the protocol requires that after the capability list, there must be a delim-pkt regardless of the number of command speficic arguments. Fix the EBNF to match. By the way, what are we matching this document to with this change? Our over-the-wire protocol implementation? I am asking because it would mean that we may be retroactigvely changing the rules under other implementations, making what used to be allowed now invalid. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] protocol-v2.txt: align delim-pkt spec with usage 2021-10-27 19:35 [RFC PATCH 0/1] protocol-v2.txt: align delim-pkt spec with usage Calvin Wan 2021-10-27 19:35 ` [RFC PATCH 1/1] " Calvin Wan @ 2021-11-11 22:00 ` Calvin Wan 2021-11-11 22:09 ` Nasser Grainawi 2021-11-11 22:53 ` Junio C Hamano 1 sibling, 2 replies; 6+ messages in thread From: Calvin Wan @ 2021-11-11 22:00 UTC (permalink / raw) To: git; +Cc: gitster, Calvin Wan, Ivan Frade The current protocol EBNF allows command-request to end with the capability list, if no command specific arguments follow, but the protocol requires that after the capability list, there must be a delim-pkt regardless of the number of command specific arguments. Fixed the EBNF to match. Both JGit and libgit2's implementation has the delim-pkt as mandatory. JGit's code is not publicly linkable, but libgit2 is linked below[1]. As for currently implemented commands on v2 (ls-ref and fetch), the delim packet is already being passed through [1]: https://github.com/libgit2/libgit2/blob/main/src/transports/git.c Reported-by: Ivan Frade <ifrade@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> --- Documentation/technical/protocol-v2.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt index 21e8258ccf..8a877d27e2 100644 --- a/Documentation/technical/protocol-v2.txt +++ b/Documentation/technical/protocol-v2.txt @@ -125,11 +125,11 @@ command can be requested at a time. empty-request = flush-pkt command-request = command capability-list - [command-args] + delim-pkt + command-args flush-pkt command = PKT-LINE("command=" key LF) - command-args = delim-pkt - *command-specific-arg + command-args = *command-specific-arg command-specific-args are packet line framed arguments defined by each individual command. base-commit: e9e5ba39a78c8f5057262d49e261b42a8660d5b9 -- 2.33.0.664.g0785eb7698 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] protocol-v2.txt: align delim-pkt spec with usage 2021-11-11 22:00 ` [PATCH v2] " Calvin Wan @ 2021-11-11 22:09 ` Nasser Grainawi 2021-11-11 22:53 ` Junio C Hamano 1 sibling, 0 replies; 6+ messages in thread From: Nasser Grainawi @ 2021-11-11 22:09 UTC (permalink / raw) To: Calvin Wan; +Cc: git, gitster, Ivan Frade > On Nov 11, 2021, at 3:00 PM, Calvin Wan <calvinwan@google.com> wrote: > > The current protocol EBNF allows command-request to end with the > capability list, if no command specific arguments follow, but the > protocol requires that after the capability list, there must be a > delim-pkt regardless of the number of command specific arguments. Fixed > the EBNF to match. Both JGit and libgit2's implementation has the > delim-pkt as mandatory. JGit's code is not publicly linkable, but Why 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 ? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] protocol-v2.txt: align delim-pkt spec with usage 2021-11-11 22:00 ` [PATCH v2] " Calvin Wan 2021-11-11 22:09 ` Nasser Grainawi @ 2021-11-11 22:53 ` Junio C Hamano 1 sibling, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2021-11-11 22:53 UTC (permalink / raw) To: Calvin Wan; +Cc: git, Ivan Frade Calvin Wan <calvinwan@google.com> writes: > The current protocol EBNF allows command-request to end with the > capability list, if no command specific arguments follow, but the > protocol requires that after the capability list, there must be a > delim-pkt regardless of the number of command specific arguments. Fixed > the EBNF to match. Both JGit and libgit2's implementation has the > delim-pkt as mandatory. JGit's code is not publicly linkable, but > libgit2 is linked below[1]. As for currently implemented commands on v2 > (ls-ref and fetch), the delim packet is already being passed through > > [1]: https://github.com/libgit2/libgit2/blob/main/src/transports/git.c Thanks for an extra level of research. Very much appreciated. Will queue. > > Reported-by: Ivan Frade <ifrade@google.com> > Signed-off-by: Calvin Wan <calvinwan@google.com> > --- > Documentation/technical/protocol-v2.txt | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt > index 21e8258ccf..8a877d27e2 100644 > --- a/Documentation/technical/protocol-v2.txt > +++ b/Documentation/technical/protocol-v2.txt > @@ -125,11 +125,11 @@ command can be requested at a time. > empty-request = flush-pkt > command-request = command > capability-list > - [command-args] > + delim-pkt > + command-args > flush-pkt > command = PKT-LINE("command=" key LF) > - command-args = delim-pkt > - *command-specific-arg > + command-args = *command-specific-arg > > command-specific-args are packet line framed arguments defined by > each individual command. > > base-commit: e9e5ba39a78c8f5057262d49e261b42a8660d5b9 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-11 22:53 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-10-27 19:35 [RFC PATCH 0/1] protocol-v2.txt: align delim-pkt spec with usage Calvin Wan 2021-10-27 19:35 ` [RFC PATCH 1/1] " Calvin Wan 2021-10-27 21:44 ` Junio C Hamano 2021-11-11 22:00 ` [PATCH v2] " Calvin Wan 2021-11-11 22:09 ` Nasser Grainawi 2021-11-11 22:53 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).