git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] protocol upload-pack-v2

From
Stefan Beller <sbeller@google.com>
Date
Mar 7, 2015, 04:28 UTC
Message-ID
<CAGZ79kZBYFSwR6E86BF6Dt7xdh0zs07tkGnQAKfEJpSduTK-aw@mail.gmail.com>
In-Reply-To
<xmqqr3t1vefz.fsf@gitster.dls.corp.google.com>
On Fri, Mar 6, 2015 at 4:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 39 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> @@ -67,7 +74,6 @@ gracefully with an error message.
>>    error-line     =  PKT-LINE("ERR" SP explanation-text)
>>  ----
>>
>> -
>>  SSH Transport
>
> Noise?
>
>> @@ -124,9 +130,56 @@ has, the first can 'fetch' from the second.  This operation determines
>>  what data the server has that the client does not then streams that
>>  data down to the client in packfile format.
>>
>> +Capability discovery (v2)
>> +-------------------------
>>
>> +In version 1, capability discovery is part of reference discovery and
>> +covered in reference discovery section.
>> +
>> +In version 2, when the client initially connects, the server
>> +immediately sends its capabilities to the client. Then the client must
>> +send the list of server capabilities it wants to use to the server.
>> +
>> +   S: 00XXcapabilities 4\n
>> +   S: 00XXcap:lang\n
>> +   S: 00XXcap:thin-pack\n
>> +   S: 00XXcap:ofs-delta\n
>> +   S: 00XXagent:agent=git/2:3.4.5+custom-739-gb850f98\n
>> +
>> +   C: 00XXcapabilities 3
>> +   C: 00XXcap:thin-pack\n
>> +   C: 00XXcap:ofs-delta\n
>> +   C: 00XXcap:lang=en\n
>> +   C: 00XXagent:agent=git/custom_string\n
>
> I do not see a good reason why we want "I am sending N caps"
> upfront, instead of "this, that, and here is the end of the group".

I thought about having an end marker, so something like capabilities start thin-pack lang ofs-delta capabilities done

(Each line a pkt-line)

Though all decisions I thought through I tried to put more weight on future expandability. Now that I think about it again, it makes no difference, whether to use a counter or an end marker.

> If you expect the recipient to benefit by being able to pre-allocate
> N slots, then that might make sense, but otherwise, I'd rather see
> us stick to a (weaker) flush that says "group ends here".

I think it's not about pre allocating but counting down. Then you know at the beginning how much to expect which might become relevant if that section grows large again. ("The server really wants to send 1500 capability lines? Nope I'll just disconnect because I am on mobile!")

Implementation wise an end marker is easier though (you don't need to count down, so it feels more stateless to me).

>
> Besides, I do not know how you counted 4 on the S: side and 3 on
> the C: side in the above example and missing LF after 3 ;-).
>
Sorry about that, I added one answer late and forgot to increment the 3.
Show 7 quoted lines
>> +----
>> +  cap              =  PKT-LINE("capabilities" SP size LF list)
>
> Isn't a cap packet terminated by LF without any "list" following it?
> The notation PKT-LINE(<blah>) is "wrap <blah> in a single packet",
> and I do not think you meant to send the capability line and a series
> of cap:foo entries in a single packet.

Yeah I meant to use one packet per line So after considering your input, you'd want to have PKT-LINE("capabilities start") PKT-LINE("no-prefix-for-capabilities") PKT-LINE("ofs-delta") PKT-LINE("agent-as-capability-2.6") PKT-LINE("capabilities end")

And additionally to that a PKT-LINE should have the ability to grow larger than 0xffff, which would be encoded with 0xffff being an escape character indicating the length is encoded somehow different. (Maybe take the next 8 bytes instead of just 4).

