Chromium Code Review Advent Calendar 2017の６日目の記事です。Chromium Browser Advent Calendar 2017の６日目の記事「Chromium コードレビュー用拡張をいれる」も合わせてご覧ください。
なぜCL説明文は重要なのか
良いCL説明文を書くことは、オープンソースプロジェクトの生産性に大きく貢献します。
まず、CL説明文がキッチリ書かれていると、コードレビューを効率的にすすめることができます。たとえば、コードレビューを行う際に心がける点としては、以下があります。
- CLで提案されている変更が必要かどうか
- CLで提案されている変更方法が妥当かどうか
- 変更後のコードで新たに問題が発生してはいないか
- コーディング規約、テスト配備など体裁がととのっているか
しかし、コード変更diffを一回読むだけでは、これらの点を全てチェックするのはなかなか困難です。よく書かれたCL説明文があると、このプロセスにおいてかなり助けになります。理想的な場合、「変更の必要性」「変更方法の妥当性」に関しては、CL説明文のみを用いて議論できます。また、コード変更の背景を予めレビューアと共有しておくことで、コード変更diffのチェックも、すばやく行えます。
また、CL説明文は、後からコードに変更を加える際に、大きな助けになります。なるべくそういう事態が発生しないように、日々気をつけてコードを書いていますが、残念ながら、「間違った処理をしているように見えるコード」や「なぜ必要なのかわからないコード」、「より良い手段が明らかなのに変なことをしているコード」に遭遇することがあります。これらのコードに遭遇した場合、git blameをかけ、そのコードが導入されたコミットを調べていくことになります。この際、CLの説明文やコードレビューのやりとりがはっきりと読みやすい形で残されていると、リファクタリングを効率的に、自信を持って行うことができます。
CL説明文おすすめテンプレ
以下のテンプレで書くのがおすすめです。
[機能名] CL一行説明サマリ
他CLへの強い依存関係
CLの背景説明
CL前のコード説明。なぜそのコードを変える必要があるのか。
CL後のコード説明。この変更はなぜ必要なのか。
Test: テスト
Bug: crbug番号
一行サマリ
CL説明文の先頭の行には、CLの内容を一行に簡潔にまとめたものを書きます。この一行サマリは、レビューア依頼メールのタイトル、git log --oneline、bisectツール表示などで抜粋されて表示されます。つまり、一番使われるのは、ある範囲のCLに対してこの一行説明文のみをざっと眺めて、探している問題に関連していそうかどうかを確かめる用途です。
まず、[ES6 module], [HTML parser]など、巨大な Chromium コードのどの機能に関わるのかをヘッダタグとして書きます。
次に、実際の変更に関して書きます。使う動詞によって、どのような種類のコード変更かの意思表示ができます。
- バグ修正: "Fix A", "B should be C"
- コード削除: "Remove D", "Clean-up unused E"
- 新規機能実装: "Introduce F", "Implement G"
- レアケース修正: "Handle case where..."
コード変更がごく小さい場合、一行サマリに変更内容を全て書くこともできます。
他CLへの強い依存関係
他のCLのコード変更に強く依存したCLを書く場合、CL説明文に明示的にかかれていると、誤ってバグを混入してしまいCLをrevertする必要が出てきた場合の助けになります。Chromiumではこのrevert作業が別のエンジニアによって行われるケースが多いため、これは特に重要です。
この依存関係の例としては、たとえば、あるCLで挙動変更をし、次のCLでその挙動変更によって冗長になった処理を削除する、といったケースが挙げられます。
CL前後の挙動説明
コード変更前の状況を書きます。"Before this CL, ..." で書き始めることが多いです。
変更前のコードにどのような問題があり、なぜコード変更が必要なのかを書きます。"However, ..."
最後に、このCLが問題をどのように解決するのか書きます。"This CL ..."
また、リファクタリングなど、実行時の挙動を変えないコード変更の場合、このことを明示しておくのが望ましいです。例えば、"No behavioral change."と書いておきます。レビューアはこれを踏まえて、本当に動作結果を変えない変更なのかどうかのダブルチェックと、変更がコードの可読性を実際に向上させるものかどうかを議論できます。
テスト
CLで解決した問題に対するリグレッションテストを書きます。
javascriptで挙動が確認できる場合、 web platform test もあると良いです。
バグトラッカー(crbug)番号参照
関連するバグ番号をすべて書きます。CLがlandしたとき、crbug側に自動で通知されるので便利です。
Chromium以外のプロジェクトのMonorailで管理されているバグの場合、[プロジェクト名]:idと書くのが一般的です。
例えば、v8バグの場合、 v8:1569などと書きます。
github issueを参照する場合、https://github.com/whatwg/html/issues/2630 など issue の URL を書くと良いです。