diff mbox series

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

Message ID 20211027193501.556540-2-calvinwan@google.com (mailing list archive)
State Superseded
Headers show
Series protocol-v2.txt: align delim-pkt spec with usage | expand

Commit Message

Calvin Wan Oct. 27, 2021, 7:35 p.m. UTC
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(-)

Comments

Junio C Hamano Oct. 27, 2021, 9:44 p.m. UTC | #1
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.
diff mbox series

Patch

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.