Comparaison entre Unified Diff et Split Diff

• Pour les petits changements, unified diff comme split diff permettent tous deux une lecture suffisante, mais sur des modifications volumineuses et complexes, une revue limitée au diff atteint vite ses limites
• Les gros changements doivent être explorés et vérifiés à partir de l’état complet de la base de code à un instant donné, comme si l’on modifiait réellement le code, plutôt qu’à partir du seul historique des changements
• L’écran de revue idéal devrait afficher côte à côte le code actuel présent sur le disque et le unified diff de cette zone, afin de suivre en même temps le contexte du code et le contenu des modifications
• Faute de support suffisant dans les outils existants, un workflow low-tech est utilisé : checkout local de la PR, suppression des commits, puis conservation des seuls changements
• Cette méthode permet de parcourir les fichiers modifiés et de marquer les hunks déjà revus, mais la position courante dans l’éditeur et le diff ne se synchronisent pas automatiquement, ce qui laisse encore des manipulations manuelles

---

Revoir l’état actuel du code plutôt que le diff

• split diff et unified diff fonctionnent tous deux très bien pour des changements petits et simples
• Quand les modifications deviennent plus grandes et plus complexes, lire uniquement le diff ne suffit plus
• Pour les gros changements, il faut examiner correctement la base de code à un instant donné
  • Il faut prêter attention aux zones récemment modifiées, mais l’aborder fondamentalement comme une revue de code classique
  • Il faut exécuter les tests
  • Il faut utiliser les fonctions de navigation de l’éditeur, comme goto definition
  • Il faut appliquer des modifications locales pour vérifier si une autre manière d’écrire le code aurait été possible
  • Il faut repérer, dans un contexte plus large, les parties qui auraient aussi dû être modifiées
  • Il faut pouvoir détecter ce qui semble anormal dans la base de code actuelle, indépendamment de l’historique des changements

Écran de revue souhaité et contournement local

• La vue diff idéale afficherait à gauche l’état actuel du code et à droite le unified diff correspondant à la zone de code actuellement visible
  • Le code à gauche doit être identique à l’état actuel sur le disque
  • Les modifications sont indiquées discrètement dans la marge
  • Le diff à droite doit correspondre à la zone de code actuellement visible à gauche
• Ce format de revue est encore mal pris en charge par les outils actuels
• En pratique, le workflow consiste à utiliser le script gpr pour checkout la PR en local
  • À la dernière étape, les commits de la PR sont supprimés et seuls les changements sont conservés
  • Dans cet état, le workflow de staging et de commit est réutilisé pour la revue de code
• Avec edamagit, on peut consulter la liste des fichiers modifiés et passer à la modification suivante/précédente dans l’éditeur
  • On peut aussi utiliser la staging area pour marquer les hunks déjà relus
• Le principal inconfort restant est l’absence de synchronisation automatique entre le magit status buffer et le fichier actuellement ouvert dans l’éditeur
  • Pour voir côte à côte le fichier actuel et le diff, il faut ouvrir le diff manuellement puis faire défiler jusqu’à la position en cours
• Pour mener une revue aussi proche du code, il faut encore aujourd’hui bricoler soi-même des outils temporaires
• L’objectif principal d’une revue de code n’est pas nécessairement la seule inspection du code ; on peut aussi lire Two Kinds of Code Review à ce sujet

[GN⁺]

Avis sur Hacker News

