commit をどう分割すべきか〜コードレビューの観点から〜

2018-01-09

普段の開発の中で git の commit の単位に気を遣っている人もいると思うんですが、どういう単位で commit すべきかみたいな話をあまり見かけない気がします。自分自身 GitHub の Pull Request（以降 PR）ベースのチーム開発を何年か経験してみて、「こうすると良さそう」というものが見えてきた気がするのでまとめてみました。

なお、小さい単位で PR を出す方針にしている場合は、以降の内容に出てくる commit を適宜 PR で読み替えてもらうと良いかもしれません。

そもそも commit の単位に気を遣った方が良いのか？

コードレビューを行う場合など、他者がコードを読む場合はある程度気を遣った方が良いと思っています。理由は次のとおりです。

コードレビューにおいてレビュアーのレビュー時間が短縮できる
コードレビューの精度が上がる
変更の経緯を追いやすい
自分にとってのメリットになる

どれも結局は「他人（未来の自分も含む）が変更内容を見た時にわかりやすい」に帰着するわけですが、順番に説明していきます。

コードレビューにおいてレビュアーのレビュー時間が短縮できる

変更量が多い場合は commit 単位で変更内容を追いかけるレビュアーもいると思います。ところが、試行錯誤だらけで途中の commit にデバッグコードが入っていたり、最終的に初期の commit の内容の大部分が変わっていたりするようなケースは、commit 単位で変更内容を追いかけることで多大な無駄が生じます。
一方で、commit 単位で見れば「このメソッドはこの処理を行うために導入されて、ここで使われている」ということが即座にわかるかもしれないのに、全体で見ると色んな処理が混ざっているので各変更箇所の関連性を掴むのに時間がかかります。

変更内容をよく理解しているレビュイーが commit の整理にかける時間と、あまり理解していないレビュアー全員が試行錯誤だらけの commit から成る PR のレビューにかける時間、どちらを削った方が良いか？レビュアーが多い場合は前者に時間をかけた方が全体最適化に繋がるでしょう。

コードレビューの精度が上がる

色んな処理の追加・変更が混ざっている膨大な commit は見落としが発生しやすいです。commit を適度に分けることで commit ごとにレビューすべきポイントが明確になり、レビューの精度も上がります。
非常に大きな commit や PR だったから見落としてしまったバグに心当たりのある人も多いと思います。

変更の経緯を追いやすい

既存のコードを読んでいると、必要性のよくわからないコードに遭遇することがあります。commit の単位が適切であれば、git blame でその処理が追加された時の commit を見ることで必要性がわかることも多いです。ところが、その commit に色んな変更が混ざっていると、どの変更に関連してその処理が追加されたのかが不明確になります。

自分にとってのメリットになる

レビューに出す前には自分で自分のコードを確認しますよね？それにかかる時間の短縮に繋がるのはもちろんのこと、確認の精度が上がることでレビューで指摘されそうなことを前もって潰した状態で PR を出せるのも大きなメリットです。コードレビューはレビュイーにとってのコミュニケーションコストもそれなりにあるでしょうから。

また、変更の経緯を追いやすいことも将来的に時間の節約に繋がります。他の人が git blame をしても変更の必要性がわからなければ自分のところに質問が飛んでくることでしょう。そこで即答できればまだ良いですが、即答できなければ経緯を思い出すために時間を使うことになります。

commit をどう分割すべきか？

次の 5 点が重要だと考えます。

リファクタリングと機能追加・変更を分ける
変更の必要性がわかるようにする
変更量が多い場合は適度に分割する
無駄な commit をなくす
どの commit でも最低限動く状態にする

最初は多大な時間を使うでしょうが、慣れてくるとあまり時間もかからなくなるし、どのように実装を進めるか順序立てて考えるクセが付き、手戻りも少なくなると思います。自分の作業手順をそのまま commit にするだけですから。

それでは順番に説明していきます。

リファクタリングと機能追加・変更を分ける

リファクタリングは仕様を変えない変更なので、レビュアーはコードの品質に対してレビューすることに専念できます。ところが、機能追加・変更も同じ commit に混ざると、変更が膨大になる上、リファクタリングの際に意図せず変わったコードなのか、仕様変更によって変わったコードなのか区別するのも難しくなり、バグの見落としにも繋がります。

変更の必要性がわかるようにする

