4 ms·

I'm a huge fan of Github. I recognize the huge effect they've had, transforming the Open Source community, and I'm grateful for it. I also love how they run the

I'm a huge fan of Github. I recognize the huge effect they've had, transforming the Open Source community, and I'm grateful for it. I also love how they run their company, how open and talkative they are about their organization. Their blog holds many gems. However, I cannot work on Github. In my opinion, the Pull-Request mechanism is weak because it fails to address a major point: It's highly improbable that I accept the PR on the first try. There's going to be a lot of back and forth discussion, reviews, remarks, etc. Standard PRs I deal with on Github usually end up with several redundant commits. When I ask people politely to rebase all the commits they're sending into one, most don't know how to do it. (It's okay not to know. It's weird when someone with several contributions per day never had to do it before). If I `git commit --amend`, my PR is actually overwritten and I lose the history of the first patch I've sent. I started disliking working with Github ever since I started working on OpenStack. The process to send a patch there might seem a bit daunting at first, but it's really not that complicated. Using Gerrit (https://code.google.com/p/gerrit/ https://code.google.com/p/gerrit/) for code review has many advantage, like limiting your patch to one commit, keeping a detailed log history of successive patch sets, and generally making reviewing more inviting. On top of that, the whole suite of tests is ran against each submitted patch in a virtually never resting CI server. And finally, I don't find Github's Pull Request that "easy, simple and fast". My workflow with OpenStack is a lot faster. 1 command: $ git review A colleague of mine says it a lot better than I do: http://julien.danjou.info/blog/2013/rant-about-github-pull-request-workflow-implementation http://julien.danjou.info/blog/2013/rant-about-github-pull-r... If I recall correctly, Linus Torvalds went on a highly publicized rant against Github PRs not that long ago. I'm not arguing that every Open Source project should have a complete QA infrastructure, and Github is a great place to deal with your first Pull Requests. However, I do argue that you can very quickly reach the limit of what Github can give to you in terms of collaborative tools. My rule of thumb: If you have more than 10 contributors, at least 4 of which are active daily, it may be worth it to invest in some real infrastructure.

