- Les revues de PR à la GitHub facilitent l’empilement de commits de correction au-dessus des changements initiaux, ce qui mélange le processus de revue avec l’historique final du code
- Une bonne série de patchs doit pouvoir se lire comme une suite de commits logiquement séparés, par exemple un refactoring, l’ajout d’une nouvelle API, puis la migration des utilisateurs existants
- Quand des commits comme “fix review” ou
"minor" s’accumulent, l’intention des changements devient floue, et retrouver la cause d’un problème avec git blame et git bisect coûte plus cher
- La revue par interdiff consiste à publier une nouvelle version de la série de patchs et à comparer les différences commit par commit avec
git range-diff main..v1 main..v2
- Le relecteur peut vérifier uniquement un incrément de 50 lignes au lieu de relire 500 lignes entières, tandis que l’auteur conserve une structure finale de commits propre, sans commits parasites du type “address review”
Outils de revue de code et point de départ
- Gerrit Code Review est un outil open source de revue de code qui fonctionne avec des dépôts Git et prend en charge le flux consistant à soumettre des patchs, faire relire le code par d’autres et laisser des commentaires
- Les méthodes de revue de code sont variées : Gerrit, GitHub, Phabricator, mais aussi le dépôt de fichiers
.patch dans un bug tracker, git send-email, ou encore une longue discussion avec un collègue qui guide ensuite l’implémentation
- Le langage Go utilise Gerrit, KDE et LLVM ont utilisé Phabricator, et Jujutsu a déjà envisagé d’utiliser Gerrit
- Le noyau Linux utilise un workflow fondé sur l’email d’une manière particulière, tandis que beaucoup de projets choisissent GitHub parce qu’il est simple à utiliser et peu coûteux à adopter au départ
- Le point essentiel est que ces outils ne proposent pas tous le même modèle de revue, et que l’expérience par défaut de GitHub n’est pas toujours suffisante
Les conditions d’une bonne série de patchs
- Une série de patchs idéale consiste à soumettre un gros changement découpé en étapes logiques
- D’abord, on effectue un nettoyage de code découvert au passage
- Ensuite, on ajoute une nouvelle API
- Enfin, on migre les utilisateurs de l’ancienne API vers la nouvelle
- Chaque patch doit avoir sa raison d’être propre, et leur application dans l’ordre doit montrer une évolution progressive du code
- Cette structure est utile non seulement pour l’auteur et le relecteur, mais aussi pour les mainteneurs qui devront plus tard retrouver la raison d’un changement avec
blame ou bisect
- La taille réelle des patchs dépend du contexte
- Il peut ne pas y avoir de refactoring
- Cela peut faire 100 lignes
- Cela peut aussi aller jusqu’à 500 lignes si, par exemple, une modification d’API centrale impose de corriger tous les points d’appel
La “diff soup” à la GitHub et l’historique mélangé qu’elle produit
- GitHub pousse explicitement et implicitement à ajouter de nouveaux commits au-dessus des commits initiaux pour intégrer les commentaires de revue
- Dans l’exemple, après les 3 commits d’origine, s’empilent successivement des commits appliquant les retours d’Alice et de Bob, l’ajout de tests et un changement d’implémentation
"minor"
- Comme le graphe de commits ne conserve que les relations parent-enfant, il ne montre pas quel problème, dans quel commit initial, un commit de correction de revue est venu résoudre
- Si un seul commit “fix review” touche plusieurs commits initiaux à la fois, le modèle conceptuel devient encore plus complexe
- On peut forcer un commit de correction à ne refléter qu’une seule remarque précise, mais cela augmente d’autant le nombre de commits fixup
- Ce mélange, dans un même ensemble, entre gros changement et corrections liées à la revue constitue la diff soup
Le signal devient plus flou pour git blame et git bisect
- Comme
git blame fonctionne ligne par ligne, il suffit qu’un commit de revue modifie une petite partie d’une ligne existante pour que l’origine affichée de cette ligne devienne un commit du type "fix alice review"
- Pour retrouver la vraie raison du changement, il faut alors refaire une forme d’archéologie des commits en remontant au commit d’origine puis aux commits de correction de revue
- Avec
git bisect, il devient aussi difficile de savoir si le commit "minor" est la vraie cause, ou si le bug était déjà là depuis l’ajout initial de la nouvelle API et n’a été révélé que plus tard
- Si la nouvelle API apporte un gros gain de performance, la revenir en arrière peut être indésirable, voire impossible
- Si la régression n’apparaît que plusieurs semaines plus tard, une situation urgente comme une réunion P1 peut obliger à lancer des investigations supplémentaires à partir d’un simple message de commit
"minor"
- Même dans un exemple où la revue n’a fait que deux allers-retours, l’historique se complique ; et dans les projets open source où les cycles de revue se répètent davantage, le problème s’aggrave
Comment l’UX de GitHub renforce ce flux
- Sur GitHub, pour faire une revue incrémentale, la manière la plus simple consiste de fait à ajouter de nouveaux commits au-dessus
- Pour le relecteur, regarder un patch
"fix alice review" de +/- 10 lignes est moins coûteux que relire 500 lignes d’une nouvelle API
- L’onglet diff d’une PR affiche par défaut tous les commits fusionnés en une seule diff
- Cela ressemble à
git diff master..foo-branch
- Ce n’est pas très bien aligné avec la façon dont les gens écrivent et lisent réellement les changements
- L’UX de GitHub est fortement pensée pour un modèle à branches nommées, ce qui rend plus difficile la comparaison entre unités autres que des branches
- Après un force push, on peut bien consulter une diff via le bouton “Compare” sur la ligne “Force Push” d’une PR
- Mais cela n’affiche que la diff complète entre l’ancienne tête de branche et la tête actuelle
- Pour une comparaison plus fine, il faut retrouver soi-même les identifiants de commits et les placer dans l’URL
La revue par interdiff et git range-diff
- La revue par interdiff n’ajoute pas des commits de correction au-dessus des 3 commits initiaux ; elle publie une nouvelle version composée des mêmes 3 commits
- Par exemple, si la série de patchs
v1 doit intégrer les commentaires de Bob, on modifie le premier et le troisième commit pour produire une série v2
- Ensuite, même si l’on intègre de la même manière les commentaires d’Alice, des tests supplémentaires et un changement
"minor", le résultat final reste toujours une série de 3 commits
- Si les commits A, B, C de
v1 ont évolué en X, Y, Z dans v2, alors git range-diff affiche les différences entre les commits correspondants
git range-diff \
main..v1 \
main..v2
- Cette commande affiche une diff par paires entre les 3 commits de la branche
v1 et les 3 commits de la branche v2
- Premier commit :
diff(A, X)
- Deuxième commit :
diff(B, Y)
- Troisième commit :
diff(C, Z)
Les effets concrets de l’approche interdiff
- Alors que GitHub montre en général une diff globale de branche comme
diff(main, C) ou diff(main, Z), l’interdiff montre les différences entre versions de commits
- Le relecteur n’a pas besoin de relire les 500 lignes de la modification d’API déjà vues ; il peut se concentrer sur les 50 lignes modifiées
- L’auteur n’a pas besoin d’accumuler 30 commits parasites du type “address review” dans l’historique
git blame a davantage de chances de rattacher chaque ligne au commit de changement d’origine, avec moins de bruitgit bisect a davantage de chances de pointer une unité plus significative, comme le commit qui introduit la nouvelle API- Au final, le rapport signal/bruit des outils de diagnostic de base s’améliore
Note sur la stratégie de fusion et le rebase
- L’explication de la stratégie de fusion des patchs reste laissée en TODO
- La position défendue est que
git rebase est acceptable tant qu’on n’est pas dans un cas où d’autres personnes sont censées empiler leurs commits au-dessus d’une branche publique
- Un système de revue par interdiff encourage des patchs plus petits et fusionnables plus rapidement
- Il n’est pas nécessaire d’attendre que les 5 commits soient tous prêts
- Si les 3 premiers sont bons et que les 2 suivants demandent encore du travail, on peut ne fusionner que les 3 premiers
- L’objectif est d’amener les gens à travailler à partir de la branche main plutôt qu’à partir de branches longues, afin de réduire les situations où plusieurs branches distantes finissent par s’entremêler à la fusion
- Certains projets, comme le noyau Linux, fusionnent des branches publiques avec d’autres dépôts ou les publient explicitement au-dessus d’autres branches
- Les développeurs Linux peuvent utiliser
git rebase lorsqu’ils créent et peaufinent une série de patchs
- Mais une fois la branche publique créée, comme elle peut être fusionnée dans l’arbre de quelqu’un d’autre, ils ne font pas de force push et créent à la place une nouvelle branche contenant de nouveaux commits
1 commentaires
Avis de Hacker News
Sur GitHub, j’utilise globalement ce flux, mais son inconvénient est qu’il me donne plus de travail et qu’il n’est pas intuitif pour les collaborateurs.
Cela dit, il conserve les avantages : le reviewer peut voir uniquement le diff correspondant à ses propres retours, sans casser
git blamenigit bisect.Quand j’intègre des retours de review, je fais un commit avec
git commit --fixup, je le pousse, puis je laisse le hash du commit fixup en réponse au commentaire de review.Une fois la PR approuvée et juste avant le merge, j’exécute
git rebase --interactive origin/main --autosquashpour fusionner les commits fixup dans les commits d’origine, puis je fais enfingit push --force-with-leaseavant de merger.Il faut faire attention aux force push avant la fin de la review. Sinon, le reviewer ne peut plus voir ce qui a été ajouté depuis sa dernière review.
Je m’appuie beaucoup sur l’autocomplétion du terminal, donc taper quelque chose comme
git resuffit à arriver à une longue commande, mais c’est un peu rudimentaire ; ce serait bien d’avoir un outil qui prenne en charge et encourage ce flux. Cela dit, si l’on est coincé avec GitHub, c’est déjà correct.Cela dit, en pratique, on ne le voit surtout que dans des équipes d’ingénierie disciplinées qui apprennent volontairement les fonctionnalités avancées de git.
~/.gitconfig[rebase] autosquash = trueEnsuite, un simple
git rebase -i origin/mainréordonne automatiquement les commits fixup/squash. C’est un petit réglage, mais il a nettement amélioré mon flux de travail.C’est le même genre de sujet que le besoin de
--update-refslorsqu’on rebase une série de commits sur laquelle reposent plusieurs branches. Pourquoi devrait-on demander à un humain de suivre les relations dans le graphe, d’insérer les commits à la main et de déplacer les branches ? Les ordinateurs savent très bien gérer les graphes, autant leur confier ça.Il existe aussi
git absorb, mais il n’est pas aussi robuste que l’implémentation de Sapling[1].Le vrai problème n’est pas tant le rebase interactif ou non, mais plutôt l’expérience utilisateur de l’outil de review lui-même et la boucle qu’il induit. Par exemple, les commits fixup ne résolvent pas le problème de GitHub qui affiche les diffs entre points de référence ; si le point de référence est volumineux, cela peut complètement ruiner la review. C’est le cas, par exemple, lorsqu’on rebase au-dessus de 10 nouveaux commits.
J’ai aussi des griefs contre l’UX de Git elle-même, mais le texte original ressemble surtout à une plainte contre GitHub.
[1] Un exemple montrant la différence entre les deux algorithmes de base se trouve dans cette issue GitHub : https://github.com/martinvonz/jj/issues/170
Je suis d’accord à 100 % que cette approche est idéale. La manière GitHub est vraiment atroce, et c’est une tragédie que tant de gens aient fini par la considérer comme normale.
Avant, on faisait ça avec Phabricator, et c’était en quelque sorte faisable manuellement grâce à une macro en ligne de commande qui mettait à jour toutes les reviews d’un coup. Mais une UI explicite serait quand même meilleure.
Oui. C’est exactement ce que j’imaginais comme vrai style de code review, pas la manière dont GitHub le fait. Je suis content d’apprendre que ça a un nom.
En plus, ce serait bien qu’un système de review puisse pousser “hors” de la review les patches qui sont prêts. Une petite correction de bug faite pendant le travail sur une grosse fonctionnalité devrait être un petit patch indépendant, et il y a de bonnes chances que le reviewer l’accepte rapidement.
Dans ce cas, je voudrais extraire ce patch de toute la série, le cherry-pick sur main, puis rebase la review au-dessus du nouveau HEAD. Ou bien rebase la dernière série de patches au-dessus de main en réordonnant le patch accepté en tête, puis faire un fast-forward de main jusqu’à ce patch.
L’idée, au fond, est de réduire le périmètre de la review aux “parties encore en discussion”, tout en permettant aux corrections de bug d’être mergées dès qu’elles sont prêtes.
L’objection serait : “il suffit d’en faire une review/PR séparée”, mais cela crée de la complexité, par exemple lorsqu’un patchset A dépend d’un patchset B, puis dépend de main une fois B mergé.
Des idées d’UX intelligentes comme l’Attention Set y contribuent aussi. En gros, cela indique “qui doit faire la prochaine action ?”, et fonctionne comme un jeu au tour par tour. Si je viens de faire une review, je ne suis plus dans l’attention set de ce patch ; c’est l’auteur qui y est.
Dans l’UI de Gerrit, cet élément descend donc dans ma file. En haut de la file se trouvent les éléments pour lesquels je suis dans l’attention set. Le travail se regroupe naturellement de cette façon.
Je n’ai même pas abordé tous les autres petits irritants de l’UX GitHub, mais même la liste des pull requests est moins bonne que les alternatives. On ne sait pas dans quel état se trouve chaque élément, et on finit par devoir tout lire.
Si c’est x, tout en bas, on peut le soumettre directement et continuer avec le reste.
C’est toujours intéressant de voir une nouvelle approche de la revue de code. GitHub a aussi ses avantages, mais il est loin d’être parfait
Dans le scénario proposé, on pourrait envisager de découper les trois patchs en pull requests distinctes qui dépendent les unes des autres. GitHub ne le prend pas en charge nativement, mais un bon outil de revue de code devrait permettre de garder les pull requests petites tout en gérant les dépendances. Par exemple, le patch 3 pourrait dépendre du patch 2, et le patch 2 du patch 1
Grâce au suivi des dépendances fourni par l’outil, on peut garantir qu’ils soient tous déployés ensemble si nécessaire
Relire chaque patch séparément rend les retours plus clairs et plus faciles à traiter. On peut aussi créer un historique de commits propre, qui reflète précisément les changements individuels, en squashant les commits dans la pull request. Mieux encore, avec un outil adapté, l’IA pourrait résumer les pull requests et les revues, ce qui simplifierait la rédaction de messages de commit exacts sans travail manuel
Un bon outil de revue de code ne devrait pas être perturbé par des opérations git comme rebase, merge ou force push. Quelles que soient les opérations git complexes qui se passent en arrière-plan, les reviewers ne devraient voir que les changements depuis leur dernière revue. Ils peuvent ainsi se concentrer sur le nouveau contenu sans devoir relire un gros diff, et l’historique de revue reste propre et séparé de l’historique des commits
Je me demande si cette manière de découper les pull requests et de suivre leurs dépendances mutuelles répondrait au besoin
C’est littéralement le workflow que git a été conçu pour prendre en charge, et c’est le cœur d’une vieille critique formulée depuis que GitHub propose des pull requests
Je me demande pourquoi cette idée, je vérifie le calendrier, a été dénigrée pendant plus de 15 ans comme la méthode des « dinosaures des listes de diffusion » avant d’arriver enfin dans les cercles de développement à la mode
J’ai longtemps pensé que beaucoup de développeurs logiciels refusaient obstinément d’apprendre correctement l’un des outils qu’ils utilisent le plus, et choisissaient donc volontairement de mauvais workflows. C’est pour ça que, quand j’ai fait passer l’entreprise à git, j’ai choisi et construit ce genre d’outils et de flux. Après tout, on ne se fait pas virer pour avoir acheté IBM
Si un contributeur externe utilise un fork du dépôt, les trois PR vont progressivement contenir A, A+B, puis A+B+C. Les deux dernières PR obligent alors à revoir dans le diff du code déjà relu dans une autre PR, ce qui rend la revue difficile
Chaque commit peut être relu indépendamment. Les dépendances entre commits peuvent aussi être suivies. Le fait qu’il n’y ait pas d’IA intégrée est également un avantage. Contrairement aux PR, on peut aussi voir les interdiffs entre commits
Les commits sont les briques de base de Git. Il suffit d’utiliser les commits
Je me demande comment GitContext suit les commits après des opérations comme fixup, rebase ou réordonnancement
Je suis l’auteur de l’article. Demandez-moi n’importe quoi
Au passage, je ne sais pas si c’est contraire aux règles, mais j’ai récemment besoin d’un nouveau poste. J’aime travailler sur des outils de développement et des problèmes difficiles. Si cet article vous a plu, si vous voulez améliorer la productivité de votre équipe de développement, ou si vous voulez découvrir et apprécier mes excellents goûts parfois éclectiques, mon e-mail est dans mon profil
Plus sérieusement, ton CV est impressionnant. J’espère que tu retrouveras quelque chose de bien, et vite. D’après mon expérience très récente de recherche d’emploi, le marché actuel du recrutement est gravement malade. Plus on est senior et expérimenté, plus les pratiques d’entretien et de RH absurdes, ainsi que les refus inexpliqués, font mal à l’âme
Un ami m’a envoyé les liens ci-dessous
https://danluu.com/hiring-lemons/
https://danluu.com/programmer-moneyball/
https://danluu.com/algorithms-interviews/
Bonne chance
Un point que tu n’as pas encore abordé : à cause de la diff soup, les gens peuvent finir par préférer une stratégie de squash merge pour se débarrasser du « bruit » des commits de fixup. Ce faisant, ils jettent aussi les trois bons commits atomiques de départ
Dans le style de revue par interdiff, les trois commits initiaux restent présents, et la décision de les intégrer séparément ou de les squasher peut être prise uniquement en fonction de ces commits eux-mêmes et de leur atomicité réelle
Il existe aussi des services externes qui tentent de réduire la douleur de GitHub et qui offrent la prise en charge des stacked diffs, mais ils semblent assez chers, surtout comparés à Gerrit auto-hébergé. Je me demande si tu as aussi regardé ces outils
Je me souviens avoir mis en place assez facilement un pattern de série de MR par le passé. En fusionnant 3 MR, on obtenait 3 merge commits, chacun avec un unique commit squashé par MR. C’était indépendant de l’historique des commits de la MR
Le compromis, c’est que si une branche située plus tôt dans la série change pendant la revue, il faut la remerger dans les branches situées plus loin dans la série
Malgré tout, pendant la revue, les gens peuvent gérer l’historique des commits comme ils le souhaitent. Comme tout est squashé ensuite, peu importe
Cela dit, ça fait longtemps que je ne l’ai pas fait, et comme je suis maintenant coincé sur GitHub, mes souvenirs sont peut-être un peu embellis
Review Board [https://www.reviewboard.org] avait déjà créé interdiff en 2006. En fait, il est possible que j’aie inventé le terme, ou que j’y sois arrivé indépendamment.
C’est encore aujourd’hui l’un de mes aspects préférés du produit et du processus de revue de code, et c’est l’une des fonctionnalités dont j’entends le plus souvent dire qu’elle manque aux gens quand ils passent à quelque chose comme GitHub.
Les commits de correctif ne m’ont jamais semblé être une vraie alternative.
D’abord, ils n’indiquent pas comment une modification amont a été intégrée dans une série de commits.
Ensuite, ils perturbent le graphe de commits, même temporairement, et rendent la revue plus difficile. Quelqu’un qui suivait déjà la revue a peut-être déjà lu le code que corrige le commit de correctif, mais une personne qui arrive nouvellement peut commencer avec une mauvaise première impression de ce que ce code cherche à faire ou de sa structure.
Troisièmement, tout le monde n’utilise pas Git ni un système de gestion de versions capable de gérer plusieurs commits. Dans le développement de jeux, on utilise souvent Perforce, et chez les fabricants de puces des systèmes de gestion de versions spécialisés comme Keysight SOS. Dans ces environnements, le commit de correctif n’est tout simplement pas une option.
Avec un système de revue de code qui prend correctement en charge interdiff, un reviewer peut suivre toutes les mises à jour depuis la première demande de revue publiée et ne voir que ce qui a changé, tandis qu’un autre reviewer peut arriver directement sur l’ensemble des dernières modifications sans avoir à se soucier d’une série de commits de correctif. Et cela fonctionne indépendamment du système de gestion de versions.
Bien conçu, cela peut aussi coexister avec des changements composés de plusieurs commits.
Par exemple, on peut avoir découpé un petit projet en plusieurs commits — gestionnaires d’API, UI front-end, documentation — pour faciliter la revue, et juger que ces commits sont suffisamment liés pour être soumis dans une seule demande de revue. S’ils étaient moins liés entre eux, l’idéal serait plutôt de créer plusieurs demandes de revue sous forme de chaîne de dépendances.
En fonction des retours de revue, on peut produire une série de modifications sur l’un de ces commits, ou sur tous. Quand les gens relisent la mise à jour, il vaut mieux qu’ils puissent voir comment chaque partie a évolué, plutôt que de devoir faire mentalement correspondre les commits de correctif et leurs changements aux modifications d’origine correspondantes.
Voilà pourquoi interdiff est vraiment excellent. Qu’il s’agisse de nombreux petits commits ou d’un gros commit, d’une demande de revue à commit unique ou à commits multiples, davantage de personnes devraient l’utiliser.
Vers 2013, le Glasgow Haskell Compiler est passé de l’approche qu’on décrivait en plaisantant comme « lire des fichiers .patch depuis le bug tracker » à l’utilisation de Phabricator. Il y avait plusieurs raisons à cela, mais à l’époque ce n’était pas à cause des stacked diffs. C’était parce que GitHub était tellement mauvais pour la revue qu’il ne proposait même pas de diff côte à côte. Rien que pour cette raison, il était totalement hors sujet, et il y en avait d’autres.
Mais ce n’était pas la première fois que je faisais passer une équipe à un outil de revue de code. En 2009, mon premier emploi était dans une toute petite équipe d’ingénieurs très soudée, réunie dans une même pièce à Houston, et je me suis dit que faire relire mon code par quelqu’un d’autre, et lire ce qu’ils écrivaient, m’aiderait à mieux comprendre la base de code.
Alors, pendant mes premiers mois après mon arrivée, j’ai tanné mon manager pour qu’il installe ReviewBoard, et tout le monde l’a vraiment apprécié.
Donc oui, merci à RB. J’y repense encore parfois avec de bons souvenirs. Grâce à lui, la revue de code a occupé une très grande place dans ma carrière presque dès le premier jour, et je pense encore aujourd’hui que je pourrais en faire davantage.
Globalement, j’ai trouvé que les systèmes comme Gerrit, centrés d’abord sur la revue de code et sur le rebase, rendaient le code beaucoup plus facile à relire.
L’un des meilleurs points est la prise en charge native de l’empilement de plusieurs patches, ce qui pousse les gens à produire des patches plus petits et plus faciles à relire.
La revue de code de GitHub donne l’impression d’être une mauvaise fonctionnalité ajoutée après coup : une UI qui gaspille de l’espace et ressemble à un fil de forum, l’incapacité à suivre au-delà d’un rebase, etc.
Ce genre de discussion à la fois « vue d’ensemble » et technique se retrouve trop souvent dans l’issue tracker. Mais je ne sais pas non plus où la mettre. Cette pile est trop temporaire et peut être complètement différente au prochain push.
J’ai entendu défendre cette stratégie plusieurs fois, mais je ne suis pas convaincu. Dans la plupart des projets sur lesquels je travaille, les feature branches sont squashées en un seul commit lors du merge, puis l’historique de la branche est supprimé
Dans le cas décrit par l’auteur, je ferais simplement trois commits : refactoring, nouvelle API, mise à jour
Ce qui a clairement été une bonne pratique pour moi, c’est d’éviter les branches qui vivent longtemps. Ce genre de scénario en commits à étapes multiples vient généralement de là : quelqu’un travaille sur une fonctionnalité pendant plusieurs jours, voire plusieurs semaines, puis veut tout envoyer d’un coup
Je préfère de loin intégrer dans main tous les jours, voire plusieurs fois par jour. Une façon d’y parvenir est d’utiliser des feature flags, afin que même des commits en cours puissent être intégrés sans problème. Si le système est correctement configuré, cela a aussi l’avantage de pouvoir les activer et les tester dans les environnements de développement et de staging. On n’est alors plus très loin du déploiement blue/green
Je ne veux pas de méthodes pour rendre les gros commits plus faciles à relire. Je veux obliger l’équipe à committer tôt et souvent de petits changements. Je comprends que tout le monde ne soit pas d’accord
L’une des raisons est que, contrairement aux PR GitHub, on peut intégrer seulement une partie de la pile. Dans l’exemple de l’article, si le diff « petit refactoring » est prêt, on peut l’intégrer d’abord sans intégrer en même temps les diffs « nouvelle API » et « migration des utilisateurs de l’API »
En se plaçant non pas autour des branches mais autour des commits, on obtient des commits globalement plus petits qu’avec des branches longues
Cela rend
git blameetgit bisectpratiquement inutilesS’il y a une régression,
git bisectpeut la réduire à un patch unique. C’est pourquoi, pour une fonctionnalité donnée, il vaut mieux avoir dans l’historique Git 50 patchs de 160 lignes qu’un seul patch de 8 000 lignesSi une ligne de code paraît suspecte, je veux que
git blame, ou une suite de commandesgit blame, me mène à un commit de 160 lignes avec un message détaillé, pas à un commit de 8 000 lignesIl faut aussi préserver l’ordre des commits d’origine. Des années plus tard, le simple fait de lire les messages de commits individuels dans l’ordre peut beaucoup aider à comprendre la conception initiale. Bien sûr, le patchset d’origine doit être organisé dans l’ordre des dépendances, compiler à chaque étape, etc., ce qui demande une étape de développement distincte au-dessus de l’implémentation de la fonctionnalité. Le code doit être présenté en étapes logiques
Intéressant. Au travail, nous utilisons les PR comme l’auteur utilise les commits, et à la fin nous faisons effectivement un squash-and-merge
Mais avec cette méthode, chaque fois qu’on modifie une PR en amont, il faut rebase les PR qui suivent. C’est assez pénible, cela se heurte aux règles de type « pas de force-push », les ingénieurs mettent du temps à l’apprendre, et cela a tendance à casser les commentaires de revue de code existants dans l’UI GitHub. Cela dit, pour deux ou trois PR, ça fonctionne à peu près
Dans notre flux, les commits ressemblent davantage à des points de sauvegarde qu’à des unités de travail
Nous utilisons aussi beaucoup stash, ainsi que le mode undo-tree d’Emacs. Cela fait donc quatre façons de suivre l’historique du code source ; dit comme ça, cela semble redondant, mais en pratique ça tourne plutôt bien
L’ergonomie de Git pour gérer tout ça est assez mauvaise. Phabricator faisait mieux, mais restait inutilement difficile. Peut-être qu’un nouvel outil de gestion de versions pourrait prendre en charge comme concepts de première classe des abstractions de plus haut niveau que les commits et les branches ; ou peut-être que cela le rendrait encore moins agréable à utiliser
Est-ce que ce serait bien d’avoir un meilleur outil que Git ? Bien sûr, moi aussi j’aimerais un outil avec moins de défauts et plus convivial. Mais comme solution de fortune, pourquoi ne pas traiter les commits comme des commits ? Inutile de vous compliquer volontairement la vie
Je me demande dans quelle mesure GitHub est responsable d’avoir mis des lunettes « PR » à tout le monde
jj,sapling,mercurialJ’ai aimé l’article de blog, et je suis content de voir des gens réfléchir réellement à la manière dont la revue de code devrait fonctionner
J’ai beaucoup utilisé quatre systèmes de revue de code, avec des avantages et inconvénients différents : Critique (interne à Google), Gerrit (utilisé chez Google, mais identique à la version externe), GitHub, et CodeApprove que j’ai créé
Critique était de loin le meilleur, mais c’était possible parce qu’il était parfaitement adapté au monorepo de Google, à son propre système de gestion de versions et à ses outils custom de lint/test. CodeApprove a été conçu pour apporter autant que possible ces avantages à GitHub, mais en pratique il est difficile de s’en approcher
Gerrit était le deuxième meilleur du point de vue de l’expérience des reviewers, mais je l’ai toujours détesté en tant qu’auteur. Il semblait y avoir plus de façons de se tromper que de faire correctement, et l’UI n’est pas franchement belle
GitHub est très accueillant pour les auteurs et fonctionne comme nous pensons : on écrit du code, on reçoit du feedback, puis on écrit encore du code. Si l’on fait un squash and merge à la fin de la PR, le problème d’historique évoqué par l’auteur ne se pose pas
En revanche, il n’est pas très accueillant pour les reviewers ni pour les équipes. Les diffs incrémentaux ne sont pas mis en avant, le diff et la conversation sont dans des onglets différents, et les force push et rebase détruisent l’historique. Les commentaires disparaissent en « outdated », on ne peut pas commenter des fichiers en dehors de la fenêtre de diff, les gros fichiers sont masqués par défaut, et ainsi de suite. Il est clair que GitHub ne s’en soucie pas vraiment, et peut-être qu’ils savent quelque chose que j’ignore
Au final, le plus frustrant est que beaucoup d’équipes acceptent simplement l’outil de revue de code intégré à leur plateforme de gestion de versions. C’est comme utiliser tel quel l’IDE préinstallé sur son ordinateur portable. Aujourd’hui, il existe de bien meilleurs choix. En dehors de CodeApprove, ceux que j’aime sont GitContext, Reviewable et Graphite, et je pourrais citer au moins six autres excellentes options. Il ne faut pas accepter les réglages par défaut
Les reviewers et les mainteneurs sont beaucoup plus rares que les contributeurs. À mon avis, les workflows et l’UI devraient optimiser le débit de revue
Je n’ai jamais utilisé les trois autres outils mentionnés, donc mon point de vue reste général