Dev Study
← 開発の実務
レッスン 8 / 12·約 8 分·第16期 開発の実務

レビューが見ているのは、プログラムではなく差分

リクエストがアプリケーションに届くまで
  1. ブラウザURL入力
  2. DNS名前→IP
  3. ルータ経路選択
  4. ロードバランサ振り分け
  5. Nginx受け口
  6. アプリ処理今ここ
  7. DB保存

前回は、隣り合う2つのコミットを比べて原因を遡りました。今回は、変更が取り込まれる前に人が読む場面です。読んでいるものが何なのかを確かめます。

まず結論

このコースの芯は「『壊れていない』は、何と比べたかでしか言えない」です。レビューが比べているのは変更前後のプログラム全体ではなく、git diff が切り出した差分です。

差分は変更した行とその前後しか見せません。1文字の変更で壊れた場所が、差分に1行も出ていないことがあります。「レビューで問題なし」は、差分と比べた結果です。

? 読む前に予測

関数 inBounds の中の `<` を `<=` に1文字だけ変えました。git diff を既定の設定で出すと、画面に出るのはどれでしょうか。

当てることが目的ではありません。外した予測ほど記憶に残るので、直感で選んでください。

抽象原理

レビュアーの画面に出るもの

テストは期待する出力と比べ、デバッグは症状と原因を比べます。レビューも比較です。では何と何を比べているのでしょうか。

レビュアーの画面に出るのは、変更前後のプログラムそのものではありません。この回では 「差分」を git diff の出力 という意味で使います。git diff は変更前と変更後を比べ、違う行とその周りだけを切り出します。周りを何行付けるかは、公式文書にこう書かれています。

The number of context lines defaults to diff.context or 3 if the configuration variable is unset.

設定が無ければ 前後3行 です。つまり、レビューで言える「壊れていない」は、この切り出された範囲と比べた結果です。範囲の外は、比べる対象に入っていません。

git diff が切り出すものと、切り出さないもの
変更前の全体
変更後の全体
git diff が比べて切り出す
差分変更した行 + 前後3行
差分に出ない行変更していないファイル・離れた行
レビュアーの画面
読まれるここで判定する
探しに行かないと見えない
  • ・文脈行の既定は3。設定 diff.context か -U<n> で変わる
  • ・変更していないファイルは、文脈行を何行にしても差分に出ない
具体実物

1文字の変更、1ファイルの差分、別ファイルの例外

3つのファイルを用意しました。bounds.js の inBounds は添字が配列に収まるかを返し、page.js の nextRow はそれを使って次の行を大文字で返します。main.js は ["a", "b", "c"] で nextRow を2回呼びます。変更前の実行結果は B と null、終了コード0です。

inBounds の i < len を i <= len に変えます。

変更後の git diff と --stat(git 2.50.1。区切りの見出し行は $ コマンドに置き換え)コマンド出力
$ git diff
diff --git a/bounds.js b/bounds.js
index 4102cf3..9320365 100644
--- a/bounds.js
+++ b/bounds.js
@@ -2,6 +2,6 @@
function inBounds(i, len) {
 if (!Number.isInteger(i)) return false;
 if (i < 0) return false;
-  return i < len;
+  return i <= len;
}
module.exports = { inBounds };
$ git diff --stat
bounds.js | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

@@ -2,6 +2,6 @@ は、2行目から6行のかたまりという意味です。変更した5行目の前に3行、後ろはファイル末尾までの2行が付いています。--stat は 1 file changed。これだけ見れば、影響は小さく見えます。

実行します。

変更後の node main.js と git grep(Node v24.21.0。一時ディレクトリのパスを ... に、スタックの残り8行と版表示を省略)コマンド出力
$ node main.js
B
.../r/page.js:5
return inBounds(j, rows.length) ? rows[j].toUpperCase() : null;
                                          ^
 
TypeError: Cannot read properties of undefined (reading 'toUpperCase')
  at nextRow (.../r/page.js:5:45)
