Re: bug in HTTP protocol spec

[Dorian Taylor]

> On Feb 22, 2018, at 12:02 PM, Junio C Hamano  wrote:
> 
> I saw somewhere "Apple-Mail" and a phrase "repaste".  So perhaps
> copy on the client is involved in the whitespace damage (of
> course the original could be broken, but I somehow doubt it).

https://doriantaylor.com/file/well-ill-be-damned.png

> For what it's worth, I am slightly negative on this addition.  It
> makes it inconsistent if we only mention it about _this_ flush and
> not any other flush.  It even gets in the way of learning the
> protocol by new people reading it, by giving an impression that
> somehow LF is outside and in between packet line.
> 
> Thanks.

This patch exists because I was asked to write it. I don’t know squat about 
this protocol other than the fact that I followed the spec and it didn’t work. 
I traced a known-good protocol endpoint and found it contained content that 
didn’t agree with the spec. I then obliged the request to submit a patch with 
*what I knew to be true* about the sample that actually worked. I then followed 
the recommendations *advertised on GitHub* for submitting the patch.

You’re welcome.

--
Dorian Taylor
Make things. Make sense.
https://doriantaylor.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: bug in HTTP protocol spec

[Junio C Hamano]

Jeff King  writes:

> This indentation is funny. But I suspect it is because your whole patch
> seems to have been whitespace-damaged (see the section on gmail in
> "git help git-format-patch").

I saw somewhere "Apple-Mail" and a phrase "repaste".  So perhaps
copy on the client is involved in the whitespace damage (of
course the original could be broken, but I somehow doubt it).

>> The client may send Extra Parameters (see
>> Documentation/technical/pack-protocol.txt) as a colon-separated string
>> -in the Git-Protocol HTTP header.
>> +in the Git-Protocol HTTP header. Note as well that there is *no* newline
>> +after the ``.
>
> I guess I'm not opposed to calling that out, but this is normal for
> pktline (the flush packet has no data; in the other lines the newline is
> not a syntactic part of the pktline stream, but is actually data
> contained inside each of those pktlines).

For what it's worth, I am slightly negative on this addition.  It
makes it inconsistent if we only mention it about _this_ flush and
not any other flush.  It even gets in the way of learning the
protocol by new people reading it, by giving an impression that
somehow LF is outside and in between packet line.

Thanks.

Re: bug in HTTP protocol spec

[Brandon Williams]

On 02/21, Dorian Taylor wrote:
> 
> > On Feb 21, 2018, at 9:37 PM, Jonathan Nieder  wrote:
> > 
> > Thanks for writing it.
> > 
> > Do you mind if we forge your sign-off?  (See Documentation/SubmittingPatches
> > item '(5) Certify your work' for details about what this means.)
> 
> Sure, or I can just re-paste:
> 
> Signed-off-by: Dorian Taylor 
> 
> ---
> Documentation/technical/http-protocol.txt | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/technical/http-protocol.txt 
> b/Documentation/technical/http-protocol.txt
> index a0e45f2889e6e..19d73f7efb338 100644
> --- a/Documentation/technical/http-protocol.txt
> +++ b/Documentation/technical/http-protocol.txt
> @@ -214,14 +214,17 @@ smart server reply:
>   S: Cache-Control: no-cache
>   S:
>   S: 001e# service=git-upload-pack\n
> +   S: 
>   S: 004895dcfa3633004da0049d3d0fa03f80589cbcaf31 
> refs/heads/maint\0multi_ack\n
>   S: 0042d049f6c27a2244e12041955e262a404c7faba355 refs/heads/master\n
>   S: 003c2cb58b79488a98d2721cea644875a8dd0026b115 refs/tags/v1.0\n
>   S: 003fa3c2e2402b99163d1d59756e5f207ae21cccba4c refs/tags/v1.0^{}\n
> +   S: 
> 
> The client may send Extra Parameters (see
> Documentation/technical/pack-protocol.txt) as a colon-separated string
> -in the Git-Protocol HTTP header.
> +in the Git-Protocol HTTP header. Note as well that there is *no* newline
> +after the ``.
> 
> Dumb Server Response
> 
> @@ -264,8 +267,8 @@ Servers MUST set $servicename to be the request parameter 
> value.
> Servers SHOULD include an LF at the end of this line.
> Clients MUST ignore an LF at the end of the line.
> 
> -Servers MUST terminate the response with the magic `` end
> -pkt-line marker.
> +Servers MUST follow the first pkt-line, as well as terminate the
> +response, with the magic `` end pkt-line marker.
> 
> The returned response is a pkt-line stream describing each ref and
> its known value.  The stream SHOULD be sorted by name according to
> @@ -278,6 +281,7 @@ Extra Parameter.
> 
>  smart_reply =  PKT-LINE("# service=$servicename" LF)
>*1("version 1")
> +  ""
>ref_list
>""
>  ref_list=  empty_list / non_empty_list
> 
> ---
> 
> > 
> >> Note I am not sure what the story is behind that `version 1`
> >> element, whether it's supposed to go before or after the null packet
> >> or if there should be another null packet or what. Perhaps somebody
> >> more fluent with the smart protocol can advise.
> > 
> > I believe the 'version 1' goes after the flush-packet.
> 
> I took a traipse through the code and couldn’t determine it one way or 
> another, but my money is on that looking something like `000aversion 1\n` on 
> the wire.

