Re: [PATCH] pack-protocol: document newline behavior in push commands

[Jeff King]

On Thu, Sep 03, 2015 at 03:17:18PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Frankly, this feels a bit like a step backwards to me. I am tempted to
> > suggest instead that git start sending the newlines, but I'm not sure
> > it's worth any potential fallout.
> 
> I actually think we should do both in the longer term.
> 
> If we say sender "SHOULD" and we know no existing receiver violates
> the "MUST NOT reject", our sender should follow that "SHOULD".

Right, it was the second "we know..." that made me worry. It is really
"we assume". :) Whether it is right according to the spec or not, the
real world is sometimes more complicated. And given that there is no
real advantage to changing the sending behavior now, I didn't think it
worth doing.

> This documentation update is good in that it makes the examples
> easier to read (by the way, the first pre-context line ends with
> '\n', which we would eventually also address) by making the reader
> understand that the convention used in this S:/C: exchange
> illustration the optional LF is not shown.

In the second patch I left them all intact, but I agree that we could
drop the "\n" entirely from the example conversations, as it is implied
(and GIT_PACKET_TRACE, for example, does not even show it).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pack-protocol: document newline behavior in push commands

[Junio C Hamano]

Jeff King  writes:

> Right, it was the second "we know..." that made me worry. It is really
> "we assume". :) Whether it is right according to the spec or not, the
> real world is sometimes more complicated. And given that there is no
> real advantage to changing the sending behavior now, I didn't think it
> worth doing.

Agreed.

>>> This documentation update is good in that it makes the examples
>> easier to read (by the way, the first pre-context line ends with
>> '\n', which we would eventually also address) by making the reader
>> understand that the convention used in this S:/C: exchange
>> illustration the optional LF is not shown.
>
> In the second patch I left them all intact, but I agree that we could
> drop the "\n" entirely from the example conversations, as it is implied
> (and GIT_PACKET_TRACE, for example, does not even show it).

Sure.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] pack-protocol: document newline behavior in push commands

[Jeff King]

Our pack-protocol spec indicates that a pushing client
should send ref update commands like:

  $old_sha1 $new_sha1 $ref\n

with each ref update in its own pktline, with a trailing
newline. However, git itself does not follow this behavior;
it omits the trailing newline.

For the most part the distinction is harmless. The
`receive-pack` on the other end will call
`packet_read_line`, which strips off the newline if it is
there, and we are fine either way.

Things are more complicated for the initial ref, which also
has capabilities. The spec says to send:

  $old_sha1 $new_sha1 $ref\0 $caps\n

