Re: bug in HTTP protocol spec

From: Jeff King <peff@peff.net>
Date: Feb 22, 2018, 10:08 UTC
Message-ID: <20180222100840.GD12442@sigill.intra.peff.net>
In-Reply-To: <614A9A36-9DE3-4A85-BFA8-8380C4AC21B8@gmail.com>

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

Show 14 quoted lines

> 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: 0000
>   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: 0000

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").

Show 5 quoted lines

> 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 `0000`.

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).

Show 10 quoted lines

> 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 `0000` end
> -pkt-line marker.
> +Servers MUST follow the first pkt-line, as well as terminate the
> +response, with the magic `0000` 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).

Show 9 quoted lines

> 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")
> +		     "0000"
> 		     ref_list
> 		     "0000"

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