prezjordan 13y ago
> When I ask people politely to rebase all the commits they're sending into one, most don't know how to do it. Very easy solution to this problem. 1) Make note of it in CONTRIBUTING.md. 2) Do it for them. Check out their branch, rebase/squash/fix whitespace/etc and merge.
jedbrown 13y ago
I agree completely about revisions on PRs. Also, proposed patches often spawn much broader technical discussions that are much better on a mailing list than on a random commit (that will likely become unreachable later since it's not going to be accepted). Nothing beats the archival quality of mailing lists and if you start your code review there, you never need to make the choice to "move" a discussion from GitHub to a mailing list. The problem is that mailing list review requires a lot of discipline and some popular MUAs make it especially painful. The value of PRs and other models is that they degrade more gracefully to lack of experience/discipline, and (perhaps) to casual involvement (subscribing to mailing lists with delivery turned off is fine as long as there is a convention to Cc people that are likely interested in a specific change; not many people know about subscribe-without-delivery).
- ak217 13y ago
  > proposed patches often spawn much broader technical discussions that are much better on a mailing list than on a random commit (that will likely become unreachable later since it's not going to be accepted) PRs are also issues, and I vastly prefer GitHub issues to mailing lists (to each his own, I guess).
  - jedbrown 13y ago
    I like inline code comments, especially when the original commits are not well factored. But I find it too common for a discussion starting from a patch to gradually become general and ultimately involve dozens of people that had no interest in the original patch, but care a lot about the general discussion. Sometimes these involve multiple projects, in which case cross-posting to another mailing list makes sense. With GitHub Issues, you generally tag individuals rather than groups of perhaps hundreds of people (most of whom filter on their own criteria). Later, how well does search find the 100-message design discussion starting on a commit in a PR that was later amended and then rejected? And what if you move the repository elsewhere?
  - gsnedders 13y ago
    PRs being issues is an issue in an of itself: it tempts one into not opening an issue for the bug the PR is fixing. Then, if the PR is closed, there is no open issue for the underlying bug it was attempting to fix.
arsenerei 13y ago
I am a huge fan of Gerrit, other than its UI. I ran it along with Jenkins[0] at a previous company. Its review model is far, far, far superior to Github's. As you said, it keeps successive patch sets, and allows for diffs against previous patch sets, rather than only against the original parent. This allows for you to see only the most lastest changes, so you don't have to wonder if anything else was changed and review the entire patch. > $ git review I actually wrote a bash script with the same name and a few other shortcuts before my company started working on OpenStack. I was quite pleased with the workflow I provided the team I was on. $ git start <new-branch> <branch-from> $ <do work as normal> $ git review $ git finish # removes the branch These three commands along with the git-flow branching model[1] (but not the tool itself), leads to clean, sensible history in my opinion. I respect that Github tries to maintain a low-barrier of entry to increase use and ease, but I believe there's a way to maintain it while still having a great patch-review model. 0: http://jenkins-ci.org/ http://jenkins-ci.org/ 1: http://nvie.com/posts/a-successful-git-branching-model/ http://nvie.com/posts/a-successful-git-branching-model/
steveklabnik 13y ago
I type out the instructions on how to squash a pull request so often that I just posted it to my blog, so I don't need to type it over and over: http://blog.steveklabnik.com/posts/2012-11-08-how-to-squash-commits-in-a-github-pull-request http://blog.steveklabnik.com/posts/2012-11-08-how-to-squash-... That said, I do like pull requests: you might lose the history locally, but the PR contains a 'see outdated diff' that shows what it used to look like.
gsnedders 13y ago
I'm using GitHub more and more — ultimately because so many more developers are willing to submit patches there than elsewhere (presumably — at least — because they already have a clue what they're doing, rather than having to work out how to submit something). In the end, what I've ended up doing is using Travis CI with its GitHub hook, which gives the whole suite of tests against each and every patch, as well as using external code review (mostly using Critic (https://github.com/jensl/critic https://github.com/jensl/critic), which supports explicitly rebasing branches, collapsing reviews into all pending commits, and so on, all gracefully — unlike GitHub's code review). While I'd like something better, the issues it throws up (not using the default code review system most obviously) as well as — as you say — the complexity of submitting a PR, are, in my opinion, outweighed by the extra contributions that are got by using something other developers are already comfortable with.
saraid216 13y ago
A coworker and I recently discussed how Github ought to invest some time into templated workflows. Conventions like "make a branch per pull request" can be mandated by a repo owner, as could "rebase all your commits". At minimum, providing a TODO list for pull request submissions would be good.
stormbrew 13y ago
Do you (or anyone else) have a link to this alleged Torvalds rant about PRs? I have mixed feelings about the idea that all contributions should be squashed. I see the attempt at fastidiousness, but I also think that git makes it very easy and reasonable to keep multi-change history while linking it to only one commit in the eventual target repository. I do wish it was easier to rebase a branch such that backmerges were pulled out where possible, cleaning up the graph at least, but I tend to look at my soup branch (master, usually) with git log --first-parent most of the time anyways and that's not much different from if they'd been squashed.
- icebraining 13y ago
  Re: Linus' rants against PRs: https://github.com/torvalds/linux/pull/17 https://github.com/torvalds/linux/pull/17
  - stormbrew 13y ago
    Thanks. Definitely an interesting read.
