Subject: Re: [PATCH 11/26] serve: introduce git-serve

From: 'Brandon Williams' <[email-masked]>
Date: Thu, 1 Feb 2018 12:08:26 -0800

On 02/01, Randall S. Becker wrote:
> On February 1, 2018 1:58 PM, Stefan Beller wrote:
> > On Thu, Feb 1, 2018 at 10:48 AM, Jeff Hostetler <[email-masked]>
> > wrote:
> > >
> > >
> > > On 1/2/2018 7:18 PM, Brandon Williams wrote:
> > >>
> > >> Introduce git-serve, the base server for protocol version 2.
> > >>
> > >> Protocol version 2 is intended to be a replacement for Git's current
> > >> wire protocol.  The intention is that it will be a simpler, less
> > >> wasteful protocol which can evolve over time.
> > >>
> > >> Protocol version 2 improves upon version 1 by eliminating the initial
> > >> ref advertisement.  In its place a server will export a list of
> > >> capabilities and commands which it supports in a capability
> > >> advertisement.  A client can then request that a particular command
> > >> be executed by providing a number of capabilities and command
> > >> specific parameters.  At the completion of a command, a client can
> > >> request that another command be executed or can terminate the
> > >> connection by sending a flush packet.
> > >>
> > >> Signed-off-by: Brandon Williams <[email-masked]>
> > >> ---
> > >>   .gitignore                              |   1 +
> > >>   Documentation/technical/protocol-v2.txt |  91 ++++++++++++
> > >>   Makefile                                |   2 +
> > >>   builtin.h                               |   1 +
> > >>   builtin/serve.c                         |  30 ++++
> > >>   git.c                                   |   1 +
> > >>   serve.c                                 | 239
> > >> ++++++++++++++++++++++++++++++++
> > >>   serve.h                                 |  15 ++
> > >>   8 files changed, 380 insertions(+)
> > >>   create mode 100644 Documentation/technical/protocol-v2.txt
> > >>   create mode 100644 builtin/serve.c
> > >>   create mode 100644 serve.c
> > >>   create mode 100644 serve.h
> > >>
> > >> diff --git a/.gitignore b/.gitignore
> > >> index 833ef3b0b..2d0450c26 100644
> > >> --- a/.gitignore
> > >> +++ b/.gitignore
> > >> @@ -140,6 +140,7 @@
> > >>   /git-rm
> > >>   /git-send-email
> > >>   /git-send-pack
> > >> +/git-serve
> > >>   /git-sh-i18n
> > >>   /git-sh-i18n--envsubst
> > >>   /git-sh-setup
> > >> diff --git a/Documentation/technical/protocol-v2.txt
> > >> b/Documentation/technical/protocol-v2.txt
> > >> new file mode 100644
> > >> index 000000000..b87ba3816
> > >> --- /dev/null
> > >> +++ b/Documentation/technical/protocol-v2.txt
> > >> @@ -0,0 +1,91 @@
> > >> + Git Wire Protocol, Version 2
> > >> +==============================
> > >> +
> > >> +This document presents a specification for a version 2 of Git's wire
> > >> +protocol.  Protocol v2 will improve upon v1 in the following ways:
> > >> +
> > >> +  * Instead of multiple service names, multiple commands will be
> > >> +    supported by a single service.
> > >> +  * Easily extendable as capabilities are moved into their own section
> > >> +    of the protocol, no longer being hidden behind a NUL byte and
> > >> +    limited by the size of a pkt-line (as there will be a single
> > >> +    capability per pkt-line).
> > >> +  * Separate out other information hidden behind NUL bytes (e.g. agent
> > >> +    string as a capability and symrefs can be requested using
> > >> + 'ls-refs')
> > >> +  * Reference advertisement will be omitted unless explicitly
> > >> + requested
> > >> +  * ls-refs command to explicitly request some refs
> > >> +
> > >> + Detailed Design
> > >> +=================
> > >> +
> > >> +A client can request to speak protocol v2 by sending `version=2` in
> > >> +the side-channel `GIT_PROTOCOL` in the initial request to the server.
> > >> +
> > >> +In protocol v2 communication is command oriented.  When first
> > >> +contacting
> > >> a
> > >> +server a list of capabilities will advertised.  Some of these
> > >> capabilities
> > >> +will be commands which a client can request be executed.  Once a
> > >> +command has completed, a client can reuse the connection and request
> > >> +that other commands be executed.
> > >> +
> > >> + Special Packets
> > >> +-----------------
> > >> +
> > >> +In protocol v2 these special packets will have the following semantics:
> > >> +
> > >> +  * '0000' Flush Packet (flush-pkt) - indicates the end of a message
> > >> +  * '0001' Delimiter Packet (delim-pkt) - separates sections of a
> > >> + message
> > >
> > >
> > > Previously, a 0001 pkt-line meant that there was 1 byte of data
> > > following, right?
> > 
> > No, the length was including the length field, so 0005 would indicate that
> > there is one byte following, (+4 bytes of "0005" included)
> > 
> > > Does this change that and/or prevent 1 byte packets?  (Not sure if it
> > > is likely, but the odd-tail of a packfile might get sent in a 0001
> > > line, right?)  Or is it that 0001 is only special during the V2
> > > negotiation stuff, but not during the packfile transmission?
> > 
> > 0001 is invalid in the current protocol v0.
> > 
> > >
> > > (I'm not against having this delimiter -- I think it is useful, but
> > > just curious if will cause problems elsewhere.)
> > >
> > > Should we also consider increasing the pkt-line limit to 5 hex-digits
> > > while we're at it ?   That would let us have 1MB buffers if that would
> > > help with large packfiles.
> > 
> > AFAICT there is a static allocation of one pkt-line (of maximum size), such
> > that the code can read in a full packet and then process it.
> > If we'd increase the packet size we'd need the static buffer to be 1MB, which
> > sounds good for my developer machine. But I suspect it may be too much for
> > people using git on embedded devices?
> > 
> > pack files larger than 64k are put into multiple pkt-lines, which is not a big
> > deal, as the overhead of 4bytes per 64k is negligible.
> > (also there is progress information in the side channel, which would come in
> > as a special packet in between real packets, such that every 64k transmitted
> > you can update your progress meter; Not sure I feel strongly on fewer
> > progress updates)
> 
> Can I request, selfishly from my own platform's (NonStop) performance heartache, that we don't require 1Mb? We're not embedded on this platform, but there is an optimized message system packet size down at 50Kb that I would like to stay under. Although above that is no problem, there is a significant cost incurred above that size point. And please make sure xread/xwrite are used in any event.

I think that it would be too much of a change to up to 1MB lines at the
moment so I'm planning on leaving it right where it is :)

> 
> > >  Granted, we're throttled by the network, so it might not matter.
> > > Would it be interesting to have a 5 digit prefix with parts of the
> > > high bits of first digit being flags ?
> > > Or is this too radical of a change?
> > 
> > What would the flags be for?
> > 
> > As an alternative we could put the channel number in one byte, such that we
> > can have a side channel not just while streaming the pack but all the time.
> > (Again, not sure if that buys a lot for us)
> 
> Cheers,
> Randall
> 
> -- Brief whoami:
>  NonStop developer since approximately 211288444200000000
>  UNIX developer since approximately 421664400
> -- In my real life, I talk too much.
> 
> 
> 

-- 
Brandon Williams