コードレビューさせないようにコミット

対象

• 開発フローの中でコードレビューを実施しているひと
• git add -p や git rebase -i でコミットの分割や統合ができるひと

コードレビューさせない

レビュー無しにマージしてもらうために同僚をいかに抱き込むか。
という話ではなく、レビュワーの コードレビューの負荷を下げる ことを意図しています。

無視できるコミットを増やす

どうすればレビュワーがより短時間で自分の書いたコードをレビューできるか。この問に対して、レビューの妨げとなるものを排除する、というアプローチがあると思っています。そのための良い方針だと考えている 無視できるコミットを増やすこと について書きます。

前提

• コミット
  • どのコミットにおいてもテストが通るようにする
• コミットメッセージ
  • ちゃんと Git のスタイルで記述する
    • (Gitのコミットメッセージの書き方 の原則を守る)
• Git の操作
  • 雑にコミットしても、後できれいにコミットをまとめられるように Git の操作に慣れる

コマンドを実行した時のコミットメッセージはコマンドにする

• コマンドによって何が生成されるのか、という知識が持ち回せる
• レビュワーが手で実装した部分のレビューのみに集中できるようになる

コマンドによる生成物は、必ずその生成物のみをひとかたまりにして、コミットすべきです。

たとえば rubygem のスケルトンを作成する bundle gem GEM というデファクトのコマンドがあります。bundle gem my-inf-gem の生成物と、bundle gem your-sup-gem の生成物とは、想像のつく通り、ほとんど同じです。

生成物すべてをひとつのコミットとし、 コミットメッセージをそのコマンド にします。

(例: git commit -m'bundle gem my-inf-gem')

bundle gem を実行したことのあるレビュワーにとっては、自分の知っている bundle gem の結果との違いが無いかを確認すればよいことになります。そのため、そのコミットのレビューは非常に短時間で済むはずです。
(bundler のバージョンによって若干テンプレートは違いますが、些細なことです。)

生成物と手で実装したものを一緒のコミットにしてレビューを依頼するというのは、カレーをかき混ぜて渡すようなものです。やってはいけません。

同じようなコミットには同じようなコミットメッセージを書く

例えば誤字の修正をするコミットが並んだとき、どちらが読みやすいかは明らかでしょう。

bad

タイポを修正しました
単語の間違いを修正
fix typo
s/usre_id/user_id/

good

タイポを修正
タイポを修正
タイポを修正
タイポを修正

後者の方が一目で、どのコミットも同じように振る舞いに変更のないタイポの修正なのだろうとわかります。
もちろん、タイポによって起きたバグを修正する場合は、コミットメッセージにバグ修正であることを書くべきでしょう。

似たコミットは違いが少ないためレビューが楽になります。コミット同士が似ていることを伝えるために、コミットメッセージを有効に使っていきましょう。

また、過去のコミットメッセージに倣って書くことで、過去のコミットと似た変更を加えたという意味を伝えることができます。逆に、過去と異なるコメントを書くことで、何かが違っていることを表明することも出来るでしょう。
git log --oneline /path/to/file で、過去のコミットメッセージをざっと眺めながら、 スタイルを汲み取ってコミットメッセージを書く、というのをおすすめしたいです。

無視できる中間コミットを作る

たとえば、次の Ruby のコード。

before.rb

numbers = {
  one:   1,
  two:   2,
  three: 3,
}

これに key が twenty-four で value が 24 の要素を追加することを考えます。
twenty-four のように - を含む場合、上のシンボル省略記法が使えないため、普通は Hash rocket (=>) の記法へと他の全てを合わせます。結果、次のようになります。

after.rb

numbers = {
  :one           => 1,
  :two           => 2,
  :three         => 3,
  :'twenty-four' => 24,
}

before から after の diff は次のようになります。

before_to_after.rb

 numbers = {
-  one:   1,
-  two:   2,
-  three: 3,
+  :one           => 1,
+  :two           => 2,
+  :three         => 3,
+  :'twenty-four' => 24,
 }

この before, after のコミットの間に次のコードへと変更する中間コミットを作りましょう。