sdesol 13y ago
"My rule of thumb: If you have more than 10 contributors, at least 4 of which are active daily, it may be worth it to invest in some real infrastructure." Disclaimer: I'm the creator of GitSense. We are working on a solution to make GitHub pull requests enterprise ready. I do agree that GitHub's pull request model is not quite enterprise ready, but they have a solid foundation that you can build on top of. With their API, were able to build a solution that I believe will address most of its short comings. For example, my first concern with GitHub's pull request system, is it is at the repository level. With Gerrit, you can see requests at the branch level. With our enhancements you'll be able to track pull requests from different repositories at the branch level: http://screenshots.gitsense.com/enterprising-github-pulls.html#pulls-at-branch-level http://screenshots.gitsense.com/enterprising-github-pulls.ht... We are also able to address the concern of dealing with new commits. With our Smart Attributes technology, it is very easy to flag what commits you have reviewed. http://screenshots.gitsense.com/enterprising-github-pulls.html#mark-unreviewed-commits http://screenshots.gitsense.com/enterprising-github-pulls.ht... And then refine the list so you'll only have to deal with the newer commits. http://screenshots.gitsense.com/enterprising-github-pulls.html#refined-commits-list http://screenshots.gitsense.com/enterprising-github-pulls.ht... We also take care of the problem of not having a side by side diff. With our solution, you'll be able to use side by side diffs to review pull requests. http://screenshots.gitsense.com/enterprising-github-pulls.html#side-by-side-diffs http://screenshots.gitsense.com/enterprising-github-pulls.ht...

prezjordan 13y ago

> When I ask people politely to rebase all the commits they're sending into one, most don't know how to do it. Very easy solution to this problem. 1) Make note of it in CONTRIBUTING.md. 2) Do it for them. Check out their branch, rebase/squash/fix whitespace/etc and merge.

jedbrown 13y ago

I agree completely about revisions on PRs. Also, proposed patches often spawn much broader technical discussions that are much better on a mailing list than on a random commit (that will likely become unreachable later since it's not going to be accepted). Nothing beats the archival quality of mailing lists and if you start your code review there, you never need to make the choice to "move" a discussion from GitHub to a mailing list. The problem is that mailing list review requires a lot of discipline and some popular MUAs make it especially painful. The value of PRs and other models is that they degrade more gracefully to lack of experience/discipline, and (perhaps) to casual involvement (subscribing to mailing lists with delivery turned off is fine as long as there is a convention to Cc people that are likely interested in a specific change; not many people know about subscribe-without-delivery).

ak217 13y ago
> proposed patches often spawn much broader technical discussions that are much better on a mailing list than on a random commit (that will likely become unreachable later since it's not going to be accepted) PRs are also issues, and I vastly prefer GitHub issues to mailing lists (to each his own, I guess).
- jedbrown 13y ago
  I like inline code comments, especially when the original commits are not well factored. But I find it too common for a discussion starting from a patch to gradually become general and ultimately involve dozens of people that had no interest in the original patch, but care a lot about the general discussion. Sometimes these involve multiple projects, in which case cross-posting to another mailing list makes sense. With GitHub Issues, you generally tag individuals rather than groups of perhaps hundreds of people (most of whom filter on their own criteria). Later, how well does search find the 100-message design discussion starting on a commit in a PR that was later amended and then rejected? And what if you move the repository elsewhere?
- gsnedders 13y ago
  PRs being issues is an issue in an of itself: it tempts one into not opening an issue for the bug the PR is fixing. Then, if the PR is closed, there is no open issue for the underlying bug it was attempting to fix.

arsenerei 13y ago

I am a huge fan of Gerrit, other than its UI. I ran it along with Jenkins[0] at a previous company. Its review model is far, far, far superior to Github's. As you said, it keeps successive patch sets, and allows for diffs against previous patch sets, rather than only against the original parent. This allows for you to see only the most lastest changes, so you don't have to wonder if anything else was changed and review the entire patch. > $ git review I actually wrote a bash script with the same name and a few other shortcuts before my company started working on OpenStack. I was quite pleased with the workflow I provided the team I was on. $ git start <new-branch> <branch-from> $ <do work as normal> $ git review $ git finish # removes the branch These three commands along with the git-flow branching model[1] (but not the tool itself), leads to clean, sensible history in my opinion. I respect that Github tries to maintain a low-barrier of entry to increase use and ease, but I believe there's a way to maintain it while still having a great patch-review model. 0: http://jenkins-ci.org/ http://jenkins-ci.org/ 1: http://nvie.com/posts/a-successful-git-branching-model/ http://nvie.com/posts/a-successful-git-branching-model/

steveklabnik 13y ago

I type out the instructions on how to squash a pull request so often that I just posted it to my blog, so I don't need to type it over and over: http://blog.steveklabnik.com/posts/2012-11-08-how-to-squash-commits-in-a-github-pull-request http://blog.steveklabnik.com/posts/2012-11-08-how-to-squash-... That said, I do like pull requests: you might lose the history locally, but the PR contains a 'see outdated diff' that shows what it used to look like.

gsnedders 13y ago

I'm using GitHub more and more — ultimately because so many more developers are willing to submit patches there than elsewhere (presumably — at least — because they already have a clue what they're doing, rather than having to work out how to submit something). In the end, what I've ended up doing is using Travis CI with its GitHub hook, which gives the whole suite of tests against each and every patch, as well as using external code review (mostly using Critic (https://github.com/jensl/critic https://github.com/jensl/critic), which supports explicitly rebasing branches, collapsing reviews into all pending commits, and so on, all gracefully — unlike GitHub's code review). While I'd like something better, the issues it throws up (not using the default code review system most obviously) as well as — as you say — the complexity of submitting a PR, are, in my opinion, outweighed by the extra contributions that are got by using something other developers are already comfortable with.

