はじめに
こんにちは、この世すべてのレビュワーの代弁者です。
もくじ
" きれい " なコミット履歴とは
PR（プルリクエスト）において、個人的に『あ～きれいでありがたいコミット履歴だな～』と思うコミット履歴には以下のような性質が備わっています。
- その１：コミットが意思決定の最小単位になっている
- その２：コミットメッセージが簡潔で分かりやすい
- その３：意思決定のプロセスが妥当である
- その４：コミットメッセージ以外の差分が含まれていない
- その５：コミット履歴が多すぎない
コミット履歴には、PRを作った開発者本人のいろんな能力、例えば技術力や開発プロセスへの洞察力、想像力、コミュニケーションスキルなど、が直に反映されます。なので、いろいろなことを考えられている開発者のコミット履歴はきれいです。むしろ『美しい...。』とすら感じます。上記の要素がすべて含まれたコミットメッセージが「美しい」のは、コミット履歴がただの単純な作業記録を越えた、開発プロジェクトにおいて志すべき精神性を象徴しているからです。
以下では、なぜ上記の要素を満たすとよいのか、もはや美しいにつながるのかをちょっとだけ説明します。
コミットが意思決定の最小単位になっている
そもそも、１つのPRで生まれる全体の差分は、１単位の目的を達成するための変更差分であるべきです。
単一責任の原則ではクラスは 1 つの責任のみを持つべきであると述べられているように、プルリクエストは１つの関心ごとのみを扱う必要がある！
１単位の目的と言っているのは例えば、
- （１単位の）新規機能の追加
- （１単位の）不具合の修正
- （１単位の）リファクタリング
などがそれにあたり、その目的を達成するために積み上げた複数の意思決定がこそがコミット履歴となるべきです。
例えば、生きていればタバコを吸いたくなるじゃないですか。このとき生まれた欲求を解消すべく、１単位の目的を『家でゆっくり電子タバコ吸う！』としたとします。そのためには、運動靴を履いて、コンビニに向かって、レジで注文して、電子マネーで支払いを済ませ、走って帰宅し、ソファーに腰かけて、一服したいわけです。このときに踏んだ「複数の意思決定」こそがPRで言うコミット履歴であり、「なぜそれをしたか」を表現するのがコミットメッセージであり、「最終的な目的達成の形」こそがPR全体の変更差分になるわけです。
| step | 意思決定 (commit) |
Why? (commit message として残すべき) |
|---|---|---|
| 1 | 運動靴を履く | 裸足で外歩くと怪我するリスクがあるから |
| 2 | コンビニに行く | 最寄りのタバコ屋よりコンビニの方が近いから |
| 3 | レジで注文する | それ以外で購入不可能（年齢確認必須）だから |
| 4 | 電子マネーで支払う | 現金よりパフォーマンス（速度）が良いから |
| 5 | 走って帰宅 | 歩くよりパフォーマンス（速度）が良いから |
| 6 | ソファーに腰かける | いっちばんリラックスできるから |
| 7 | 吸う | 目的が達成されるから（step 1 ~ 6はそのためのsetup！） |
※ 最小の意思決定にしては粒度が粗めですがイメージはつくかなと思います
開発プロセスも同様で、目的に対して複数のステップを踏むことがほとんどですよね。リファクタリングであれば単体テストをあてる ⇒ メソッドの内部実装変える ⇒ テストのリファクタリングする、みたいな。
こうした１つ１つの小さな意思決定という単位で履歴（意図）を残すことは、開発者の責任です。なぜなら、変更の差分はPRを見ればわかりますが、「なんでその意思決定（コードの変更）をしたのか？」はコミットメッセージを見ないと分からないからです。そういう意味で、開発者の Why? を把握するために、最終的なPRの差分がどのようにして出来上がったのかを知るために、最小単位の意思決定の履歴はレビュワーが欲しいと思う重要な情報なのです。
コミットメッセージが簡潔で分かりやすい
コミットが意思決定の最小単位になっている と関連しますが、意思決定の理由がきちんとレビュワーに伝わらないとメッセージを残している意味がありません。分かりやすいコミットメッセージには、意思決定の根拠が "コミットメッセージとして" 反映されています。
なので、良いコミットメッセージとは『なぜその意思決定をしたのか（Why?）』が伝わるよう表現されたものと考えていいと思います（ t-wadaさん が以下の投稿で言っていることはそういうことのはず）
なぜその意思決定をしたのか（Why?）がコミットメッセージに含まれているだけでめちゃくちゃありがたいのですが、各メッセージに prefix があるとなおいいですね。コミットメッセージの頭に prefix をくっつけることで、開発プロセス内で必要な作業の種類をラベルとして表現できます。それゆえ、少ない文字数でレビュワーに実際に行われた変更の種類を伝えることができるので、コスパがいいです。
簡潔で分かりやすいコミットメッセージは当然メリットがありますが、最も大きなメリットはコードレビューが効率的になる点ですね。
コミットメッセージが簡潔で分かりやすいとコード変更の理由をすぐに理解できるので、何がどう変わったかを深く調べなくても済みます。また、レビュワーがコードをチェックする際にその妥当性を速く、正確にフィードバックできるようになります。そんなこんなで、雑に言えばPDCAが回りやすくなるので、結果として開発のスピードが向上するうえに品質の高い製品を作り出すことにつながるというわけです。
$$『─────コミットメッセージはレビュワーへのラブレターだと思いなさい。』$$
意思決定のプロセスが妥当である
プロセスが妥当であるというのは、言い換えれば「必要なプロセスを正しい順番で踏んでいる」ということになります。
コミット履歴は適切な順番で意思決定をしていることを証跡として残すべきです。理由は品質保証とレビューのしやすさの２つの観点が考えられます。
例えば、テスト駆動な不具合修正時の適切なプロセスは以下のようになります。
- 手元で不具合を再現させる。
- コードを注意深く調べ、不具合を発生させている最小の部分を絞り込む。
- 最小レベルで不具合を再現させ、不具合が修正されたら通るような自動テストコードを書く。
- 3で書いたテストコードを実行し、落ちることを確認する。
- 不具合を修正する。
- 3で書いたテストコードが通ることを確認する。
- 既存のすべてのテストを実行し、不具合修正が他の部分を壊していないことを確認する。
上記をコミットで表現するならこんな感じになるでしょう（上から順）
1. 【機械的修正】XXXの処理をhoge()メソッドとして（IDEの機能によって）機械的に抽出
2. 【仕様化テスト】hoge()メソッドの振る舞いを網羅するため、仕様化テストを追加
3. 【仕様化テスト】hoge()メソッドの修正後に期待される、失敗するテストを追加
4. 【プロダクトコード修正】hoge()メソッドがXXXするようにプロダクトコードを変更
5. 【仕様化テスト】hoge()メソッドの誤った振る舞いのテストコードを削除
6. 【リファクタリング】hoge()メソッドのリファクタリング
品質保証の観点
このコミット履歴を見れば、プロダクトコードを変更する前にきちんと単体テストで既存の振る舞いを担保したうえで修正していることが分かりますよね。このとき、テストを書く前にプロダクトコードの修正をしているコミット履歴があった場合どうでしょうか？テストがない状態でプロダクトコードに手を加えているので、意図しないデグレードを引き起こすリスクを排除することができません。
最終的な修正内容が正しいように見えても、「振る舞いを変えていない根拠」である単体テストが存在しないまま修正を加えた場合、プロダクトコードの修正はバグってないことを担保できません。さらにそのバグってないことを担保できない状態で新たに追加された単体テストも当然、信用できないものになります。
結果、最終的な差分が妥当に見えても必要なプロセスを適切な順序で踏んでいないので、マージするのが怖い修正になってしまいます。そういうことがあるので、意思決定のプロセスが適切かどうかは重要なレビュー観点になります。（なのでプロセスをきちんとコミット履歴に残しましょう）
レビューのしやすさの観点
これはそのままの意味です。レビューをする際にコミット履歴を追いながら変更を見ていくことが一般的だと思います。（差分が少ない場合はPRの Files Changed だけ見る場合もあるけど）ここで、コミット履歴（変更のプロセス）が論理的なステップを踏んでいる場合、レビュワーの思考回路（目的に応じて想定される一般的な開発手順）と大きくズレることがないので理解しやすいです。
一方で、コミット履歴の中に目的達成のプロセスと関係ない箇所の修正（例えばまったく関係ないファイルの Code Smells の修正とか）が入っていた場合、レビュワーは『え、なにこれ...??』と立ち止まって考える必要があります。だってプロセスの一部としてコミットされてるように見えるんだもん、なんか関係あるんかなってなるじゃないですか！なんか気になるし、レビューいったん中断して『なにこれ...??』の答え探しがはじまります、不毛です。命がもったいない。
やるならそういうPRをつくるか、プロセスの本筋とは関係ないようなところでやりましょう。（なので目的に関連するコミットだけをいれましょう）
コミットメッセージ以外の差分が含まれていない
コミットメッセージはコードの変更に対するドキュメントの１つです。ゆえに適切なメッセージはレビュワーの理解容易性を高め、開発者のコード変更の意図を正確に理解することができます。
しかし、コミットメッセージで説明されている内容とは関係ない変更差分が含まれていたらどうでしょうか？『ナニコレ？』てなりますよね。なので、各コミットの中に "コミットメッセージ以上の余計な差分が含まれない" 状態は、レビュワーが効率的かつ効果的にレビューを行うための必須条件です。
各コミットに余計な差分が含まれている場合、レビューの遅延とバグを埋め込むリスクが生まれます。（これはコミットメッセージが意思決定の最小単位になっておらず、曖昧な表現をしている場合も多いので気をつけましょう）
レビューの遅延
例えば、コミットメッセージでは「仕様化テストの追加」となっているのに、それに加えてプロダクトコードの既存メソッドが削除されてさらに新しいメソッドが追加されている、というケースを考えてみます。このコミットを見たときにレビュワーは確実に『え、ちょっと待ってなにこれ、とりあえず何しようとしたのか読み解くか。』となるでしょう。そう、シンプルに理解に時間とコストがかかるのです。
で、これが複数箇所に存在している場合はもう最悪で、レビュワーがそれぞれの変更に対するコンテキストを理解するために本来（コミットメッセージ以外の差分が含まれていなければ）必要なかった時間を要することになります。さらに、レビュワーは関連のない変更に意識が奪われて集中力、注意力が散漫になってさらに時間がかかります。
そんなこんなでレビューの時間は増大していき、レビュワーは疲れちゃうし開発者はフィードバックを得るのが遅くなるしでお互いにとって良くない状況が生まれてしまうのです。（避けたいよね！）
バグを埋め込むリスク
コミットメッセージと関連性のない差分がたくさんある場合はバグのリスクを排除することが難しくなります。理由はシンプルで、レビュワーが見逃しやすくなるからです。
適切なコミットメッセージは、個別のコミット単位でのレビューを可能にします。コードの変更が意思決定（コミットメッセージの内容）に対して必要十分であるかをレビュワーがチェックできるからです。一方で余計な差分が混入するとコミット単位での確認がしにくくなり、どうしてもバグやデグレードのリスクを見逃してしまうリスクが高くなってしまいます。（結局、目検だからね）
そういう理由で、コミットメッセージに対応するコードの変更だけが含まれているということは、レビュワーにとってすごくレビューしやすくてありがたいのです。また、バグった場合にチームの責任になるとはいえ、バグの原因を生んだのが自分だとしたらけっこうつらいです。開発者各位はプロ意識を持って、バグを混入させるようなコミットを行わないように注意したいですね。
コミット履歴が多すぎない
コミット履歴が多いということは、その分全体の変更差分も多くなりがちです。変更差分が多いということは、レビュワーも見なきゃいけない範囲が増えるのでより大変になります。そもそも、PRの目的は１つであるべき（１つのPRで複数の関心ごとを扱うとレビュワーに負荷を強いることになる）なので、長くなる場合にはタスクを適切な粒度に分けられていないことを疑った方がいいかもです。
この点はプロジェクトの性質やコミットの粒度やチームのポリシーによって異なるため、一概に「多すぎる」を定義するのは難しい...。
コミット履歴が多くなってくると以下のような問題を引き起こす可能性があります。
レビューコストの増大
シンプルにしんどいです。加えて変更差分がめっちゃ多い場合は泣いちゃいます。
コミットの数が多いと、当然それらを１つ１つ確認するために必要な時間や労力が増えます。個別のコミットに対する変更差分を理解して、その妥当性を判断する必要があります。変更対象が簡単なロジックであればいいのですが、複雑なコードだったりするとレビュワーの負担も当然大きくなります。
また、レビューを受けて観点漏れが複数見つかった場合に、レビューをお願いした開発者側も修正がめっちゃ大変ですよね。レビュイーもレビュワーもどっちもきつい。メンタルに効く！
バグを埋め込むリスク
これは先ほども言及しましたが、履歴が多くなっていくにつれてレビュワーが見逃す可能性のあるコードが増えるからですね。さらに、コードの変更点が多くなるとコード理解の難易度が上がるので、潜在的バグを見逃しやすくなります。
てなわけで、変更する箇所が多いと変更内容が広範囲になり、バグの存在がその他の変更点に埋もれて見逃されちゃうかもしれないのでリスキーです。（怖いね！）
コストが高いことが見込まれるのでレビューを後回しにされる
レビュワーが見たときに『あ、これ時間かかるやつだ』と瞬時に判断されます。人間は怠惰な生き物ですから『ん～いったんお昼ご飯食うか！』『絶対あとで見る！』とどうしても他のタスクよりも後回しにされちゃうことが多いんじゃないでしょうか。
基本的にみんな忙しいですから、コスト高めと見積もられるとレビューは後回しにされがちです。これは開発のサイクル的には最悪で、フィードバックはもらえないしもらえても遅いし戻りが発生したら遅くなるし...の繰り返しでPRがマージされるのいつになるカナ？という感じです。
コミット履歴が多くなりそうなら、そうならないような粒度に分解して複数のPRに分けてレビューしてもらうのをオススメします。
レビュワーに優しいプルリクエストを作ろう
はい、ここまでいろいろ説明してきましたが、いかがでしたでしょうか。
この辺のレビュワーの痛みは、「自分がレビュワーにならないと感じにくい！」という点が難しいなぁと思います。最近は開発生産性の文脈の中で「小さい単位でPRをつくろう！」がベストプラクティス化してそうな気がするのでそうでもないのかもしれませんが。
そこまで厳密にやる必要はないのですが、きれいなコミット履歴を作る癖は絶対自分に跳ね返ってくるので、ぜひ意識してもらえたらなと思います。
おわりに
この世すべてのレビュワーの代弁ができていたでしょうか。
誰かの参考になったらうれしいです。