Unified Diff und Split Diff im Vergleich

 (matklad.github.io)
3 Punkte von GN⁺ 2023-10-25 | 1 Kommentare | Auf WhatsApp teilen
  • Kleine Änderungen lassen sich sowohl mit Unified Diff als auch mit Split Diff ausreichend gut betrachten, doch bei großen und komplexen Änderungen stößt ein Review, der nur den Diff liest, an Grenzen
  • Große Änderungen sollten weniger anhand der Änderungshistorie geprüft werden, sondern anhand des Gesamtzustands der Codebase zu einem bestimmten Zeitpunkt; man sollte sie so erkunden und verifizieren, als würde man den echten Code bearbeiten
  • Die ideale Review-Ansicht würde den aktuellen Code auf der Festplatte und den Unified Diff des betreffenden Bereichs nebeneinander zeigen, sodass man Codekontext und Änderungen gleichzeitig verfolgen kann
  • Da bestehende Tools dafür zu wenig Unterstützung bieten, wird ein Low-Tech-Workflow genutzt: den PR lokal auschecken, dann die Commits entfernen und nur die Änderungen übrig lassen
  • Mit diesem Ansatz kann man geänderte Dateien durchsuchen und geprüfte Hunks markieren, doch die aktuelle Position im Editor und der Diff werden nicht automatisch synchronisiert, sodass man weiterhin manuell eingreifen muss

Den aktuellen Codezustand statt nur den Diff reviewen

  • Split Diff und Unified Diff funktionieren beide gut bei kleinen, einfachen Änderungen
  • Wenn Änderungen größer und komplexer werden, reicht es nicht mehr aus, einfach nur den Diff zu lesen
  • Bei großen Änderungen muss man die Codebase zu einem bestimmten Zeitpunkt gründlich betrachten
    • Man sollte besonders auf die zuletzt geänderten Bereiche achten, sie aber grundsätzlich wie bei einem normalen Code Review betrachten
    • Tests müssen ausgeführt werden
    • Editor-Navigationsfunktionen wie „goto definition“ sollten genutzt werden
    • Lokale Änderungen sollten angewendet werden, um zu prüfen, ob eine andere Umsetzung möglich gewesen wäre
    • Man sollte Stellen finden, die im größeren Kontext ebenfalls hätten geändert werden müssen
    • Man sollte unabhängig von der Änderungshistorie Auffälligkeiten in der aktuellen Codebase entdecken können

Gewünschte Review-Ansicht und lokaler Workaround

  • Die ideale Diff-Ansicht würde links den aktuellen Codezustand und rechts den Unified Diff des aktuell sichtbaren Codebereichs gemeinsam anzeigen
    • Der Code links sollte dem aktuellen Zustand auf der Festplatte entsprechen
    • Änderungen werden dezent am Rand markiert
    • Der Diff rechts sollte dem links aktuell sichtbaren Codebereich entsprechen
  • Für diese Review-Form fehlt derzeit die Tool-Unterstützung
  • Der tatsächliche Workflow besteht darin, den PR mit dem gpr script lokal auszuchecken
    • Im letzten Schritt werden die Commits des PR entfernt, sodass nur die Änderungen übrig bleiben
    • In diesem Zustand wird der Workflow für Staging und Commits für den Code Review zweckentfremdet
  • Mit edamagit kann man die Liste der geänderten Dateien ansehen und im Editor zur nächsten/vorherigen Änderung springen
    • Über die Staging Area lassen sich auch bereits reviewte Hunks markieren
  • Die verbleibende Unbequemlichkeit ist, dass es keine automatische Synchronisierung zwischen dem magit status buffer und der aktuell im Editor geöffneten Datei gibt
    • Um die aktuelle Datei und den Diff nebeneinander zu sehen, muss man den Diff manuell öffnen und selbst bis zur gerade betrachteten Stelle scrollen
  • Für Reviews, die dem Code so nahe kommen, muss man sich derzeit provisorische Tools selbst bauen
  • Das Hauptziel von Code Reviews ist nicht zwangsläufig nur die Codeprüfung; als verwandter Beitrag kann auch Two Kinds of Code Review gelesen werden