exit=1
$ git grep -n inBounds
bounds.js:2:function inBounds(i, len) {
bounds.js:7:module.exports = { inBounds };
page.js:1:const { inBounds } = require("./bounds.js");
page.js:5:  return inBounds(j, rows.length) ? rows[j].toUpperCase() : null;

nextRow(rows, 2) で j が3になり、inBounds(3, 3) が真を返すようになりました。page.js は存在しない rows[3] を読んで落ちます。例外が指すのは page.js の5行目で、このファイルは差分に1行も出ていません。

呼び出し側は、差分の外へ git grep しに行って初めて見えました。差分だけを読んで「境界の条件を直しただけ」と承認するのは、差分と比べた判定としては正しいのです。

✓ ここまでの確認

実演で `<` を `<=` に変えたあと、例外が出たのはどこでしたか。

抽象原理

レビューで見つかりやすいもの

では、レビューは何を見つけているのでしょうか。Microsoft 社内のレビューを観察・面接・調査した Bacchelli & Bird(ICSE 2013)は、こう要約します。

Our study reveals that while finding defects remains the main motivation for review, reviews are less about defects than expected and instead provide additional benefits such as knowledge transfer, increased team awareness, and creation of alternative solutions to problems.

欠陥の発見は、今も主な動機です。ただ、分類したコメントの中で欠陥に関するものについては次のとおりです。

Defect related comments comprise a small proportion and mainly cover small logical low-level issues.

そして同じ論文は、context and change understanding is the key of any review と書きます。

ここからは教材側の整理です。差分の中で完結する小さな論理の誤りは、画面に出ているので指摘できます。変更していない場所に影響が出る誤りは、レビュアーが差分の外まで文脈を取りに行かない限り、比べる対象に入りません。論文が「文脈と変更の理解」を鍵と呼ぶのと、同じところを指しています。

具体実物

全部出ているのに、読めない差分

逆向きの例もあります。依存パッケージを足すと、lockfile(npm の package-lock.json など、依存の解決結果を道具が書き出すファイル)には、足したパッケージが連れてくる依存まで書き込まれます。書き換わる量はその連れてくる量で決まります。このサイトのリポジトリでは、依存6つを直接追加したコミット 5480b11 で package-lock.json が1,782行(追加1,726・削除56)変わりました。

こちらでは、変更は すべて差分に出ています。外に隠れている行はありません。それでも、道具が生成した大量の行を人が1行ずつ読むことは、ふつう想定されていません。名前の似た悪性パッケージ(タイポスクワッティング)が1項目混ざっても、画面には出ているのに読まれない、ということが起こります。

実演は「差分に出ていないから比べられない」でした。こちらは「差分に出ているが、人の目では比べきれない」です。どちらでも「レビューで問題なし」は、人が実際に比べた範囲についての判定です。

✓ ここまでの確認

Bacchelli & Bird(ICSE 2013)の結果の要約として、原文に沿っているのはどれですか。

⇄ 2つを見比べる
具体例 A

1文字の変更(実演)

差分は1ファイル1行で、読むのは簡単です。しかし壊れた page.js の5行目は差分に出ません。比べる対象から外れていたのは、画面の外にある行です。

差分と、比べた範囲表
差分の大きさ      1 file changed
壊れた場所        page.js:5(差分の外)
見るための手段    git grep で呼び出し側を探す
具体例 B

lockfile の書き換え

変更は全部差分に出ています。しかし道具が生成した行を1行ずつ読むことは想定されておらず、混ざった1項目は画面にあっても読まれません。比べる対象から外れていたのは、画面の中の行です。

小さすぎる差分と大きすぎる差分。逆の状況に見えますが、承認が見落とすものの共通点は何でしょうか。

「差分に出たか」と「人が比べたか」を分けて考えてください。

この章のまとめ

  • この回の「差分」は git diff の出力。変更行と前後3行(既定)だけを切り出す。
  • 1文字の変更で、差分に1行も出ていない呼び出し側 が落ちた。--stat は 1 file changed。
  • 文脈行を増やしても、変更していないファイルは出ない。外は git grep などで 探しに行く。
  • Bacchelli & Bird: 欠陥発見は 今も主な動機。ただし欠陥コメントは少数で、小さく局所的な論理の問題が主。「レビューはバグを見つけない」とは書かれていない。
  • lockfile のように、差分に全部出ていても 人が比べきれない ことがある。
  • 「レビューで問題なし」は、人が実際に比べた範囲に対する判定。

自分の現場で確かめる

読んだだけでは身につきません。手元の環境で実際に見てみてください。

理解度チェック

読んだ直後は「分かった」と感じやすいものです。実際に解いて確かめてください。

問題 1 / 3正解: 0

git diff -U10 のように文脈行を増やすと、実演の呼び出し側 page.js は差分に出てきますか。

用語集(3語)
差分(この回の意味)
git diff の出力。変更した行と、その前後の文脈行を並べたもの。/training の他の回では別の意味で使うことがある。
文脈行
差分で、変更した行の前後に添えて表示される変更のない行。git diff では既定で前後3行(設定 diff.context で変えられる)。
ハンク (hunk)
差分の中の、`@@ -2,6 +2,6 @@` のような見出しで始まるひとかたまり。変更行と文脈行から成る。

出典(一次ソース)