29

One of the differences between svn and git is the ability to control access to the repository. It's hard to compare the two because there is a difference of perspective about who should be allowed to commit changes at all!

This question is about using git as a centralized repository for a team at a company somewhere. Assume that the members of the team are of varying skill levels, much the way they are at most companies.

Git seems to assume that your only your best (most productive, most experienced) programmers are trusted to check in code. If that's the case, you are taking their time away from actually writing code to review other people's code in order to check it in. Does this pay off? I really want to focus this question on what is the best use of your best programmer's time, not on best version-control practices in general. A corollary might be, do good programmers quit if a significant portion of their job is to review other people's code? I think both questions boil down to: is the review worth the productivity hit?

10
  • 5
    Define "best programmer"? Best at what? Following arbitrary rules? Cranking out code? Writing zero-defect code? Jan 7, 2013 at 20:49
  • 3
    Sorry, I'm still trying to get my brain around the concept of reviewing uncontrolled (ie non-checked in) code... surely one of the key benefits of using a SCS is that the Review can be made against a known/controlled iteration of the code?
    – Andrew
    Jan 7, 2013 at 21:11
  • 2
    @Andrew with git any developer can have his own repo (on his personal computer) and a public personal repo (the one on a server, behind apache) that he can only add changes to. The difference is, that only the lead developers repo is the "blessed one", the one from which everyone should checkout from. The lead checkouts code from developer's public repos and merges them to his public repo. You both have known/controlled iteration as well as source control at all times. Jan 7, 2013 at 21:24
  • 32
    "Git seems to assume that your only your best (most productive, most experienced) programmers are trusted to check in code" is an incorrect presumption. Git can be configured how you want. The "Pull request" model is just one way - ideally suited to open source projects with a potential large number of unknown contributors. In most commercial environments, the "pull request" model would be a red flag, indicating poor SDLC and QC processes and procedures.
    – mattnz
    Jan 7, 2013 at 21:45
  • 4
    I believe @mattnz is correct here. This is solely a result of a strong open source influence on git where there is a core dev team that controls the state of the repo, but others are welcome to contribute as well. Jan 7, 2013 at 21:52

11 Answers 11

52

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.

4
  • 10
    +1: For ...Good programmers spend a lot of time dealing with other programmers' broken code anyway.
    – Jim G.
    Jan 8, 2013 at 2:32
  • 3
    +1 Best answer. Especially pointing out that one dev committing a build-breaking bug negatively impacts everybody. Jan 8, 2013 at 3:48
  • Been in that situation, turned out out best two programmers were full time used to review and correct other people's code. Sure the code quality on the VCS was good but morale for these two dwindled faster than a toilet flush. What started as a seemingly good idea turned to nightmare when these two ran out the door to places where they could get, say, more creative tasks.
    – Newtopian
    Sep 9, 2016 at 15:08
  • 1
    That's a good point, @Newtopian. The places where I've seen this succeed have more of a microservice model, and only one scrum team has commit access to any given microservice, but responsibility is spread around for the system as a whole. If you don't have at least a couple experienced programmers per scrum team your hiring practices need improving. Sep 9, 2016 at 16:45
40

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.

15
  • 8
    @HubertKario - If your best developers are spending time doing code reviews and the rest are effectively blocked until that's complete, I don't see too much practical difference.
    – Telastyn
    Jan 7, 2013 at 21:16
  • 6
    How they are blocked? You create a patch (commit locally), submit it upstream, and keep working on new widget (create new local commits). If your change is applied verbatim you just need checkout and merge the lead's repo. If it wasn't applied verbatim you can still rebase your later work. If the change is really critical, you can publish it in your own public repo and tell people to checkout it from there or just send them patches. In this case git will detect that a change was already made and just skip applying the specific upstream patch. Jan 7, 2013 at 21:30
  • 9
    The last line in this question is really the whole point in my eyes. A developer not trusted will be ineffective at best and hateful of his job at worst. Don't hire people you won't trust. It's pointlessly wasting money on people you won't allow to do the job you're paying them for anyway. Jan 7, 2013 at 21:41
  • 1
    @HubertKario - you know better than I. The environment I was in made it annoying to juggle the different branches/changesets.
    – Telastyn
    Jan 7, 2013 at 22:08
  • 5
    @Telastyn I don't know if I'm supposed to interpret your answer as literally as I did, but another downside to that would be the annotation/blame history would be all wrong. If you found some code you didn't understand, you'd end up asking the reviewer that committed it, not the programmer who wrote it. Jan 8, 2013 at 1:00
29

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.

9
  • 1
    This. If you're committing without unit and build tests, having a code review requirement is an imperfect bandage. Jan 7, 2013 at 21:25
  • yeah. That's why I mentioned the alternatives. Code reviews is better than nothing. Not being able to version the code is a pain no developer should be exposed to.
    – jgauffin
    Jan 7, 2013 at 21:28
  • 2
    Unit tests don't help if they're written by the same <insert your fav 4 letters here> as the unit code.
    – ott--
    Jan 7, 2013 at 22:03
  • @BrianKnoblauch: one could argue that the opposite is true, too. Ideally, you should have both.
    – Doc Brown
    Jan 8, 2013 at 7:02
  • @ott-- I just heard a story about a dev who left after committing a horrible mess of a fix and commenting out all the Asserts in his unit tests. Tests succeed by default so took a while to notice the issue!
    – Alex
    Jan 8, 2013 at 10:09
8

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.

1
  • 2
    We use gerrit with great success. Solves every issue the OP has a problem with and even some he does not know he has.
    – mattnz
    Jan 8, 2013 at 4:32
8

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.

1
  • 1
    +1. Let the team review the team - both, the reviewer and the reviewed, can profit, even if the reviewer is less experienced than the reviewed. And you can do all the review AFTER check-in. IMO, if you prevent people from checking in, their productivity will decrease (despite their motivation).
    – Andy
    Jan 9, 2013 at 12:08
5

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?
4

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.

2

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.

0

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.

0

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.

2
  • 1
    -1: It keeps programmers honest - if you know it will be reviewed, people will take less shortcuts. - Hmm... I'd be concerned about the introduction of moral hazard. That is, developers could get lazy or sloppy because they know that a more senior developer will always take responsibility for their code in the form of a code review.
    – Jim G.
    Jan 8, 2013 at 2:34
  • 1
    The reviewer does not take responsibility for the code at all, but instead gives advice and instruction on issues with the code. The original developer must fix the issues and is still responsible for the code.
    – Steve
    Jan 8, 2013 at 7:07
-1

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.