Show 9 quoted lines
>
>> +  size             =  *DIGIT
>> +  capability-list  =  *(capability) [agent LF]
>> +  capability       =  "cap:" keyvaluepair LF
>> +  agent            =  keyvaluepair LF
>
> I do not see a reason to make 'agent' as part of capability.  That
> was an implementation detail of v1 but v2 does not have an
> obligation to consider agent announcement as capability.

So I think we don't want to drop the agent announcement as it may reveal useful information ("How many outdated clients connect to our $HOSTING_SITE?", "I need to debug failures which happen only rarely, Oh! it can be narrowed down to clients with agent xyz.")

So then we need to decide where to put the agent. And as it is only small but useful (meta?)-information I'd rather put it at the beginning of the data exchange, so in case the other side seems to be missbehaving, it is easier to debug in the hope the agent transmission was still successful.

Show 6 quoted lines
>
> server-announcement = PKT-LINE("capabilities" ...) capability-list [agent-announcement]
> capability-list = as you have it w/o "[agent LF]"
> agent-announcement = PKT-LINE("agent=" agent-token LF)
>
> or something, perhaps?

This looks like me as if all capabilities are in one PKT-LINE, which you seemed to oppose?

Show 9 quoted lines
>
>> +The client MUST ignore any data before the pkt-line starting with "capabilities"
>> +for future easy of extension.
>
> s/easy/ease/; but I am not sure if this makes sense.  Without
> knowing the extended preamble, you cannot even tell if a packet line
> that happens to start with "capabilities" is a proper beginning of
> 0-th iteration of v2 protocol, or an embedded data in the preamble,
> no?

I rather thought about the case where the implementation would just close the connection on sight of unknown preamble. If we want to extend the protocol again and the string "capabilites" should be part before the actual capabilities start, we will think about escaping it in the future as then we can still talk to clients as of this design.

In case we'd close the connection we would have a similar problem as of now, it cannot be really extended.

Show 22 quoted lines
>
>> +Reference Discovery (v2)
>> +------------------------
>> +
>> +In version 2, reference discovery is initiated by the client with
>> +"want-refs" line. The client may skip reference discovery phase
>> +entirely by not sending "want-refs" (e.g. the client has another way
>> +to retrieve ref list).
>> +
>> +----
>> +  want-refs  =  PKT-LINE("want-refs" SP mode *argument)
>> +  mode       =  "all"
>> +  argument   =  SP arg
>> +  arg        =  1*(LC_ALPHA / DIGIT / "-" / "_" / "=")
>> +----
>> +
>> +Mode "all" sends all visible refs to the client like in version 1. No
>> +arguments are accepted in this mode. More modes may be available based
>> +on capabilities.
>
> I tend to agree with Duy that the protocol must anticipate that args
> can become longer.
ok, so PKT-LINE needs to be able to deal with larger lines, I'll add that.
Show 11 quoted lines
>
>> +----
>> +  advertised-refs  =  (no-refs / list-of-refs)
>> +                   *shallow
>> +                   flush-pkt
>
> I am not sure if defining "shallow" as part of "refs advertisement"
> is a good idea.  The latter lives in the same place in the v1
> protocol merely because that was how it was later bolted onto the
> protocol.  But this concern can easily be allayed by retitling
> "advertised-refs" to something else.
I don't quite understand this. What are your concerns about shallow here?

Thanks on the feedback! Stefan

