Chromium Code Review Advent Calendar 2017の6日目の記事です。Chromium Browser Advent Calendar 2017の6日目の記事「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 を書くと良いです。