Go Proposal: Accept GitHub PRs
github.com
github.com
As far as I can tell, Github's latest additions to their code review flow pretty much brings Github on par with Gerrit (and with also a much larger mind share. Gerrit has always remained quite obscure and marginal).
I don't think the issues were really that serious, but the Go team likely saw no reason to change away from the tool they were comfortable with for a slightly worse tool that they would have to adapt to.
I regularly collaborate on github with people who are familiar with either Gerrit or Phabricator, and I've yet to see someone praise github for having a better workflow. Much of what they announced recently feels like alpha quality/playing catch up.
I haven't used Gerrit in a few years though, maybe it's changed.
Github has certainly come far this last year, but these are all oh so simple and basic stuff.
Changesets take the typical GitHub workflow where the author force pushes to the PR to address comments and creates another "version" of that PR, with a new set of inline comments, but sharing the overarching PR comments. You can compare and contrast any set of "PR versions" in order to determine the changes applied by the author.
I desperately wish GitHub PRs worked this way.
For example, if a contributor force pushes a new commit to a PR branch, as a reviewer, I can't see the old version and compare against it. I'd have to tell the contributor to not do that (doesn't work for first time contributors), or always try to manually backup each stage of a PR and compare with locally. This doesn't happen on Gerrit because it tracks the entire history of the change.
Second thing. Once a PR gets large and lasts a while, it's hard to see what is going on, what review comments still need to be addressed, and where new replies are showing up. Consider this PR [0] as an example, can you tell where my "latest reply" was? It's not at the bottom because it was a reply to one of the early inline conversations, and you have to expand all the "Show outdated" links manually to find it.
Those are just a few things off the top of my head, as someone who used PRs and Gerrit. I prefer PRs but I don't think they're viable for a project that takes code quality and code review as seriously as Go does. I do hope PRs get the remaining missing pieces to be able to match Gerrit.
> For example, if a contributor force pushes a new commit to a PR
> branch, as a reviewer, I can't see the old version and compare
> against it.
For example, I submitted this PR against uWSGI a few months ago: https://github.com/unbit/uwsgi/pull/1392
Initially I screwed it up and inserted a field in the middle of a struct, whereas within a stable release it should of course be added to the end. If you click on "show outdated" you can see that old diff where I'm adding the field to the middle of the struct, but click on "files changed" and it's the current change where I fixed the issue.
I do think GitHub could improve here, they could expose forced pushes via refs/pull/NUM-COUNTER instead of just refs/pull/NUM as they do now, but it's important to point out that they're handling the common case. When someone force-pushes a pull request they're usually doing so in response to comments on the request, and those comments will still make sense because GitHub saves away the old diff, which you can compare with the current state.
The automation would be even more important for a codebase like Chromium that receives 5-8 change lists every minute [2].
We don't need to roll back tree-breaking changes because the bots are the only ones allowed to land changes in the first place, and they make sure all tests pass before they land. There's no reason a bot couldn't do this, though.
That said, Google might consider it easier to work with Gerrit, Rietveld, Gitiles, etc. because they can work with the source code of these projects directly, than having to write potentially ugly code to work around missing pieces in GitHub's API.
I'd love to learn whether a large scale project such as Rust has run into these issues with GitHub.
There's some stuff that could be nicer with GitHub generally, but I don't think having the source would make it any easier to build out this automation; github's API is pretty solid.
We don't enforce its use. If a newcomer makes a pull request, we'll just use the GH review interface. In general simple PRs go through the GH review interface, regardless of who they're from. We break out Reviewable when a PR actually needs it (this is usually the choice of the reviewer).
This means that Servo continues to have a lower bar to contribution, without hampering maintainers on PRs that need more than GHs set of review features.
This has worked out well for us.
4 open PR's! https://github.com/elixir-lang/elixir/pulls
> And besides, if they want to they can just diff it on their own machines with whatever tools they like!
Oh yes, a very compelling argument when discussing workflow tools is to suggest local diffs. Bravo!
That's a pretty tough thing to do. Most (all?) Apache projects require a CLA, as does Go, Java, Mysql, Cassandra... It's pretty standard for any project that is both managed by a corporation and accepts third-party contributions.
EDIT: removed Postgres. Thanks, 'anarazel!
Edit: another post states that Postgres doesn't require a CLA for contributors. So I don't use any software requiring a CLA.
CLAs aren't about making a project more bureaucratic, they're about transferring the rights over the code you're contributing from you to the project, so that there's no dispute over who the code belongs to.
Say I work at MegaCorp (they make heavy use of Go in some backend systems), and didn't bother making sure I had permission to contribute on MegaCorp time (and thus, contribute MegaCorp IP). I make some PRs to Go which improve the Go garbage collector, which Google happily accepts. These changes help Google and they could help MegaCorp if some of their projects use Go. Hooray!
Somewhere down the line, someone at MegaCorp notices that I've been contributing to Go's garbage collector on MegaCorp time without permission. I may or may not be fired. However, Google has now benefited from what is legally MegaCorp IP without the permission of MegaCorp. If this is in the USA, there's a decent chance that MegaCorp tries to sue Google. That's a headache that I'm sure nobody at Google would want to deal with.
If there was a CLA which I had to sign, saying that I had permission to contribute MegaCorp IP, Google has a document clearly saying that I had permission to contribute MegaCorp IP. This is a far better situation for Google than if they had no such document.
And Google's benefit from your MegaCorp contributions remains the same. MegaCorp legal recourses remain the same.
The situation is exactly the same.
(2) Either way Google competitor has damages on MegaCorp.
afaik, CLAs and "open source licenses" are different things with different purposes. as such, you can have both a CLA and be BSD.
Not the case - they can claim it is so, but a BSD license with extra clauses is not a BSD license.
As an example, many projects demand copyright assignment through a CLA from contributors, which does not add any restrictions to your use of the project allowed through the BSD license. You still can modify, redistribute, ... the source and products build on it as you wish, the CLA is only relevant if you ask the upstream project to merge some of your changes. The BSD license says nothing about how the upstream project has to accept your proposed changes.
--- scratch that! Ok, I'm a bit out of date: linux's DCO solves this problem without requiring a DCA. But think of other projects that want to have long term growth and what challenges could arise in the face of changing attitudes over time.