Go Proposal: Accept GitHub PRs(github.com)
github.com
Go Proposal: Accept GitHub PRs
https://github.com/golang/go/issues/18517
54 comments
While recent PR improvements have made it a lot better, there are still a few things missing that I know of.
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.
[0] https://github.com/google/go-github/pull/497
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.
[0] https://github.com/google/go-github/pull/497
> 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.
Changesets are a big deal for seeing progress towards code review feedback.
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.
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.
I use a tool https://reviewable.io/ which adds this functionality (and some other stuff) on top of GitHub PRs and it's invaluable.
+1 for reviewable. In addition to this feature reviewable's diff display is a lot nicer. For example it does a much better job handling whitespace only changes when a block of code gets reindented but otherwise doesn't change.
Oh yeah, collapsing whitespace-only changes is excellent. But even better is how it can detect and collapse changes introduced by rebasing the PR.
You can hide whitespace-only changes in GitHub by appending ?w=1 to the end of the url too.
But that makes the whitespace changes literally invisible. Reviewable strikes a good balance, where it will show you whitespace changes that appear in diff hunk contexts, and also explicitly call out when it's skipping a chunk of whitespace-only changes, and you can click on that callout to ask it to show you those whitespace changes anyway.
Reviewable doesn't hide whitespace changes, it shows just enough so you can tell that whitespace changed without getting in the way of reading the rest of the diff.
You should try Phabricator. Diffs work this way.
The new github workflow is an improvement, but still has a lot of things which need to be better design/implementation.
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 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.
Phabricator is shit from my experience but only used it with svn backend. Gerrit's main downside for me personally is that the web interface captures bowsers search function. While it's nice, I don't like my 'os' level functionality to be overwritten.
I strongly prefer GitHub's flow to Gerrit or Phabriactor. Those tools have far too much complexity; I find the new GitHub stuff to strike a nice balance.
I haven't used Gerrit in a few years though, maybe it's changed.
I haven't used Gerrit in a few years though, maybe it's changed.
What I miss at github is a single view that list all the commits in a pull request, in sequence, with both commit messages and diff contents, with possibility to comment both message and diff. Oh, and have the commits show in the actual order the commits will show up in the history.
Github has certainly come far this last year, but these are all oh so simple and basic stuff.
Github has certainly come far this last year, but these are all oh so simple and basic stuff.
Automation would be a primary reason. Gerrit and Rietveld [1] allow Google to build bots and tooling to land commits, allocate build infrastructure resources for tests, and rollback tree-breaking changes. GitHub would not be quite so hackable.
The automation would be even more important for a codebase like Chromium that receives 5-8 change lists every minute [2].
[1] https://github.com/rietveld-codereview/rietveld
[2] https://codereview.chromium.org/
The automation would be even more important for a codebase like Chromium that receives 5-8 change lists every minute [2].
[1] https://github.com/rietveld-codereview/rietveld
[2] https://codereview.chromium.org/
GitHub absolutely is scriptable this way; we at the Rust project use various bots extensibly for landing commits, build infrastructure for tests, welcoming new contributors...
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.
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.
Apologies for the misleading GitHub portion in my previous comment. I agree, Rust is wonderfully maintained on GitHub.
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.
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.
It's all good! I didn't think you were saying Rust was somehow mis-managed.
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.
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.
IIRC one of the problems was github notifies you as soon as a comment is posted, potentially wasting your time if you go look before the comments are finalized. For example you may see a comment asking how something works, and waste time replying only to see the comment was since deleted because the reviewer figured it out.
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 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.
The new GitHub changes allow you to group a set of comments into a "review" that then notifies the recipient all at once, along with letting you mark the PR as "approved" or "needs changes".
Interesting. I found Gerritt's behavior (the flipside) to be a little frustrating. I just have one comment, right here: <x> -- why do I need to publish it and give it a meta-comment?
GitHub's new flow gives you both choices: a "just this one comment" button and a "start a new review" button.
Id be surprised if there's a really compellingreason to go with gerrit over github in terms of features, but the overhead of switching all the tooling (gocontribute tool, trybots etc.) is probably pretty high. No reason to spend a big chunk of time switching from one pretty good toolset to another pretty good tool set if you can avoid it.
[deleted]
The other arguable positive here is one that may be hard for them to say: That using GitHub to "lower the bar for contribution" may itself not be a positive.
How many other review / CVS have you used. I really hate github and pull requests. They are by far the worst tools I've used and I've used too many to count.
Gerrit has superior configuration in best programming language Prolog. Hail Prolog!
With Servo we use https://reviewable.io/ on github.
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.
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.
I've contributed to another Google project (http://vitess.io) that used the the same process. So while Go is a completely different team, it's not exactly unknown within Google. Reviewable is a dramatic improvement over the stock Github tooling.
Seems like a lot of hoops to jump through, I wonder why they want to use Gerrit. Meanwhile Elixir you open a PR and Jose and the gang address it quickly.
4 open PR's! https://github.com/elixir-lang/elixir/pulls
4 open PR's! https://github.com/elixir-lang/elixir/pulls
Hey, we're getting downvoted! I guess gophers aren't so pragmatic after all, I thought they followed New Jersey style!
I can't help thinking that requiring a specific code review tool is a bit ridiculous, especially considering the hoops they want to jump through to enforce it.
Because maintainer timer is, in your world, a worthless thing?
[deleted]
No, because the tools are very similar and they can just ask contributors to adapt to the maintaners wishes. Code diffs are such a small part of reviewing a new feature. "Just deal with it" is what I think. I don't require notepad on my Mac, so why should they require gerrit on github? And besides, if they want to they can just diff it on their own machines with whatever tools they like!
Gerrit and Github are not "very similar". They are quite different. (Something that would be rather immediately clear to anyone who used either for some duration)
> 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!
> 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!
Well, I happen to have used both. And it all depends on your perspective. I would change my tools in a heartbeat to streamline the process for users or contributors. To me it depends on if you're user focused or technology driven. I think it's kind of akin to switching to git but insist that all merges have been done with perforce merge, because I think it's better in some way. Sure, it might be true, but do you really want to go against the stream to prove your point?
I contribute pull requests to quite a few github projects. However I avoid contributing to (or even using) projects with a Contributor License Agreement. Such projects are too bureaucratic.
> I avoid contributing to (or even using) projects with a Contributor License Agreement.
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!
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!
~~You got me on Postgres. Don't use the others.~~
Edit: another post states that Postgres doesn't require a CLA for contributors. So I don't use any software requiring a CLA.
Edit: another post states that Postgres doesn't require a CLA for contributors. So I don't use any software requiring a CLA.
Postgres doesn't have a CLA.
All of those projects also have alternatives that don't require a CLA. The solution is to avoid projects by bureaucratic corporations.
Why? Aside from having to click a button saying that you agree to the CLA, how does it affect you in any way?
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.
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.
I don't like the idea of handing over my contributions to another party under their terms. Either release software under a common open source license (BSD, MIT) or not. Don't add additional terms with some custom license that certainly wouldn't help me.
Another aspect of a CLA is the ability for a company to cover themselves legally.
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.
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.
I don't understand. Whether you had the document or not, MegaCorp's lack of permission remains the same.
And Google's benefit from your MegaCorp contributions remains the same. MegaCorp legal recourses remain the same.
The situation is exactly the same.
And Google's benefit from your MegaCorp contributions remains the same. MegaCorp legal recourses remain the same.
The situation is exactly the same.
No, because before Google was guilty of using MegaCorp IP, but now, you're guilty of telling them they could, so you are responsible now, not Google.
I'd like to hear that opinion from a lawyer. While it makes sense, the reality of the law doesn't always make sense.
(1) Either way you're guilty.
(2) Either way Google competitor has damages on MegaCorp.
(2) Either way Google competitor has damages on MegaCorp.
> Either release software under a common open source license (BSD, MIT) or not
afaik, CLAs and "open source licenses" are different things with different purposes. as such, you can have both a CLA and be BSD.
afaik, CLAs and "open source licenses" are different things with different purposes. as such, you can have both a CLA and be BSD.
> 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.
Not the case - they can claim it is so, but a BSD license with extra clauses is not a BSD license.
A CLA generally does not add clauses to the license. It generally contains extra rules contributors have to follow to contribute to the "main" codebase, but these are unrelated to the license. If a project has a CLA, you still have the full rights the license grants you, and you can do everything that license allows you to do without signing the CLA. The CLA only governs contributions to upstream, and I'm not aware of any open-source license that says anything about those.
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.
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.
I agree that CLAs are a bit of a hassle, but it makes things much saner for licensors and licensees alike. One high profile project that doesn't use them is the linux kernel. It's mostly only the uniformity of mind among contributors that keeps things on course. Can you imagine the consequences if one or more developers decided that they wanted to change the terms for their contributions?
--- 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.
--- 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.
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).