Since it’s not clear from your question, I just want to point out that a gatekeeper workflow is by no means required with git. It’s popular with open source projects because of the large number of untrusted contributors, but doesn’t make as much sense within an organization. You have the option to give everyone push access if you want.

What people are neglecting in this analysis is that good programmers spend a lot of time dealing with other programmers’ broken code anyway. If everyone has push access, then the build will get broken, and the best programmers tend to be the ones frequently integrating and tracking down the culprits when things break.

The thing about everyone having push access is that when something breaks, everyone who pulls gets a broken build until the offending commit is reverted or fixed. With a gatekeeper workflow, only the gatekeeper is affected. In other words, you are affecting only one of your best programmers instead of all of them.

It might turn out that your code quality is fairly high and the cost-benefit ratio of a gatekeeper is still not worth it, but don’t neglect the familiar costs. Just because you are accustomed to that productivity loss doesn’t mean it isn’t incurred.

Also, don’t forget to explore hybrid options. It’s very easy with git to set up a repository that anyone can push to, then have a gatekeeper like a senior developer, tester, or even an automated continuous integration server decide if and when a change makes it into a second, more stable repository. That way you can get the best of both worlds.

I have worked at a job where check-ins were limited to team leads only (and team leads couldn’t check in their own code). This served as our mechanism to enforce code reviews, largely because of a number of incidents where bad commits got into the codebase even around gated check-ins and static analysis.

On one hand, it did it’s job. A number of bad commits were found before they got into the codebase (and promptly forgotten for a week or so until someone stumbled upon them). This caused less disruption in the codebase. Plus, I could push back some formatting/structure things before they became tech debt; catch some bugs before they became bugs. And it gave me a great feel for what my team was doing.

On the other hand, it caused me to spontaneously go into murderous rages when my 3 line change took 4 hours to commit because of having to track down another lead and get them to do the commit. It pushed me to do far less frequent commits than is best practice, and occasionally led to issues trying to track changes to the developer that did them.

I would not generally recommend it except in the most needing environments. Doing the reviews and commits wasn’t too bad actually. Having my own process dependent on the whims of others though was infuriating. If you can’t trust your developers to check in code, get better developers.

No. Anyone should be able to commit.

If you have problems with bugs being committed it’s not the source control policy which is wrong. It’s the devs who fails to make sure that what he/she commit works. So what you have to do is to define clear guidelines on what to commit and when.

Another great thing is called unit tests 😉

There is an alternative though.

a) If you use distributed version control you can create a main repos to which only pull requests can be made. In that way all devs can get versioning of their own code while you get control of the main branch.

b) In subversion and similar you could use branches where each dev has to create patches to get it into the main branch.

You should have a look at projects such as Gerrit which allows all developers to push their code into ‘review’ branch and once you senior / lead devs are happy with these changes they can push them into master/release.

If they are not happy, they can leave comments next to a line of code, ask for updated patch etc.

This way anybody with a change pending can get it out as soon as it is ready and only qualified people (with the right +2 privileges in Gerrit) will be able to push that code to test and later to production.

No, it’s a poor use of your best talent. Imagine a publishing company taking their most successful authors and making them do editing; bad idea.

There should be code reviews, but that doesn’t mean it’s always a Sr. checking a jr’s code. Eventually, everyone on the team should be expected to get to the level where they can contribute code with minimal guidance. They go through the three levels of trust:

1. None – I want to see every line of code before you check it in.
2. Some – Let me know what you’re doing and I’ll provide feedback
3. Most – Go do your job and only ask for help when needed.

Advantages of freeing up your talent:

• focus on design
• involvement in setting up coding standards and enforcement strategies (without manually doing it all themselves)
• tackle the tough coding problems
• provide mentorship (without having approve every line of code)

There are developers interested in a management path who may prefer not to code all day long; leave the others alone.

is the review worth the productivity hit?

It depends on team “balance” and on how reviews are set up. Both are matters of management and teamwork, no amount of version control technological magic (centralized or distributed) can have a substantial influence on that.

If done wrong, productivity hit will of course kill any benefits of the review; the answer is though not to drop idea of reviews but to find out how to do it right.

One approach to find out if your reviews are OK is to use issue tracking tool to track time spent on reviews (some code review tools allow for that, too). If you find out reviews are taking quite a lot of time, invest some effort into finding the reasons and ways to improve things. Also, it wouldn’t hurt to have regular 1:1s with team members to discover potential issues with code reviews.

If “best” programmers in the team are forced to spend hours digging through incomprehensible garbage produced by crappy coders, solution is to fire crap-makers, not to appeal to VCS technology.