saraid216 13y ago

A coworker and I recently discussed how Github ought to invest some time into templated workflows. Conventions like "make a branch per pull request" can be mandated by a repo owner, as could "rebase all your commits". At minimum, providing a TODO list for pull request submissions would be good.

stormbrew 13y ago

Do you (or anyone else) have a link to this alleged Torvalds rant about PRs? I have mixed feelings about the idea that all contributions should be squashed. I see the attempt at fastidiousness, but I also think that git makes it very easy and reasonable to keep multi-change history while linking it to only one commit in the eventual target repository. I do wish it was easier to rebase a branch such that backmerges were pulled out where possible, cleaning up the graph at least, but I tend to look at my soup branch (master, usually) with git log --first-parent most of the time anyways and that's not much different from if they'd been squashed.

icebraining 13y ago
Re: Linus' rants against PRs: https://github.com/torvalds/linux/pull/17 https://github.com/torvalds/linux/pull/17
- stormbrew 13y ago
  Thanks. Definitely an interesting read.

sdesol 13y ago

"My rule of thumb: If you have more than 10 contributors, at least 4 of which are active daily, it may be worth it to invest in some real infrastructure." Disclaimer: I'm the creator of GitSense. We are working on a solution to make GitHub pull requests enterprise ready. I do agree that GitHub's pull request model is not quite enterprise ready, but they have a solid foundation that you can build on top of. With their API, were able to build a solution that I believe will address most of its short comings. For example, my first concern with GitHub's pull request system, is it is at the repository level. With Gerrit, you can see requests at the branch level. With our enhancements you'll be able to track pull requests from different repositories at the branch level: http://screenshots.gitsense.com/enterprising-github-pulls.html#pulls-at-branch-level http://screenshots.gitsense.com/enterprising-github-pulls.ht... We are also able to address the concern of dealing with new commits. With our Smart Attributes technology, it is very easy to flag what commits you have reviewed. http://screenshots.gitsense.com/enterprising-github-pulls.html#mark-unreviewed-commits http://screenshots.gitsense.com/enterprising-github-pulls.ht... And then refine the list so you'll only have to deal with the newer commits. http://screenshots.gitsense.com/enterprising-github-pulls.html#refined-commits-list http://screenshots.gitsense.com/enterprising-github-pulls.ht... We also take care of the problem of not having a side by side diff. With our solution, you'll be able to use side by side diffs to review pull requests. http://screenshots.gitsense.com/enterprising-github-pulls.html#side-by-side-diffs http://screenshots.gitsense.com/enterprising-github-pulls.ht...