「このメソッドを追加するのはここで使うから」、「この処理が消えるのはこの変更が入るから」等、commit ごとにわかるのが望ましいです。例えば、メソッドの追加だけで利用箇所がないと、その処理自体の妥当性やインタフェースの妥当性がわかりません。
ライブラリ的な立ち位置で、まだ他の箇所から使われないような処理を追加する場合はドキュメントコメントに使用例を書くのが良いと思います。

変更量が多い場合は適度に分割する

1 commit の変更量が多いとレビューの精度が下がることは前述したとおりです。例えば、新機能開発やリファクタリングのように変更量が大きくなる場合は「新機能のうちこの処理を実装した」「特定のメソッドと関連するメソッドを移動した」等で 1 commit にするのが良いと思います。

無駄な commit をなくす

試行錯誤によって意味のなくなる commit をレビューしてもらう必要はありません。別の実装方法も検討した証を残しておきたい場合は、別ブランチにするのが良いでしょう。

どの commit でも最低限動く状態にする

「動作確認してから commit する」や「テストが動く状態で commit する」に言い換えられると思います。
特定の commit で動かない状態だと、レビュアーはそのことを指摘するか、最終的に問題の箇所が直っているかどうかを確認することになります。実装途中で動かないことに気付いたら git rebase でサクッと直すのが良いでしょう。

もし変更量を抑える意図で動かない状態になっているのであれば、TODO コメントなどで後に実装する予定であることを明示するのが良いと思います。例えばメッセージを送信する処理 DeliverMessageJob#run を実装することを考えると、まず次のように run メソッドを実装し、別の commit で build_message, delivery を実装するイメージです。

class DeliverMessageJob
  def run(user)
    message = build_message(user)
    deliver(user, message)
  end

  private

  def build_message(user)
    raise NotImplementedError
  end

  def deliver(user, message)
    raise NotImplementedError
  end
end

アンチパターン

前述した内容に照らし合わせて、よく見るアンチパターンについて言及したいと思います。

コードの移動と修正が混ざっている

「リファクタリングと機能追加・変更を分ける」、「変更量が多い場合は適度に分割する」に関連します。

例えば、あるファイルの中の数百行のコードを別のファイルに移動したとします。その後、移動したコードの中身を数十行書き換えたとします。これが 1 commit で行われると、数百行の削除と数百行の追加で膨大な変更量になるし、コードを移動した後にどのような変更が行われたか把握しづらくなります。

これは、単純な移動の commit と修正の commit に分ければ、2 つ目の commit の内容を集中的に見るだけでよくなるし、見るべき変更量も数十行に抑えられます。

特定のクラスだけを追加している

「変更の必要性がわかるようにする」に関連します。

例えば、特定のリソースに対する CRUD 操作を実装する際に、モデル（リソース）の追加、コントローラの追加、ビューの追加でそれぞれ commit を分けたとします。
そうすると、最初の commit ではモデルのメソッドがどこでどう使われるかわからないので、断片的なコードに対するレビューしかできません。次の commit も同様です。ビューでどういう情報が使われるか、どういうリクエストが飛んでくるかわからなければレビューできません。仮にテストがあっても、そのテストの妥当性がわかりません。

CRUD 操作を実装する場合、次のように commit を分けると、変更量を抑えつつ変更の必要性のわかる状態にできると思います。

index action を実装
- リソースを一覧できるページを動かすのに必要な一連の処理だけを追加する
new action と create action を実装
- 一覧ページに「新規作成」ボタンを追加して、リソースを追加するのに必要な一連の処理だけを追加する
edit action と update action を実装
- 以下同様
destroy action を実装

存在しないファイルに依存している

「変更の必要性がわかるようにする」、「どの commit でも最低限動く状態にする」と関連します。

存在しないファイルに依存していると、そのファイルの内容がわからないのでそれに依存しているコードの妥当性がわかりません。また、レビュアーにとっては、存在しないファイルが単なる追加漏れなのか、どこかで作成される処理があるのを把握していないだけなのか区別がつかず、負担になります。

デバッグコードが入っている

「無駄な commit をなくす」に関連します。

開発中は動作確認のために条件式を変えたり設定値を変えるようなこともあると思います。その場合はレビューに出す前に git rebase でデバッグコードを消して、存在しなかったことにするのが良いでしょう。そこまでしないにしても、せめて TODO コメントや commit メッセージでデバッグコードであることを明示した方が良いです。

最後に

以上、コードレビューの観点に立って個人の考えをまとめてみましたが、共感が得られるものもあれば、開発スタイルに合わないものもあると思います。良さそうなものを取り入れてもらって、チーム開発のスピードが上がれば幸いです！