threads / rfc / 56796

RFC patchprotocol-v2.txt: align delim-pkt spec with usage

Subject: [RFC PATCH 0/1] protocol-v2.txt: align delim-pkt spec with usage

## tl;dr

6 messages between Oct 27, 2021 and Nov 11, 2021. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Calvin Wan· Oct 27, 2021, 19:35 UTC · lore
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
Calvin Wan· Oct 27, 2021, 19:35 UTC · re: Calvin Wan · lore

[RFC PATCH 1/1] protocol-v2.txt: align delim-pkt spec with usage

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(-)
Show changes to Documentation/technical/protocol-v2.txt +3 −3
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
Junio C Hamano· Oct 27, 2021, 21:44 UTC · re: Calvin Wan · lore

Re: [RFC PATCH 1/1] protocol-v2.txt: align delim-pkt spec with usage

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.

Show 15 quoted 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.

Show 5 quoted lines
>  		      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.

Calvin Wan· Nov 11, 2021, 22:00 UTC · re: Calvin Wan · lore

[PATCH v2] protocol-v2.txt: align delim-pkt spec with usage

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(-)
Show changes to Documentation/technical/protocol-v2.txt +3 −3
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
Nasser Grainawi· Nov 11, 2021, 22:09 UTC · re: Calvin Wan · lore

Re: [PATCH v2] protocol-v2.txt: align delim-pkt spec with usage

Show 8 quoted lines
> 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 ?
Junio C Hamano· Nov 11, 2021, 22:53 UTC · re: Calvin Wan · lore

Re: [PATCH v2] protocol-v2.txt: align delim-pkt spec with usage

Calvin Wan <calvinwan@google.com> writes:
Show 10 quoted lines
> 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.
Show 28 quoted lines
>
> 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

← back to recent threads