Yes the version string goes along with the ref_list in v1 like so:

  # service=
  
  version 1
  ref_list
  

This is because it is part of the payload which is actually delivered to
the git fetch/push binary where as the "# service" bit is used by the
remote helper to identify smart vs not smart servers.

> 
> --
> Dorian Taylor
> Make things. Make sense.
> https://doriantaylor.com
> 



-- 
Brandon Williams

Re: bug in HTTP protocol spec

[Dorian Taylor]

> On Feb 22, 2018, at 2:08 AM, Jeff King  wrote:
> 
> 
> This indentation is funny. But I suspect it is because your whole patch
> seems to have been whitespace-damaged (see the section on gmail in
> "git help git-format-patch").

That is, bit-for-bit, what came out of that submitGit thing that is advertised 
when you try to open a pull request on the git repository on Github. I was a 
bit surprised myself.

--
Dorian Taylor
Make things. Make sense.
https://doriantaylor.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: bug in HTTP protocol spec

[Jeff King]

On Wed, Feb 21, 2018 at 11:23:52PM -0800, Dorian Taylor wrote:

> diff --git a/Documentation/technical/http-protocol.txt 
> b/Documentation/technical/http-protocol.txt
> index a0e45f2889e6e..19d73f7efb338 100644
> --- a/Documentation/technical/http-protocol.txt
> +++ b/Documentation/technical/http-protocol.txt
> @@ -214,14 +214,17 @@ smart server reply:
>   S: Cache-Control: no-cache
>   S:
>   S: 001e# service=git-upload-pack\n
> +   S: 
>   S: 004895dcfa3633004da0049d3d0fa03f80589cbcaf31 
> refs/heads/maint\0multi_ack\n
>   S: 0042d049f6c27a2244e12041955e262a404c7faba355 refs/heads/master\n
>   S: 003c2cb58b79488a98d2721cea644875a8dd0026b115 refs/tags/v1.0\n
>   S: 003fa3c2e2402b99163d1d59756e5f207ae21cccba4c refs/tags/v1.0^{}\n
> +   S: 

This indentation is funny. But I suspect it is because your whole patch
seems to have been whitespace-damaged (see the section on gmail in
"git help git-format-patch").

> The client may send Extra Parameters (see
> Documentation/technical/pack-protocol.txt) as a colon-separated string
> -in the Git-Protocol HTTP header.
> +in the Git-Protocol HTTP header. Note as well that there is *no* newline
> +after the ``.

I guess I'm not opposed to calling that out, but this is normal for
pktline (the flush packet has no data; in the other lines the newline is
not a syntactic part of the pktline stream, but is actually data
contained inside each of those pktlines).