• Dire que, pour un gros changement, on ne veut pas faire une revue de diff, mais plutôt examiner correctement la base de code à un instant donné en prêtant attention aux zones récemment modifiées ne me semble pas vraiment correspondre à une revue de code classique
  Cela dépend des équipes et des tickets, mais si le relecteur n’a pas un rôle du type principal engineer, principalement responsable de l’ensemble du code, une revue de code ressemble généralement davantage à une simple vérification de cohérence faite par une deuxième paire d’yeux
  Si les commits de la PR sont bien organisés ou squashés, il est aussi relativement facile d’examiner séparément les changements incrémentaux
  Au fond, la critique semble plutôt porter sur la différence entre types de revue. Il y a les revues courantes de vérification de cohérence, et les revues pre-merge approfondies d’architecture/de fonctionnalité ; les revues de code dans les outils web semblent pour la plupart proches du premier cas

  • Voir la revue de code comme une simple vérification de cohérence est une perspective bien trop pauvre. La revue de code est un mécanisme clé pour réduire la propriété individuelle du code, diffuser les conventions et assurer le transfert de connaissances techniques
    Une bonne revue de code commence par une bonne PR. Une PR doit expliquer l’objectif et la manière dont cet objectif a été atteint
    Le premier point d’une bonne revue est de vérifier que le code atteint l’objectif annoncé ; le deuxième, qu’il existe des tests validant la fonctionnalité revendiquée ; le troisième, qu’il suit les conventions d’architecture du système existant ; le quatrième, que le style correspond aux attentes ; et enfin, qu’il y a éventuellement des suggestions pour améliorer l’usage de l’API
    Une revue de code qui ne fait que de la vérification de cohérence est peu utile, car c’est quelque chose que la CI peut faire. La revue de code étant un processus humain, il faut exploiter au maximum ce que seuls les humains peuvent faire, et laisser le reste aux machines
    Dans les points ci-dessus, la question « ce changement sera-t-il un bon exemple pour les contributions futures ? » est implicitement présente
  • À mon avis, les revues de code superficielles ne produisent que des résultats superficiels. C’est pour cela que, malgré la fréquence des revues de code, beaucoup de bugs arrivent tout de même en production
    J’ai toujours fait les revues de code avec une perspective de testeur. Je construis et exécute le code pour voir s’il se comporte comme prévu, je cherche ses points faibles et j’essaie de le casser
    Le diff n’est pas la fin, mais le point de départ. Ce n’est qu’un indice indiquant où aller titiller le code ; au bout du compte, la preuve vient du comportement réel
    On pourrait dire que « c’est le travail des testeurs, pas celui des ingénieurs », mais je me demande qui peut mieux tester que la personne qui comprend le code et sait où regarder
    Je sais que cette attitude est minoritaire. Beaucoup d’ingénieurs semblent avoir une réticence étonnante à exécuter eux-mêmes le code, voire à utiliser un débogueur. Ils construisent d’énormes infrastructures pour tout automatiser, mais l’automatisation reste du code, donc elle peut elle aussi contenir des bugs. Qui teste les tests, Quis custodiet ipsos custodes ?
  • Je suis d’accord avec ce qu’on peut attendre d’une revue de code, mais pour que cela fonctionne correctement, un certain mode de travail doit d’abord être en place
    La revue de code est, hors CI, la dernière ligne de défense, mais dans les équipes où j’ai travaillé, la grande orientation introduite par une PR avait déjà été discutée avec plusieurs personnes avant ce stade. Cela peut être du pair programming, des explications détaillées, des discussions autour d’un café, peu importe tant que c’est adapté à la complexité
    La PR devient alors l’endroit où l’on examine la tactique qui met en œuvre cette stratégie, et non la stratégie elle-même, comme séparer un nouveau module, introduire une grosse dépendance ou retravailler la surface d’API
    Si l’on se contente de vérifier la cohérence de morceaux de code isolés sans communication préalable, on risque de fragmenter la propriété du code. On se retrouve avec des situations du genre : « Il fait quoi, ce module ? » « Je ne sais pas, demande à Bob »
    À part cela, quel que soit le mode de revue, j’aime bien l’idée de vue diff proposée par l’auteur
  • Il y a de bonnes remarques, mais à force d’en faire, les revues de diff deviennent souvent tout simplement incompréhensibles, donc assez vaines
    Avec un bloc mêlant lignes rouges et lignes vertes, il est difficile de faire une quelconque revue. Ce n’est pas une vue utile du code, et ce n’est certainement pas approprié comme vue par défaut
    Cela dit, comme c’est le travail, on s’y colle quand même, mais je suis d’accord à 100 % avec l’auteur : ce n’est pas une vue très utile
  • La culture des entreprises tech modernes s’est trop éloignée de processus plus formels de gestion du développement
    En l’absence de processus de base pour traiter les défauts, les fonctionnalités, etc., chaque ingénieur doit réinventer à chaque fois, par la collaboration, la manière de livrer une fonctionnalité
    La plupart finissent par ne presque rien faire, et la revue de code devient la seule collaboration menée à un niveau de détail significatif sur le sujet. Résultat : on essaie d’y faire entrer toutes les interventions possibles en une seule fois