intermediate.rb

numbers = {
  :one   => 1,
  :two   => 2,
  :three => 3,
}

これにより、

• before から intermediate
• intermediate から after

のそれぞれの diff がシンプルになります。

before_to_intermediate.rb

 numbers = {
-  one:   1,
-  two:   2,
-  three: 3,
+  :one   => 1,
+  :two   => 2,
+  :three => 3,
 }

intermediate_to_after.rb

 numbers = {
-  :one   => 1,
-  :two   => 2,
-  :three => 3,
+  :one           => 1,
+  :two           => 2,
+  :three         => 3,
+  :'twenty-four' => 24,
 }

コミットは増えますが、before から intermediate は動作の変更がないため、レビュワーはこのコミットを ほぼ無視できます。 そして、intermediate から after への「実際にやりたいこと」だけを注意してレビューすればよいことになります。

今回は after をまずは書いて、そこから intermediate という挙動に変更のない部分のコミットを切り出しています。

「ひとつのコミットはひとつのことをする」とよく言いますが、はじめからやるのはむずかしいです。こんな風に after まで雑に細かくコミットしながら進めていき、後できれいにまとめるのが良いと思っています。レビューの依頼を出す直前に分割・統合して整える方が楽だと思います。

リファクタリングのコミットを commits の先頭にまとめる

「無視できる中間コミット」が複数できたときは、git rebase -i でそれらを先頭に移動してしまいましょう。 (下は rebase の指示をイメージ)

before

pick f7d09f1 Aの行末の空白を削除      ...(1)
pick e969c39 Aに機能を追加           ...(2)
pick b7d6449 Bのインデントずれを修正   ...(3)
pick bd2b63f Bがアレするように変更    ...(4)
pick 35787da Cの変数名をコレに変更    ...(5)
pick 0c8885d CとDの処理を統合        ...(6)

after

pick f7d09f1 Aの行末の空白を削除      ...(1)   
pick b7d6449 Bのインデントずれを修正   ...(3)
pick 35787da Cの変数名をコレに変更    ...(5)
pick e969c39 Aに機能を追加           ...(2)
pick bd2b63f Bがアレするように変更    ...(4)
pick 0c8885d CとDの処理を統合        ...(6)

はじめに無視できるコミットをまとめておくことで、レビュワー視点でコミットを見た場合に、何をレビューするかという意識のスイッチ回数が減ってメリハリがつきます。振る舞いの変わらないことが正となるコミットが途中で挟まると、集中力が分断されます。上の場合だと 「振る舞いが変わっていないこと」 と 「振る舞いが変わっていること」 を交互にレビューする必要があるからです。

before の方が編集したファイルという点ではコミットが並んでいるため綺麗に見えます。
しかし、after の方がレビューのしやすさという点では優れていると思っています。

また、「同じようなコミットには同じようなコミットメッセージを書く」ことでより効果がわかりやすくなると思います。例えばリファクタリングした時のメッセージをできる限り統一していくことで commits を全体で見たときに、前半はリファクタリングが中心で、後半が振る舞いを変更するコードなのだなというのがわかりやすくなると思います。

improved

pick f7d09f1 スタイル修正: Aの行末の空白を削除
pick b7d6449 スタイル修正: Bのインデントずれを修正
pick 35787da リファクタリング: Cの変数名をコレに変更
pick e969c39 Aに機能を追加
pick bd2b63f Bがアレするように変更
pick 0c8885d CとDの処理を統合

おわりに

この記事では、 無視できるコミットを増やす という切り口でレビュワーに負担をかけないようにする、次の方法を紹介しました。

• コマンドを実行した時のコミットメッセージはコマンドにする
• 同じようなコミットには同じようなコミットメッセージを書く
• 無視できる中間コミットを作る
• リファクタリングのコミットを commits の先頭にまとめる

相手が理解しやすい形で変更内容を伝えるための方法は他にもたくさんあると思います。自分はこういうところに気をつけているよ〜という情報があれば、ぜひコメントなどで教えていただきたいです。