> Dumb Server Response
> 
> @@ -264,8 +267,8 @@ Servers MUST set $servicename to be the request parameter 
> value.
> Servers SHOULD include an LF at the end of this line.
> Clients MUST ignore an LF at the end of the line.
> 
> -Servers MUST terminate the response with the magic `` end
> -pkt-line marker.
> +Servers MUST follow the first pkt-line, as well as terminate the
> +response, with the magic `` end pkt-line marker.

In theory there can actually be one or more headers after the "service"
line. But I don't think they've ever been used (and the current client
just throws them away).

> The returned response is a pkt-line stream describing each ref and
> its known value.  The stream SHOULD be sorted by name according to
> @@ -278,6 +281,7 @@ Extra Parameter.
> 
>  smart_reply =  PKT-LINE("# service=$servicename" LF)
>*1("version 1")
> +  ""
>ref_list
>""

I think Jonathan is right that the version must go after the flush
packet (just looking at the v2 protocol patches elsewhere on the list,
the version tag is really part of the ref_list).

Not related to your patch, but I suspect it should also be
PKT-LINE("version 1").

-Peff

Re: bug in HTTP protocol spec

[Dorian Taylor]

> On Feb 21, 2018, at 9:37 PM, Jonathan Nieder  wrote:
> 
> Thanks for writing it.
> 
> Do you mind if we forge your sign-off?  (See Documentation/SubmittingPatches
> item '(5) Certify your work' for details about what this means.)

Sure, or I can just re-paste:

Signed-off-by: Dorian Taylor 

---
Documentation/technical/http-protocol.txt | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/http-protocol.txt 
b/Documentation/technical/http-protocol.txt
index a0e45f2889e6e..19d73f7efb338 100644
--- a/Documentation/technical/http-protocol.txt
+++ b/Documentation/technical/http-protocol.txt
@@ -214,14 +214,17 @@ smart server reply:
  S: Cache-Control: no-cache
  S:
  S: 001e# service=git-upload-pack\n
+   S: 
  S: 004895dcfa3633004da0049d3d0fa03f80589cbcaf31 refs/heads/maint\0multi_ack\n
  S: 0042d049f6c27a2244e12041955e262a404c7faba355 refs/heads/master\n
  S: 003c2cb58b79488a98d2721cea644875a8dd0026b115 refs/tags/v1.0\n
  S: 003fa3c2e2402b99163d1d59756e5f207ae21cccba4c refs/tags/v1.0^{}\n
+   S: 

The client may send Extra Parameters (see
Documentation/technical/pack-protocol.txt) as a colon-separated string
-in the Git-Protocol HTTP header.
+in the Git-Protocol HTTP header. Note as well that there is *no* newline
+after the ``.

Dumb Server Response

@@ -264,8 +267,8 @@ Servers MUST set $servicename to be the request parameter 
value.
Servers SHOULD include an LF at the end of this line.
Clients MUST ignore an LF at the end of the line.

-Servers MUST terminate the response with the magic `` end
-pkt-line marker.
+Servers MUST follow the first pkt-line, as well as terminate the
+response, with the magic `` end pkt-line marker.

The returned response is a pkt-line stream describing each ref and
its known value.  The stream SHOULD be sorted by name according to
@@ -278,6 +281,7 @@ Extra Parameter.

 smart_reply =  PKT-LINE("# service=$servicename" LF)
 *1("version 1")
+""
 ref_list
 ""
 ref_list=  empty_list / non_empty_list

---

> 
>> Note I am not sure what the story is behind that `version 1`
>> element, whether it's supposed to go before or after the null packet
>> or if there should be another null packet or what. Perhaps somebody
>> more fluent with the smart protocol can advise.
> 
> I believe the 'version 1' goes after the flush-packet.

I took a traipse through the code and couldn’t determine it one way or another, 
but my money is on that looking something like `000aversion 1\n` on the wire.

--
Dorian Taylor
Make things. Make sense.
https://doriantaylor.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: bug in HTTP protocol spec

[Jonathan Nieder]

(+cc: bmwill@, who is working on protocol v2)
Hi,

