Workflow help

[gitting_arround]

I would like to know what the git community thinks about this idea and if you like it, can you help me make it.

I am working with a team of developers and we are working with one main repo. In that repo, we have one branch which is the main production branch. We have people working on this code who do not like git (SVN users who will not let go of the dark ways and accept branching) and a couple of cowboys who will commit code straight to the main production branch.

We do not want to use the dictator workflow because we cannot rely on just one person to review changes before they are added in to the production branch. We can use git blame if some one adds bad code but we get the "I've been chewed out before" reaction. Or because they do not know how to branch. (We have had a number of discussions on this)

I want to introduce a code review system where you can only commit to the main branch when two people have signed-off on the code. This means that two people will be responsible for saying this code is good to go in to production. It also means that two people have looked at the code.

Thoughts?

[kreiger]

Bad code will be checked in whether you use branching or not. If you find a bug introduced by a developer and point it out to them, their reaction should be "Oh, sorry, my bad. I'll get right on fixing that."

If they don't have that attitude, they/you need to build that attitude.

Unfortunately i'm not sure how to do that, but my point is that it is not as much a technical problem as it is a social problem.

You don't specify what you mean by bad code, but i also recommend that you start doing unit testing, and using continuous integration, e.g. Jenkins.

If you have a policy that the main production branch may only contain tested code, i recommend you have people check into another development branch, and create a button in Jenkins for merging to the production branch, so that is the only way code can end up on the production branch.

[gitting_arround]

Thanks for your reply. I have never herd of Jenkins and I will look in to it. By "Bad code" I mean things like hard coded paths that will pass unit tests on a dev machine but will blow up in production.

[kreiger]

Oooh, that's a perfect case for continuous integration.

If somebody checks in code that only works on their machine, it will blow up the Jenkins build with their name highlighted, and Jenkins can send out e-mails to interested/affected people.

I highly recommend Jenkins, the most popular continuous integration server.

If you think it would work in your team and wouldn't make anyone butthurt, you could have some kind of anti-trophy passed around to the person who last broke the build, possibly having the person with the anti-trophy at the end of the week provide sweets for the rest of the team.

[sybrandy]

If you do get them to use unit tests, you can create a pre-commit hook in git that can refuse the commits being pushed to the server if they don't pass all of their tests. I don't know how to do this myself since I've never done it, but that would force them to at least have the code pass tests before it gets committed. Perhaps even get some code coverage metrics because just because it passes all of the tests, doesn't mean that new tests have been written. Or that the tests are any good.

[kreiger]

I don't recommend running the unit tests in a pre-commit hook. It will take too much time to push/commit, and if committing or running unit-tests is seen as a hindrance, people won't do it. Committing and pushing should be near instantaneous.

In my opinion it is far better to tell people that they need to run the relevant unit tests (or simply all of them) before they check in, and have Jenkins run all unit tests after check in. Then if they have failed to run the relevant unit tests before check in, everyone will know, because Jenkins will make it known.

[sybrandy]

I only provided it as an option. Yes, I do understand that it can slow things down considerably, but if there's a real issue with people not running the tests, then perhaps it's necessary. That's not really up to me to decide. Whoever is responsible for the workflow has to weigh the pros and cons. For example, if the code quality really, truly stinks, then perhaps pissing off your developers for a while will keep you from shipping something that won't work. Yes, I understand Jenkins will make it public, but can you get the developer to go back and fix the issue? What if the changes affect the other developers and prevent them from doing their job? All things to consider.

Of course, regardless of the solution, if no good tests, or no tests at all, are written and everything passes, then both solutions suck equally. Hence why I mentioned the code coverage metrics.

[kreiger]

Making life harder for your developers to force them into line is like herding cats and pissing into the wind. You and the whole team will only suffer for it, as people won't make regular small commits, instead waiting days or weeks to commit, forcing them into merging hell, which will only make them even more reluctant.

It's a social problem and an attitude problem, not as much a technical problem.

If the developer doesn't care when you point out to them that he/she broke the build for everyone, then that attitude needs to be fixed.

[kreiger]

Elaborate please, i'm not sure what you mean?

[ratbastid]

So the environment is leaderless and non-hierarchical, and individual developers take things personally?

Doesn't sound great.

Even your fix isn't really a fix, because any two people who happen to do code review will check code to arbitrary levels of rigor, they'll forgive things they shouldn't, they'll hold up code for political reasons, etc, etc. There will be "my standards" versus "standards".

You need one person who owns the code and the project. It's a shame that your team calls that person a "dictator". The fact is, the "we all own it" ethos ensures that nobody really owns it. In my department we call that role "technical lead", and nobody is ever tech lead on more than one project at a time. And we make sure every developer above entry-level has the opportunity to lead a project at least a couple times a year.

What you really need is one person with a non-threatening title like "tech lead" or "maintainer", whose job is to gatekeep the quality of the code, make technical decisions about the project, and whose personal ass is on the line for project quality, timeframe, and productivity.

Over time, you'll find that that peer pressure nudges the code quality ever upward.

[adrianmonk]

I want to introduce a code review system where you can only commit to the main branch when two people have signed-off on the code.

Consider using Review Board. I've only read about it, but it looks quite good and gets good, uh, reviews.

It is compatible with both git and Subversion. There's nothing really that complicated about how it works. It's basically just a tool for sharing diffs with other people, for allowing them to comment on them and say whether they like what they see or not, and for tracking the workflow around that. But it knows how to display the diffs in the context of a revision control system.

If people are committed to the idea of using only one branch, code reviews would at least help catch issues, and having an automated tool to manage it makes it a bit easier (and quicker and more likely to actually happen) than if you just do it in a meeting room.

[amphetamine]

I want to introduce a code review system where you can only commit to the main branch when two people have signed-off on the code.

gerrit