Gerrit Best Practice

Ein Kollege hat mich gefragt, welche Erfahrungen und Empfehlungen ich im Zusammenhang mit Code Reviews in Gerrit habe. Die folgenden Punkte stellen meine ganz persönliche Meinung dar und dürfen gerne als Diskussionsgrundlage dienen 🙂

Der Einfachheit halber gehe ich davon aus, dass direkt gegen den master gearbeitet wird um nicht immer „Zielbranch“ schreiben zu müssen. Das Upstream Repository heißt in allen Beispielen „origin“ und nicht „gerrit“. Wer dadurch Problem mit git-review hat sollte einen kurzen Blick auf meinen Artikel dazu werfen. Alle, die Gerrit noch nicht kennen, sollten zuerst meinen Einführungsartikel in die Arbeit mit Gerrit lesen.

Entweder Entwickeln oder Refaktorieren oder Formatieren

Ich empfehle immer wieder, diese drei Tätigkeiten nicht zu mischen. Unter das verbotene Refaktorieren fällt hier vor allem das Umbenennen von Dateien und Klassen. Der Grund hierfür ist ganz einfach, dass ich als Reviewer sonst große Mengen von Code durcharbeiten muss, die eigentlich nur verschoben oder umformatiert wurden. Wenn ich eine Funktion im Rahmen der Entwicklung refaktoriere um Clean Code zu behalten, ist das sicherlich ein Grenzfall.

Keine komplexen Changes

Wie auch mein Kollege Tobias Kilian immer wieder betont: Baby Steps. Wenn ein Change zu umfangreich ist, verliert der Reviewer schnell die Übersicht. Auch die Wahrscheinlichkeit für Konflikte und Breaking Changes steigt deutlich, wenn umfangreiche Änderungen als „großer Wurf“ umgesetzt werden.

Dependent Changes vermeiden

Wenn ich zwei Commits nacheinander zu Gerrit schicke, ohne dass der erste schon veröffentlicht ist, habe ich Dependent Changes. Das bedeutet, dass ich den zweiten Change rebasen muss, falls der erste nicht in Ordnung war. Wenn die Änderungen in den Commits also nicht wirklich aufeinander aufbauen sollte man lokal zwischen zwei Changes auf den aktuellen master wechseln.

git fetch
git reset --hard origin/master

Keine lokalen Merges! Keine!

Gerrit verbietet aus gutem Grund Merge Commits hochzuladen. Merge Commits sind generell schwierig zu handhaben, weil mit ihnen ein rebase nicht mehr möglich ist. Dadurch kann es für Gerrit extrem kompliziert werden einen Change in den master zu bringen. An Stelle von git pull  und git merge  muss immer git pull –rebase  verwendet werden.

Commit Messages nach Gerrit-Konvention

Gerrit hat klare Konventionen für die Commit Message:

• Die erste Zeile soll eine kurze, beschreibende Überschrift enthalten. Als Präfix hierfür wird häufig die betroffene Komponente oder eine Ticket-ID verwendet.
• Die zweite Zeile bleibt leer
• Alle weiteren Zeilen enthalten eine kurze Beschreibung dessen, was gemacht wurde. Die Zeilen sollten nach 76 Zeichen umgebrochen werden, damit es in der Web-UI ordentlich aussieht.
• Als letzte Zeile kommt die Change-ID

Letzteres ist keine Konvention sondern für das Funktionieren von Gerrit unerlässlich.

Vor dem ersten Patch Set auf den master rebasen

Wenn die Arbeiten an einem Change abgeschlossen sind sollte der Commit vor dem ersten Push zu Gerrit auf den aktuellen master rebased werden. Das erspart so manchen Merge Conflict beim Submit.

Nach dem ersten Patch Set nicht mehr rebasen

Zwischen zwei Patch Sets sollte kein Rebase auf den master mehr gemacht werden, weil der Reviewer sonst sehr seltsame Effekte erlebt:  Wenn er die Patch Sets direkt miteinander vergleicht sind eventuell plötzlich eine Menge neue Änderungen im Code, die dann natürlich auch gereviewt werden müssen. Vergleicht er das letzte Patch Set aber mit Base, sind die Änderungen plötzlich weg. Wer seinen Reviewer lieb hat, lässt sowas.

Hiervon ausgenommen ist natürlich ein lokales Rebase im Fall von Merge-Konflikten beim Submit.

Keine Neuentwicklung zwischen Patch Sets

Bitte keine „schnellen Änderungen“ zwischen zwei Patch Sets. Das macht eurem Reviewer das Leben unnötig schwer. Ziel ist es schließlich, den neuen Code möglichst schnell in den master zu bekommen. Ein neues Patch Set sollte daher nur die Änderungen enthalten, die durch Kommentare oder fehlgeschlagene Tests notwendig sind. Alles Andere gehört in einen neuen Change. Baby Steps, remember?

Lauffähigkeit wird durch einen Build Server und Tests geprüft, nicht manuell

Nach meiner Erfahrung klappt das mit der lokalen Analyse auf Lauffähigkeit nicht zuverlässig genug. Zumindest ein Lint Checking sollte bei jedem Change durch den Build Server durchgeführt werden um die syntaktische Korrektheit der Software zu prüfen. Automatisierte Tests helfen darüber hinaus die fachliche Korrektheit zu prüfen und Regression frühzeitig zu entdecken – aber das führt an dieser Stelle vielleicht zu weit.

Gerrit Plugin für die IDE verwenden

Es gibt ein recht gutes Plugin für IDEA, mit dem man komfortabel Änderungen aus Gerrit auschecken kann. Das senkt die Hürde deutlich, den Code in der IDE statt nur im Browser zu reviewen. Ich selbst mache kleine Reviews meistens im Browser, steige aber vermehrt auf Reviews in der IDE um.

Reviews sind wichtig und brauchen Zeit

Ich habe mit Erschrecken festgestellt, dass Reviews zu oft keinen sehr hohen Stellenwert haben und wenig Ansehen genießen. Häufig gelten sie (genauso wie Tests) als Zeitverschwendung 🙁 Wer kann der arbeite darauf hin, dass Reviews als wichtig dastehen! Ich hatte in einem Projekt den Ruf eines sehr strengen Reviewers, was der Code Qualität gut getan hat. Wer mag kann mal einen Blick auf die Geschichten rund um das IBM Black Team werfen (z.B. auf http://www.themarysue.com/ibm-black-team/). So muss ein Review laufen 😉