Hacker News Reader

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).

> 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).

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?

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.

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/ 1: http://nvie.com/posts/a-successful-git-branching-model/

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-...

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.

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), 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.

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.

Re: Linus' rants against PRs: https://github.com/torvalds/linux/pull/17

Thanks. Definitely an interesting read.

"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.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.ht...

And then refine the list so you'll only have to deal with the newer commits.

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.ht...

> 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.

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.