Table of Contents

Background

The first commit I sent for an organisation just out of college, was a 200 line change with the commit message as “Fix Index Bug”. I would never forget the mortified look on my team lead’s face, when he saw the commit message. He tried to explain me as patiently as possible about the importance and power of a commit message. The 22-year-old-me was convinced that it was not a big deal, and in no way commit messages would botch engineering productivity.

With the influx of new people in various teams at Zendrive, I was able to see what my team lead saw back then: it was becoming difficult to have an “unsaid” standard of Git / Gerrit commits and it did make things difficult.

Introduction

This document is nothing more than a set of “guidelines” which an individual, teams, the entire org, can follow. These are not strongly defined rules. Consistency is the key here. Feel free to suggest changes to this page. Feel free to use this page as it is or with modifications, for your own organisation, team or just yourself.

Chatham House Rules Apply.

Guidelines for CL Authors

General Guidance

Put the JIRA ticket number in the title of the CL, like “[ATBN-13] CL title goes here”. This makes the Jira integration work, and helps reviewers understand the context of the change later on. This is the Jira equivalent of putting “closes XXX” in the body of the CL.

Write a description that answers “Why?“. Assume that years from now, someone is going to track “git blame” all the way back to your change, and you won’t be around to explain it or be blamed. Explain the reason behind the change. Add any background information necessary, and note any decisions you made while doing the change. Read how to write a git commit message here and here.

Use Work-In-Progress CLs. Open your CL as a draft CL by default, so that no-one is notified that it is ready for review. Only move it to “ready for review” once you’re ready for more eyes – which means you’ve done all the things necessary and some things mentioned in this guide. (Gerrit has a setting that lets you mark all your changes as “WIP” by default)

Review your own CL first. Before you ask for a review, go through the code yourself and look for anything that you can prevent someone else from having to point out: formatting, missing newlines, leftover debugging statements, etc. You should also ensure bot automation runs are successful before asking someone to review. Run the unit-tests yourself, because it should not be your reviewer’s responsibility to do the same.

Add “review” comments to code that could use explanations for your reviewers or to highlight sections of the code where you want them to pay special attention, but also consider that it might be better for that review comment to be a code comment.

Prefer smaller CLs, or at least reviewable commits. If you are doing a major change that doesn’t have to be released all at once, you will help your reviewers, and thus help yourself, by breaking things up into manageable chunks.

Do not add unrelated code to your CL. You should ensure that a commit should not contain multiple unrelated changes. It is okay to do small formatting/convention fixes in the file you’re touching, but not full-fledged unrelated changes.

Address all comments. Ask for clarification, offer clarification, explain the decisions you made to reach a solution in question. Be convinced your reviewers feel all comments have been addressed before you commit. Questions are addressed by providing an answer. Suggestions can be addressed in one of three ways: adopt it immediately (“Done.”), defer it to a subsequent change (TODO with a bug #) or push back with additional information.

If your CL is “approved with changes”, don’t merge it without making the changes unless the situation is exceptional; have a conversation with the reviewer instead. But if you find yourself completely blocked because the original reviewer is unavailable and you are still opposed to the changes, you can bring in a second opinion.

Do not “+2” a CL yourself. As a rule of thumb, get someone else to +2 a CL. (Exceptions: experimental folders, or Personal branches (u/username/branchname) or whatever exceptions your organisation follows)

After the review

Code reviews are in large part about having others watch your back. Don‘t hesitate to say “*Thank you*” once the review is completed. Additionally, if you’re new to code reviews, take a few moments to reflect on what went well or didn’t.

Guidelines for CL Reviewers

General Guidelines

Be constructive, kind and humble. For instance, the difference between “What do you think about trying this out?” versus “This is stupid, don’t do it” is massive. Remember that text has a negative bias. If the phrasing is neutral, the recipient in most cases will assume the tone is negative.

Provide positive feedback, too. No need to be all fake smiles, but if there’s a good decision, or if somebody takes on a really grungy task, acknowledging that is a nice thing to do. Reviews aren’t just about pointing out what needs to be changed. Point out what’s good about the CL, decisions you agree with, bits you find clever. Acknowledging the positives both helps keep things civil, and brightens the recipient’s day.

Expect the CL author to follow the guidelines above. By agreeing to these guidelines, the team is also agreeing that we’ll keep each other accountable on things like ticket numbers in titles, meaningful descriptions, and so on.

Respond with an “approved with changes” message to require a minor change before merge. If someone approves a CL “with changes”, they mean that their approval is conditional on those changes, but they don’t want to block the author if they’re unavailable. If you want to “approve with changes”, +2 your change usually, but do write “Approved with changes” in the review.

The point of a “+1”. Make sure you understand what the implications of a patch might be, or leave the review to others. Partial reviews, reviewing code style, for example, should be given a +1 instead of a +2. This also applies if you think the patch looks good, but may not have the experience to know if there may be unintended consequences.

Reserve “-1” for things you want to be blocked on you. “-1” needs to signify that you want a particular change to be blocked on you before it goes to production. It could be because you are unsure of a particular change, or need more information, or an important unit-test is missing without which you do not want the change to be merged.

Reserve “-2” for things you know can’t go to production. “-2” is an andon cord that stops the whole “production line”, because even if someone else approves the PR, your review will block it from continuing. Only use it when you want to guarantee a change doesn’t reach production because you anticipate harmful consequences.

Nits are okay, but they’re still nits. If you see missing trailing newlines, for instance, offer to help the author configure their editor, but still feel free to point out the missing trailing newlines. Consistent style is important because it means the next developer to work on the repo won’t have unrelated style changes.

Don’t review unrelated code. Only ask for changes to unrelated code if it impacts the change itself. Don’t ask CL authors to “fix this while you’re in there”; if it’s important to fix, open a new Jira ticket.

Find an end. Don’t drag things out for longer than necessary, please. It‘s soul-deadening for the recipient. Keep in mind that “LGTM” does not mean “I am willing to put my soul, my life, my property on the line that this piece of code will never fail”, but “looks good to me”. If it looks good, move on. (That doesn’t mean you shouldn‘t be thorough, but take that judgment call.) And if there is a bigger refactoring to be done, create a new ticket.

tl;dr

Reviews are about the code. It’s easy to take it personally when someone is criticising your code, but the whole idea is to get better code into our codebase. Again, this also applies in the other direction: review code, criticise code, but don’t make it personal.

Further reading

This post is a vebatim modification of a post I saw on Rands’, about the Git Commit. It has been modified to fit the Zendrive guidelines and the Gerrit usage. Many of the suggestions in this guideline, especially around “Be kind…“, were taken from this classic GitHub blog post about writing PRs. A must-read: Sandya Sankarram’s wonderful “Unlearning toxic behaviors in a code review culture”, based on her equally wonderful Alterconf talk with the same title, outlines both unhelpful and helpful behaviors for both PR reviewers and authors. Chromium’s guide for authors and reviewers is also extremely helpful.

The power of a good commit message: https://dhwthompson.com/2019/my-favourite-git-commit