Verwandte Beiträge

1 Kommentare

 
GN⁺ 2023-10-25
Meinungen auf Hacker News

  • Die Aussage, man wolle bei großen Änderungen nicht ein Diff-Review machen, sondern die Codebasis zu einem bestimmten Zeitpunkt ordentlich reviewen und dabei besonders auf kürzlich geänderte Bereiche achten, passt für mich persönlich nicht so recht zu einem normalen Code-Review.
    Das hängt sicher vom Team und vom Ticket ab, aber wenn der Reviewer nicht eine Rolle wie ein Principal Engineer hat, der primär für den gesamten Code verantwortlich ist, dann ist ein Code-Review normalerweise eher eine einfache Plausibilitätsprüfung durch ein zweites Augenpaar.
    Wenn die Commits eines PRs gut aufgeräumt oder gesquasht sind, ist es auch vergleichsweise einfach, die inkrementellen Änderungen separat zu prüfen.
    Am Ende scheint die Unzufriedenheit eher auf unterschiedliche Arten von Reviews hinauszulaufen. Es gibt die verbreitete Plausibilitätsprüfung und tiefe Architektur-/Feature-Reviews vor dem Merge; Code-Reviews in Web-Tools scheinen größtenteils eher Ersteres zu sein.

    • Code-Reviews nur als einfache Plausibilitätsprüfung zu sehen, ist eine viel zu dürftige Sichtweise. Code-Reviews sind ein zentraler Mechanismus, um individuelle Code-Ownership zu verringern, Konventionen zu verbreiten und Wissenstransfer zu leisten.
      Ein gutes Code-Review beginnt mit einem guten PR. Ein PR sollte das Ziel beschreiben und wie dieses Ziel erreicht wurde.
      Der erste Punkt eines guten Reviews ist zu prüfen, ob der Code das beschriebene Ziel erreicht; der zweite, ob es Tests gibt, die die behauptete Funktionalität verifizieren; der dritte, ob er den Architekturkonventionen des bestehenden Systems folgt; der vierte, ob der Stil den Erwartungen entspricht; und zuletzt, ob es Vorschläge gibt, die API-Nutzung zu verbessern.
      Code-Reviews, die nur Plausibilitätsprüfungen sind, haben wenig Wert, weil CI das ebenfalls erledigen kann. Code-Review ist ein menschlicher Prozess; daher sollte man das, was nur Menschen leisten können, bestmöglich nutzen und den Rest den Maschinen überlassen.
      In den obigen Punkten steckt implizit die Frage: „Wird diese Änderung ein gutes Beispiel für künftige Beiträge sein?“
    • Oberflächliche Code-Reviews führen meiner Ansicht nach nur zu oberflächlichen Ergebnissen. Genau darin liegt der Grund, warum trotz allgegenwärtiger Code-Reviews so viele Bugs ausgeliefert werden.
      Ich habe Code-Reviews immer aus der Testerperspektive gemacht: den Code bauen und ausführen, schauen, ob er wie beabsichtigt funktioniert, Schwachstellen suchen und versuchen, ihn kaputtzumachen.
      Das Diff ist nicht das Ende, sondern der Anfang. Es ist nur ein Hinweis darauf, wo man im Code herumstochern sollte; der Beweis kommt letztlich aus dem tatsächlichen Verhalten.
      Man könnte sagen: „Das ist Aufgabe von Testern, nicht von Engineers“, aber wer könnte besser testen als jemand, der den Code versteht und weiß, wo man hinschauen muss?
      Mir ist klar, dass diese Haltung in der Minderheit ist. Viele Engineers scheinen eine merkwürdige Abneigung dagegen zu haben, Code selbst auszuführen, sogar dagegen, einen Debugger zu verwenden. Sie bauen riesige Infrastrukturen, um alles zu automatisieren, aber auch Automatisierung ist letztlich nur Code, der Bugs haben kann. Wer testet die Tests — Quis custodiet ipsos custodes?
    • Ich stimme den Erwartungen an ein Code-Review zu, aber damit das richtig funktioniert, muss zuerst eine bestimmte Arbeitsweise etabliert sein.
      Code-Review ist, abgesehen von CI, die letzte Verteidigungslinie; in den Teams, in denen ich gearbeitet habe, war die große Richtung, die ein PR einführt, zu diesem Zeitpunkt aber bereits mit mehreren Leuten besprochen worden. Das kann Pairing sein, eine ausführliche Erklärung, ein Coffee Chat — was auch immer der Komplexität angemessen ist.
      Dann wird der PR zu einem Ort, an dem nicht die Strategie selbst reviewt wird, etwa das Abtrennen eines neuen Moduls, das Einführen einer großen Dependency oder die Überarbeitung der API-Oberfläche, sondern die Taktik, mit der diese Strategie umgesetzt wurde.
      Wenn man ohne vorherige Kommunikation nur einen Teil des Codes isoliert auf Plausibilität prüft, besteht die Gefahr, dass Code-Ownership zersplittert. Dann landet man bei Zuständen wie: „Was macht dieses Modul?“ „Keine Ahnung, frag Bob.“
      Unabhängig davon gefällt mir die vom Autor vorgeschlagene Idee für die Diff-Ansicht, egal welcher Review-Modus verwendet wird.
    • Da sind zwar gute Punkte dabei, aber Diff-Reviews werden, wenn man sie wiederholt erlebt, oft schlicht unverständlich und damit ziemlich sinnlos.
      Mit einem Haufen durcheinandergewürfelter roter und grüner Zeilen lässt sich kaum irgendein Review machen. Das ist keine nützliche Ansicht auf Code und zumindest als Standardansicht nicht geeignet.
      Trotzdem macht man es zähneknirschend, weil es eben Arbeit ist; aber ich stimme dem Autor zu 100 % zu, dass es keine besonders nützliche Ansicht ist.
    • Die Kultur moderner Technologieunternehmen hat sich zu weit von formaleren Prozessen für Entwicklungsmanagement entfernt.
      Weil es keine grundlegenden Prozesse für Defekte, Features usw. gibt, muss jeder Engineer jedes Mal selbst erfinden, wie ein Feature durch Zusammenarbeit ausgeliefert werden soll.
      Die meisten tun am Ende fast gar nichts, und Code-Reviews werden zur einzigen Zusammenarbeit, die auf einer für das Thema sinnvollen Detailebene stattfindet. Deshalb versucht man dann, alle möglichen Eingriffe in diesen einen Moment hineinzupressen.

  • Als dritte oder vierte Option ist auch difftastic erwähnenswert. Es nutzt kein zeilenbasiertes Diff, sondern ein „strukturelles“ Diff und hebt Änderungen dadurch feiner hervor.
    https://github.com/Wilfred/difftastic

    • Als vierte oder fünfte Option gibt es auch patdiff: https://opensource.janestreet.com/patdiff/
      Soweit ich mich erinnere, machte es Diffs manchmal, vielleicht in etwa 35 % der Fälle, besser lesbar; normalerweise gab es in 60 % keinen Unterschied, und selten, in 5 %, wurden sie schwerer lesbar.
      Ich habe es vor ein paar Jahren benutzt und erinnere mich nicht mehr gut an die konkreten Probleme. Der einzige Grund, warum ich aufgehört habe, war, dass ich für Git-Diffs angefangen habe, magit zu verwenden.
    • Ein wirklich guter Ansatz. Schade ist nur, dass er nicht breit integriert ist. Soweit ich weiß, gibt es erst seit Kurzem eine ordentliche strukturierte JSON-Ausgabe, die andere Tools nutzen können.
    • ediff in Emacs macht das ebenfalls. Die Funktion zum Hervorheben unterschiedlicher Wörter heißt Refine.
    • git diff --word-diff hat eine ähnliche Funktion und ist ziemlich gut.

  • Beim Reviewen in Vim fühlt es sich ähnlich an
    Ich habe ein kleines Skript, das den PR öffnet und für geänderte Dateien ungefähr etwas wie vimdiff <(git show baseref:file) file ausführt. Vims Tabs sind im Grunde Views, deshalb ist es praktisch, einzelne Buffer gleichzeitig in unterschiedlichen Zuständen offen halten zu können
    In der Standardansicht funktioniert Scroll-Lock wie erwartet, und bei Bedarf kann man ihn in einem separaten Tab umgehen
    [c und ]c springen zwischen Hunks anhand der im PR vorhandenen Änderungsblöcke, nicht anhand des Arbeitsverzeichnisses
    Mit dp und dg kann man Hunks in den schreibgeschützten Diff-Buffer pushen bzw. daraus pullen und sie so als „erledigt“ markieren; dann werden sie aus den hervorgehobenen Änderungen im Live-Buffer ausgeblendet
    Änderungen im Live-Buffer kann man direkt committen oder als Kommentar pushen
    Auch normale Editorfunktionen wie Go to Definition, Build-Integration und Popup-Dokumentation funktionieren im aktuellen Tree-Zustand weiterhin
    Diese Arbeitsweise zeigt die Änderungen des PR gegenüber dem Basis-Branch, hält das Arbeitsverzeichnis aber sauber. Das wirkt viel besser als matklads Lösung, Änderungen bei verschmutztem Arbeitsverzeichnis anzusehen
    In meinem Arbeitsumfeld ist das besonders praktisch, weil Änderungen oft als --fixup-Commits ergänzt werden und ein Tool sie beim Merge wieder zusammenführt, indem es per Aufruf von git-interpret-trailers die Autoren der Fixup-Commits dem ursprünglichen Commit zuordnet. Außerdem extrahiert es Textkommentare aus dem PR und hängt, wenn passend, einen Reviewed-by-Trailer an oder bei einem +1 einen Acked-by-Trailer

    • Mich würde interessieren, was mit „als Kommentar pushen“ und „Textkommentare extrahieren“ genau gemeint ist. Ist das so etwas wie Custom-Logik nur für die Firma intern?

  • In sehr großen, komplexen Codebasen mit vielen beteiligten Engineers ist das ein großes Problem
    Code Review ist schwierig, weil Diffs immer plausibel aussehen, Tests immer durchlaufen und die grundlegenden Dinge immer geprüft sind
    Aber selbst wenn eine Änderung vernünftig aussieht, ist sie in Wirklichkeit oft falsch
    Eine einzige schlecht gemachte Änderung, die plausibel wirkt, kann die gesamte Architektur in eine falsche Richtung treiben
    Wie so oft ist das streng genommen kein Tool-Problem, sondern eher ein Problem von Kultur und Wissensaustausch. Bessere Tools allein lösen es nicht

    • „Die Tests laufen immer durch“ und „Änderungen sehen vernünftig aus, sind aber oft falsch“ sind zwei Aussagen, die ich zusammen schwer akzeptieren kann
      Beim Code Review findet man selten den Bug selbst. Stattdessen schaut man, ob es Tests gibt, die solche Bugs finden würden

  • Ich habe das Gefühl, etwas zu übersehen. Der Autor sagt, dass Split Diff für ihn nicht passt, sagt aber nicht warum
    Der ideale Diff sieht fast genauso aus wie ein Split Diff ohne duplizierten Kontext. Statt dass der Kontext auf beiden Seiten doppelt erscheint, steht er nur links
    Ich frage mich, welchen Nutzen es bringt, den duplizierten Kontext im rechten Panel zu entfernen

    • Der Autor schreibt, dass er Tests ausführen, Go to Definition und andere Editor-Navigationsfunktionen nutzen, lokale Änderungen anwenden will, um zu prüfen, ob etwas anders hätte gemacht werden können, mehr Kontext sehen möchte, um Stellen zu finden, die ebenfalls hätten geändert werden sollen, und außerdem im aktuellen Codebestand merkwürdige Dinge erkennen will, unabhängig vom historischen Änderungspfad
      Die Editoren/IDEs, die ich verwendet habe, unterstützen solche Funktionen innerhalb eines Diffs meist nur rudimentär. Go to Definition funktioniert dann nur innerhalb derselben Datei, Autovervollständigung klappt bei neu hinzugefügten Dingen oft nicht gut, und Refactoring-Funktionen fehlen häufig
    • Links sieht man den aktuellen Code ohne Änderungen. Wenn man auf der einen Seite den vollständigen Kontext mit Code-Navigation und auf der anderen die Änderungen hat, kann man sehen, welche Änderungen in einem bestimmten Kontext angewendet werden
      So wie ich es verstehe, möchte der Autor den Diff umdrehen. Statt die Änderung selbst zu betrachten, möchte er den Code betrachten und prüfen, ob es dazu relevante Änderungen gibt
      Der Artikel hat wenig Details, aber es wirkt so, als wolle er sich zu Implementierungen oder Aufrufstellen navigieren und prüfen, ob dort passende Änderungen vorhanden sind
    • Ich bin nicht der Autor, kann das aber nachvollziehen
      Diff-Hervorhebungen können erstaunlich ablenken, und wenn man sie ausschaltet, wird der Code manchmal deutlich lesbarer. Auch ich schalte sie im Diff-Viewer gelegentlich aus, um den Code mit frischem Blick zu sehen, aber eine Option, den Unified Diff parallel in einem Split Pane anzuzeigen, gibt es nicht

  • Wenn man auf GitHub . drückt, gelangt man in eine vollständige IDE im Browser
    Beim Reviewen war das sehr nützlich. Statt nur Ausschnitte zu sehen, konnte ich Änderungen im Kontext der ganzen Datei betrachten und dadurch subtile Designprobleme viel besser erkennen

    • Das ist wirklich gut. Ich war bisher nur beim direkten Bearbeiten von Dateien auf GitHub in dieser IDE, wusste aber nicht, dass es eine Tastenkombination gibt, um direkt aus einem Commit-Diff dorthin zu gelangen
      Übrigens funktioniert diese Tastenkombination auch in GitLab

  • Ich vermisse einige Funktionen von p4merge. Zum Beispiel gibt es dort eine Funktion, die im Zwischenraum zwischen Split Diffs zeigt, welcher Teil einer Datei welchem Teil der anderen Datei entspricht, und gleichzeitig auf beiden Seiten die Auswirkungen des Diffs darstellt: https://www.perforce.com/manuals/p4merge/Content/P4Merge/dif...
    Auch die 3-Way-Merge-Funktion gefällt mir: https://www.perforce.com/manuals/p4merge/Content/P4Merge/dif...
    Solche Funktionen sind in verbreiteten webbasierten Diff-Tools nicht angekommen. Ich halte einen 3-Way-Diff für eine gute Methode, um das Ergebnis schwieriger Merges darzustellen

  • Wenn man GitHub nutzt, kann man auf der Pull-Request-Seite von github.com . drücken oder die Domain in github.dev ändern
    Dann öffnet sich VSCode im Browser, und der Pull Request wird als Diff-Ansicht angezeigt
    Der Vorteil ist, dass man die ganze Datei sehen kann und der Diff-Algorithmus ein anderer ist als auf github.com, wodurch komplexe Diffs manchmal leichter zu lesen sind

    • Der Nachteil ist, dass es langsam ist und viele Bugs hat
      Eine Side-by-Side-Diff-Ansicht, die PR-Kommentare als Liste außerhalb der Zeilen anzeigt, könnte sehr nützlich sein, aber es ist schade, dass man dafür eine komplette VSCode-Instanz im Browser starten muss
      Eigentlich war das so nützlich, dass GitHub früher so eine Split View hatte und man die Leute nicht in eine Buzzword-Suppe aus AI/ML/crypto/blockchain/cloud-enabled Entwicklungsumgebung drängen musste
      Die Idee ist gut, die Umsetzung aber miserabel. Das klingt fast wie eine Zusammenfassung des heutigen GitHub insgesamt

  • Meld eignet sich für diesen Zweck ziemlich gut
    https://meldmerge.org/

    • https://kdiff3.sourceforge.net/ ist ebenfalls erwähnenswert, da es viele gute Ansichten bietet
    • Es wirkt dem in den Apple-Entwicklertools enthaltenen FileMerge recht ähnlich

  • Mein Lieblingstool ist diff2html-cli: https://diff2html.xyz/
    https://www.npmjs.com/package/diff2html-cli
    Damit lassen sich Diffs als HTML anzeigen, unterstützt werden Side-by-side- und Unified-Darstellungen