Monthly Archives: June 2011

Spring cleaning? Hell no!

http://flic.kr/p/xSB58

It’s summer time already, but every time I hear about cleaning revolution it reminds me of spring cleaning. And I can tell you I hate spring cleaning! I don’t get the whole idea of one time movements just because you didn’t care enough to keep your things tidy all year long!

The whole thing gets triggered now and again, because another developer gets annoyed with the amount of rubbish in the error log. Through years of not caring “suddenly” it’s hard to spot major issues through the flood of notices, warnings and stricts. So Tech Debt Tuesdays, unit test Thursdays and  clean up Fridays take place. Yada yada yada. Everybody get annoyed trying to fix it and after that all gets to normal. Why?! Have Starbucks Mondays and Orange Wednesdays instead – it’s fun! Do your day to day job and keep your code tidy. Every day. Every hour. Every minute. You spot something, you fix it. Simple. And it doesn’t matter whether it’s your code or not. Whether you are going to change it or not. The fact you have seen it is good enough to make you fix it.

I can only repeat after Uncle Bob Martin:

Cleaning code is just part of the normal everyday job of a programmer. It should not be something special on the schedule.

Teams should gradually fix code quality _outside_ the schedule. The improvements should be long term an on-going. Never schedule them!

Code quality issues should _never_ be in the backlog, or on the schedule. They should never get to that point.

Go team!

Running php linter before pushing changes to a git repository

A problem

Have you ever reviewed code that contained parse errors and wasn’t even proper PHP? I have and that’s why I have finally decided to stop that. Well, I have not decided to stop reviewing code, I have decided to prevent people committing PHP code which contain parse errors. With git hooks it’s fairly easy.

One way with pre-commit hook

One way to do it would be with pre-commit hook. Travis Swicegood explained it very well in his blog post. One problem with that is that every developer would have to deploy that hook locally in every repository. So that was really not an option for us.

Or another with pre-receive hook

What I wanted to do was to prevent people pushing broken code to git repositories stored on our git server (sort of in-house github). So I would deploy a hook on the server and the check would be done in there. pre-receive hook seems to be an ideal place for that. You can find our current version of the hook below:

Installation

To get it installed simply copy pre-receive hook into hooks subdirectory of a repository on a git server and make the file executable.

Internals

So how does it work? It’s actually pretty simple. If the hook exists with a non zero error code the push will fail, otherwise it will succeed. So all we have to do is to get a list of files changed in a given changeset and run php linter against them.

The hook accepts 3 parameters:

  • old revision id (ie. 325fbce03ba542c37c8529f36bf66998594e8384)
  • new revision id (ie. 58127dc11f4fd0c433cc6485a10925be5b56e1bb)
  • full reference name (ie. refs/heads/master)

Running git diff with –name-only option will give us list of changed files. But we cannot simply run php linter at this point as you have to remember that on git server there is just bare repository, without working directory tree. So we have to fetch the files straight from git context. We can fetch an object contents from index using git cat-file command, but it requires a unique object identifier to return file contents, so first we need to get this first. git ls-tree command is just what we need. Running it with new revision id and file name, we will get all the information we need!

#> git ls-tree 58127dc11f4fd0c433cc6485a10925be5b56e1bb Test/Application/WebTest.test.php
100644 blob 30a5e1914fe075db11c31780454fcc9d1d83aa9d	Test/Application/WebTest.test.php

Now, when we run git cat-file with the object type we got back (blob in case of files) and object id, we will get contents of that file, that we can finally pipe into php linter process. If the process returns any non-zero error code we display an appropriate message and stop script execution with a non-zero code:

smarek@28:~/git/modules/Sandbox (master)$ git push origin master
Counting objects: 14, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (8/8), done.
Writing objects: 100% (9/9), 857 bytes, done.
Total 9 (delta 2), reused 0 (delta 0)

Running php linter...
Error parsing file 'Test/Application/WebTest.test.php'

error: hooks/pre-receive exited with error code 255
To git:Sandbox.git
 ! [remote rejected] master -> master (pre-receive hook declined)
error: failed to push some refs to 'git:Sandbox.git'

otherwise we just stop the script with a zero code!

smarek@28:~/git/modules/Sandbox (master)$ git push origin master
Counting objects: 8, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (6/6), done.
Writing objects: 100% (6/6), 658 bytes, done.
Total 6 (delta 0), reused 0 (delta 0)

Running php linter...
No errors detected

To git:Sandbox.git
90f43b9..36582e1  master -> master

No more parse errors! FTW!