Git merge network: past, present, and near future

[lukego]

Here is an update on the evolution of our Git merge workflow.

Past

The workflow we used last year was simple: people send PRs to Github, I merge them onto next, then @eugeneia merges that onto master for release.

This is a simple structure but it did not scale because it depended on me to ultimately accept/reject every individual change. I don't have the time to give every change the attention it deserves, nor does any one person.

Present

The workflow we have adopted more recently is that PRs are first merged by @eugeneia onto max-next or by @wingo onto wingo-next and then upstreamed in batches from here. This setup is very new and we are still working out how to divide up incoming PRs, how frequently to merge branches, and arriving at common expectations about documentation and so on.

This should scale better. The workload is divided up between @eugeneia and @wingo and I am sanity-checking and combining the results. The three of us are thrashing out a common understanding of what makes a change acceptable. (There is actually some flexibility here: wingo-next can PR to either max-next or next depending on how much extra review @wingo wants.)

See relevant discussions on #732 and #722.

Near future

The big thing that I want to do next is have application-specific branches merge onto master. I would like to have @alexandergall's ALX application, Igalia's lwaftr application, and @capr's lisper application all upstream in the master repository (just like packetblaster and snabbnfv).

The benefit I see is being able to address a large amount of code when testing, benchmarking, refactoring, and redesigning. The challenge I see is to find a workflow that benefits everybody. Upstream needs the core libraries to be simple and consistent while application developers need to be in charge of what code they ship to their users. This workflow will have to be thrashed out with lively discussions here on Github.

I hope that we will come to an agreement but it remains to be seen. @alexandergall and I have discussed this privately in the past and that seemed encouraging, and was the root of the src/programs/foo subdirectory as a make-my-own-rules sanctuary for developers of the foo application.

[lukego]

I would be curious to know what @eugeneia, @alexandergall, @capr, @wingo think of this plan: both the general idea of merging applications upstream and the specific working relationships suggested in the diagram.

One reflection is that it will probably be important to focus on the content of the merges more than the history. The command git merge combines two parallel repository histories together and these can be long: each one is the life story of the birth and development of an application. I suspect it will be practical to embrace the rich tapestry of our project history rather than spend a lot of effort on rewriting history with rebases and so on.

So when reviewing the merge of an application branch the main things I would think to look at are:

• Is it clear what the application is? what does it do, how do you run it, where do you find out more, how do you contribute?
• Is there a src/program/foo subdirectory? Let this be: application developers are in charge of this code except in exceptional cases (e.g. license conflicts, build complications, etc).
• Are there changes elsewhere e.g. in core, lib, and apps? This should be subject to the same review and revision other code submitted upstream. The review could either be done directly on the application branch or the changes could be spun out and submitted separately, whatever is most practical. If it takes too long to reach agreement on these changes then they could be moved into src/program/foo and then refactored later once we see eye to eye.

[eugeneia]

I am on board and agree with the above “review” bullet points.

[eugeneia]

@mention-bot though is already annoying me. I tried to turn it off by blocking it, in case that doesn't prevent it from notifying me I will complain more.

[lukego]

Thought for the day: "Bootstrapping a distributed merge network is as easy as riding a bike."

Hopefully it will be easier once we are moving... :-)

[lukego]

This is starting to work!

Today I pulled from both max-next and wingo-next onto next. This is the first turn of the pedals on our metaphorical tandem bicycle.

We are still fumbling around and not yet really working in unison but I think this will improve and we will gather momentum with every merge.

Let me paint a picture of how I see upstream contribution working:

Contributor opens a PR and one person is unambiguously assigned as the "upstream". The upstream either merges the change directly or engages with the contributor to tell them exactly what they can do to get the change merged. The interaction should be something crisp like:

• Merged, thanks!
• This needs a README and a selftest and then I will merge it.
• I need an ack from [persons] before I am confident enough to merge this.
• Have you considered doing FOO? Would you like to revise or do you prefer that I merge as-is?

That's it! This process is then applied recursively from the first upstream to the next upstreams until the change lands on master.

Everybody in the community is welcome to give feedback on all pull requests, of course! However the contributor is free to evaluate the merits of this advice for themselves unless their upstream decides that it is important and says that it is a blocker.

