いきなりだけど、こんなコミット、よく見ない？

（例１とする）

見た瞬間に腕まくりして修正依頼と理由を書き出したくなっちゃう感じ！

これが良くない理由を、持論だけど挙げてみるよ

このコミットについて

コミットは作業単位でするものではない

プルリクエスト（以下 PR）をレビューする側に立って考えるとよくわかるのだけど、
PRレビューをする人が欲しているのは(= masterブランチが欲している、チームが欲していると言っても良い）作業者の作業ログではない。

レビューする人にとって、そのPR作成者が「どれくらいの時間をかけた」とか「休日もコミットを重ねてた」とか
「何ファイルも直さなくちゃあならなくて大変だった」とか「いつどこでブランチを切り換えて自分の出したPRを見てくれていた」とか、
そんなことはぶっちゃけ全く興味がないんだ。

欲しいのは「生成物と成果」であって、それを「どの様な作業工程」で作ったかはどーでも良い。

コミットは実現した事単位で行う

価値のあるコミットと言うのは、何かを実現できる様になる最低単位、もしくはその一部なんじゃあないか、と思っている。

仕様を実現するなり、リファクタリングをするなり、何かしらの要求に基づいてコードの改修をしているはずなので、
その問題を解決する単位がコミットなんじゃあないか、と思っている。

冒頭のPRのコミット群は、せめてこうなっていて欲しい。

（例２とする）

レビューしづらい

2つのスクショを見比べてみると、どちらがPRを(= コミットを）理解しやすいかは明白だ。

前者のコミットだと、「まず最終的に、こいつはこのPR内で何をやったの？」ということが不明瞭だ。
目で関係ないコミットをはじきながら、要求実現に関係しそうなコミットメッセージだけを探さないとならない。

マージされたコミットはリポジトリがある限り、永劫に残る

正直なところ、この様なコミットは「かなり恥ずかしい行為」だと思っている。
マージされてしまったら、よほどのことが無い限りは残り続け、コミッターもマージした人も恥をさらし続けることになる。

まぁそれは許容してしまうのだとしても、実害という観点から見てもやはりマージされて欲しくないコミットだ。

例えばreleaseブランチをmasterに取り込むときとか、過去のコミットを一覧で見るという事がある。
その時にやはり意味の無いコミットが混じっていると見通しが悪いものだ。

まぁそれも許容してしまうのだとしても、最初のPRの問題の本質は「コミットに意味が無い」ことなので、
マージして残すという事は断固としてあってはならない。

少し発展して、差分について

コミットメッセージだけを見ると、良くなった。

次はもう一歩進んで、差分について考えてみようと思う。

実はこのPRは、「本質であるタイプ判定」に加え「クラス名変更」と「パッケージの変更」も行っている。

PRの差分を見てみよう。

コミットメッセージとやったことが一致していない

例２のPRの差分は、こんな感じになっている。

全部削除して、全部新規作成になってしまっているが、そんなはずはないだろう。

これは小さいコミットだから、と「タイプ変更」以外もやってしまうパターンで、良くあるパターンだと思う。
そして、ついでにやってしまうことの内、最も罪深いのが「クラス名変更」もしくは「クラス移動」だ。

何度もPRを出したり、見たりしているとわかるはずなんだけど、それを行ってしまうと差分がこんな風になってしまう。

こんなPRは、見るときに
「え〜と、クラス名はＸ→Ｏで、パッケージが〜〜で、あ、で、タイプ変更ってこれね？」
ってなってしまい、見る方はすごい辛いしレビューの質も落ちる。

これは「1つのコミットで1つを実現する（解決する）」を満たせていないと、すぐ発生する。

そしてまたタチの悪いのは、PR出した側はあまり自分のPRをみないので、なかなか気付かないことだ。
見つけたらちゃんと指摘をしよう。

1つのコミットで1つを行う

次に、最終的な差分は全く同じだが、コミットの仕方を変えた（例３）を見てみたい。

これは「パッケージ変更」と「クラス名変更」と「タイプ判定」を全て別にコミットしている。

PRの差分は当然同じで全削除、全新規なのだが、このPRはコミット単位でレビューすることが出来る。

どちらのレビューがしやすいか、バグを見つけやすいかは一目瞭然だ。

実際には

今回はあくまで例であるけど、本質は実業務でもかわらないと思う。

すこし難しくなるのは、パッケージ変更等を別コミットにするのは当然として、「別PRにするか」だ。
これはもうチーム内での調整とか、さじ加減としか言いようが無い。

例えば「その本質の部分の改修、待ってるから早くマージして欲しい」とか言われたら、あえて同じPRで移動まで済ます必要はない。

他には例えば、他の人のPRを見ていて「あ、このファイル俺これから移動しようとしてたけど、移動と改修 vs 改修はコンフリクトが超辛いやつだ」って
気づけたら、別PRで移動するか、直接話してマージが後の方がそれをするか、とか調整すれば良い。

この辺はどちらかと言うと、PRを出すよりPRを見る事によって得られる経験で出来る様になることだ。

おまけ悪いコミット単位の実例っぽい例

新しい参照APIを作る　というPRだったとする

ﾋﾟｷって来る感じねｗ

「コミットメッセージはお前のツイッターじゃあねーから（怒）」
「お前、これで俺に何をレビューしろ、と？（怒）」

ってなるやつねｗ

これだけ大きいとPR全体差分では絶対にレビューの質が落ちるので、大きい改修の時こそ、コミット単位でレビューが出来る様になっているのが理想だ。
コミットをするときに、自分のPRが最終的にどんな感じになるのか、考えながらコミットすると良い。

おまけ良いコミット単位の実例っぽい例

価値のあるコミットと言うのは、何かを実現できる様になる最低単位、もしくはその一部

もしくはその一部と言うのは、〜〜を実現するにはAしてBしてCしないといけません、のA, B, C ひとつずつのこと

先述の通り、PRは何かを実現するために出されるので、コミットはその必要事項の列挙みたいになるはず。

例えばこう

新しい参照APIを作るには、以下が必要です。
それぞれの改修については、各コミットを参照してください。

ってな感じだ。

レビューしやすそうだね？

具体的な操作

これらを発生させない方法をいくつか挙げてみる。

ここからはもう個人のスタイルなので、軽く、キーワードを載せる程度に。
知らなかったらggって欲しい。

すぐ思いつくのは3つ

stash

見ていると案外多いのが「ブランチを切り換えるためにコミット」ってパターン。

そんなんはgit stashでも使え。

commit amend

直前のコミットに改修を足し込むことができる。

僕は一息付いたりする時は、とりあえず出来たつもりでちゃんとしたコミットメッセージを書いて、
残りはamendでくっつけちゃうってパターンが多い。

ブランチ切り替えにも使えなくもない。

先にコミットだけ作っておいて、そこにポイポイと改修を放り込んでいくイメージ、かな？

rebase -i

これは複数のコミットをまとめることが出来る。

ローカルマシンでは「作業中」「作業中」「作業中」「タイプ判定」みたいにコミットしていても、
PRを出す前にrebase -iして1つのコミットにまとめてくれれば全く文句ない。

けど、僕は面倒だからこれは全くやらない。

ちょっと気をつけていれば、そもそも最初から綺麗にコミット出来るし、なんかあっても大抵はすぐ気付くのでamendで十分だから。

まぁGitなんて案外なんでもどうにかなるんで、イケてないコミットをしない様に気をつけつつ、してしまったら何とかして直そう。
それも訓練になるはずだ。

それでは、ばいばいﾉｼ