Unified Diff と Split Diff の比較
(matklad.github.io)- 小さな変更は unified diff と split diff のどちらでも十分に見られるが、大きく複雑な変更では diff を読むだけのレビューは限界にぶつかる
- 大きな変更は変更履歴よりも、特定時点の コードベース全体の状態 を基準に、実際にコードを修正するように探索して検証すべき
- 理想的なレビュー画面は、現在ディスク上のコードとその領域の unified diff を並べて表示し、コードの文脈と変更内容を同時に追えるようにすること
- 既存ツールのサポートが不十分なため、PR をローカルにチェックアウトした後でコミットは消し、変更だけを残す ローテクなワークフロー を使っている
- この方法なら変更ファイルの探索と reviewed hunk の表示は可能だが、エディタの現在位置と diff が自動同期されないため手動操作が残る
diff より現在のコード状態をレビューする
- split diff と unified diff は 小さく単純な変更 ではどちらもよく機能する
- 変更が大きく複雑になると、単に diff を読むだけでは十分ではない
- 大きな変更では、特定時点のコードベースをきちんと見る必要がある
- 最近変更された領域に注意を払いながらも、基本的には 通常のコードレビュー のように見るべき
- テストを実行する必要がある
- goto definition のようなエディタのナビゲーション機能を使う必要がある
- ローカル変更を適用して、別の書き方が可能だったかを確認する必要がある
- より広い文脈で変更されるべきだった部分を探す必要がある
- 変更履歴とは無関係に、現在のコードベースで不自然な点を見つけられるべき
望ましいレビュー画面とローカルでの回避策
- 理想的な diff ビューは、左側に現在のコード状態、右側に現在表示しているコード領域の unified diff を一緒に表示する形
- 左側のコードはディスク上にある現在の状態と同じであるべき
- 変更点は余白にさりげなく表示される
- 右側の diff は左側で現在表示しているコード領域に対応しているべき
- このようなレビュー形式は、現状ではツールのサポートが不足している
- 実際のワークフローは、gpr script で PR をローカルにチェックアウトする方式
- 最後の段階で PR のコミットは消し、変更だけを残す
- この状態で staging と commit 用のワークフローをコードレビューに活用する
- edamagit で変更ファイル一覧を見て、エディタで次／前の変更に移動できる
- staging area を使って、すでにレビューした hunk を表示することもできる
- 残る不便さは、magit status buffer とエディタで現在開いているファイルの間に自動同期がない点
- 現在のファイルと diff を並べて見るには、diff を手動で開き、今見ている位置まで自分でスクロールしなければならない
- ここまでコードに近いレビューを行うには、現時点では一時的なツールを自作する必要がある
- コードレビューの主な目的は必ずしもコード検査だけではなく、関連する記事として Two Kinds of Code Review も参照できる
1件のコメント
Hacker News の意見
大きな変更で diff レビューではなく、ある時点のコードベースをきちんとレビューし、最近変更された領域に注意を払いたいという話は、個人的には一般的なコードレビューとは合わないように見える
チームやチケットによって違うだろうが、レビュアーがコード全体に主な責任を持つ principal engineer のような役割でないなら、通常のコードレビューは第二の目による簡単な整合性確認に近いものだと思う
PR のコミットがきれいに整理されていたり squash されていたりすれば、差分の変更を別途確認するのも比較的容易
結局、不満はレビューの種類の違いに近いように見える。よくある整合性確認レビューと、深いアーキテクチャ／機能の pre-merge レビューがあるが、Web ツールのコードレビューはほとんど前者に近いようだ
良いコードレビューは良い PR から始まる。PR は目標と、その目標をどのように達成したかを説明すべきだ
良いレビューの最初の項目は、コードが説明された目標を達成しているか確認すること。二つ目は、主張された機能を検証するテストがあるか。三つ目は、既存システムのアーキテクチャ上の慣習に従っているか。四つ目は、スタイルが期待に合っているか。最後に、API の使い方をより良くする提案があるかどうかだ
整合性確認しか行わないコードレビューは CI でもできることで、価値は小さい。コードレビューは人間のプロセスなので、人間にしかできないことを最大限活用し、残りは機械に任せるべきだ
上記の項目には「この変更は将来の貢献にとって良い例になるか？」という問いが暗黙に含まれている
いつもテスターの視点でコードレビューをしてきた。コードをビルドして実行し、意図どおりに動作するかを見て、弱点を探し、壊そうとする
diff は終わりではなく始まりだ。コードのどこを突けばよいかを教えてくれる手がかりにすぎず、最終的な証明は実際の動作から得られる
「それはテスターの仕事であってエンジニアの仕事ではない」と言う人もいるかもしれないが、コードを理解し、どこを見るべきかを知っている人よりうまくテストできる人がいるだろうかと思う
こうした態度が少数派であることは分かっている。多くのエンジニアは、自分でコードを実行すること、さらにはデバッガを使うことにさえ、妙なほど抵抗感があるように見える。あらゆるものを自動化しようとして巨大なインフラを作るが、自動化も結局はバグを含みうるコードにすぎない。テストは誰がテストするのか、Quis custodiet ipsos custodes?
コードレビューは CI を除けば最後の防衛線だが、私が働いたチームでは、PR が導入する大きな方向性は、その時点より前にすでに複数人と議論されていた。ペアリング、詳細な説明、コーヒーチャットなど、複雑さに見合う方法なら何でもよい
そうすれば PR は、新しいモジュールの分離、大きな依存関係の導入、API サーフェスの作り直しといった戦略そのものではなく、その戦略を実装した戦術をレビューする場になる
事前のコミュニケーションなしにコードの一部だけを切り出して整合性確認すると、コード所有が分断される危険がある。「あのモジュールは何をするもの？」「知らない、Bob に聞いて」のような状態になる
それとは別に、レビューのモードが何であれ、筆者が提案した diff ビューのアイデアは気に入っている
赤い行と緑の行が入り混じった塊では、どんなレビューもしにくい。コードの有用なビューではなく、少なくともデフォルトのビューとしては適切ではない
それでも仕事なので何とかやってはいるが、筆者が言うように、あまり有用なビューではないという点には 100% 同意する
欠陥や機能などを扱う基本的なプロセスがないため、各エンジニアが協業を通じて機能をどうリリースするかを毎回自分で発明しなければならない
たいていはほとんど何もしないことになり、コードレビューだけがそのテーマについて意味のある詳細レベルで行われる唯一の協業になる。そのため、可能なあらゆる介入を一瞬に押し込もうとする状態になる
三つ目、あるいは四つ目の選択肢として difftastic も挙げる価値がある。行単位の diff ではなく「構造的」diff を使い、より細かく変更を強調してくれる
https://github.com/Wilfred/difftastic
記憶では、ときどき 35% ほど diff を読みやすくし、通常 60% は差がなく、まれに 5% はかえって読みにくくしていた
数年前に使っていたので具体的な問題はよく覚えていない。使うのをやめた唯一の理由は、Git diff に magit を使い始めたからだ
git diff --word-diffにもこれに似た機能があり、かなり良いVimでレビューするときも似たように感じる
PRを開くちょっとしたスクリプトがあり、変更ファイルに対しておおむね
vimdiff <(git show baseref:file) fileのような動作を行う。Vimのタブは実質的にビューなので、個別のバッファを異なる状態で同時に開いておけるのがよいデフォルトのビューではスクロールロックが期待どおりに動作し、必要なら別タブでは回避できる
[cと]cは作業ディレクトリではなく、PRに存在する変更のまとまりを基準にhunk間を移動するdpとdgでhunkを読み取り専用のdiffバッファへpush/pullして「完了」扱いにでき、そうするとライブバッファ内の強調表示された変更から隠れるライブバッファで行った変更は、そのままコミットするか、コメントとしてpushできる
現在のツリー状態で、go to definition、ビルド統合、ポップアップドキュメントのような通常のエディタ機能もそのまま動作する
この方法は、PRの基準ブランチとの差分を見ながらも作業ディレクトリをクリーンに保てる。作業ディレクトリを汚した状態で変更を見るmatkladの解法より、はるかによさそうに見える
私の作業環境では、変更がしばしば
--fixupコミットとして追加され、ツールがmerge時点でgit-interpret-trailersを呼び出してfixupコミットの作者を元のコミットに帰属させるように再度まとめてくれるので、特に都合がよい。PRのテキストコメントを抽出して、適切ならReviewed-bytrailerを付けたり、+1相当ならAcked-bytrailerを付けたりもするエンジニアが多数関わる非常に大きく複雑なコードベースでは、これは大きな問題になる
コードレビューが難しい理由は、diffは常にもっともらしく見え、テストは常に通り、基本的なことはいつもチェック済みだからだ
ところが、変更が合理的に見えても、実際には間違っていることがよくある
もっともらしく見える悪い変更ひとつで、アーキテクチャ全体が漂流してしまうことがある
いつものことだが、これは厳密にはツールの問題というより、文化と知識共有の問題に近い。よりよいツールだけでは解決できない
コードレビューでバグそのものを見つけることはまれだ。代わりに、そうしたバグを捕まえられるテストがあるかを見る
何か見落としている気がする。筆者はsplit diffが自分には合わないと言っているが、理由を述べていない
理想的なdiffは、重複するコンテキストを取り除いたsplit diffとほとんど同じに見える。コンテキストが両側で重複するのではなく、左側にだけある形だ
右側のパネルから重複コンテキストをなくすと、どんな効用があるのか気になる
私が使ったことのあるエディタ/IDEでは、通常diff内でこうした機能のサポートは基礎的なものにとどまる。go to definitionも同じファイル内でしか効かなかったり、自動補完も新しく追加された項目にはうまく効かなかったり、リファクタリング機能はないことが多い
私の理解では、筆者はdiffを反転させたいのだ。変更そのものを見るのではなくコードを見て、そこに関連する変更があるかを確認したいのだと思う
記事は詳細に欠けるが、実装や呼び出し地点へ移動しながら、適切な変更があるかを確認したいように見える
diffの強調表示は思った以上に気が散ることがあり、オフにすると可読性が明らかによくなる場合がある。私もdiffビューアで時々オフにしてコードを新鮮な目で見るが、統合diffをsplit paneに一緒に表示する選択肢はない
GitHubで
.を押すと、ブラウザ内の完全なIDEに入れるレビュー時に非常に有用だった。断片だけを見るのではなく、ファイル全体のコンテキストの中で変更を見られるので、微妙な設計上の問題をずっと見つけやすくなる
ちなみに、このショートカットはGitLabでも動作する
p4mergeの機能のいくつかが恋しい。たとえばsplit diff間の余白に、ファイルのどの部分が別ファイルのどの部分に対応しているかを表示しつつ、両側でdiffの影響も一緒に見せる機能がある: https://www.perforce.com/manuals/p4merge/Content/P4Merge/dif...
3-way merge 機能も気に入っている: https://www.perforce.com/manuals/p4merge/Content/P4Merge/dif...
こうした機能は、広く使われているWebベースのdiffツールには入ってこなかった。難しいmergeの結果を見せるには、3-way diffがよい方法だと思う
GitHubを使っているなら、github.comのpull requestページで
.を押すか、ドメインをgithub.devに変えればよいそうするとブラウザでVSCodeが開き、pull requestがdiffビューで表示される
利点はファイル全体を見られることと、github.comとはdiffアルゴリズムが違うため、複雑なdiffでは読みやすいことがある点だ
PRコメントを行外の一覧として表示するside-by-side diffビューは非常に有用になり得るが、そのためにブラウザでVSCodeインスタンス全体を立ち上げなければならないのが残念だ
実際、とても有用なので、GitHubには以前こうしたsplit viewがあり、人々をAI/ML/crypto/blockchain/cloud-enabledな開発環境のような流行語の塊へ押し込む必要はなかった
アイデアは良いが、実行はひどい。最近のGitHub全体を要約する言葉のようでもある
Meld はこうした用途にかなりよく合う
https://meldmerge.org/
いちばん気に入っているツールは diff2html-cli: https://diff2html.xyz/
https://www.npmjs.com/package/diff2html-cli
diff を HTML で表示でき、side-by-side や unified 形式に対応している