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
