Princess Git Workflow

[alexpatel]

Ming said it was too early to decide, but I wanted to get my thoughts down just to move the conversation online.

Assuming we will use Github pull request/code review, I figure there are two possibilities:

1. One fork (just Guppy)

   • Use / in all branch names (git/github supports this)
   • have two master branches: princess/master and barrelfish/master
   • hack on branches named either princess/feature-x or barrelfish/feature-y, depending on nature of feature
     • when you start a feature, branch from princess/master for project code and barrelfish/master for upstream changes
   • pull request and code review princess/feature-x to princess/master (and same with barrelfish/*
   • anytime barrelfish/master branch is updated, merge into princess/master
   • pull request barrelfish/master against BarrelfishOS/barrelfish to push OS changes upstream

2. Two forks (Guppy and Harvard-PRINCESS/barrelfish)

   • Guppy is a fork of Harvard-PRINCESS/barrelfish (which is a fork of BarrelfishOS/barrelfish)
   • on Guppy: hack on either personal branches ("alex-dev") or feature branches ("feature-my-cool-feature")
     • princess pull requests go to Guppy/master
     • barrelfish pull requests go to Harvard-PRINCESS/barrelfish/master
       • then the Harvard-PRINCESS/barrelfish/master gets merged into Guppy/master
     • whenever merge happens on Guppy/master or Harvard-PRINCESS/barrelfish/master, run full (our definition of "full") test suite against seas testing infrastructure
   • pull request Harvard-PRINCESS/barrelfish/master to BarrelfishOS/barrelfish/master

I kind of like one fork for simplicity and that any Github integrations/CI doesn't need to be duplicated between two repos. Some other notes:

• Even for pandaboards/other hw, if we can connect to them remotely we can set up continuous integration (for example, an on-premise Jenkins server) that will automagically run all the tests upon branch push/pull request - this would be a great mini-project
• david mentioned enforcing merge everywhere (to avoid rebase); I concur

[ghost]

I think we already decided we were going to have two forks, so the first stage can be public but our own project doesn't have to be.

I am a gitophobe and I don't have a good sense of which manipulations are necessary to keep git's sharp edges from drawing blood and which aren't. (With my preferred tooling, you just commit and push and the rest is automated.) But I do have thoughts about development practices.

First of all, I'm not convinced of the value of manually pulling into master. Everyone on this project is a grownup and nobody needs to be baby-sat. If your changes are ready, push them to master yourself; if they aren't, don't. I recognize that filing pull requests is a way to include a code review step in the data flow, but I don't think it's a good way. If we decide we're determined to manage it this way anyhow, my suggestion would be to do it as follows:

• file a pull request when your code is ready;
• any ONE other person on the project can handle the pull request by reading/reviewing the diff and then rejecting it or approving it and merging it to master;
• if nobody handles the pull request promptly (however that's defined; maybe within 24 hours, maybe less during a deadline rush) push to master anyway, and add a ticket or something to make sure the review gets done later.

In the past we've tried to do review of all commits; it has not worked well. Trying to have the whole group review all commits is too expensive, not so much because it takes time but because it takes time when everyone can be in the same room at once. Having one (other) person review each commit is still useful and much more likely to get done promptly. It is also a really bad idea to have review procedures that cause commits to sit in limbo for long periods of time. This leads to people pulling unreviewed changes from each other under the table and other de facto processes that both destroy the value of the intended scheme and make a mess.

(Another possible way to deal with the delay problem is to have a second sub-master branch that everyone pushes to without review; but this then in practice becomes the de facto master branch and the official master bitrots, and then reviews don't get done anyway.)

I think the right way to handle this for projects where it matters is for everyone to push to master and then have a second "stable" branch where everything is reviewed, plus the expectation is that most commits aren't suitable for the stable branch. This is how release branches are handled outside the git bubble (e.g. by NetBSD) and it works.

The goal is still for the master branch to never be broken, but the way to accomplish this is to test your changes before pushing... and in fact to test your changes while you're working. It is not the case that only the final result of a topic branch should get the full test suite run on it.

The other goal of keeping history is that when the master branch does break, it's possible to identify as closely as possible what change broke it. In general in any complicated code base bugs get introduced and not found promptly, even in the presence of good practices. This happens for a variety of reasons, but one of the big ones is the existence of unrecognized interactions between otherwise distinct subsystems/code units: a change to one violates tacit assumptions in another and things stop working, usually in ways that are only apparent in end-to-end tests and only under specific circumstances. For this reason it's very valuable to be able to bisect the change history to find where a problem appeared. In fact, arguably this is the primary reason for keeping the history around at all.

One should never rebase (even privately) because it creates false history and that breaks bisecting: the changesets generated by a rebase reflect changes to one code unit accompanied by versions of other code units they were not developed or tested against. These combinations may be flatly invalid; but even if they are valid (or superficially valid) they may contain new and different bugs... and even if not, they don't reflect the development history of the interaction and produce meaningless results if reached while bisecting. This is not a theoretical concern; in practice, the changesets generated by rebasing often don't even compile or pass basic tests.

Merging is also bad for bisecting because you can't follow both sides of a merge at once; thus there's a tendency for bisecting to only be able to tell you that a problem was introduced when change series A was merged with change series B; that is, something in change series A and something in change series B don't get along, but you're out of luck trying to get any more detail. For this reason, everyone should always merge early and often. It's fine to work on a "topic branch", but when doing so you should merge with the parent branch as often as reasonably possible. The absolute worst thing to do is to create a topic branch, tool away in isolation for two weeks, then do a bulk merge. Such merges are virtually guaranteed to break something, and they're a lot more work than merging incrementally. If you must hack in isolation for a long period of time, consider doing a zipper merge when you're done. (And similarly, one should never do octopus merges.)

This is another reason why it's important not to create arbitrary procedural delays for getting new changes merged into master.

One other thing: in the past we found at one point that we needed to deploy a rule where code not pushed to the main repo doesn't exist for the purposes of reporting what you've gotten done to Margo. I think this is a good idea in general, and in this environment even more so because of the importance of merging frequently.

If you're starting a topic branch that you expect to last more than a day or two, push it to github as a branch.

[ghost]

Oh, one other thing: we should certainly have a nightly/periodic build run. We could use a 100-megabyte Java blob download that's a whole project to set up (and will doubtless require someone to pet it regularly every time it gets indigestion) ... or we could use a couple hundred lines of shellscript. The former's the modern github-era way! i'll open a separate ticket for this.

[alexpatel]

If your changes are ready, push them to master yourself

I think this may work for CS161 pair programming, but at least in my experience with teams >3 this ends poorly. Maybe I'm just not disciplined enough..

If we decide we're determined to manage it this way anyhow, my suggestion would be to do it as follows:

• file a pull request when your code is ready;
• any ONE other person on the project can handle the pull request by reading/reviewing the diff and then rejecting it or approving it and merging it to master;
• if nobody handles the pull request promptly (however that's defined; maybe within 24 hours, maybe less during a deadline rush) push to master anyway, and add a ticket or something to make sure the review gets done later.

Does "any ONE other person" mean like any random person on the project just needs to read through the code for sanity checks, or is there some hierarchy/delegation for this sort of patch? I would love to be able to have my code reviewed as rigorously as possible as often as possible, but I know that gets time intensive.

In the past we've tried to do review of all commits; it has not worked well. Trying to have the whole group review all commits is too expensive, not so much because it takes time but because it takes time when everyone can be in the same room at once.

What do you consider an appropriate commit? An upstream-worthy feature? I would imagine everyone has quite different (Git) commit styles.

I think the right way to handle this for projects where it matters is for everyone to push to master and then have a second "stable" branch where everything is reviewed, plus the expectation is that most commits aren't suitable for the stable branch. This is how release branches are handled outside the git bubble (e.g. by NetBSD) and it works.

This seems pretty similar to having staging/master; isn't this stable method better suited for release-intensive public projects that require strict versioning, etc.? Do we have those kinds of requirements?

The goal is still for the master branch to never be broken, but the way to accomplish this is to test your changes before pushing... and in fact to test your changes while you're working. It is not the case that only the final result of a topic branch should get the full test suite run on it.

If rebasing and merge are bad for bisect, is there another way to use the Git workflow or are you cornered into using one branch?

For this reason, everyone should always merge early and often. It's fine to work on a "topic branch", but when doing so you should merge with the parent branch as often as reasonably possible. The absolute worst thing to do is to create a topic branch, tool away in isolation for two weeks, then do a bulk merge.

I agree that's bad to do a merge dump like this, but I think Github solves this by tying issues to branches and then submitting pull requests for those branches to independent features or bugs. Are you suggesting that one just Pull Requests with whatever one gets done in a day and that gets merged into master, regardless of whether it resolves a complete issue? I think I am understanding topic branches / this workflow incorrectly..

Oh, one other thing: we should certainly have a nightly/periodic build run. We could use a 100-megabyte Java blob download that's a whole project to set up (and will doubtless require someone to pet it regularly every time it gets indigestion) ... or we could use a couple hundred lines of shellscript. The former's the modern github-era way! i'll open a separate ticket for this.

With CircleCI, we could have this run on every single commit that is pushed to Github... and you would get a Slack/e-mail when the build/tests are done, out of the box! As long as any on-premise tests can be triggered remotely.

[ghost]

I don't think it's a matter of being disciplined so much as a matter of community expectations. I've worked on a lot of shared projects over the years, in and out of academia, with between three and a couple dozen active committers, and they've all used the equivalent of "push directly to master" and rarely seen problems. The only significant problems traceable to this that I can recall were all caused by inadequate supervision of junior staff; rejecting their pull requests avoids the consequences to everyone else but isn't, ultimately, a good way of handling the problem. (Note that as far as I know there's nobody this junior in the group.)

What happens when it goes bad in your experience? Do people make a regular habit of pushing code they haven't even tried compiling, or assume that running the test suite is someone else's problem? Part of being able to bisect effectively is having all changesets work, so it's not good to commit a bunch of crap and then make more commits that fix it, regardless of whether it's merged into master in one lot or not. (And it's even worse to collapse all the changes into one huge one, because then you can't bisect within it.)

If git explodes when used this way then obviously we shouldn't do it. It's certainly the case that git gives people room to screw everything up, like overwriting other people's changes... but I'm under the impression that github has restrictions in place that should mostly prevent that.

What do you consider an appropriate commit? An upstream-worthy feature? I would imagine everyone has quite different (Git) commit styles.

An appropriate commit to having everyone review? I dunno offhand but I guess a good first approximation would be "I wrote this and i'd like to review it in committee because it changes delicate things / I've not touched the frobnitz subtree before / it has far-reaching implications / I'm feeling nervous today", and we do what we can based on the time available and the jointly perceived priority.

Does "any ONE other person" mean like any random person on the project just needs to read through the code for sanity checks, or is there some hierarchy/delegation for this sort of patch?

I was suggesting any one random person just so there's been a second set of eyeballs, because that's one step beyond "nothing". And that's assuming we conclude this is worthwhile; I don't think it is.

I would love to be able to have my code reviewed as rigorously as possible as often as possible, but I know that gets time intensive.

It does. Don't get me wrong; code reviews are generally worthwhile and we should do as much of it as we can afford. That includes reading the diffs from other people's commits as they go by and hollering if you see something wrong. However, based on past experience around here the manpower to do it systematically on the scales needed for the work we try to do isn't available. So what I'm concerned about at the moment is not creating elaborate procedures that we won't be able to follow. Because, as a general rule of organizations, when you do that the procedures that occur in practice are different and usually end up being regrettable.

I think the right way to handle this for projects where it matters is for everyone to push to master and then have a second "stable" branch where everything is reviewed, plus the expectation is that most commits aren't suitable for the stable branch. This is how release branches are handled outside the git bubble (e.g. by NetBSD) and it works. This seems pretty similar to having staging/master; isn't this stable method better suited for release-intensive public projects that require strict versioning, etc.? Do we have those kinds of requirements?

It is. If you have a staging branch that everyone pushes to and that everyone pulls from, it becomes the de facto master (I think I already said this) and then the official master becomes of no value unless it's got some kind of other downstream like a release series. It only works to have everyone push to staging and pull only from master if the delay for things getting from staging to master is minimal.

We might be making releases, maybe, but it's unlikely we'll be maintaining public release branches or making point releases, so we really don't need that level of review.

If rebasing and merge are bad for bisect, is there another way to use the Git workflow or are you cornered into using one branch?

Merge is not bad as long as you do it regularly. I think using large numbers of branches that aren't closely coordinated is a bad idea, except for branches that are explicitly curated as release or stable branches. Basically though this is not a choice between "I'm going to go create a topic branch for my port of Barrelfish to the VAX" and "I'm porting Barrelfish to the VAX and I'm going to push every commit separately to master", it's between "I'm going to go create a topic branch for my port of Barrelfish to the VAX and i'll see you in six months" and "I'm going to go create a topic branch for my port of Barrelfish to the VAX and I'll update my branch from master every morning, or anytime something major goes in".

I think maybe I wasn't clear: when I said "merge with the parent branch as often as reasonably possible" that means "In my branch do 'git merge master'", not "In master do 'git merge mybranch'".

The latter you should do when you're ready, which in ordinary cases shouldn't be more than a couple days if you're working full-time. If you then want a one-to-one correspondence between pushes to master and resolved issues, you might need to subdivide some issues. (To me that seems like silly administrative overhead but I can see how in some environments it might be useful.)

A different approach, which I generally favor but is totally against the git religion as far as I can tell, is to always push any complete non-broken change to master immediately. This has a number of advantages: (a) if there's something subtle wrong with the change, that gets exposed sooner rather than later; (b) it forces you to plan your changes carefully so that you can do smallish complete working changes that are in pursuit of some longer-term goal, and planning is almost always good; (c) it reduces the chance that some bozo will pay no attention to what you're doing until you've spent six weeks hacking and then when you're all done object (rightfully or wrongfully) to absolutely everything about what you've done.

With CircleCI, we could have this run on every single commit that is pushed to Github... and you would get a Slack/e-mail when the build/tests are done, out of the box! As long as any on-premise tests can be triggered remotely.

Let's discuss this in the other ticket... this one is getting long already.

(Is my quoting working?)

[alexpatel]

What happens when it goes bad in your experience? Do people make a regular habit of pushing code they haven't even tried compiling, or assume that running the test suite is someone else's problem?

My experience with a >5 member team was building a real-time bidding server for online ads that served 50k req/sec (and was $$ to run so the code had to work). It was just never was a question of whether anyone ought to submit code that isn't code reviewed on Github by another team member or merge into master directly, etc. It's not that anyone screwed up, but certainly if we had all been left to our own devices the project wouldn't have completed or become profitable imo.

But for this project you've convinced me - what you've outlined seems like the most productive ("agile") approach. From my understanding of the path forward, I also can't imagine that there will be a lot of conflicts because we'll all be in different parts of the tree. (I was also reading "everyone should always merge early and often" as everyone should merge their developments branches into the master branch, not master -> devel, which was why I was confused.)

Part of being able to bisect effectively is having all changesets work, so it's not good to commit a bunch of crap and then make more commits that fix it, regardless of whether it's merged into master in one lot or not. (And it's even worse to collapse all the changes into one huge one, because then you can't bisect within it.)

I agree that all commits should work & pass test suite; maybe enforce git squash into one commit for every pull request? Or is it worthwhile to have commits like "first pass at graph script" lying around in the tree for bisect.

If git explodes when used this way then obviously we shouldn't do it. It's certainly the case that git gives people room to screw everything up, like overwriting other people's changes... but I'm under the impression that github has restrictions in place that should mostly prevent that.

No, this is a perfectly reasonable way to use Git - definitely.

If you have a staging branch that everyone pushes to and that everyone pulls from, it becomes the de facto master (I think I already said this) and then the official master becomes of no value unless it's got some kind of other downstream like a release series.

I think this would only happen if master is just a hard mirror of staging. We could do something like: to get a pull request accepted into staging, you have to pass all the virtualizable tests. To get it onto master, you then have to pass the virtualizable and on-premise tests. If the master tests fail, you fix the staging branch until it works and then merge.

I think maybe I wasn't clear: when I said "merge with the parent branch as often as reasonably possible" that means "In my branch do 'git merge master'", not "In master do 'git merge mybranch'".

yes, haha

A different approach, which I generally favor but is totally against the git religion as far as I can tell, is to always push any complete non-broken change to master immediately.

Seems fine to me, I would only say: (a) yes, but we could just enforce this downstream (b) you should do this anyway (c) again, I don't think we need to be forced into this, we can just have a policy that you have to submit sane units of work

[alexpatel]

@dh6713 I am going to close this we should talk about this over lunch or something, I don't want to misallocate your keyboard time

edit: waste

[ghost]

We should talk I guess, but we still ultimately need to pick something, so I'm going to reopen it.

anyway it's not a waste of time, I think the text above is the best so far I've managed to articulate why rebasing is bad, and that's kind of important in the broad scheme of things...

[alexpatel]

Fair enough - yes, thank you for articulating, I wasn't trying to cut you off and myself have more to say/show - i was told we'd have this discussion in group meeting tomorrow and didn't want to distract you

[margoseltzer]

This is on the agenda for tomorrow.

• M

On Jun 8, 2017, at 5:39 PM, dh6713 notifications@github.com wrote:

Reopened #26.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[ghost]

Nobody told me that (until just now)... anyway if you want to talk before the meeting tomorrow let's do that, assuming there's time available (I'll be going to Dan's practice talk at 1).

It would probably be useful to get on the same page beforehand. I feel like my input's likely to be ignored otherwise because obviously it's just ranting about git. :-/

[alexpatel]

Cool - I'll be around - there are definitely some core parts of what you've outlined that I would also be behind, for sure

[alexpatel]

From group meeting 6/9:

- stable = master, dev can also be committed to
- need pull requests from dev to master, this happens infrequently, needs to work
- in dev can be more flexible about what can be committed
- stable: what we ship to LL, where we tag things that correspond to papers we write,
- getting into dev:
    - pull request into dev too
    - the minimum is that you've run all the regression tests
    - ideally it should be code reviewed, but this isn't strict - if code
    review is lagging, you can push into dev and get yelled out if it's broken
    - everyone should do code reviews
- gonna take us a while to differentiate stable and dev, just always run all the tests on whatever you can

- repos:
    - there are changes that are good for bf
    - there are also changes relevant to synthesis that eth won't take
    - one repo or two repos
    - the one repo solution is going to fail because guppy will diverge from bf 
    - guppy will be a subset of bf, not bf + other things
    - two repos is better for diverging princess + barrelfish trees

- guppy grows into a big fish by starting with minimal kernel from barrelfish put into guppy
    - david thinks this is a bad idea unless we want to put a lot of time in, will take months to get minimal kernel
- mark will write a script to deal with build artifacts

- don't put synthesized files in the tree
- put random tools you write into the tree

[mwookawa]

i'm going to leave this on backlog as we organically wander into our workflow, but please feel free to add notes on how things are going.

[ghost]

Also from the meeting we decided we wanted to have a pull request for every merge into dev, in order to track metadata that git itself throws away.

This adds a fair amount of administrative overhead (every time you want to push you have to first push to a branch on github, then log into the website and muck around) but we'll see how it goes I guess.

[alexpatel]

hub is a very usable (MIT-licensed) CLI client that adds Github features like PRs etc. You can certainly get along just fine without ever having to use their website.

[marikgoldstein]

Alex and I just had a conversation about the princess git workflow. I just put down what Alex and I decided from our conversation. Please add to this page.

https://github.com/Harvard-PRINCESS/Guppy/wiki/PRINCESS-GIT-WORKFLOW

[mwookawa]

hub is awesome, by the way, and it interacts very cleanly with the new github desktop gui. gitk still also displays a very nice image of your local repo in terms of hashed points, histories, joins and so on.