[PATCH] http-protocol.txt: add missing flush to example

TimTIM via GitGitGadget· Nov 18, 2021, 11:16 UTC · lore

From: TimTim Wong <t2@live.hk>

Signed-off-by: Timothy Wong <i@timtim.hk>
---
    Fix example in documentation
    
    wants must be flushed with 0000 before haves

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1141%2Fwegylexy%2Fpatch-1-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1141/wegylexy/patch-1-v1
Pull-Request: https://github.com/git/git/pull/1141

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

Show changes to Documentation/technical/http-protocol.txt +4 −3

diff --git a/Documentation/technical/http-protocol.txt b/Documentation/technical/http-protocol.txt
index cc5126cfeda..facb315a993 100644
--- a/Documentation/technical/http-protocol.txt
+++ b/Documentation/technical/http-protocol.txt
@@ -314,8 +314,9 @@ Clients MUST first perform ref discovery with
    C: Content-Type: application/x-git-upload-pack-request
    C:
    C: 0032want 0a53e9ddeaddad63ad106860237bbf53411d11a7\n
-   C: 0032have 441b40d833fdfa93eb2908e52742248faf0ee993\n
    C: 0000
+   C: 0032have 441b40d833fdfa93eb2908e52742248faf0ee993\n
+   C: done
 
    S: 200 OK
    S: Content-Type: application/x-git-upload-pack-result
@@ -337,9 +338,9 @@ server advertises capability `allow-tip-sha1-in-want` or
 `allow-reachable-sha1-in-want`.
 
   compute_request   =  want_list
+		       "0000"
 		       have_list
-		       request_end
-  request_end       =  "0000" / "done"
+		       "done"
 
   want_list         =  PKT-LINE(want SP cap_list LF)
 		       *(want_pkt)

base-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc

-- 
gitgitgadget

Jeff King· Nov 18, 2021, 16:38 UTC · re: TimTIM via GitGitGadget · lore

Re: [PATCH] http-protocol.txt: add missing flush to example

On Thu, Nov 18, 2021 at 11:16:43AM +0000, TimTIM via GitGitGadget wrote:

Show 7 quoted lines

> From: TimTim Wong <t2@live.hk>
> 
> Signed-off-by: Timothy Wong <i@timtim.hk>
> ---
>     Fix example in documentation
>     
>     wants must be flushed with 0000 before haves

This explanation would probably make more sense in the commit message.

I think this is only true for the v0 protocol. In the v2 protocol, we take all of the wants/haves as a single sequence. That may be OK for our purposes here, though. While the v2 docs do refer to http-protocol.txt, it is only for the "Initial Client Request" section. The actual v2 fetch primitive is defined separately in protocol-v2.txt.

Show 12 quoted lines

> diff --git a/Documentation/technical/http-protocol.txt b/Documentation/technical/http-protocol.txt
> index cc5126cfeda..facb315a993 100644
> --- a/Documentation/technical/http-protocol.txt
> +++ b/Documentation/technical/http-protocol.txt
> @@ -314,8 +314,9 @@ Clients MUST first perform ref discovery with
>     C: Content-Type: application/x-git-upload-pack-request
>     C:
>     C: 0032want 0a53e9ddeaddad63ad106860237bbf53411d11a7\n
> -   C: 0032have 441b40d833fdfa93eb2908e52742248faf0ee993\n
>     C: 0000
> +   C: 0032have 441b40d833fdfa93eb2908e52742248faf0ee993\n
> +   C: done

OK, so the "have" lines moves after the flush, which makes sense. There's no longer a trailing flush, though. Instead, we put in a "done" command. This is OK, and what we'd often send in practice, but it is optional (due to "no-done"). I wonder if it is worth keeping things simpler. I dunno.

(BTW, if you run with GIT_TRACE_PACKET here, it's a bit misleading. We _do_ send a flush after the "done", but it's the flush to tell remote-curl that it should send the stateless-rpc packet; it doesn't actually go over the wire with the HTTP request).

Show 10 quoted lines

> @@ -337,9 +338,9 @@ server advertises capability `allow-tip-sha1-in-want` or
>  `allow-reachable-sha1-in-want`.
>  
>    compute_request   =  want_list
> +		       "0000"
>  		       have_list
> -		       request_end
> -  request_end       =  "0000" / "done"
> +		       "done"
>

OK, so here we put the flush after want_list, which is good. But this loses the optionality of "done". I think it's valid to just send the flush, but this BNF no longer reflects that. Or if I'm wrong, I think this needs to be discussed in the commit message.

I wondered if the non-http protocol had a similar problem, but it treats the "want" and "have" phases more independently. E.g., we get:

    upload-request    =  want-list
                       *shallow-line
                       *1depth-request
                       [filter-request]
                       flush-pkt

and then later:

    upload-haves      =  have-list
                         compute-end
    compute-end       =  flush-pkt / PKT-LINE("done")

which is OK.

-Peff

patchhttp-protocol.txt: add missing flush to example