Perhaps you have heard the Git-oriented review system, Gerrit, but one side-effect of this system is how it encourages creating a good Git commit history. The following is a screen-shot of part of the commit history of a repository at work before using Gerrit:
And here is a screen-shot of the same repository a month later once we started using Gerrit:
While the details of the history don’t matter, the older history is much more complicated. Rolling back a commit, or comparing differences with a complicated commit history is painful.
Sometimes, the pain is removed from the history and transferred to the developer trying to add a new change. ;-)
The Gerrit-oriented work-flow may best be illustrated as a collection of three branches, and a little story.
The newest engineer, Fritz, is assigned
TICKET-123 to add a
feature. After syncing the
master branch with a
git pull (she had
already cloned the repository from Gerrit), she began this dance:
- She issued:
git checkout -b TICKET-123to begin her work on her own development branch (in the diagram above, this is labeled,
- She issued lots of small commits while she worked on the project.
- The project took longer than she thought, so she issued:
git syncto pull from
master. While she had to do a merge, she felt it better to merge now than later.
- She squashed all the little commits into one commit.
- She created the Gerrit review.
- She went to the Gerrit Dashboard, and found her change, however, by the time she did, the Jenkins validation system discovered that she had a syntax error in the code. Good thing no one on the team could even pull the change into their code base.
- She quickly made the fix on her development branch, and committed
git commit --amend
- Re-ran the Gerrit review command, patched her original review.
- She added the members of her team to the list of reviewers
(breathing a sigh of relief since Jenkins had already given the
review a +1 validation). Gerrit sent them all email messages, but
one of the reviewers noticed that she hadn’t updated the
READMEfile, and gave the review a
- She updated the
READMEfile on her development branch, and committed, but using another
git commit --amendcommand.
- She re-ran the Gerrit review command again, which applied another “patch” to the review.
- This time, the review is given a +2 by a reviewer, the change was
merged to the
masterbranch through the Gerrit interface.
Now, every member of the team, including Fritz, will get this change once they pull from master.
In this case, using Gerrit stopped both a syntactical bug as well as an oversight that would have led to increased technical debt. Gerrit reviews are a good thing, but here is some advice:
- Re-read your comments before posting. While code should be viewed
objectively, people write code, and comments can be viewed as a
personal attack. Soften the tone by:
- Phrase your opinion as a question.
- Use the passive English voice1.
- Comment on the code, not the programmer.
- Have a thick skin, and try not to take offense during a review of your recent commit (we’re all on the same team with the same goals, right?) Comments on code is just that … comments on code. Don’t assume it is personal.
- If the code doesn’t make sense, request a comment insertion explaining the code. Doesn’t make sense to you now, it won’t make sense to others next month.
- Not sure if the code change actually addresses the goal of the commit? No problem, you can actually pull the review branch from Gerrit to your workstation and try it out.
In the example above, when Fritz committed and pushed her changes to Gerrit, Gerrit created a review branch. This branch, while temporary, is actually accessible.
Let’s suppose that you are to review Fritz’ commit, and Jenkins stamped a green check on it during the validation phase, you aren’t convinced it does the right thing. Well, you can checkout this review branch and play with it, by clicking on the “Copy” button under the “Download” section.
This may be the first time any one ever tells you to use the passive voice, and yes, Mrs. McDougal would cringe if she read such insipid prose, but don’t worry, she’s retired now.
What’s passive? This is a very passive voice because it is written with is. Why is it better? It shifts the attack. For instance:
You wrote both tabs and spaces on this line.
Puts the emphasis on the subject: the programmer. Sure, for prose it is better, but the following is a better review comment:
This line was written with both tabs and spaces.
Not only the verb, was written such ghastly prose, the phrase does not accuse the writer… besides, such weak verbs suck the fighting spirit from a person.