Previous: Junio C HamanoNext: Duy Nguyen
Message 57 of 80 in “protocol v2”
  1. 0/3 protocol v2Stefan Beller, Feb 24, 2015
  2. 1/3 Document protocol capabilities extensionStefan Beller, Feb 24, 2015
  3. 2/3 receive-pack: add advertisement of different protocol optionsStefan Beller, Feb 24, 2015
  4. 3/3 receive-pack: enable protocol v2Stefan Beller, Feb 24, 2015
  5. Duy NguyenFeb 24, 2015
  6. Stefan BellerFeb 24, 2015
  7. Junio C HamanoFeb 24, 2015
  8. Stefan BellerFeb 24, 2015
  9. Duy NguyenFeb 25, 2015
  10. Junio C HamanoFeb 25, 2015
  11. Stefan BellerFeb 26, 2015
  12. Duy NguyenFeb 26, 2015
  13. Stefan BellerFeb 26, 2015
  14. Junio C HamanoFeb 26, 2015
  15. Stefan BellerFeb 27, 2015
  16. Stefan BellerFeb 27, 2015
  17. Junio C HamanoFeb 27, 2015
  18. Stefan BellerFeb 27, 2015
  19. Junio C HamanoFeb 28, 2015
  20. Stefan BellerFeb 28, 2015
  21. 0/5 protocol v2 for upload-packStefan Beller, Feb 28, 2015
  22. 1/5 upload-pack: only accept capabilities on the first "want" lineStefan Beller, Feb 28, 2015
  23. 2/5 upload-pack: support out of band client capability requestsStefan Beller, Feb 28, 2015
  24. Kyle J. McKayFeb 28, 2015
  25. Duy NguyenFeb 28, 2015
  26. Kyle J. McKayFeb 28, 2015
  27. Duy NguyenMar 1, 2015
  28. Duy NguyenFeb 28, 2015
  29. 3/5 connect.c: connect to a remote service with some flagsStefan Beller, Feb 28, 2015
  30. Torsten BögershausenFeb 28, 2015
  31. Junio C HamanoMar 1, 2015
  32. 4/5 daemon.c: accept extra service argumentsStefan Beller, Feb 28, 2015
  33. Junio C HamanoMar 1, 2015
  34. 5/5 WIP/Document the http protocol changeStefan Beller, Feb 28, 2015
  35. Duy NguyenFeb 28, 2015
  36. Johannes SixtMar 1, 2015
  37. Junio C HamanoMar 2, 2015
  38. Junio C HamanoMar 2, 2015
  39. Duy NguyenMar 2, 2015
  40. Duy NguyenMar 2, 2015
  41. Duy NguyenMar 3, 2015
  42. Junio C HamanoMar 3, 2015
  43. Junio C HamanoMar 3, 2015
  44. Duy NguyenMar 4, 2015
  45. Shawn PearceMar 4, 2015
  46. Duy NguyenMar 4, 2015
  47. Shawn PearceMar 4, 2015
  48. Stefan BellerMar 5, 2015
  49. Stefan BellerMar 5, 2015
  50. Stefan BellerMar 24, 2015
  51. Junio C HamanoMar 24, 2015
  52. protocol upload-pack-v2Stefan Beller, Mar 6, 2015
  53. Stefan BellerMar 6, 2015
  54. Duy NguyenMar 6, 2015
  55. Duy NguyenMar 7, 2015
  56. Junio C HamanoMar 7, 2015
  57. Stefan BellerMar 7, 2015
  58. Duy NguyenMar 7, 2015
  59. Junio C HamanoMar 8, 2015
  60. Junio C HamanoMar 31, 2015
  61. Duy NguyenApr 2, 2015
  62. Junio C HamanoApr 2, 2015
  63. Martin FickApr 2, 2015
  64. Junio C HamanoApr 2, 2015
  65. Stefan BellerApr 2, 2015
  66. Stefan BellerApr 2, 2015
  67. Duy NguyenMar 10, 2015
  68. Kyle J. McKayMar 10, 2015
  69. Duy NguyenFeb 28, 2015
  70. Junio C HamanoFeb 28, 2015
  71. Junio C HamanoMar 1, 2015
  72. Duy NguyenMar 1, 2015
  73. Stefan BellerMar 1, 2015
  74. David LangMar 2, 2015
  75. Junio C HamanoMar 1, 2015
  76. David LangMar 2, 2015
  77. Junio C HamanoMar 2, 2015
  78. Philip OakleyMar 1, 2015
  79. Duy NguyenMar 2, 2015
  80. Stefan BellerFeb 27, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.