Re: [RFC] push: add documentation on push v2

From: Brandon Williams <bmwill@google.com>
Date: Jul 25, 2018, 17:46 UTC
Message-ID: <20180725174622.GA4850@google.com>
In-Reply-To: <CACsJy8C9z14GsxfyPm_pDuGwAQqm6Cdi2dO3bsqiYDE0scVbkQ@mail.gmail.com>

On 07/25, Duy Nguyen wrote:

Show 38 quoted lines

> On Tue, Jul 24, 2018 at 9:29 PM Brandon Williams <bmwill@google.com> wrote:
> >
> > On 07/17, Brandon Williams wrote:
> > > Signed-off-by: Brandon Williams <bmwill@google.com>
> > > ---
> > >
> > > Since introducing protocol v2 and enabling fetch I've been thinking
> > > about what its inverse 'push' would look like.  After talking with a
> > > number of people I have a longish list of things that could be done to
> > > improve push and I think I've been able to distill the core features we
> > > want in push v2.  Thankfully (due to the capability system) most of the
> > > other features/improvements can be added later with ease.
> > >
> > > What I've got now is a rough design for a more flexible push, more
> > > flexible because it allows for the server to do what it wants with the
> > > refs that are pushed and has the ability to communicate back what was
> > > done to the client.  The main motivation for this is to work around
> > > issues when working with Gerrit and other code-review systems where you
> > > need to have Change-Ids in the commit messages (now the server can just
> > > insert them for you and send back new commits) and you need to push to
> > > magic refs to get around various limitations (now a Gerrit server should
> > > be able to communicate that pushing to 'master' doesn't update master
> > > but instead creates a refs/changes/<id> ref).
> > >
> > > Before actually moving to write any code I'm hoping to get some feedback
> > > on if we think this is an acceptable base design for push (other
> > > features like atomic-push, signed-push, etc can be added as
> > > capabilities), so any comments are appreciated.
> > >
> > >  Documentation/technical/protocol-v2.txt | 76 +++++++++++++++++++++++++
> > >  1 file changed, 76 insertions(+)
> >
> > Pinging this thread again to hopefully reach some more people for
> > commentary.
> 
> Could you send a v2 that covers all the push features in pack version
> 1? I see some are discussed but it's probably good to summarize in
> this document too.

I can mention the ones we want to implement, but I expect that a push v2 would not require that all features in the current push are supported out of the box. Some servers may not want to support signed-push, etc. Also I don't want to have to implement every single feature that exists before getting something merged. This way follow on series can be written to implement those as new features to push v2.

Show 9 quoted lines

> 
> A few other comments
> 
> If I remember correctly, we always update the remote refs locally
> after a push, assuming that the next 'fetch' will do the same anyway.
> This is not true for special refs (like those from gerrit). Looking
> from this I don't think it can say "yes we have received your pack and
> stored it "somewhere" but there's no visible ref created for it" so
> that we can skip the local remote ref update?

This is one of the pain points for gerrit and one of the reasons why they have this funky push syntax "push origin HEAD:refs/for/master". Because its not a remote tracking branch for the current branch, we don't update (or create) a local branch "for/master" under the "refs/remotes/origin" namespace (at least that's how i understand it).

So in order to support the server doing more things (rebasing, or creating new branches) based on what is pushed the status that the server sends in response needs to be more fluid so that the server can describe what it did in a way that the client can either: update the remote tracking branches, or not (and in this case maybe do what you suggest down below).

Show 7 quoted lines

> 
> Is it simpler to tell a push client at the end that "yes there's new
> stuff now on the server, do another fetch", sort of like HTTP
> redirect, then the client can switch to fetch protocol to get the new
> stuff that the server has created (e.g. rebase stuff)? I assume we
> could reuse the same connection for both push and fetch if needed.
> This way both fetch and push send packs in just one direction.

I really, really like this suggestion. Thank you for your input. This would actually make the protocol much simpler by keeping push for pushing packs and fetch for fetching packs. And since my plan is to have the status-report for push include all the relevant ref changes that the server made, if there are any that don't correspond with what was pushed (either the server rebased a change or created a new ref I didn't) then we can skip the ref-advertisement and go straight to fetching those refs. And yes, since protocol v2 is command based we could reuse the existing connection and simply send a "fetch" request after our "push" one.

This does mean that it'll be an extra round trip than what I originally had in mind, but it should be simpler.

-- 
Brandon Williams