Code-Review kann besser werden
(tigerbeetle.com)- Viele Entwickler sind mit der Code-Review-Erfahrung auf GitHub unzufrieden und probieren neue Ansätze aus, um sie zu verbessern
- Das experimentelle Tool
git-reviewwurde so entworfen, dass Code-Reviews nicht über eine Browser-Weboberfläche, sondern direkt lokal zusammen mit dem Code bearbeitet werden - Reviews werden als einzelner Commit verwaltet; Review-Kommentare werden wie Kommentare im Code hinterlassen, und Reviewer und Autor überarbeiten diesen Commit gemeinsam
- Wenn der Code während des Reviews geändert oder rebased wird, entstehen jedoch Unannehmlichkeiten bei der Konfliktbehandlung und beim Einsatz von
--force-with-lease, sodass der Ansatz keinen großen Erfolg hatte - Am Ende kehrte man wieder zu webbasierten Reviews zurück, doch die Idee, den Review-Status direkt im Git-Repository zu speichern, bleibt attraktiv, und zusammen mit künftigen Git-Verbesserungen wie der Einführung einer Gerrit-artigen Change-Id könnte eine bessere Alternative entstehen
Problembewusstsein bei Code-Review-Systemen
- Derzeit sind viele Menschen mit dem Code-Review-Prozess auf GitHub unzufrieden
- Die Hauptprobleme sind fehlende Unterstützung für gestapelte Pull Requests und Interdiff-Reviews sowie außerdem:
- Der Review-Status wird nicht im Repository gespeichert
- Reviews über eine Remote-first-Weboberfläche sind zwingend erforderlich
- Mein Problem liegt im Mangel an Dezentralisierung beim Review und in der Ineffizienz der Benutzeroberfläche
Vergleich von Workflows für Code-Erstellung und Review
- Beim Schreiben von Code verwenden Menschen lokal einen Editor
- Die Umgebung hat geringe Speicher- und NVMe-Latenzen und ist auf den individuellen Workflow des Nutzers optimiert
- Auch für Code-Reviews wird bevorzugt, den Source-Branch lokal zu pullen und dort zu arbeiten
- Mit Tools wie Magit lässt sich nicht nur der Diff, sondern auch der gesamte Code-Kontext erkunden
- Man kann eine leistungsfähige Entwicklungsumgebung nutzen, etwa zum Ausführen von Tests, zum Springen zur Code-Definition oder zum Ausprobieren von Refactorings
- Um dagegen Feedback in einem PR zu hinterlassen, muss man in den Browser und zu einer langsamen Weboberfläche wechseln; bei großen Diffs kommt es zudem zu deutlichen Eingabeverzögerungen
Ideale Code-Review-Oberfläche und Speicherstruktur
- Am natürlichsten ist es in der Praxis, Kommentare direkt inline im Code zu hinterlassen oder den Code direkt zu ändern
// CR(matklad): Hm, this check seems imprecise to me. // Shouldn't we compare `replica.view` instead of `header.view` here? if (header.view != view) return; - Dadurch, dass Daten in einer entfernten DB statt im lokalen Git-Repository gespeichert werden, entstehen zusätzlich Probleme mit Latenz und Vendor Lock-in
Die Idee von git-review und die Erfahrungen in der Praxis
- Die Idee von
git-reviewist wie folgt:- Das Code-Review besteht aus einem einzelnen Commit an der Spitze des PR-Branches
- Diesem Commit werden Code-Kommentare mit speziellen Markern hinzugefügt
- Reviewer und Autor bearbeiten diesen Commit abwechselnd, wobei die Zusammenarbeit auf
push --force-with-leasebasiert - Wenn alle Kommentare als gelöst markiert (
//? resolved) sind, bleibt beim Abschluss des Reviews durch einen Revert-Commit ein Verlauf erhalten
- Die Idee ist einfach und praktisch, in der Realität traten jedoch folgende Probleme auf:
- Bei Code-Änderungen während des Reviews kommt es häufig zu Konflikten zwischen Kommentaren und nachgelagerten oder neuen Commits
- Der Force-Push führt zu Reibung in der Zusammenarbeit und erhöht die Komplexität der Arbeit
- Die Diskrepanz zwischen Code-Änderungshistorie und Review-Fortschritt sowie die Verwaltung von Merge-Konflikten werden schwierig
Neue Veränderungen und künftige Möglichkeiten
- Künftig könnte eine Gerrit-artige Change-Id in Git upstream eingeführt werden
- Dadurch ließe sich der Änderungsverlauf pro Commit leichter nachverfolgen, und die Unterstützung für Interdiff-Reviews dürfte zunehmen
- Allerdings sind teilweise Konflikte mit dem Ansatz von
git-reviewzu erwarten - Mit einer neuen Change-Id-Struktur könnten auch ungewöhnliche Ansätze möglich werden, etwa Review-Kommentare direkt zum Commit selbst hinzuzufügen
Fazit und empfehlenswerte Systeme als Referenz
- Letztlich ist man derzeit wieder bei weboberflächenbasierten Code-Reviews gelandet
- Der Bedarf an einer besseren Lösung besteht weiterhin
- Vorstellung verwandter Systeme und Tools, die einen Blick wert sind
- Fossil: Ein SCM-System, das alle Informationen im Repository selbst speichert
- NoteDb: Integriert den Verlauf des gespeicherten Review-Status von Gerrit in Git
- git-bug: Speichert Issue-Informationen in Git
- git-appraise: Bewahrt Review-Informationen in Git selbst auf
- prr: Implementiert eine Review-Oberfläche im Editor in Verbindung mit der GitHub API
- How Jane Street Does Code Review: Vorstellung eines Beispiels aus der Praxis, wie es besser gehen kann
- git-pr: Ein Projekt, das den gesamten PR-Workflow durch native Git-Funktionen ersetzt
Abschluss
- Eine perfekte Lösung gibt es noch nicht, und die Suche nach einer besseren Developer Experience geht weiter
- Die Erwartungen an die zukünftige Entwicklung sind hoch
2 Kommentare
Ich weiß nicht, ob der Ansatz von
git-reviewgut ist, aber ich stimme zu, dass PR-Reviews auf GitHub-Basis furchtbar sind..Hacker-News-Kommentare
Was mich an Code-Reviews schon lange stört, ist, dass wirklich nützliches Feedback (also nicht bloß Hinweise zu Geschmacksfragen) fast immer viel zu spät kommt. Das einzige oder seltene Ergebnis eines Reviews ist dann etwas wie: „Das muss komplett neu entworfen und von vorn begonnen werden“ oder „Diese Arbeit hätte von Anfang an gar nicht gemacht werden müssen“. Es wirkt oft so, als sei das Code-Review der einzige Zeitpunkt, an dem alle Beteiligten tatsächlich mitmachen und ernsthaft über eine Änderung nachdenken. Vielleicht wurde etwas in Meetings oder Jira-Tickets besprochen, aber häufig erfährt jemand aus einem anderen Team oder einer anderen Abteilung erst durch die Code-Review-Benachrichtigung von der Änderung. Auch ich erfahre oft erst durch solche Benachrichtigungen, wenn ein anderes Team etwas Seltsames umgesetzt hat. Es ist unrealistisch zu erwarten, dass alle Beteiligten alles im Voraus mitverfolgen. In Uni-Kursen in den 90ern haben wir auch Design-Reviews gemacht, aber im echten Berufsalltag habe ich so etwas nie gesehen. Ich glaube auch nicht, dass Design-Reviews garantieren würden, alle Probleme frühzeitig zu erkennen.
In der Welt des Software Engineerings gibt es erstaunlich wenig tatsächliches Engineering. Ein Teil davon ist, dass die Branche die Langsamkeit eines sauberen Engineering-Prozesses nicht akzeptiert. Die meiste Software ist nicht sicherheitskritisch, und Bugs oder Fehler können später behoben werden. Die Stakes und die Möglichkeit, nachträglich zu patchen, sind andere als bei Brücken, Fabriken oder Flugzeugtriebwerken, wo Versagen nicht akzeptabel ist.
Unser Team ist eine kleine Gruppe von 4 bis 6 Entwickler:innen. Wenn sich bei einer neuen Funktion ein erster Entwurf im Kopf abzeichnet, sprechen wir sofort mit Kolleg:innen darüber. Weil alle das so machen, drehen sich Code-Reviews vor allem um Kleinigkeiten wie Code Smells, und die Gesamtarchitektur wird meist schon vorher von 2 bis 3 Personen festgelegt. Wenn Teammitglieder dem Code nicht zustimmen, fassen sie ungern Code an, der von anderen geschrieben wurde, und dann wird die Situation schnell unangenehm. Ich glaube aber, dass das auch in größerem Maßstab gut funktioniert, wenn Verantwortung sauber geteilt wird.
Dass sich alle Stakeholder erst zum Zeitpunkt des Code-Reviews beteiligen, ist kein Problem von Git oder dem Versionsverwaltungssystem, sondern ein organisatorisches Problem. Der Kontext der PR-Erstellung, die Ticket-Diskussionen und der Entscheidungsprozess werden nicht geteilt. Das ist ein Beispiel für eine dysfunktionale Organisation und so, als würde man den Veröffentlichungsprozess im Verlagswesen dafür kritisieren, dass sich alle erst dann ernsthaft einbringen, wenn das gedruckte Buch erscheint.
In unserer Organisation schreiben wir für grundlegende Architekturentscheidungen zwingend ein RFC. Was genau als grundlegende Architekturentscheidung gilt, entscheidet das Team laufend selbst. Wenn in der Epic-Phase in Jira noch nicht klar ist, wie die Implementierung aussehen soll, wird zunächst das Schreiben eines RFC zugewiesen. Ein RFC kann nur für unser Team sein oder für das gesamte Software-Team. Im zweiten Fall können es vor dem alle zwei Wochen stattfindenden Meeting alle lesen und Kommentare hinterlassen. So mühsam es manchmal ist: Es ist deutlich besser als an Orten ohne kollaborativen Designprozess auf RFC-Basis.
Meiner Erfahrung nach kann ich der Kritik an Design-Reviews zustimmen. Früher hatten wir formale Designdokumente und Reviews, sind dann aber zu Prototyping und iterativem Design übergegangen. In der Designphase wurden oft wichtige Details übersehen, und weil schon viel Zeit investiert war, wurde später vieles nur noch halbherzig behandelt. Es war außerdem ineffizient, das ganze Team zur Review zusammenzubringen, sodass Probleme am Ende doch erst im Code-Review auffielen. Viele Leute schreiben auch einfach keine guten Designdokumente oder sind nicht motiviert dafür. Ab fünf Personen lässt sich diese Ineffizienz letztlich kaum vermeiden. Ideal ist ein Setup mit dem PO, den wichtigsten Nutzer:innen und etwa fünf Entwickler:innen.
Ich finde die Beiträge über stacked pull requests auf HN extrem interessant. Als wir früher graphite.dev gestartet haben, kannten viele diesen Workflow nicht, sofern sie nicht bei Facebook oder Google gewesen waren. Es ist spannend zu sehen, wie schnell sich Trends bei Code-Reviews in den letzten 3 bis 4 Jahren verändert haben.
Als Nutzer von pre-mercurial arcanist werbe ich weiterhin aktiv für Graphite bei Teams, die noch unter großen PRs oder Merge-Commits leiden. Besonders beeindruckt mich der Mut und das Ergebnis, die Integration mit PRs überhaupt möglich gemacht zu haben. Ich wünschte, Graphite hätte einen prescriptive mode, in dem ein Repo mit einem Hardcore-Setup initialisiert wird und dadurch noch stärkere Annahmen möglich sind.
Graphite ist trotz der tollen Lösung ziemlich teuer, und es ist schwer, die Entscheider:innen von einem Kauf zu überzeugen. Ich hoffe, dass ein so gutes Tool wie Graphite Open Source wird oder direkt in GitHub eingebaut wird.
Ich vermisse den fig workflow ein bisschen.
Wegen des jüngsten Sicherheitsvorfalls bei CodeRabbit zögere ich, neue Tools zu testen, die LLMs mit der Codebasis integrieren. Solche spannenden Experimente verwandeln sich schnell in Sicherheitsprobleme.
Stacked pull requests fügen im Kern unnötige Komplexität hinzu. Häufige, kleine Änderungen sind eine deutlich bessere Entwicklungspraxis. trunk-based development oder continuous integration passen zu diesem Ziel viel besser.
Immer mehr Entwickler:innen scheinen sich darin einig zu sein, „wie ein Review-Tool aussehen sollte“. Jetzt ist der Punkt erreicht, an dem die Organisationen und Akteure wichtig werden, die das tatsächlich realistisch einführen und nachhaltig machen können. Die jüngste Einführung von git change-id ist ein guter Fortschritt (Danke an jj, git butler, gerrit usw.). Graphite oder GitHub konzentrieren sich auf Lösungen nur für ihre eigenen Nutzer und sind kein offener Ansatz für alle. Auch die vielen clientbasierten Kommandozeilen-Tools haben nicht besonders viel Einfluss. Was wirklich nötig ist:
Mein größter Kritikpunkt an GitHub ist, dass die App viel zu langsam ist. Wirklich so langsam, dass Browser-Tabs einfrieren. Azure DevOps war mit Abstand das beste Code-Review-Tool, das ich bisher genutzt habe.
Ich habe Azure DevOps beim Entwickeln von .NET in einem Microsoft-Umfeld genutzt, und es ist wirklich ein Tool, das sehr gut zum .NET-Ökosystem passt.
Ich frage mich, ob du GitLab intensiv genutzt hast. Von den Big Four gefällt mir GitLab am besten.
Mich würde interessieren, was dir an DevOps so gut gefallen hat. Ich nutze es täglich, aber für mich wirkt es GitHub ziemlich ähnlich. Eher vermisse ich die GitHub-Funktion, mit der vorgeschlagene Änderungen automatisch übernommen werden können.
Der massive Einsatz von JavaScript plus Druck auf schnelle Releases führt zu diesen langsamen Umgebungen. Trotzdem immer noch besser als Atlassian.
Die Idee, Git direkt für Code-Reviews zu nutzen, finde ich attraktiv. Es ist praktisch, Änderungen lokal direkt anfassen zu können. Ich verstehe nicht, warum Reviews unbedingt auf einen einzelnen Commit zugeschnitten sein müssen — ein Ansatz, bei dem Reviewer ihre Kommentare oder Korrekturen direkt in den PR-Branch committen, finde ich ebenfalls interessant. Das ist eine Hybridform aus traditionellem GitHub-Flow und dem Linux-Workflow mit Mailinglisten und Patches.
Ist GitHub-PRs nicht ohnehin read-only? Ich habe erlebt, dass Teammitglieder direkt mit einem „suggestion“-Kommentar Änderungen vorgeschlagen haben, die dann mit einem Klick in den Commit übernommen wurden.
Dass ein Review-Commit nur ein einzelner Commit sein soll, ist merkwürdig. Wenn mehrere Leute reviewen, entstehen gleichzeitig Bearbeitungskonflikte, und es passt auch nicht gut zum Code. Es wirkt natürlicher, wenn alle in eigenen Branches an ihren Reviews arbeiten und am Ende squash & rebase machen. Wenn die Diskussion länger wird, könnten sogar comment commits als Kette erhalten bleiben. Wichtig ist, dass diese Daten irgendwo außerhalb des main branch erhalten bleiben.
Beim Code-Review ziehe ich Änderungen gern in einen lokalen Branch, mache dann ein soft-reset und schaue sie mir an, als hätte ich sie selbst geschrieben. Wenn Commits nicht gut aufgeteilt sind, müssen Mitarbeitende sie erst selbst zerlegen, um sie reviewen zu können, und das ist ineffizient. Wenn der Review-Gegenstand zu groß ist, kann man kaum behaupten, dass ihn irgendjemand vollständig verstanden hat. Das Ganze ist nicht einfach nur die Summe seiner Teile.
Ich teile ein Beispiel für eine Shell-Funktion für einfaches Code-Review. Sie checkt bei einem sauberen Tree automatisch einen voreingestellten review-Branch aus, zeigt den Diff in nvim und räumt den Branch nach Abschluss wieder auf.
Gute Commits und die daraus entstehenden PRs gut zu bauen, ist fast genauso wichtig wie gutes Coding. In der Praxis gibt es aber überraschend viele Entwickler:innen, die schwach darin sind, PRs sinnvoll aufzuteilen oder gute Commit-Messages zu schreiben.
In Teams mit paralleler Arbeit im Data-Science-Bereich checken wir für Code-Reviews 2 bis 3 Branches gleichzeitig aus.
Für PR-Reviews nutze ich https://github.com/sindrets/diffview.nvim in Neovim. Die UI ähnelt dem Diff in vscode, nutzt aber den Diff-Modus von vim. Für leichte Reviews ist auch
git log -p --function-contextnützlich.Mich interessiert ein Vorgehen, bei dem eine Person den ersten Draft schreibt und eine andere ihn ausarbeitet und merged. Hat das jemand schon praktisch gemacht?
Ich stimme einem Stil zu, bei dem über 90 % des eigentlichen Codings von einer Person gemacht werden, die Reviewer aber selbst die Verantwortung für die finale Übernahme und den Merge tragen. In meinem früheren Job war immer der Reviewer für den Merge zuständig; bei großen Änderungen wurden nur Kommentare weitergegeben. Das ist viel effizienter, als jemanden ständig darum zu bitten, irgendwo auf „Approve“ zu klicken.
Wenn man am Review-Code selbst mitarbeitet, kann man deutlich tiefere Rückmeldungen geben. So entsteht eher das Gefühl von „unser Code“ statt „mein Code/dein Code“. Diese Struktur passt gut zu einer iterativen, kollaborativen Kultur wie bei TDD.
Das ähnelt asynchronem Pair Programming.
Ich kenne Leute, die bei trunk-based development Pair Programming einsetzen. Zwei Personen schreiben gemeinsam Code, und wenn alles OK ist, wird direkt in main gemerged; wenn die Tests durchlaufen, wird sofort deployt. Das funktioniert in der Praxis gut.
Ich nutze das GitHub-Pull-Request-Erweiterungs-Plugin in VSCode für lokale Reviews, und das ist ziemlich bequem. Man kann direkt im Editor kommentieren und Reviews durchführen.
Im GitHub-Web werden bei vielen geänderten Dateien einige Dateien ausgeblendet, aber in VSCode kann man frei navigieren, was deutlich angenehmer ist. Da diese Funktion in der Kombination aus VSCode und GitHub umgesetzt wurde, vermute ich, dass sie sich auch auf andere Editoren ausweiten ließe.
Es wird zwar besser, aber die gegenseitige Vendor-Lock-in-Abhängigkeit von GitHub und VSCode ist immer noch stark.
Es ist auch deutlich besser, im GitHub-Web eine PR zu öffnen und dann „.“ zu drücken, um die Review direkt in VSCode Web zu machen.
Die Nachricht, dass Git first-class change IDs bekommt, freut mich riesig! Das erinnert an das Revision-Tracking von diffs in Facebooks Phabricator. Ich hätte gern einen Link, um mehr Details dazu zu lesen.
Ich möchte auch Sourcehut erwähnen. Es führt den klassischen Workflow mit Patches, Issues, Bugs und Diskussionen per E-Mail fort und ist mit Mailinglisten und CI integriert. Auf git-send-email.io und git-am.io stellt Drew Devault außerdem Ressourcen dazu bereit, wie man Patches sendet und empfängt.