- Bei GitHub-artigen PR-Reviews lassen sich leicht immer weitere Korrektur-Commits auf die ursprüngliche Änderung stapeln, wodurch der Review-Prozess und die endgültige Code-Historie vermischt werden
- Eine gute Patch-Serie sollte logisch getrennte Commits wie Refactorings, das Hinzufügen einer neuen API und die Migration bestehender Nutzer in der richtigen Reihenfolge lesbar machen
- Wenn sich Commits wie „fix review“ oder
"minor" ansammeln, wird die Änderungsabsicht unklarer, und der Aufwand steigt, mit git blame und git bisect die Ursache von Problemen zu finden
- Beim interdiff-Review wird eine neue Version der Patch-Serie veröffentlicht und mit
git range-diff main..v1 main..v2 der Unterschied pro Commit verglichen
- Reviewer müssen nicht erneut alle 500 Zeilen lesen, sondern können nur die 50 Zeilen Inkrement prüfen; Autorinnen und Autoren können die endgültige Commit-Struktur ohne Rausch-Commits wie „address review“ beibehalten
Code-Review-Tools und Ausgangspunkt
- Gerrit Code Review ist ein Open-Source-Code-Review-Tool, das zusammen mit Git-Repositories funktioniert und einen Ablauf unterstützt, bei dem Patches eingereicht, Code von anderen geprüft und Kommentare hinterlassen werden
- Code-Review-Methoden sind vielfältig: Gerrit, GitHub und Phabricator, aber auch das Hochladen von
.patch-Dateien in Bugtracker, git send-email oder ein Ansatz, bei dem man Kolleginnen und Kollegen nach langer Diskussion zur Implementierung führt
- Die Programmiersprache Go nutzt Gerrit, KDE und LLVM nutzten Phabricator, und Jujutsu hat die Nutzung von Gerrit einmal geprüft
- Der Linux-Kernel verwendet einen E-Mail-basierten Workflow auf besondere Weise, während viele Projekte GitHub wählen, weil es einfach zu bedienen ist und geringe Einstiegskosten hat
- Das Kernproblem ist, dass die Tools nicht dasselbe Review-Modell anbieten und die Standarderfahrung von GitHub nicht immer ausreicht
Bedingungen für eine gute Patch-Serie
- Eine ideale Patch-Serie reicht eine große Änderung in logischen Schritten ein
- Zunächst wird nebenbei entdeckter Code aufgeräumt
- Danach wird eine neue API hinzugefügt
- Zum Schluss werden bestehende Nutzer der alten API auf die neue API migriert
- Jeder Patch sollte einen eigenständigen Grund haben, und wenn sie der Reihe nach angewendet werden, sollte sichtbar werden, wie sich der Code schrittweise weiterentwickelt
- Diese Struktur ist nicht nur für Autorinnen, Autoren und Reviewer vorteilhaft, sondern auch für Maintainer, die später mit
blame oder bisect den Grund einer Änderung finden müssen
- Die tatsächliche Patch-Größe hängt von der Situation ab
- Es kann auch kein Refactoring geben
- Es können 100 Zeilen sein
- Wenn wie bei einer zentralen API-Änderung alle Aufrufstellen angepasst werden müssen, können es auch 500 Zeilen werden
Die gemischte Historie durch GitHubs „diff soup“
- GitHub fördert explizit und implizit den Ablauf, beim Einarbeiten von Review-Kommentaren neue Commits auf die ursprünglichen Commits zu legen
- Im Beispiel werden nach den ursprünglichen drei Commits nacheinander Änderungen aus den Reviews von Alice und Bob, zusätzliche Tests und ein
"minor"-Commit mit Implementierungsänderungen angehängt
- Im Commit-Graphen bleiben nur Eltern-Kind-Beziehungen übrig; dadurch wird nicht sichtbar, welches Problem in welchem ursprünglichen Commit ein Review-Fix-Commit behoben hat
- Wenn ein einzelner „fix review“-Commit mehrere ursprüngliche Commits gleichzeitig berührt, wird das konzeptuelle Modell noch komplexer
- Man kann zwar dafür sorgen, dass ein Korrektur-Commit genau einen angesprochenen Punkt umsetzt, aber dadurch steigt die Zahl der Fixup-Commits entsprechend
- Diese Form, bei der ein großes Änderungspaket und Review-Korrekturen in einer Schüssel vermischt werden, ist „diff soup“
Die Signale von git blame und git bisect werden unschärfer
git blame arbeitet zeilenweise; wenn ein Review-Fix-Commit nur einen kleinen Teil einer bestehenden Zeile ändert, kann die Herkunft dieser Zeile als ein Commit wie "fix alice review" erscheinen- Um den tatsächlichen Grund der Änderung zu finden, wird Commit-Archäologie nötig: Man muss den ursprünglichen Commit und den Review-Fix-Commit erneut verfolgen
- Auch bei
git bisect kann unklar werden, ob der "minor"-Commit die tatsächliche Ursache ist oder ob ein Bug, der seit dem ersten Hinzufügen der neuen API vorhanden war, erst später sichtbar wurde
- Wenn die neue API eine große Performance-Verbesserung bietet, kann ein Zurückrollen unerwünscht oder unmöglich sein
- Tritt die Regression erst Wochen später auf, entsteht in einer dringenden Situation wie einem P1-Meeting der Aufwand, allein anhand einer Commit-Nachricht wie
"minor" weiter nachforschen zu müssen
- Schon im Beispiel mit nur zwei Review-Runden wird die Historie komplex; in Open-Source-Projekten, in denen Reviews mehrfach wiederholt werden, wird das Problem noch größer
Wie die GitHub-UX diesen Ablauf verstärkt
- Auf GitHub ist das Hinzufügen neuer Commits obenauf praktisch der einfachste Weg für inkrementelle Reviews
- Für Reviewer ist es weniger belastend, einen
"fix alice review"-Patch mit +/- 10 Zeilen anzusehen, als 500 Zeilen einer neuen API erneut zu lesen
- Der Diff-Tab eines PRs zeigt standardmäßig alle Commits zu einem einzigen Diff zusammengeführt an
- Das ähnelt dem Blick auf
git diff master..foo-branch
- Es passt nicht gut dazu, wie Menschen Änderungen schreiben und lesen
- Die GitHub-UX ist stark auf das Modell benannter Branches zugeschnitten und macht es schwierig, Unterschiede zwischen Einheiten außerhalb von Branches zu betrachten
- Nach einem Force-Push kann man über den „Compare“-Button in der „Force Push“-Zeile des PRs einen Diff ansehen
- Dieser zeigt jedoch nur den gesamten Diff zwischen dem vorherigen Branch-Head und dem aktuellen Branch-Head
- Für feinere Vergleiche muss man Commit-IDs selbst finden und in die URL einfügen
interdiff-Review und git range-diff
- Beim interdiff-Review werden keine Korrektur-Commits auf die ursprünglichen drei Commits gelegt; stattdessen wird eine neue Version veröffentlicht, die wieder aus denselben drei Commits besteht
- Wenn etwa in einer
v1-Patch-Serie Bobs Kommentare eingearbeitet werden müssen, werden der erste und der dritte Commit angepasst und daraus eine v2-Serie erstellt
- Selbst wenn danach Alices Kommentare, zusätzliche Tests und eine
"minor"-Änderung auf dieselbe Weise eingearbeitet werden, besteht das Endergebnis weiterhin aus drei Commits
- Wenn sich die Commits A, B und C aus
v1 zu X, Y und Z in v2 weiterentwickelt haben, zeigt git range-diff die Unterschiede zwischen den Commits an den jeweiligen Positionen
git range-diff \
main..v1 \
main..v2
- Dieser Befehl zeigt die paarweisen Diffs zwischen den drei Commits des
v1-Branches und den drei Commits des v2-Branches
- Der erste Commit ist
diff(A, X)
- Der zweite Commit ist
diff(B, Y)
- Der dritte Commit ist
diff(C, Z)
Praktische Auswirkungen des interdiff-Ansatzes
- Anders als GitHub, das normalerweise den gesamten Branch-Diff wie
diff(main, C) oder diff(main, Z) zeigt, zeigt interdiff die Unterschiede zwischen Commit-Versionen
- Reviewer können nur die geänderten 50 Zeilen prüfen, statt die bereits gelesene API-Änderung mit 500 Zeilen erneut zu lesen
- Autorinnen und Autoren müssen keine 30 Rausch-Commits wie „address review“ in der Historie anhäufen
git blame verknüpft Zeilen mit höherer Wahrscheinlichkeit und weniger Rauschen mit dem ursprünglichen Änderungs-Commitgit bisect verweist bei Regressionen mit höherer Wahrscheinlichkeit auf eine sinnvollere Einheit wie den neuen API-Commit- Dadurch verbessert sich insgesamt das Signal-Rausch-Verhältnis der grundlegenden Diagnosewerkzeuge
Hinweise zu Merge-Strategien und rebase
- Die Erklärung zu Patch-Merge-Strategien bleibt als TODO offen
- Die Position ist, dass
git rebase in Ordnung ist, solange es sich nicht um einen öffentlichen Branch handelt, auf den andere voraussichtlich ihre Commits aufbauen werden
- interdiff-Review-Systeme fördern kleinere Patches, die sich schneller mergen lassen
- Man muss nicht warten, bis alle 5 Commits fertig sind
- Wenn die ersten 3 in Ordnung sind und die letzten 2 noch mehr Arbeit brauchen, kann man nur die ersten 3 mergen
- Ziel ist es, Menschen dazu zu bringen, auf Basis des main-Branches statt auf langlebigen Branches zu arbeiten, damit weniger Situationen entstehen, in denen mehrere Remote-Branches ineinander verschränkt gemergt werden
- Es gibt auch Projekte wie den Linux-Kernel, die öffentliche Branches mit anderen Repositories mergen oder sie explizit auf anderen Branches veröffentlichen
- Linux-Entwicklerinnen und -Entwickler können
git rebase verwenden, wenn sie Patch-Serien erstellen und verfeinern
- Nachdem sie jedoch einen öffentlichen Branch erstellt haben, kann dieser in den Tree einer anderen Person gemergt werden; deshalb machen sie keinen Force-Push, sondern erstellen einen neuen Branch mit neuen Commits
1 Kommentare
Meinungen auf Hacker News
Auf GitHub nutze ich im Großen und Ganzen diesen Ablauf; der Nachteil ist, dass ich mehr zu tun habe und er für Mitwirkende nicht intuitiv ist.
Trotzdem bleiben die Vorteile erhalten: Reviewer können genau die Änderungen sehen, die nur ihr Feedback betreffen, und
git blamesowiegit bisectwerden nicht kaputtgemacht.Wenn ich Review-Feedback einarbeite, committe ich mit
git commit --fixup, pushe danach und hinterlasse in der Antwort auf den Review-Kommentar den Hash des Fixup-Commits.Wenn der PR genehmigt ist und kurz vor dem Merge steht, führe ich
git rebase --interactive origin/main --autosquashaus, squashe die Fixup-Commits in die ursprünglichen Commits, mache zuletztgit push --force-with-leaseund merge dann.Vor Ende des Reviews sollte man mit Force-Pushes vorsichtig sein. Sonst können Reviewer nicht sehen, was seit ihrem letzten Review hinzugekommen ist.
Ich verlasse mich stark auf Autovervollständigung im Terminal, sodass schon
git rereicht, um zu langen Befehlen zu kommen. Aber es ist etwas holprig; ein Tool, das diesen Ablauf unterstützt und fördert, wäre schön. Wenn man aber an GitHub gebunden ist, ist das so schon okay.Allerdings wird es in der Praxis eher nur von disziplinierten Engineering-Teams genutzt, die bewusst fortgeschrittene Git-Funktionen lernen.
~/.gitconfigsetzen:[rebase] autosquash = trueDann werden Fixup-/Squash-Commits künftig schon mit
git rebase -i origin/mainautomatisch neu angeordnet. Eine kleine Einstellung, aber sie hat meinen Workflow deutlich verbessert.Das ist ein ähnlicher Bereich wie die Notwendigkeit von
--update-refs, wenn man eine Commit-Serie rebased, auf der mehrere Branches liegen. Warum sollten Menschen Graphbeziehungen nachverfolgen, Commits manuell einfügen und Branches verschieben müssen? Computer können gut mit Graphen umgehen, also sollte man sie das machen lassen.Es gibt auch
git absorb, aber es ist nicht so robust wie die Sapling-Implementierung[1].Das eigentliche Problem ist nicht, ob man interaktiv rebased, sondern meist die User Experience des Review-Tools selbst und die Schleifenstruktur, die es erzwingt. Fixup-Commits lösen zum Beispiel nicht GitHubs Problem, Diffs zwischen Bezugspunkten anzuzeigen; wenn der Bezugspunkt groß ist, kann das ein Review komplett ruinieren. Etwa wenn man auf zehn neue Commits rebased hat.
Ich habe auch Beschwerden über Git selbst, aber der Originaltext ist hauptsächlich eine Beschwerde über GitHub.
[1] Ein Beispiel, das den Unterschied zwischen den beiden zugrunde liegenden Algorithmen zeigt, gibt es in diesem GitHub-Issue: https://github.com/martinvonz/jj/issues/170
Ich stimme zu 100 % zu, dass dieser Ansatz ideal ist. Der GitHub-Ansatz ist wirklich furchtbar, und es ist tragisch, dass so viele Menschen ihn als selbstverständlich hinnehmen.
Früher haben wir das mit Phabricator so gemacht, und mit einem Kommandozeilen-Makro, das alle Reviews auf einmal aktualisierte, war es einigermaßen manuell machbar. Eine explizite UI dafür wäre trotzdem besser.
Genau. Das ist das, was ich mir im Kopf als echten Code-Review-Stil vorgestellt habe, nicht das, was GitHub macht. Schön zu erfahren, dass es dafür einen Namen gibt.
Zusätzlich wäre es gut, wenn das Review-System fertige Patches aus dem Review „hinausschieben“ könnte. Kleine Bugfixes, die man während der Arbeit an einem großen Feature erstellt, sollten kleine, unabhängige Patches sein und lassen sich wahrscheinlich schnell mit dem Reviewer abstimmen.
Dann möchte ich diesen Patch aus der ganzen Serie herausnehmen, auf
maincherry-picken und das Review auf den neuenHEADrebasen. Oder ich möchte die aktuelle Patch-Serie aufmainrebasen, dabei aber die abgestimmten Patches nach vorne sortieren und anschließendmainper Fast-Forward auf diesen Patch bringen können.Im Kern geht es darum, den Review-Umfang auf die „noch diskutierten Teile“ einzugrenzen, während Bugfixes zusammengeführt werden, sobald sie bereit sind.
Der Einwand wäre: „Mach einfach ein separates Review/einen separaten PR daraus.“ Aber dann entsteht Komplexität, bei der Patchset A von Patchset B abhängt und nach dem Merge von B dann von
mainabhängt.Dazu tragen auch kluge UX-Ideen wie das Attention Set bei. Im Grunde zeigt es an: „Wer muss als Nächstes handeln?“ Es funktioniert wie ein rundenbasiertes Spiel. Wenn ich gerade ein Review gemacht habe, bin ich nicht mehr im Attention Set dieses Patches, sondern der Autor ist es.
Deshalb wandert dieser Eintrag in der Gerrit-UI in meiner Queue nach unten. Ganz oben in der Queue stehen die Einträge, bei denen ich im Attention Set bin. So bündelt sich die Arbeit ganz natürlich.
Damit sind noch nicht einmal all die anderen nervigen Kleinigkeiten der GitHub-UX abgedeckt, aber selbst die Pull-Request-Liste ist schlechter als Alternativen. Man kann nicht erkennen, in welchem Zustand ein Eintrag ist, und muss am Ende alles lesen.
maincherry-picken, CI-Erfolg und normales Review prüfen und den Rest danach darauf rebasen.Wenn es der unterste Patch x ist, kann man ihn direkt einreichen und mit dem Rest weitermachen.
Es ist immer interessant, neue Ansätze für Code-Reviews zu sehen. GitHub hat zwar seine Vorteile, ist aber weit von perfekt entfernt.
Für das beschriebene Szenario könnte man überlegen, die drei Patches in separate Pull Requests aufzuteilen, die voneinander abhängen. GitHub unterstützt das nicht nativ, aber ein geeignetes Code-Review-Tool sollte Abhängigkeiten verwalten können, während Pull Requests klein bleiben. Zum Beispiel könnte Patch 3 von Patch 2 abhängen und Patch 2 von Patch 1.
Dank des vom Tool bereitgestellten Dependency-Trackings lässt sich bei Bedarf sicherstellen, dass alles gemeinsam ausgerollt wird.
Wenn jeder Patch separat reviewed wird, wird Feedback klarer und leichter umzusetzen. Innerhalb des Pull Requests kann man Commits squashen und so eine saubere Commit-Historie erstellen, die die einzelnen Änderungen genau widerspiegelt. Darüber hinaus könnte ein passendes Tool Pull Requests und Reviews per AI zusammenfassen und so das Schreiben präziser Commit-Messages ohne manuelle Arbeit vereinfachen.
Ein gutes Code-Review-Tool sollte sich nicht von Git-Operationen wie Rebase, Merge oder Force Push aus dem Takt bringen lassen. Egal, welche komplexen Git-Operationen im Hintergrund passieren: Reviewer sollten nur die Änderungen seit dem letzten Review sehen. So können sie sich auf neue Inhalte konzentrieren, ohne einen großen Diff erneut zu reviewen, und die Review-Historie bleibt sauber von der Commit-Historie getrennt.
Ich frage mich, ob diese Art, Pull Requests aufzuteilen und ihre gegenseitigen Abhängigkeiten zu verfolgen, die Anforderungen erfüllen würde.
Es ist buchstäblich der Workflow, für den Git entwickelt wurde, und seit GitHub Pull Requests hat, steht genau das im Kern einer alten Kritik daran.
Ich frage mich, warum diese Idee – kurzer Blick in den Kalender – über 15 Jahre lang als Methode von „Mailinglisten-Dinosauriern“ abgetan wurde und erst jetzt in angesagteren Entwicklerkulturen ankommt.
Ich dachte lange, viele Softwareentwickler würden sich bewusst für schlechte Workflows entscheiden, weil sie sich beharrlich weigern, eines ihrer meistgenutzten Werkzeuge richtig zu lernen. Deshalb habe ich beim Umzug der Firma auf Git genau solche Tools und Abläufe gewählt und gebaut. Schließlich wird man nicht gefeuert, weil man IBM gekauft hat.
main, sondern auf einem anderen Branch im Haupt-Repository aufsetzen.Wenn externe Beitragende einen Fork des Repositorys verwenden, enthalten die drei PRs jeweils schrittweise A, A+B und A+B+C. Dann muss man in den letzten beiden PRs im Diff auch Code erneut ansehen, der bereits in einem anderen PR reviewed wurde, was das Review erschwert.
Jeder Commit ist unabhängig reviewbar. Abhängigkeiten zwischen Commits lassen sich ebenfalls verfolgen. Dass keine AI eingebaut ist, ist auch ein Vorteil. Anders als bei PRs kann man zwischen Commits auch Interdiffs ansehen.
Commits sind die Grundbausteine von Git. Man sollte einfach Commits verwenden.
Ich frage mich, wie GitContext Commits nachverfolgt, die Operationen wie Fixup, Rebase oder Neuordnung durchlaufen haben.
master, dann mergt man B inmasterund setzt A danach wieder aufmasterzurück – oder übersehe ich etwas?Ich bin die Person, die diesen Artikel geschrieben hat. Fragt mich alles.
Außerdem: Ich weiß nicht, ob das gegen die Regeln verstößt, aber ich brauche seit Kurzem einen neuen Job. Ich arbeite gern an Entwicklerwerkzeugen und schwierigen Problemen. Wenn euch dieser Beitrag gefallen hat, ihr die Produktivität eures Entwicklungsteams steigern wollt oder ihr meinen ausgezeichneten und gelegentlich eklektischen Geschmack erleben und genießen möchtet: Meine E-Mail steht im Profil.
Ernsthaft: Dein Lebenslauf ist beeindruckend. Ich hoffe, du findest gut und schnell etwas. Nach meiner sehr aktuellen Erfahrung ist der derzeitige Arbeitsmarkt tödlich krank. Je senioriger und erfahrener man ist, desto mehr verletzen einen die absurden Interview- und HR-Praktiken und die unerklärlichen Absagen.
Ein Freund hat mir die folgenden Links geschickt:
https://danluu.com/hiring-lemons/
https://danluu.com/programmer-moneyball/
https://danluu.com/algorithms-interviews/
Viel Glück.
Ein Punkt, der noch nicht behandelt wurde: Wegen der Diff Soup könnten Leute dazu neigen, eine Squash-Merge-Strategie zu bevorzugen, um das „Rauschen“ von Fixup-Commits loszuwerden. Dabei wirft man dann auch die ursprünglich guten drei atomaren Commits mit weg.
Beim Interdiff-Review-Stil bleiben die ursprünglichen drei Commits erhalten, und ob man sie einzeln übernimmt oder squasht, kann vollständig anhand dieser Commits selbst und ihrer tatsächlichen Atomarität entschieden werden.
Es gibt auch externe Dienste, die den Schmerz mit GitHub verringern wollen und Support für Stacked Diffs bieten, aber sie wirken ziemlich teuer – besonders im Vergleich zu einem selbst gehosteten Gerrit. Ich frage mich, ob du dir solche Tools ebenfalls angesehen hast.
Ich erinnere mich, dass sich früher ein MR-Serienmuster ziemlich einfach einrichten ließ. Wenn man drei MRs gemergt hat, entstanden drei Merge-Commits, jeweils mit einem einzelnen Squash-Commit pro MR. Das war unabhängig von der Commit-Historie des MRs.
Der Trade-off ist, dass man, wenn sich während des Reviews ein früherer Serien-Branch ändert, wieder in die späteren Serien-Branches mergen muss.
Trotzdem können die Leute während des Reviews mit der Commit-Historie machen, was sie wollen. Da am Ende ohnehin gesquasht wird, spielt es keine Rolle.
Allerdings ist das schon lange her, und da ich heutzutage an GitHub gebunden bin, verkläre ich es vielleicht in meiner Erinnerung.
Review Board [https://www.reviewboard.org] hatte bereits 2006 interdiff entwickelt. Tatsächlich könnte es sein, dass ich den Begriff geprägt habe oder unabhängig darauf gekommen bin.
Bis heute ist das einer meiner liebsten Teile des Produkts und des Code-Review-Prozesses, und es ist eine der Funktionen, von denen ich oft höre, dass die Leute sie am meisten vermissen, wenn sie zu etwas wie GitHub wechseln.
Fix-it-Commits haben sich für mich nie wie eine echte Alternative angefühlt.
Erstens zeigen sie nicht, wie Änderungen auf höherer Ebene in die Commit-Serie eingeflossen sind.
Zweitens bringen sie, selbst wenn nur vorübergehend, den Commit-Graph durcheinander und erschweren Reviews. Wer die Review schon die ganze Zeit verfolgt hat, hat den Code, den der Fix-it-Commit korrigiert, vielleicht bereits gelesen; wer neu dazukommt, kann aber mit einem schlechten ersten Eindruck davon starten, was dieser Code eigentlich tun soll oder wie er strukturiert ist.
Drittens nutzt nicht jeder Git oder ein Versionsverwaltungssystem, das mit mehreren Commits umgehen kann. In der Spieleentwicklung wird oft Perforce verwendet, bei Chipherstellern auch spezielle Versionsverwaltungssysteme wie Keysight SOS. Dort sind Fix-it-Commits gar keine Option.
In einem Code-Review-System mit ordentlicher interdiff-Unterstützung kann ein Reviewer vom zuerst veröffentlichten Review-Request an alle Updates mitverfolgen und nur sehen, was sich geändert hat, während ein anderer Reviewer direkt in die aktuellste Gesamtänderung einsteigen kann, ohne sich um eine Fix-it-Serie kümmern zu müssen. Und das funktioniert unabhängig vom Versionsverwaltungssystem.
Wenn es richtig umgesetzt ist, kann es auch mit Änderungen aus mehreren Commits koexistieren.
Man kann zum Beispiel ein kleines Projekt zur Erleichterung der Review in mehrere Commits für API-Handler, Frontend-UI und Dokumentation aufteilen und entscheiden, dass die Commits eng zusammenhängen und daher sinnvollerweise als ein einzelner Review-Request hochgeladen werden. Wenn sie weniger stark zusammenhängen, wäre es idealer, mehrere Review-Requests als Abhängigkeitskette zu erstellen.
Auf Basis des Review-Feedbacks kann man dann eine Reihe von Änderungen an einem oder allen dieser Commits vornehmen. Wenn Leute das Update reviewen, wäre es gut, wenn sie sehen könnten, wie sich die einzelnen Teile entwickelt haben, statt Fix-it-Commits und deren Änderungen im Kopf den jeweiligen ursprünglichen Änderungen zuzuordnen.
Deshalb ist interdiff wirklich großartig. Ob viele kleine Commits oder ein großer Commit, ob ein Review-Request mit einem einzelnen Commit oder mit mehreren Commits: Mehr Leute sollten es nutzen.
Um 2013 herum wechselte der Glasgow Haskell Compiler von der scherzhaft so bezeichneten Methode „.patch-Dateien aus Bug-Reports lesen“ zur Nutzung von Phabricator. Dafür gab es mehrere Gründe, aber damals lag es nicht an stacked diffs. Der Grund war, dass GitHub für Reviews so schlecht war, dass es nicht einmal side-by-side diffs gab. Allein deshalb war es völlig außer Frage, und es gab noch weitere Gründe.
Aber das war nicht das erste Mal, dass ich ein Team auf ein Code-Review-Tool umgestellt habe. In meinem ersten Job 2009 war ich in einem sehr kleinen, eng verbundenen Engineering-Team in einem Raum in Houston, und ich dachte, ich könnte die Codebase besser verstehen, wenn andere meinen Code reviewen und ich lese, was sie geschrieben haben.
Also habe ich in den ersten Monaten nach meinem Einstieg meinen Manager so lange gedrängt, bis er ReviewBoard installiert hat, und alle fanden es wirklich gut.
Das heißt: Danke an RB. Ich denke immer noch gelegentlich mit guten Erinnerungen daran zurück. Dadurch war Code Review praktisch vom ersten Tag an ein sehr großer Teil meiner Karriere, und ich denke immer noch, dass ich mehr davon machen könnte.
Insgesamt fand ich Systeme wie Gerrit, die Code Review in den Vordergrund stellen und stark auf Rebase ausgerichtet sind, deutlich einfacher für Code-Reviews.
Einer der besten Aspekte ist die eingebaute Unterstützung dafür, mehrere Patches zu stapeln, was Leute dazu bringt, kleinere und leichter zu reviewende Patches zu erstellen.
GitHubs Code Review wirkt wie ein schlechtes nachträgliches Add-on: eine nach Forum-Thread aussehende UI, die Platz verschwendet, und die Unfähigkeit, über Rebases hinweg nachzuverfolgen, unter anderem.
Solche „Big Picture“-Diskussionen, die zugleich technisch sind, finden viel zu oft im Issue-Tracker statt. Aber ich weiß auch nicht, wo man sie sonst unterbringen sollte. Dieser Stack ist zu temporär und kann sich beim nächsten Push komplett ändern.
Ich habe diese Strategie schon ein paarmal vertreten gehört, bin aber nicht überzeugt. Bei den meisten Projekten, an denen ich arbeite, wird ein Feature Branch zu einem einzelnen Commit gesquasht und gemergt, und die Branch-Historie wird gelöscht
Wenn es den vom Autor beschriebenen Fall gäbe, würde ich Refactoring, neue API und Update einfach zu drei Commits machen
Was sich für mich eindeutig als gute Praxis erwiesen hat, ist, langlebige Branches zu vermeiden. Solche mehrstufigen Commit-Szenarien entstehen meist genau dort. Also wenn jemand mehrere Tage, im schlimmsten Fall Wochen, an einem Feature arbeitet und es dann auf einmal abwerfen will
Ich bevorzuge es deutlich, täglich oder sogar mehrmals am Tag nach main zu integrieren. Eine Möglichkeit dafür sind Feature Flags, damit auch Commits im laufenden Zustand problemlos übernommen werden können. Wenn das System richtig aufgesetzt ist, hat das auch den Vorteil, dass man sie in Entwicklungs- und Staging-Umgebungen einschalten und testen kann. Bis zu Blue/Green-Deployments ist es dann nur noch ein kleiner Schritt
Ich will keine Methode, um große Commits leichter reviewen zu können. Ich will das Team dazu zwingen, kleine Änderungen früh und häufig zu committen. Mir ist klar, dass nicht alle dem zustimmen
Einer der Gründe ist, dass man anders als bei GitHub-PRs nur einen Teil des Stacks übernehmen kann. Im Beispiel des Artikels könnte man, wenn der Diff „kleines Refactoring“ bereit ist, diesen zuerst einbringen, ohne die Diffs „neue API“ und „Migration der API-Nutzer“ mit zu übernehmen
Wenn man nicht Branches, sondern Commits in den Mittelpunkt stellt, hat das den Effekt, dass die einzelnen Commits insgesamt kleiner werden als bei langlebigen Branches
Damit werden
git blameundgit bisectpraktisch nutzlosBei einer Regression kann
git bisectsie bis auf einen einzelnen Patch eingrenzen. Deshalb ist es besser, für ein bestimmtes Feature 50 Patches à 160 Zeilen in der Git-Historie zu haben als einen einzigen Patch mit 8000 ZeilenWenn eine bestimmte Codezeile verdächtig aussieht, möchte ich, dass
git blameoder aufeinanderfolgendegit blame-Befehle mich zu einem 160-Zeilen-Commit mit ausführlicher Commit-Message führen, nicht zu einem 8000-Zeilen-CommitAuch die Reihenfolge der ursprünglichen Commits sollte erhalten bleiben. Jahre später kann es enorm helfen, die ursprüngliche Gestaltung zu verstehen, wenn man die einzelnen Commit-Messages der Reihe nach liest. Natürlich muss das ursprüngliche Patchset in Abhängigkeitsreihenfolge aufgebaut sein, in jedem Schritt kompilieren usw.; zusätzlich zur Feature-Implementierung braucht es also eine eigene Entwicklungsphase. Code sollte in logischen Schritten präsentiert werden
Interessant. Bei der Arbeit verwenden wir PRs so, wie der Autor Commits verwendet, und am Ende machen wir tatsächlich squash-and-merge
Diese Methode bedeutet aber, dass man jedes Mal, wenn man einen der vorderen PRs ändert, die dahinterliegenden PRs rebasen muss. Das ist ziemlich mühsam, kollidiert mit „kein Force-Push“-Regeln, braucht lange, bis Engineers es lernen, und hat die Tendenz, bestehende Code-Review-Kommentare in der GitHub-UI kaputtzumachen. Bei zwei oder drei PRs funktioniert es trotzdem irgendwie
In unserem Ablauf sind Commits eher Speicherpunkte als Arbeitseinheiten
Ich nutze auch viel stash und Emacs’ undo-tree-mode. Damit gibt es vier Methoden, um die Source-Code-Historie nachzuverfolgen; das klingt redundant, funktioniert in der Praxis aber ganz gut
Die Usability dafür in Git ist ziemlich schlecht. Phabricator war zwar besser, aber immer noch unnötig schwierig. Vielleicht könnte ein neues Versionsverwaltungstool Konzepte auf einer höheren Ebene als Commits und Branches als First-Class-Konzepte unterstützen; vielleicht würde es dadurch aber auch noch schlechter benutzbar
Wäre es schön, ein besseres Tool als Git zu haben? Klar, ich hätte auch gern ein Tool mit weniger Macken und besserer Benutzerfreundlichkeit. Aber wie wäre es als Zwischenlösung, Commits wie Commits zu behandeln? Macht es euch nicht unnötig selbst schwer
Ich frage mich, wie viel Verantwortung GitHub dafür trägt, dass es den Leuten eine PR-Brille aufgesetzt hat
jj,saplingundmercurialanzusehenDer Blogbeitrag war gut, und es freut mich zu sehen, dass es Leute gibt, die sich wirklich Gedanken darüber machen, wie Code Reviews funktionieren sollten
Ich habe vier Code-Review-Systeme mit unterschiedlichen Vor- und Nachteilen intensiv genutzt: Critique (Google-intern), Gerrit (bei Google genutzt, aber identisch mit extern), GitHub und das von mir gebaute CodeApprove
Critique war mit großem Abstand das beste, aber das war möglich, weil es perfekt auf Googles Monorepo, das eigene Versionsverwaltungssystem und die Custom-Lint-/Test-Tools abgestimmt ist. CodeApprove wurde entworfen, um möglichst viele dieser Vorteile nach GitHub zu bringen, aber in der Praxis ist es schwer, dem nahezukommen
Gerrit war in Bezug auf die Reviewer Experience das zweitbeste, aber als Autor habe ich es immer gehasst. Es schien mehr falsche als richtige Wege zu geben, und die UI ist auch nicht gerade schön
GitHub ist für Autoren sehr freundlich und funktioniert so, wie wir denken: Code schreiben, Feedback bekommen, mehr Code schreiben. Wenn man am Ende des PRs squash and merge macht, gibt es das vom Autor angesprochene Historienproblem nicht
Für Reviewer oder Teams ist es aber nicht besonders freundlich. Inkrementelle Diffs werden nicht hervorgehoben, Diff und Diskussion befinden sich in unterschiedlichen Tabs, und Force Push sowie Rebase zerstören die Historie. Kommentare verschwinden als „outdated“, außerhalb des Diff-Fensters kann man keine Dateien kommentieren, große Dateien werden standardmäßig ausgeblendet und so weiter. Es ist ziemlich offensichtlich, dass GitHub sich nicht besonders dafür interessiert; vielleicht wissen sie aber auch etwas, das ich nicht weiß
Am frustrierendsten ist letztlich, dass viele Teams einfach das in ihre Versionsverwaltungsplattform eingebaute Code-Review-Tool akzeptieren. Das ist, als würde man einfach die auf dem Laptop vorinstallierte IDE verwenden. Heute gibt es sehr viel bessere Optionen. Neben CodeApprove mag ich GitContext, Reviewable und Graphite; außerdem könnte ich mindestens ein halbes Dutzend weiterer hervorragender Optionen nennen. Man sollte die Defaults nicht einfach akzeptieren
Reviewer und Maintainer sind deutlich knapper als Contributors. Meiner Ansicht nach sollten Workflow und UI auf Review-Durchsatz optimiert sein
Die drei anderen genannten Tools habe ich nicht ausprobiert, daher ist meine Aussage allgemein gemeint