Subject: Re: [PATCH v2 00/27] protocol version 2

From: Brandon Williams <[email-masked]>
Date: Tue, 6 Feb 2018 16:58:20 -0800

On 01/31, Derrick Stolee wrote:
> Sorry for chiming in with mostly nitpicks so late since sending this
> version. Mostly, I tried to read it to see if I could understand the scope
> of the patch and how this code worked before. It looks very polished, so I
> the nits were the best I could do.
> 
> On 1/25/2018 6:58 PM, Brandon Williams wrote:
> > Changes in v2:
> >   * Added documentation for fetch
> >   * changes #defines for state variables to be enums
> >   * couple code changes to pkt-line functions and documentation
> >   * Added unit tests for the git-serve binary as well as for ls-refs
> 
> I'm a fan of more unit-level testing, and I think that will be more
> important as we go on with these multiple configuration options.
> 
> > Areas for improvement
> >   * Push isn't implemented, right now this is ok because if v2 is requested the
> >     server can just default to v0.  Before this can be merged we may want to
> >     change how the client request a new protocol, and not allow for sending
> >     "version=2" when pushing even though the user has it configured.  Or maybe
> >     its fine to just have an older client who doesn't understand how to push
> >     (and request v2) to die if the server tries to speak v2 at it.
> > 
> >     Fixing this essentially would just require piping through a bit more
> >     information to the function which ultimately runs connect (for both builtins
> >     and remote-curl)
> 
> Definitely save push for a later patch. Getting 'fetch' online did require
> 'ls-refs' at the same time. Future reviews will be easier when adding one
> command at a time.
> 
> > 
> >   * I want to make sure that the docs are well written before this gets merged
> >     so I'm hoping that someone can do a through review on the docs themselves to
> >     make sure they are clear.
> 
> I made a comment in the docs about the architectural changes. While I think
> a discussion on that topic would be valuable, I'm not sure that's the point
> of the document (i.e. documenting what v2 does versus selling the value of
> the patch). I thought the docs were clear for how the commands work.
> 
> >   * Right now there is a capability 'stateless-rpc' which essentially makes sure
> >     that a server command completes after a single round (this is to make sure
> >     http works cleanly).  After talking with some folks it may make more sense
> >     to just have v2 be stateless in nature so that all commands terminate after
> >     a single round trip.  This makes things a bit easier if a server wants to
> >     have ssh just be a proxy for http.
> > 
> >     One potential thing would be to flip this so that by default the protocol is
> >     stateless and if a server/command has a state-full mode that can be
> >     implemented as a capability at a later point.  Thoughts?
> 
> At minimum, all commands should be designed with a "stateless first"
> philosophy since a large number of users communicate via HTTP[S] and any
> decisions that make stateless communication painful should be rejected.

I agree with this and my next version will run with this philosophy in
mind (v2 will be stateless by default).

> 
> >   * Shallow repositories and shallow clones aren't supported yet.  I'm working
> >     on it and it can be either added to v2 by default if people think it needs
> >     to be in there from the start, or we can add it as a capability at a later
> >     point.
> 
> I'm happy to say the following:
> 
> 1. Shallow repositories should not be used for servers, since they cannot
> service all requests.
> 
> 2. Since v2 has easy capability features, I'm happy to leave shallow for
> later. We will want to verify that a shallow clone command reverts to v1.
> 
> 
> I fetched bw/protocol-v2 with tip 13c70148, built, set 'protocol.version=2'
> in the config, and tested fetches against GitHub and VSTS just as a
> compatibility test. Everything worked just fine.
> 
> Is there an easy way to test the existing test suite for clone and fetch
> using protocol v2 to make sure there are no regressions with
> protocol.version=2 in the config?

Yes there already exist interop tests for testing the addition of
requesting a new protocol at //t/interop/i5700-protocol-transition.sh

> Thanks,
> -Stolee

-- 
Brandon Williams