How do I protect the trunk from hapless newbies?

Question

A coworker relayed the following problem, let's say it's fictional to protect the guilty:

A team of 5-10 works on a project which is issue-driven. That is, the typical flow goes like this:

1. a chunk of work (bug, enhancement, etc.) is created as an issue in the issue tracker
2. The issue is assigned to a developer
3. The developer resolves the issue and commits their code changes to the trunk
4. At release time, the frozen, and heavily tested trunk or release branch or whatever is built in release mode and released

The problem he's having is that a couple newbies made several bad commits that weren't caught due to an unfortunate chain of events. This was followed by a bad release with a rollback or flurry of hot fixes.

One idea we're toying with:

Revoke commit access to the trunk for newbies and make them develop on a per-developer branch (we're using SVN):

• Good: newbies are isolated and can't hurt others
• Good: committers merge newbie branches with the trunk frequently
• Good: this enforces rigid code reviews
• Bad: this is burdensome on the committers (but there's probably no way around it since the code needs reviewed!)
• Bad: it might make traceability of trunk changes a little tougher since the reviewer would be doing the commit--not too sure on this.

---

Update: Thank you, everyone, for your valuable input.

I have concluded that this is far less a code/coder problem than I first presented. The root of the issue is that the release procedure failed to capture and test some poor quality changes to the trunk. Plugging that hole is most important. Relying on the false assumption that code in the trunk is "good" is not the solution.

Once that hole--testing--is plugged, mistakes by everyone--newbie or senior--will be caught properly and dealt with accordingly.

Next, a greater emphasis on code reviews and mentorship (probably driven by some systematic changes to encourage it) will go a long way toward improving code quality.

With those two fixes in place, I don't think something as rigid or draconian as what I proposed above is necessary. Thanks!

Answer 1

My first advice would be hire better newbies, followed by hiring a better QA department. The approach you suggest could work, but your treating the symptom and not the problem. Code reviews and unit tests would help this a lot.

Answer 2

You are explicitly stating that new people can't be trusted, which isn't going to help their morale, in fact it will destroy it.

Because ultimately, you are trying to make a set of "trusted" developers, and a set of "untrusted" developers.

I'm going to go out on a limb and say that there are DEFINETLY 'new' (new to your company) coders who would be less likely to make mistakes than some of your 'experienced' developers.

The failing in your original system was a failing of TESTING not a problem that can be solved by segregating developers.

You need a stronger QA and UAT system in order to make sure that such errors, regardless of who makes them are caught before the deploy time. That way, you don't have to blame the 'hapless newbies'.

Answer 3

It seems your existing process is already pretty good. The problem isn't that bad code made it onto the trunk but rather bad code was not caught during testing or review. Perhaps weekly code review sessions would be helpful? Pair up a beginner with someone more experienced and just go through the code because at some point there will be far too many changes for just a specific person or group of people to be doing all the commits/merging to trunk.

Per developer branch works too, but I would suggest letting the developer merge the changes (again perhaps with another developer).

Answer 4

There is several other things you could do:

• Project Management: Communicate the problem with the team, emphasizing the responsibility that comes with commits to the trunk

• Team Structure: Assign a mentor to the newbies which will do a code review before the commit. That's also a good opportunity for the newbies to get to know the project, the coding standards in place and to get familiar with the culture of the team.

• Unit Tests:: Require everyone to also write a unit test for their code.

• Regular Builds and Tests: Establish a build server and do nightly builds (including unit test) to discover problems early.

• Code Review Set a post-commit hook on your SVN server to send an email notification on new commits, and have someone experienced do a quick review early.

Answer 5

Um... This may be obvious, but how about fixing the unfortunate chain of events that led to the bad commits not being caught?

Your process seems reasonable; but if you're going to be doing Continuous Integration, you need to verify that the Integrated trunk isn't broken. Sounds like that process got defeated somehow; it seems to me like you need to find and fix the issue that defeated that process.

Answer 6

• Developers should run the unit tests before committing, so they can easily avoid bad commits.

• CI should run the unit tests again after committing, just to make sure.

• Developers should be committing to branches, which should be merged after being reviewed. For a senior programmer who is trusted, that "review" might just be "Do the tests pass? Does the code look reasonable?" For the "newbies", it can be more extensive.

As far as traceability, once it's in the trunk, who cares? If the code is broken, the reviewer who let it through is at much at fault, if not MORE at fault, than the developer (since they KNOW the developer is a "hapless newbie" and "not to be trusted", they should know to look extra carefully).

Answer 7

Code review the changes on the trunk and have them apply the fixes (if any). Eventually they'll get better and everyone will be happy.

Answer 8

Continuous integration and automated unit tests -- perhaps as a requirement of the check in process. Set up continuous integration on checked in code. Set up notifications or public indicators of build status (red/green lights). Have your unit tests (and integration tests, etc., if possible) run during the build process. People who break the build can't work on any new features until the build is fixed. Couple this with good-natured, public shaming. :-)

Also -- hand-hold the newbies through the process the first few times so they're not too scared to commit anything.

Answer 9

I think it is far too rigid. If you do code reviews you can do this on the trunk. Instead of merging the developer branch (which is tedious) which is a lot of work you can just have a look what the developers did. That's the whole idea about a revision system. You can even rollback. But then you do it if something is bad. You don't need a lot of actions if the code is good. The outcome is that it is less work when the code is good. You can spent that extra time in teaching your newbies. The other approach is purely demotivating for your fellow workers.

The "chain of unlucky events" ;). To be honest that sounds like your Q&A is not that rigid like your proposal. A better test coverage and maybe a CI server will help. I personally like svn commit messages. It keeps me easily up-to-date. I get an email with a diff that someone committed. Easy to do and the unread commit mails are a todo list, too.

So, it is better to embrace newbies and changes. Don't put the time and effort into control structures. Spent it at better in teaching newbies and improving the process.

Answer 10

You could put up a staged review system like ReviewBoard. See also the google talk on Mondrian. We're starting to use reviewboard here, but in a "post-commit" manner, which it's not directly designed for.

Paired even with a frequent (triggered on commit) build system, this will give you a lot more confidence in code. (We do this now.)

Add automated unit testing and/or automated functional testing and you've got even more confidence. (We have done this in the past, but it's succumbed to some bit-rot.)

