I became curious with these statements regarding self-review (committer==reviewer) and so I ran a couple of queries against the gerrit database to see how often this occurs:
- For the puppet repo, 84.1% of the commits is self-reviewed.
- For the mediawiki core repo, 27.9% of the commits is self-reviewed.
I'm actually very surprised to see such a high number for MediaWiki core. Why is this the case?
When you say committer==reviewer, so do you mean the reviewer did a +2 and merged? Does this take into account committers that also just added comments?
- For the mediawiki extensions repos, 67.8% of the commits is self-reviewed.
This doesn't surprise me at all. I have a *really* hard time getting my extensions reviewed and I work for the foundation. I have to beg for reviews, and even then it can take up to a couple weeks sometimes. It's problematic.
That said, I don't necessarily think my extensions need pre-merge review. I'd really like Gerrit to support post-merge review in some fashion.
I think we need to take a step back from a tool-focused discussion and first hash out what our commit workflows are / should be. In particular:
- Should there be one commit workflow that applies to all teams? Looking at current practise, the answer seems to be no but I am curious to hear what other people think. If the answer is that it's okay for different teams to have different commit workflows, then we should also look for tools that support this.
Things that are going into production should be reviewed, preferably pre-merge. It should definitely be reviewed before deployment. Insecure code on a wikimedia.org domain has the ability to affect everything else too.
- If self-review is so prevalent, does that mean that the pre-commit review workflow has failed?
Self-review is so prevalent in puppet because it's very difficult to do operations any other way. We occasionally do 50-100 deployments a day. Labs isn't a fully functional replica of everything in production yet, either, so we have no way to stage deployments, which means we can't wait on reviews.
That said, pre-merge review is working exactly as intended. The fact that nearly 20% of the merges are reviewed shows exactly how well it's working. If you take a look at that more closely, I'm betting you'll see that those commits are almost completely outside of the operations staff. That's an amazingly huge win for a team that had 0 merged commits outside of the operations staff. This process simply wouldn't work without pre-merge review.
- Ryan