which is also OK by the current `receive-pack code (the
newline is at the end of the packet, so we still strip it).

As an aside, it would _not_ be OK to send:

  $old_sha1 $new_sha1 $ref\n\0 $caps\n

The spec does not allow that and receive-pack would reject
it, but it is perhaps a mistake that a naive client
implementation might make.

So what is in the current spec is quite reasonable, and
simply follows the normal rules for pkt-line framing (we say
LF, but it really is optional). But it does not document
how git actually behaves.

Signed-off-by: Jeff King 
---
+cc Junio, Dave, and Shawn, as this is somewhat related to the
discussion in
http://thread.gmane.org/gmane.comp.version-control.git/273175/focus=273444

I happened to be looking into some protocol stuff recently and noticed
that we do not follow the spec here (but interestingly, libgit2 does!).

Frankly, this feels a bit like a step backwards to me. I am tempted to
suggest instead that git start sending the newlines, but I'm not sure
it's worth any potential fallout.

I'm also tempted to scrap this and say it just falls under the rule
that every PKT-LINE is "sender SHOULD include LF" and "receiver MUST NOT
complain about missing LF" (which does appear earlier in the document,
though in a different context). Or maybe we should write out that rule
explicitly and drop the "LF" from all parts of the grammar (which is
what the thread above advocates).

 Documentation/technical/pack-protocol.txt | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt 
b/Documentation/technical/pack-protocol.txt
index 4064fc7..9ce53b4 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -469,8 +469,8 @@ references.
 
   shallow   =  PKT-LINE("shallow" SP obj-id LF)
 
-  command-list  =  PKT-LINE(command NUL capability-list LF)
-  *PKT-LINE(command LF)
+  command-list  =  PKT-LINE(command NUL capability-list)
+  *PKT-LINE(command)
   flush-pkt
 
   command   =  create / delete / update
@@ -586,8 +586,8 @@ An example client/server communication might look like this:
S: 003f74730d410fcb6603ace96f1dc55ea6196122532d refs/heads/team\n
S: 
 
-   C: 003e7d1665144a3a975c05f1f43902ddaf084e784dbe 
74730d410fcb6603ace96f1dc55ea6196122532d refs/heads/debug\n
-   C: 003e74730d410fcb6603ace96f1dc55ea6196122532d 
5a3f6be755bbb7deae50065988cbfa1ffa9ab68a refs/heads/master\n
+   C: 003e7d1665144a3a975c05f1f43902ddaf084e784dbe 
74730d410fcb6603ace96f1dc55ea6196122532d refs/heads/debug
+   C: 003e74730d410fcb6603ace96f1dc55ea6196122532d 
5a3f6be755bbb7deae50065988cbfa1ffa9ab68a refs/heads/master
C: 
C: [PACKDATA]
 
-- 
2.5.1.812.ge796bff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pack-protocol: document newline behavior in push commands

[Junio C Hamano]

Jeff King  writes:

> Frankly, this feels a bit like a step backwards to me. I am tempted to
> suggest instead that git start sending the newlines, but I'm not sure
> it's worth any potential fallout.

I actually think we should do both in the longer term.

If we say sender "SHOULD" and we know no existing receiver violates
the "MUST NOT reject", our sender should follow that "SHOULD".

This documentation update is good in that it makes the examples
easier to read (by the way, the first pre-context line ends with
'\n', which we would eventually also address) by making the reader
understand that the convention used in this S:/C: exchange
illustration the optional LF is not shown.

> I'm also tempted to scrap this and say it just falls under the rule
> that every PKT-LINE is "sender SHOULD include LF" and "receiver MUST NOT
> complain about missing LF" (which does appear earlier in the document,
> though in a different context). Or maybe we should write out that rule
> explicitly and drop the "LF" from all parts of the grammar (which is
> what the thread above advocates).

Hmm, I view this patch as a first step in that direction.

>
>  Documentation/technical/pack-protocol.txt | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/technical/pack-protocol.txt 
> b/Documentation/technical/pack-protocol.txt
> index 4064fc7..9ce53b4 100644
> --- a/Documentation/technical/pack-protocol.txt
> +++ b/Documentation/technical/pack-protocol.txt
> @@ -469,8 +469,8 @@ references.
>  
>shallow   =  PKT-LINE("shallow" SP obj-id LF)
>  
> -  command-list  =  PKT-LINE(command NUL capability-list LF)
> -*PKT-LINE(command LF)
> +  command-list  =  PKT-LINE(command NUL capability-list)
> +*PKT-LINE(command)
>  flush-pkt
>  
>command   =  create / delete / update
> @@ -586,8 +586,8 @@ An example client/server communication might look like 
> this:
> S: 003f74730d410fcb6603ace96f1dc55ea6196122532d refs/heads/team\n
> S: 
>  
> -   C: 003e7d1665144a3a975c05f1f43902ddaf084e784dbe 
> 74730d410fcb6603ace96f1dc55ea6196122532d refs/heads/debug\n
> -   C: 003e74730d410fcb6603ace96f1dc55ea6196122532d 
> 5a3f6be755bbb7deae50065988cbfa1ffa9ab68a refs/heads/master\n
> +   C: 003e7d1665144a3a975c05f1f43902ddaf084e784dbe 
> 74730d410fcb6603ace96f1dc55ea6196122532d refs/heads/debug
> +   C: 003e74730d410fcb6603ace96f1dc55ea6196122532d 
> 5a3f6be755bbb7deae50065988cbfa1ffa9ab68a refs/heads/master
> C: 
> C: [PACKDATA]
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html