• Comme troisième, ou quatrième option, difftastic mérite aussi d’être mentionné. Il utilise un diff « structurel » plutôt qu’un diff ligne par ligne, ce qui permet de mettre en évidence les changements plus finement
  https://github.com/Wilfred/difftastic

  • Comme quatrième, ou cinquième option, il y a aussi patdiff : https://opensource.janestreet.com/patdiff/
    De mémoire, il rendait parfois environ 35 % des diffs plus lisibles, dans 60 % des cas cela ne changeait généralement rien, et, plus rarement, dans 5 % des cas, il les rendait plus difficiles à lire
    Je l’ai utilisé il y a quelques années, donc je ne me souviens plus bien des problèmes précis. La seule raison pour laquelle j’ai arrêté, c’est que j’ai commencé à utiliser magit pour les diffs Git
  • C’est une très bonne approche. Dommage toutefois qu’elle ne soit pas largement intégrée. À ma connaissance, une vraie sortie JSON structurée utilisable par d’autres outils n’est apparue que récemment
  • ediff dans Emacs fait aussi cela. La fonction qui met en évidence les mots différents s’appelle Refine
  • git diff --word-diff propose aussi quelque chose de similaire, et c’est plutôt bien

• On ressent quelque chose de similaire quand on fait une review avec Vim
  J’ai un petit script qui ouvre la PR et qui, pour les fichiers modifiés, effectue grosso modo une opération du type vimdiff <(git show baseref:file) file. Les onglets de Vim étant en réalité des vues, c’est pratique parce qu’on peut garder des buffers individuels ouverts simultanément dans différents états
  Dans la vue par défaut, le verrouillage du défilement fonctionne comme prévu, et on peut l’éviter dans un onglet séparé si nécessaire
  [c et ]c naviguent entre les hunks en se basant sur les groupes de modifications présents dans la PR, et non sur le répertoire de travail
  Avec dp et dg, on peut push/pull un hunk vers/depuis le buffer de diff en lecture seule pour le marquer comme « terminé », ce qui le masque alors parmi les changements mis en évidence dans le buffer live
  Les changements faits dans le buffer live peuvent être commités directement ou pushés sous forme de commentaires
  Les fonctions habituelles de l’éditeur, comme go to definition, l’intégration du build ou la documentation en popup, continuent aussi de fonctionner telles quelles sur l’état courant de l’arbre
  Cette approche permet de voir les changements par rapport à la branche de base de la PR tout en gardant le répertoire de travail propre. Ça me paraît bien meilleur que la solution de matklad, qui consiste à consulter les changements avec un répertoire de travail modifié
  Dans l’environnement où je travaille, c’est particulièrement utile parce que les changements sont souvent ajoutés sous forme de commits --fixup, puis l’outil les regroupe à nouveau au moment du merge en appelant git-interpret-trailers afin d’attribuer l’auteur du commit de fixup au commit d’origine. Il extrait aussi les commentaires textuels de la PR pour ajouter, le cas échéant, un trailer Reviewed-by, ou un trailer Acked-by si cela correspond à un +1

  • Je me demande ce que signifient exactement « pousser sous forme de commentaires » et « extraire les commentaires textuels ». C’est une logique personnalisée propre à l’entreprise ?

• Dans une très grande base de code complexe, avec beaucoup d’ingénieurs dessus, c’est un gros problème
  La review de code est difficile parce que les diffs ont toujours l’air plausibles, les tests passent toujours, et les éléments de base sont systématiquement vérifiés
  Mais même lorsqu’un changement paraît raisonnable, il est souvent faux en réalité
  Un seul mauvais changement qui semble plausible peut faire dériver toute l’architecture
  Comme toujours, ce n’est pas vraiment un problème d’outils, mais plutôt de culture et de partage des connaissances. De meilleurs outils seuls ne le résoudront pas

  • J’ai du mal à accepter ensemble les deux phrases « les tests passent toujours » et « même si le changement paraît raisonnable, il est souvent faux »
    Lors d’une review de code, on trouve rarement le bug lui-même. On vérifie plutôt s’il existe des tests capables de détecter ce type de bug

