Gáspár Nagy on software

coach, trainer and bdd addict, creator of SpecFlow

Git for code reviews

by Gáspár on February 5, 2014

Have you ever considered git as a collaboration tool to discuss code issues or alternative solutions? Me neither. My colleague,Viktor, has conducted a code review recently and used git for supporting it. He quickly won me for this idea and let me share it.

We do quite a lot of code reviews at TechTalk as a part of the agile development process we use. We do this in two stages. On the first stage, the development team itself takes care of the code quality. This is either done with pair programming or with team review meetings. On the second stage (ideally bi-monthly or so), we ask someone outside of the team to spend some time with the code and discuss the findings. This someone is usually an experienced developer from another team.

I have also conducted such “external” code reviews. I like to do it low-tech and very informal, so I collect my findings as simple notes. (I use OneNote for this, but a notepad would be just as fine.) I categorize these notes into general and concrete issues. In the code review meeting, I go through these notes and discuss it with the team. The majority of these notes are related to a small piece of code in the application. For the concrete issues, I even give suggested solutions usually. These are all in the notes I have prepared.

It works fine, but finding these code parts during the meeting is pretty annoying sometimes. For trivial issues where everyone is fine with the proposed fix, the process of handing over the notes to someone who will later update the code feels cumbersome.

review brances (here wihtout prefix)The idea of Viktor was to use git, more precisely git branches for these code-related comments, especially where there is a suggested alternative. Since creating switching branches is super easy in git, this sounds like a good tool to try.

Actually the idea is not totally new. For example, the code review feature of TFS also uses the source control features to control the process (but does not work with git unfortunately). Also the github pull requests can be used like that. Still, using pure git has the big advantage of being simple and geeky.

review suggestion as a commitThese are our consolidated rules now:

  1. Create a new branch for every topic. I recommend using some naming convention (e.g. “review-inputmodel”)
  2. Create a commit for every note you want to make. You can use the commit comments for describing the problem more in detail.
  3. Make sure that these comment-commits are isolated and cherry-pickable.
  4. For questions or problems without a suggested solution, you can have commits that add (TODO)comments to the code.review note as a comment
  5. For these kind of smaller comments, you can create a single branch, like “review-commits”, because these will never be merged back anyway.
  6. In the meeting, you can go through the branches/commits. You can show the commit diff and also checkout the branch to show how the code behaves.
  7. The review-branches can be pushed up. If you don’t want to “pollute” your main repository with review branches, you can use an alternative remote for this (stored in a file share, for example).
  8. The team can review the commits later once more and either use them as a starting point for a proper fix.
  9. Once, the necessary review result actions have been taken, the branches can be deleted.

Happy gitting and reviewing! (Thanks to Viktor Nemes for the inputs.)

2 thoughts on “Git for code reviews

  1. Jonas says:

    Interesting. I am still sceptical about this highly distributed model:
    – Is it really practicable to make those commits isolated and cherry-pickable?
    – Isn’t there still a big chance for semantic merge conflict with that many branches
    – What about continuous integration (i.e. “advanced tests”): That is still happening on trunk?

    • Gáspár says:

      Well, I think the primary goal is not to be able to merge these ideas back without special considerations or following the project’s branching guidelines. A good example for this might be, that the reviewer points out a bug, even provides a proposed fix, but do not perform all necessary checks from the definition of done, like writing a unit tests.
      Still, the review comment and a proposed fix is somehow has a live connection to the code base. By cherry-pickable, I was more referring to having isolated commits for the different review notes (only one note per commit).