Great work @eugeneia and @wingo thank you for your perseverance while we are working this out!

Anybody else want to volunteer to be an upstream, btw? :-)

[kbara]

I'll volunteer.

[lukego]

@kbara awesome!!! 😄

So how about this workflow as the next step based on what seems to be working now:

I reckon I can handle up to around 5 downstream branches so we should be okay without rebalancing for a while.

One challenge now will be dispatching PRs send to master between more upstream maintainers. How about if a maintainer self-nominates as the upstream for a given change via the Assignee field on the PR? Then this maintainer is the one engaging with the submitter according to the process sketched above (i.e. focused on getting the change merged) whereas everybody else is just providing supporting comments and ideas.

@kbara I added you to the "maintainers" group on Github so that you can manipulate issues (e.g. set assignee and apply tags). I think the last step towards making it official is to register a branch in branches.md.

N=4! log5(N)=0.86! Onward!

[kbara]

Sounds good to me. :-)

[eugeneia]

Contributor opens a PR and one person is unambiguously assigned as the "upstream". The upstream either merges the change directly or engages with the contributor to tell them exactly what they can do to get the change merged. The interaction should be something crisp like:

• Merged, thanks!
• This needs a README and a selftest and then I will merge it.
• I need an ack from [persons] before I am confident enough to merge this.
• Have you considered doing FOO? Would you like to revise or do you prefer that I merge as-is?

That's it! This process is then applied recursively from the first upstream to the next upstreams until the change lands on master.

I struggled a bit wrapping my head around what's required of me in the new model but this explains it very well. This is inspiration material for CONTRIBUTING.md!

[eugeneia]

Would it be sensible to close PRs that we merged and re-open them when further amendments are needed? I see this as a way to keep the PR tab lean that's not invasive.

[lukego]

Good question.

There are several tools available for putting metadata on PRs:

• Open / Closed
• Assignee
• Tags

So question is: how should we use these states? what does it mean for a PR to be "open" - that it is not released on master yet or that no upstream maintainer has merged it yet?

and is there other information that we should be keeping track of: waiting for a maintainer to be assigned, waiting for feedback, waiting for a revision to address problems, ...?

(not to suggest that we need to answer all of this at the same time, just thinking of the broader context.)

[eugeneia]

I feel the desire to close PRs that I merged because that way I don't have to remember that I merged them when looking for PRs I need to act on.

[lukego]

I understand that you need a way to search for issues that require action. I wonder if open/closed is the best search criteria though? How about applying a tag upstream and using an advanced search to filter those out when looking for work to do?

Just reflecting that Open/Closed is only one bit of information so it is a fairly limited way to encode the status of pull requests. If somebody is going to have to use more sophisticated queries it could make sense for that to be maintainers rather than e.g. application developers or casual users.

[lukego]

I feel like it would be particularly useful for application developers to see PRs for code that is accepted by a maintainer but not released yet. Then they would have the chance to adopt it early and provide feedback based on their testing.

For example if there is a LuaJIT update that is accepted upstream but not released yet then it could be productive for an application developer to merge that for testing and provide early feedback about whether it causes them any problems or not.

For general users and contributors it could also be handy for the PR tab to show "what's going on this month?" as a window into the work that is going on and showing what might be valuable to test and review (even if it is accepted by the first hop it may still need update/revert on the way to master).

Or could be that I am overcomplicating things :).

[eugeneia]

A label would serve my purpose just as well. Is it possible to create new labels? I know I can't just add a new label on my own. My first instinct was to give the PR I merged the merged label, but GitHub doesn't allow me to.

[lukego]

I can create a label by clicking Issues -> Labels -> New Label. You too? If not, which labels do you want?

Github permissions are confusing. I actually think we already have it setup too loosely e.g. too many of us can accidentally push to master or other public branches with a mistake on the command line. Should work out what is the optimal solution some time.

[eugeneia]

Yep, works for me this way. Didn't look hard enough.

I have added a light purple merged label that I used to label the PRs merged: https://github.com/SnabbCo/snabbswitch/labels/merged

@kbara @wingo Would this be useful to you as well to label PRs you merged?

[wingo]

@eugeneia sure, i'll use it :) thanks!