Gerrit and Code Review Best Practices

1. Best Practices - Making Changes

2. Best Practices - Reviewing Changes

3. Best Practices - Project Setup

Best Practices - Making Changes

Making Changes

each change should add something usable

  change = complete feature

  change = bug-fix

⇒ Have a releasable state after every change.

Making Changes

each change should focus on only one thing

do not mix features, bug-fixes

Q: Why is this bad?

A:

You need the bug-fix in another branch.
→ git cherry-pick is not easy
The feature broke something.
→ git revert cannot be used
It’s more difficult to review.

Making Changes

push only changes that are ready

Q: Who wants to merge unfinished changes?

Q: Who wants to review unfinished changes?

A: It is unclear for reviewers

what is an issue and
what is simply not done yet

Making Changes

mark unfinished and experimental changes as [RFC] (Request For Comments),
but tell reviewers which feedback you expect

RFC

Examples:

Making Changes

write good commit message

make the change attractive to reviewers

Q: Who wants to review this change?

Bad Commit Message

Making Changes

write good commit message

make the change attractive to reviewers

Q: This one looks more interesting, doesn’t it? Good Commit Message

Making Changes

write good commit message

have precise subjects

Making Changes

explain the motivation for the change

what was changed can be seen from the diff

Q: Why is this commit message bad?

Commit Message without motivation

A: No info about why this had to be disabled.
git blame gets less useful.

Making Changes

prefer small changes

Q: Who wants to review this change?

Huge Change

Making Changes

make big features as series of small changes

each change adds something usable
mark the change series as a topic

Example:

Topic

https://gerrit-review.googlesource.com/#/q/status:merged+project:gerrit+branch:master+topic:ssh-list-groups,n,z

Making Changes

split refactoring from feature

makes change smaller
the refactoring change is "easier" to review
- even without knowing anything about the change,
  one can check if the old and new code do the same

Making Changes

provide screenshots for UI changes

Screenshot Attached

Example:

https://gerrit-review.googlesource.com/#/c/36460/

Making Changes

reply to every comment

use Reply Done if issue was addressed
write reply why comment was ignored

Making Changes - Summary

Better one developer invests time to polish a change
than n reviewers waste time on review.

Best Practices - Reviewing Changes

Reviewing Changes

review each change

Q: Looks good!?

Small Change

Reviewing Changes

review each change

Q: And now?

Small Change

Reviewing Changes

everyone should do reviews

helps to distribute knowledge
share the review effort

follow reviews of others

learn from it

Reviewing Changes

cooperate to find best solution

no fault finding, no finger pointing
propose improvements

Reviewing Changes

mark optional comments as [optional]

Optional Comment

comment on nits

Nit Comment

Reviewing Changes

try out changes

fetch the change
try to break the feature
if something doesn’t work,
find the error in the code and
comment inline
vote in Verified

Reviewing Changes

issue in merged code

upload a change that fixes the issue
find the change that introduced the issue
and comment on it
report the issue in the issue tracker system

Reviewing Changes

Code Review Voting

+2: fully understood, all correct, looks good
+1: looks good, don’t see anything wrong
0: optional comment, just a question
-1: feature is good, but there are issues that should be fixed
-2: veto, must not be merged

Reviewing Changes

do not self approve changes

4-eyes principle
may be enforced via Prolog submit rule

Reviewing Changes

review takes time

days, weeks or even months
merge changes only when they are ready
some changes may never be merged

Reviewing Changes - Summary

Being a good reviewer is a different qualification than being a good developer.

Code Review works!

https://gerrit-review.googlesource.com/#/c/30171/

Best Practices - Project Setup

Project Setup

automate verification

Gerrit Trigger Plugin for Jenkins

Project Setup

define code styles
agree on line endings
have a contributor guide
https://gerrit-review.googlesource.com/Documentation/dev-contributing.html

Project Setup

not everyone needs submit privileges

committer vs. contributor
committer election process

Project Setup

use text format for documentation

e.g. ASCIIDOC
makes documentation changes reviewable
version documentation together with the code
vote -1 if documentation is missing

Project Setup

customize submit rules via Prolog

Project Setup

choose right submit type

merge/rebase effort vs. risk to break main build
enable Automatically resolve conflicts

Project Setup

do not version large binaries

results in bad performance
use Maven repo, Nexus to store build artefacts
configure max object size to make push fail

find right granularity for the repositories

avoid too big repositories
version together what needs to be released together

Project Setup

learn Git/Gerrit tooling and workflow

offer Git/Gerrit trainings
offer support via mailing list