have a look at gerrit[1] if you are a git user. it's a complete rewrite of rietveld (in java) and tied closely to git which is because the main contributor is Shawn Pearce (who is also main contributor to git[2]).
I think Gerrit is a really impressive piece of software. Please correct me if I'm wrong but if you're looking for a major user of Gerrit I believe the Android OSP uses it.
As much as I like Gerrit I don't think I can use it for our team because Gerrit wants you to review commits, and our team works in terms of whole branches. Reviewing your individual commits made early in the branch history probably isn't useful as bugs may be fixed--or the code entirely changed--by later commits. Anyone else have this problem with Gerrit, reviewing commits vs. reviewing branches?
I'm sorry, but I think that your team has this backwards.
If you wait until a branch is done, then reviewers are being asked to look at big chunks of code. That is a lot more effort, and it is harder to get reviewers to do it. And when they do do it, it is harder to get them to do a thorough job of it. Furthermore when they notice design issues, it is harder to get developers to go back and redo all of their working code to make it better.
Therefore if you want effective code review, you need to review early and review often. The smaller a change is, the easier it is to review, the easier it is to be thorough, and the easier it is to accept suggested changes.
My conclusion is therefore that choosing to do code review after the fact means that you are guaranteeing no proper code review. But when you review early - literally on every commit - it is then much easier to maintain a truly effective code review process. It initially "feels" much heavier. But after you get in the rhythm, your code will be much, much improved.
We use gerrit at my company, and what you say is true. Developers are told to do one feature or bug fix per commit/gerrit change. It really does help with the review process and things move much faster.
No matter how often you want to review code, I think it could work for your team like this:
- dev works on a feature branch, making multiple commits and pushes
- once ready, squash all the commits and submit it to gerrit
[- perhaps have hudson/jenkins run the unit tests at this point automatically]
- have the code review in gerrit
- once the review is done, gerrit would merge it into the develop or master branch (depending on your git workflow)
That approach blends in nicely with git-flow (1). If you want to be sure that no single dev is pushing to the develop or master branches you'd need to setup per-branch permissions, which can be done with gitolite (2). Too bad github (even the self-hosted version) doesn't support per-branch permissions, which forces organizations that use it and only want gerrit to be able to push into the main branch to do excessive repo forking instead of using feature branches. Also I'd love to be able to do ad-hoc code reviews in github, as the interface is the most beautiful of all imo.
I have used it for a team of 10 people (at 2 locations) and it actually reduced some overhead in communication. I had set it up on a desktop machine for about 2 months and only adjustment I needed was to increase heap size for JVM. If you are using git, give it a try. It is dead easy to set up.
I'm very unhappy with gerrit. It doesn't support git style branches, and instead uses a made up "Change-Id" scheme where you embed some magic in commit messages. If you want to fix issues with a commit after a review, you can reuse this "Change-Id" to update the changeset for the review.
It get worse. If you have a nice branch with several commits, they must all be reviewed separately with little help to review the branch as a whole. This feels like a throwback to the old days with CVS and patch-sets. Branching and merging suddenly becomes expensive.
If you combine this with a policy of auto-merging upon successful review, you are basically screwed if you have multiple depending commits. You must use rebase and squash to make it livable.
Sorry for the bitterness :-) I just had to review a single commit with 4000+ lines in gerrit.
The things you are complaining about are all things that I enjoy about gerrit. I tend to rebase on pull and use cherry-pick as the submit mechanism so it fits naturally into the flow of my groups.
Doesn't bugzilla (although ugly) handle the same workflow that he is describing? We have svndiff linked in for the side by side diffs, but that is the only thing missing.
I don't see how bugzilla handles the same problem. The key feature of the workflow is that a commit cannot succeed until code review is complete. This is very important for reducing how often you break the build. With bugzilla integration your bugzilla tickets contain the history of commits that are tied to that bug, but that is a different problem.
We take for granted today that there are several fast, stable DVCS generally understood by an average engineer (in other words, the bar for using a DVCS has been lowered in the last few years and it's not a purely academic endeavor.) When Google was ramping up, Perforce was dramatically faster and more reliable than the almost all of its competitors. It was one of the most common source control systems to find at Valley companies.
I was way happier at Google using Perforce than I am today using Mercurial.
The essential difference that annoys me to no end:
- In Mercurial / Git, you must be synced to tip in order to push. (to avoid creating new heads)
- In Perforce (and other old style VCS), you can commit as long as the files that you are committing are based on the newest version.
It is impossible to have any sort of sizeable team commit to the same DVCS repo. With ~12 software engineers, I already have to sync 25 times a day, even if I'm the only one touching my area of the codebase.
Most Google engineers commit to the same repository, and everyone gets the benefits of building from head (instant fix propagation from other systems, rather than waiting for releases). There is no way for everyone to work on the same repo with a DVCS afaik. It would require some sort of complicated hierarchy of repos.
If you don't sync and code against the latest, how do you know things still interact properly?
This is the old optimistic vs. pessimistic locking debate. I remember that at my first job, we used SourceSafe, and it would lock the files when you check them out so that nobody else could edit it. When we switched to CVS, I asked "But what happens if people make incompatible changes to different regions of a file and conflict detection doesn't catch it?" The answer was "Then the next person who checks out the code gets a compile error and we deal with it. It just doesn't happen that often in practice."
I've found that the problem you've had just doesn't happen that often. In the two years and hundreds of changes I've made at Google, I can think of a handful (< 5) of times that a CL has broken the build or caused a bug because of a bad sync and yet not caused any conflicts. And even then, the continuous build catches it and it either gets rolled back and tested properly before resubmit, or somebody patches it and we move on.
I've found that any feature branch that lives longer than a week becomes essentially impossible to integrate - in the time necessary to bring it up to head, head has changed enough that you then need to integrate it again, and so on. This obviously depends on team size though - if you've got 10 developers working on a piece of code, it's going to have a lower change rate than if you have 500 developers working on it. Of course, if you have 10 developers working on the code, the chances are miniscule that one will submit something that conflicts with your change in the window after you sync & test, and you can just yell out across the room "Hey, anybody submitting anything that conflicts with my change?"
"I've found that any feature branch that lives longer than a week becomes essentially impossible to integrate - in the time necessary to bring it up to head, head has changed enough that you then need to integrate it again, and so on."
DCVSs don't stop you from doing your commit, doing a naive merge, and then optimistically pushing. That option is still there, and is partly why so many people rebase before pushing their changes. Of course, pessimistic integration is equally available as an option.
If you find the overhead so high, it suggests that it's process holding you back and forcing you to pessimistically integrate, rather than the DCVS doing so.
> I've found that any feature branch that lives longer than a week becomes essentially impossible to integrate - in the time necessary to bring it up to head, head has changed enough that you then need to integrate it again, and so on.
Not disagreeing. My current working theory is that integration difficulty increases exponentially with merge distance.
And yes, team size is a factor - but I'd argue that if 500 developers all work in the same area of the code, something else is wrong :)
To get back to the original point: You sync to head, and you compile. I'm not suggesting a full QA cycle, but especially if you changed APIs, that's the polite thing to do.
If you do not sync and test your changes against the latest, I'd argue that you're being careless - how do you know things still interact properly?
This would suggest that the developer who checked in changes to A, which is used by B, did not also make the changes to B so that it works with the changes to A. That seems to be the actual careless action to me.
Well, that and that migrating to anything else would be a huge nightmare. They have a ton of infrastructure written around Perforce, just for starters. Then there's the fact that moving to, say, git, would require significant refactoring of their heavily incestuous code base. Moving to Subversion would be easier, but svn's problems with branching and merging would probably make that a non-starter.
Back in November 2009 I was on vacation in San Francisco and decided to drop by the Google campus in Mountain View to visit some friends.
Of all the things they showed me there, Mondrian (of which Rietveld is a fork of) impressed me the most. More so than dinosaur skeletons, space ships, 3D Google Earth terminals, or free amazing meals.
I was working in a startup at the time and we did not do code review at all. After seeing the Google workflow and having their build processes explained to me in detail, I was trying to set up code review before commits at the startup too. I hoped to improve code quality with this. Unfortunately the tools we tried were terrible and so it never took off.
To this day, not getting the necessary processes in place remains one of the biggest regrets of working there. Now I work at another place where we don't do that either. Too bad.
It is something that came to my attention recently which I thought would be a best practice of interest to people on HN. I hadn't seen it discussed here. Therefore I posted the link, and apparently some other people agree with me about its value.
My last team switched from Subversion/Rietveld to Mercurial/Review Board about a year ago, and I have to say that I much prefer the latter.
Even leaving aside the advantages of a DVCS such as Mercurial, I think that Review Board has a cleaner, more responsive UI. The "interdiff" feature (i.e., show diffs between changesets in this review request, not against the trunk) is pretty great, too.
Also, as a fairly mainstream Django app, it's not dependent on the App Engine runtime, which lets you run it on any standard Python/WSGI-capable web server on any recent version of Python. (App Engine only supports 2.5.)
If you're using Mercurial or Git, I'd highly recommend you check out Review Board; the initial setup is easy if you've deployed any WSGI apps before, and the web API makes it pretty easy to tie into your ticketing/CI/IRC bot/etc.
I think the Go project uses Mercurial but does code reviews using Rietveld. They have some python code which helps in sending patches for codereview to Rietveld.
I haven't used Review Board, but I have however used Rietveld and I don't have any complaints about the UI. It also provides the "interdiff" feature, just choose the patchsets you want compared for a specific file, when you go to the diff page.
If this would have come around 6 months ago, then I would have used it. Now, I'm satisfied with GitHub and its code-review facilities. We already ditched SVN.
Why not just use Crucible if you are running a small startup? There license cost is $10 right now, and it has a lot of niceties.
http://www.atlassian.com/software/crucible/
Seconding this. We are using it, alongside Jira and Fisheye and they are really great, well-done tools. Atlassian will gain a customer when we grow large enough.
34 comments
[ 2.9 ms ] story [ 111 ms ] thread[1]: http://code.google.com/p/gerrit/ [2]: http://git-scm.com/about
As much as I like Gerrit I don't think I can use it for our team because Gerrit wants you to review commits, and our team works in terms of whole branches. Reviewing your individual commits made early in the branch history probably isn't useful as bugs may be fixed--or the code entirely changed--by later commits. Anyone else have this problem with Gerrit, reviewing commits vs. reviewing branches?
If you wait until a branch is done, then reviewers are being asked to look at big chunks of code. That is a lot more effort, and it is harder to get reviewers to do it. And when they do do it, it is harder to get them to do a thorough job of it. Furthermore when they notice design issues, it is harder to get developers to go back and redo all of their working code to make it better.
Therefore if you want effective code review, you need to review early and review often. The smaller a change is, the easier it is to review, the easier it is to be thorough, and the easier it is to accept suggested changes.
My conclusion is therefore that choosing to do code review after the fact means that you are guaranteeing no proper code review. But when you review early - literally on every commit - it is then much easier to maintain a truly effective code review process. It initially "feels" much heavier. But after you get in the rhythm, your code will be much, much improved.
- dev works on a feature branch, making multiple commits and pushes
- once ready, squash all the commits and submit it to gerrit
[- perhaps have hudson/jenkins run the unit tests at this point automatically]
- have the code review in gerrit
- once the review is done, gerrit would merge it into the develop or master branch (depending on your git workflow)
That approach blends in nicely with git-flow (1). If you want to be sure that no single dev is pushing to the develop or master branches you'd need to setup per-branch permissions, which can be done with gitolite (2). Too bad github (even the self-hosted version) doesn't support per-branch permissions, which forces organizations that use it and only want gerrit to be able to push into the main branch to do excessive repo forking instead of using feature branches. Also I'd love to be able to do ad-hoc code reviews in github, as the interface is the most beautiful of all imo.
[1] http://nvie.com/posts/a-successful-git-branching-model/, https://github.com/nvie/gitflow, http://jeffkreeftmeijer.com/2010/why-arent-you-using-git-flo...
[2] https://github.com/sitaramc/gitolite
It get worse. If you have a nice branch with several commits, they must all be reviewed separately with little help to review the branch as a whole. This feels like a throwback to the old days with CVS and patch-sets. Branching and merging suddenly becomes expensive.
If you combine this with a policy of auto-merging upon successful review, you are basically screwed if you have multiple depending commits. You must use rebase and squash to make it livable.
Sorry for the bitterness :-) I just had to review a single commit with 4000+ lines in gerrit.
The things you are complaining about are all things that I enjoy about gerrit. I tend to rebase on pull and use cherry-pick as the submit mechanism so it fits naturally into the flow of my groups.
Why are they still using it? Inertia, I assume.
The essential difference that annoys me to no end: - In Mercurial / Git, you must be synced to tip in order to push. (to avoid creating new heads) - In Perforce (and other old style VCS), you can commit as long as the files that you are committing are based on the newest version.
It is impossible to have any sort of sizeable team commit to the same DVCS repo. With ~12 software engineers, I already have to sync 25 times a day, even if I'm the only one touching my area of the codebase.
Most Google engineers commit to the same repository, and everyone gets the benefits of building from head (instant fix propagation from other systems, rather than waiting for releases). There is no way for everyone to work on the same repo with a DVCS afaik. It would require some sort of complicated hierarchy of repos.
Feature branches are the way to work (for me). You're pretty much on your own in there, and once the thing's done you push to main.
This is the old optimistic vs. pessimistic locking debate. I remember that at my first job, we used SourceSafe, and it would lock the files when you check them out so that nobody else could edit it. When we switched to CVS, I asked "But what happens if people make incompatible changes to different regions of a file and conflict detection doesn't catch it?" The answer was "Then the next person who checks out the code gets a compile error and we deal with it. It just doesn't happen that often in practice."
I've found that the problem you've had just doesn't happen that often. In the two years and hundreds of changes I've made at Google, I can think of a handful (< 5) of times that a CL has broken the build or caused a bug because of a bad sync and yet not caused any conflicts. And even then, the continuous build catches it and it either gets rolled back and tested properly before resubmit, or somebody patches it and we move on.
I've found that any feature branch that lives longer than a week becomes essentially impossible to integrate - in the time necessary to bring it up to head, head has changed enough that you then need to integrate it again, and so on. This obviously depends on team size though - if you've got 10 developers working on a piece of code, it's going to have a lower change rate than if you have 500 developers working on it. Of course, if you have 10 developers working on the code, the chances are miniscule that one will submit something that conflicts with your change in the window after you sync & test, and you can just yell out across the room "Hey, anybody submitting anything that conflicts with my change?"
DCVSs don't stop you from doing your commit, doing a naive merge, and then optimistically pushing. That option is still there, and is partly why so many people rebase before pushing their changes. Of course, pessimistic integration is equally available as an option.
If you find the overhead so high, it suggests that it's process holding you back and forcing you to pessimistically integrate, rather than the DCVS doing so.
Not disagreeing. My current working theory is that integration difficulty increases exponentially with merge distance.
And yes, team size is a factor - but I'd argue that if 500 developers all work in the same area of the code, something else is wrong :)
To get back to the original point: You sync to head, and you compile. I'm not suggesting a full QA cycle, but especially if you changed APIs, that's the polite thing to do.
This would suggest that the developer who checked in changes to A, which is used by B, did not also make the changes to B so that it works with the changes to A. That seems to be the actual careless action to me.
So unless you catch up to reality (i.e. sync), you won't even know you should make those changes to B. Hence my post ;)
See Android, which already has a wrapper around git ( http://source.android.com/source/git-repo.html ) to sync multiple repositories as a single unit.
Well, that and that migrating to anything else would be a huge nightmare. They have a ton of infrastructure written around Perforce, just for starters. Then there's the fact that moving to, say, git, would require significant refactoring of their heavily incestuous code base. Moving to Subversion would be easier, but svn's problems with branching and merging would probably make that a non-starter.
Of all the things they showed me there, Mondrian (of which Rietveld is a fork of) impressed me the most. More so than dinosaur skeletons, space ships, 3D Google Earth terminals, or free amazing meals.
I was working in a startup at the time and we did not do code review at all. After seeing the Google workflow and having their build processes explained to me in detail, I was trying to set up code review before commits at the startup too. I hoped to improve code quality with this. Unfortunately the tools we tried were terrible and so it never took off.
To this day, not getting the necessary processes in place remains one of the biggest regrets of working there. Now I work at another place where we don't do that either. Too bad.
Even leaving aside the advantages of a DVCS such as Mercurial, I think that Review Board has a cleaner, more responsive UI. The "interdiff" feature (i.e., show diffs between changesets in this review request, not against the trunk) is pretty great, too.
Also, as a fairly mainstream Django app, it's not dependent on the App Engine runtime, which lets you run it on any standard Python/WSGI-capable web server on any recent version of Python. (App Engine only supports 2.5.)
If you're using Mercurial or Git, I'd highly recommend you check out Review Board; the initial setup is easy if you've deployed any WSGI apps before, and the web API makes it pretty easy to tie into your ticketing/CI/IRC bot/etc.
I haven't used Review Board, but I have however used Rietveld and I don't have any complaints about the UI. It also provides the "interdiff" feature, just choose the patchsets you want compared for a specific file, when you go to the diff page.