You can even go as far as a continuous integration buildserver (as several others have mentioned).

At one extreme, the commit process could be:

• post a patch (or branch URL) to ReviewBoard; if not this, then solicit one or more peers for a review
• have it reviewed and accepted
• have the CI buildsystem build it and run unit and functional tests
• have the CI buildsystem commit it for you only if it succeeded
• (then maybe run static code analysis -- like Gimpel PC-Lint -- on it too, before or after commit).

(Our process here at the moment is "guided peer review", "commit", "autobuild with really simple boot/functionality test", potentially "formal peer review", and eventually "address static code analysis results".)

At some point for any given organization this may be "overkill" -- so the typical "YMMV" applies. Look at your existing build process and figure out what the "low hanging fruit" are, the stuff that will be relatively easy to implement, give you good results, and will be easy for people to get used to. You can always add more later, time and resources permitting.

And ++ for all the people who mentioned the "embrace the newbie" approach. It's far more important to have newcomers feel like they have a ton of learning resources and tools to become better at their disposal than it is to have them feel "punished" or restricted.

When we added PC-Lint here the response was overwhelmingly positive, at least once we figured out how to show people only the warnings they were likely responsible for. It became less of a responsibility and more of a self-improvement resource. (Helps if you hyperlink examples and explanations from these reports.)

Answer 11

ahem, use mercurial? then your newbies can damage their own archive as much as they please...

Answer 12

I may just be echoing things other people have said but...

The interview process should be intense enough to weed out those that can't handle what's required by the position. So, that's my preemptive solution.

Another solution would be to have a Mentoring/Sponsoring-type program for interns/co-ops/newbies, where company policy and procedures are not just reviewed in principle, but in practice. So, for example, the newbie would work side-by-side with the Sponsor for a period of time so he or she can become familiar with how the business works on a day-to-day basis. That way, in addition to gaining experience, he or she can become comfortable and used to performing their job the way it's supposed to be done.

Answer 13

Every open-source project I've ever seen disallows commits on the Trunk, and nobody perceives it as "draconian." Instead it's perceived as an obvious necessity to protect the integrity of code. I think developer branches are a perfectly acceptable solution, with the clear intent of moving the new developers to "full committers" as quickly as they can prove themselves.

Answer 14

Karl Fogel discusses this situation and several similar ones in his famous book, Producing Open Source Software - How to Run a Successful Free Software Project, and comes to the same conclusion: strict testing and code review are more helpful than strict authorization.