• ^{In one of the past projects I was assigned to review code changes done by permanently under-performing team member, in a component that took almost an hour to just build and run tests. I started reading the diffs and when I noticed an uncompilable change, I simply finished review, posted necessary comments and asked management to ensure that further review requests come with written confirmation that their code compiles. There were no “review requests” since and soon the guy left.}

On the other side, when team is reasonably balanced, code reviews are fun and educative. In my prior project, we had a requirement for 100% code review and it neither took much time nor was distracting. There were bugs discovered through review, and there were debates about coding style and design choices, but that felt just… normal.

If code changes are being blocked for days… weeks from getting to QA for testing “because of reviews”, studying about VCS tricks would be the least likely way to solve this problem. Instead one would better focus their effort on finding out issues in the way how review process is organized.

• ^{– Oh integration of this change was much delayed because reviewer got suddenly sick, what a misfortune.
  – Hello! Hell-lo-o-o, did you ever think of having backup reviewers to deal with cases like that?}

Yes. But only if you’re talking about distributed source control. With centralized — it depends.

If there are only few programmers, it takes little time. Certainly less than the fixes that will be needed to remove bugs and technical debt later.

If there are very many programmers, you can delegate the task of actual code-review to lieutenants and have the lead developer pull their changes (nearly) unquestionably. It works for Linux kernel, I don’t think that there are any larger software projects…

Again, if the project is small, the lead will quickly see who gives good code and who produces bad code. He will quite quickly see that J.Random writes good code that needs only checking for architectural decisions while the intern writes bad code that needs to be reviewed line by line before merging. The feedback this way generated will reduce maintenance burden down the line and give first hand experience on whatever the intern actually learns and should be kept in company. Pulling and merging branch from other git repo takes literally a (couple) dozen seconds, usually reading the titles of commit messages will take more time, so after I know who can be trusted to write good code merging other people’s code is a non-issue.

Code review doesn’t necessarily require the attention of only your very best programmers. IMO, it should be an informal thing. Just a second opinion or a second pair of eyes on a piece of code from a non-rookie before it gets checked into production. It helps mitigate major oversights while also helping people get better at coding as a craft by being exposed to other dev perspectives.

Sort of a less-obnoxious pair-programming lite. In other words, it shouldn’t take long and you shouldn’t have to wait for somebody for hours. Anything in your development process that involves people waiting for things is a waste of money and crippling to momentum/morale, IMO.

If code review were meant to stop 99.5% of bugs before they got into your code base in the first place, there’d be no real point to sophisticated version control. That said, git is intimidating at first but basic general use isn’t that complicated and it’s highly configurable.. You should be able to stop for a few hours to teach everybody how to use it. Everybody commits. All but the noobiest rookies review until they demonstrate expertise at something.

As long as the changes being submitted have been reviewed by the ‘best programmers’ anyone should be allowed submit code. The only person who should have the ability to enforce control on a repository is the Release Engineer, if that person exists.

Personally, I’d be well pissed off if I had to check in other people’s code.

Some input on your edit:
No, they shouldn’t. Reviews are a necessary evil, they do more good than harm and good programmers will appreciate this. Maybe there’s a reluctance to participate in reviews because they don’t like the idea of ‘lesser programmers’ criticising their code. That’s just too bad. They would be far more likely to quit if the codeline is constantly buggy and they instead spend their time cleaning up after other people’s half-baked submissions.

Yes, review is worth it. I’m not sure there is a productivity hit if the review process is proportional for the following reasons:

• It keeps programmers honest – if you know it will be reviewed, people will take less shortcuts
• It helps new programmers learn from more experience programmers
• It helps transfer domain specific knowledge
• Review is another gate where bugs and potential issues can be found and fixed

By not allowing all programmers to use source control they lose the ability to track changes, undo mistakes and see a reasonable history of changes. I’m not sure you would ever want only your “best” programmers to be able to check in to git.

Having said that, I think it is reasonable that you have someone who is in charge of certain key branches, such as a release branch. In this case I would imagine that everyone can use the git repository, but only certain people merge into the release branch. I’m not sure there is a way to enforce this in git, but it should be possible to do by process and just checking no one else has checked into it.

Merging into the release branch could be done by the “best” programmers, or more likely done by competent people after sufficient review has taken place.

do good programmers quit if a significant portion of their job is to review other people’s code?

If they are not enjoying the job and forced to do this activity, then YES. It is very likely to happen. As finding next interesting work for a good developer is not a big challenge nowadays.

Should your best programmers have to check everyone else’s code into source control?

Absolutely NO. It is for sure waste of their time, except for some critical logic which needs to be in rock-solid state.

However, junior or in-experienced developers probably should be on a probation time for a code quality, just to be on a safe side and making sure that their code follows team development guidelines, at least for couple weeks before getting privilege to commit themselves.