Dorian Taylor wrote:
> On Feb 21, 2018, at 2:15 PM, Jeff King  wrote:

>> Thanks, I agree the document is buggy. Do you want to submit a patch?
>
> Will this do?

Thanks for writing it.

Do you mind if we forge your sign-off?  (See Documentation/SubmittingPatches
item '(5) Certify your work' for details about what this means.)

> Note I am not sure what the story is behind that `version 1`
> element, whether it's supposed to go before or after the null packet
> or if there should be another null packet or what. Perhaps somebody
> more fluent with the smart protocol can advise.

I believe the 'version 1' goes after the flush-packet.

Thanks,
Jonathan

Re: bug in HTTP protocol spec

[Dorian Taylor]

> On Feb 21, 2018, at 2:15 PM, Jeff King  wrote:
> 
> Thanks, I agree the document is buggy. Do you want to submit a patch?

Will this do?

Note I am not sure what the story is behind that `version 1` element, whether 
it's supposed to go before or after the null packet or if there should be 
another null packet or what. Perhaps somebody more fluent with the smart 
protocol can advise.

---
Documentation/technical/http-protocol.txt | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/http-protocol.txt 
b/Documentation/technical/http-protocol.txt
index a0e45f2889e6e..19d73f7efb338 100644
--- a/Documentation/technical/http-protocol.txt
+++ b/Documentation/technical/http-protocol.txt
@@ -214,14 +214,17 @@ smart server reply:
   S: Cache-Control: no-cache
   S:
   S: 001e# service=git-upload-pack\n
+   S: 
   S: 004895dcfa3633004da0049d3d0fa03f80589cbcaf31 refs/heads/maint\0multi_ack\n
   S: 0042d049f6c27a2244e12041955e262a404c7faba355 refs/heads/master\n
   S: 003c2cb58b79488a98d2721cea644875a8dd0026b115 refs/tags/v1.0\n
   S: 003fa3c2e2402b99163d1d59756e5f207ae21cccba4c refs/tags/v1.0^{}\n
+   S: 

The client may send Extra Parameters (see
Documentation/technical/pack-protocol.txt) as a colon-separated string
-in the Git-Protocol HTTP header.
+in the Git-Protocol HTTP header. Note as well that there is *no* newline
+after the ``.

Dumb Server Response

@@ -264,8 +267,8 @@ Servers MUST set $servicename to be the request parameter 
value.
Servers SHOULD include an LF at the end of this line.
Clients MUST ignore an LF at the end of the line.

-Servers MUST terminate the response with the magic `` end
-pkt-line marker.
+Servers MUST follow the first pkt-line, as well as terminate the
+response, with the magic `` end pkt-line marker.

The returned response is a pkt-line stream describing each ref and
its known value.  The stream SHOULD be sorted by name according to
@@ -278,6 +281,7 @@ Extra Parameter.

  smart_reply =  PKT-LINE("# service=$servicename" LF)
 *1("version 1")
+""
 ref_list
 ""
  ref_list=  empty_list / non_empty_list

--
Dorian Taylor
Make things. Make sense.
https://doriantaylor.com

Re: bug in HTTP protocol spec

[Jeff King]

On Wed, Feb 21, 2018 at 10:29:35AM -0800, Dorian Taylor wrote:

> I didn’t get an insight until I ran with GIT_TRACE_PACKET=true on a 
> known-good remote (i.e. GitHub), that the null packet-line `` has to 
> follow the service line. This is not reflected in the example here:
> 
> https://github.com/git/git/blob/6464679d9620d91b639e2681b9cc6473f3856d09/Documentation/technical/http-protocol.txt#L216

This is missing the trailing flush after the ref advertisement, too.

> It is also not reflected in the BNF:
> 
> https://github.com/git/git/blob/6464679d9620d91b639e2681b9cc6473f3856d09/Documentation/technical/http-protocol.txt#L279
> 
> (Note these links are from the most recent commit of this file as of this 
> writing.)
> 
> Just thought somebody would like to know.

Thanks, I agree the document is buggy. Do you want to submit a patch?

-Peff