• J’ai l’impression de rater quelque chose. L’auteur dit que le split diff ne lui convient pas, mais n’explique pas pourquoi
  Le diff idéal ressemble presque à un split diff dont on aurait supprimé le contexte dupliqué. Au lieu d’avoir le contexte répété des deux côtés, il ne serait présent qu’à gauche
  Je me demande quel bénéfice apporte la suppression du contexte dupliqué dans le panneau de droite

  • L’auteur écrit qu’il a besoin d’exécuter les tests, d’utiliser go to definition et d’autres fonctions de navigation de l’éditeur, d’appliquer les changements localement pour voir s’ils auraient pu être écrits autrement, de consulter un contexte plus large pour repérer les parties qui auraient dû être modifiées en même temps, et de remarquer, dans la base de code actuelle, les éléments étranges indépendamment du chemin historique des changements
    Dans les éditeurs/IDE que j’ai utilisés, la prise en charge de ces fonctions dans un diff est généralement rudimentaire. Go to definition ne fonctionne souvent qu’au sein du même fichier, l’autocomplétion marche mal sur les éléments nouvellement ajoutés, et les fonctions de refactoring sont souvent absentes
  • La gauche montre le code actuel, non modifié. Si un côté affiche le contexte complet avec les fonctions de navigation dans le code, et l’autre les changements, on peut voir quelles modifications s’appliquent dans un contexte donné
    Si je comprends bien, l’auteur veut inverser le diff. Au lieu de regarder les changements eux-mêmes, il veut regarder le code et vérifier s’il y a des changements associés
    L’article manque de détails, mais il semble vouloir naviguer vers les implémentations ou les points d’appel et vérifier si les changements appropriés sont présents
  • Je ne suis pas l’auteur, mais je comprends
    La mise en évidence des diffs peut être plus distrayante qu’on ne le pense, et la désactiver améliore parfois nettement la lisibilité. Moi aussi, je la coupe parfois dans un visualiseur de diff pour relire le code avec un regard neuf, mais il n’existe pas d’option pour afficher en même temps un diff unifié dans un split pane

• Sur GitHub, appuyer sur . ouvre un IDE complet dans le navigateur
  Je l’ai trouvé très utile pour les reviews. Plutôt que de ne voir que des fragments, on peut examiner les changements dans le contexte complet du fichier, ce qui aide beaucoup à repérer les problèmes de conception subtils

  • C’est vraiment excellent. Je n’étais entré dans cet IDE sur GitHub que pour modifier directement des fichiers, je ne savais pas qu’il y avait un raccourci pour y accéder directement depuis un commit diff
    À noter que ce raccourci fonctionne aussi sur GitLab

• Certaines fonctionnalités de p4merge me manquent. Par exemple, il y a une fonction qui indique, dans l’espace entre les deux panneaux du split diff, quelle partie d’un fichier correspond à quelle partie de l’autre, tout en montrant aussi l’impact du diff des deux côtés : https://www.perforce.com/manuals/p4merge/Content/P4Merge/dif...
  J’aime aussi la fonctionnalité de merge à 3 voies : https://www.perforce.com/manuals/p4merge/Content/P4Merge/dif...
  Ces fonctionnalités n’ont pas été intégrées aux outils de diff web couramment utilisés. Je pense qu’un diff à 3 voies est une bonne manière de montrer le résultat d’un merge difficile

• Si vous utilisez GitHub, il suffit d’appuyer sur . depuis la page de pull request sur github.com, ou de remplacer le domaine par github.dev
  VSCode s’ouvre alors dans le navigateur et la pull request s’affiche en vue diff
  L’avantage est qu’on peut voir les fichiers en entier, et que l’algorithme de diff diffère de celui de github.com, ce qui peut le rendre plus lisible sur des diffs complexes

  • L’inconvénient, c’est que c’est lent et truffé de bugs
    La vue side-by-side diff qui affiche les commentaires de PR dans une liste hors des lignes pourrait être très utile, mais c’est dommage de devoir lancer une instance complète de VSCode dans le navigateur
    En fait, c’est tellement utile que GitHub avait autrefois ce genre de split view, sans avoir besoin de pousser les gens vers un environnement de développement rempli de buzzwords du genre AI/ML/crypto/blockchain/cloud-enabled
    L’idée est bonne, mais l’exécution est catastrophique. Cela résume peut-être aussi GitHub dans son ensemble ces derniers temps

• Meld convient plutôt bien à cet usage
  https://meldmerge.org/

  • https://kdiff3.sourceforge.net/ mérite aussi d’être mentionné, car il propose beaucoup de bonnes vues
  • Il semble assez proche de FileMerge, inclus dans les outils de développement Apple

• Mon outil préféré est diff2html-cli : https://diff2html.xyz/
  https://www.npmjs.com/package/diff2html-cli
  Il permet de consulter un diff en HTML et prend en charge les formats side-by-side ou unified