調査をサクサク進めるために。伊藤淳一が考える「良いプルリクエスト、悪いプルリクエスト」

• keyboard_arrow_down はじめに
• keyboard_arrow_down TL;DR （最初にまとめ）
• keyboard_arrow_down なるべく小さいプルリクエストにすること
• keyboard_arrow_down プルリクエストのタイトルを具体的かつわかりやすく書くこと
• keyboard_arrow_down プルリクエストの説明を具体的かつわかりやすく書くこと
• keyboard_arrow_down 無関係なdiffを含めないこと
• keyboard_arrow_down ドラフト（WIP）のプルリクエストを早めに作成すること
• keyboard_arrow_down 無駄に細かいコミットを積み重ねないこと
• keyboard_arrow_down amendオプションを活用する
• keyboard_arrow_down コラム：テキストだけでやりとりせず、直接話すのも効果的
• keyboard_arrow_down まとめ

前回の記事ではgitのコミットに焦点を当て、良いコミットはどうあるべきかを議論しました。

しかし、業務で開発しているプログラムに対して変更を加える場合、いきなりmainブランチにコミット＆プッシュするケースはまれだと思います。みなさんの現場でもほとんどの場合、以下のようなワークフロー（もしくはこれに類似した手順）で変更を適用しているはずです。

1. 1. mainブランチから開発用ブランチを作成する
2. 2. 開発用ブランチで開発を進める（＝コミットを重ねる）
3. 3. 開発用ブランチからmainブランチにマージするためのプルリクエスト（プルリク、PRとも呼ばれる）を作成する
4. 4. 他の開発者にプルリクエストのコードをレビューしてもらう
5. 5. 他の開発者がプルリクエストを承認（approve）したらmainリポジトリにマージする

つまり、プログラムを修正する最小の単位はコミットですが、それをまとめた一段階大きな単位としてプルリクエスト（略称はプルリクやPR）があります。

そこで今回の記事ではコミットではなくプルリクエストに焦点を当て、良いプルリクエストと悪いプルリクエストについて議論してみようと思います。

前回の記事と同様、対象読者はまだ業務に入って間もない新人エンジニアですが、新人さんだけでなく、開発チームで新人エンジニアを指導しているリードエンジニアのみなさんも、チーム内で「良いプルリクエストとは何か？」を議論する叩き台として本記事を活用してもらえると嬉しいです。

なお、この記事の本文には前回書いたコミットの記事を読んでいる前提の箇所がいくつかあります。まだ読んでいない方は先にそちらを読んでからこの記事に戻ってきてください。

おことわり

コミットに関する話題もそうでしたが、プルリクエストに対する考え方もまた、組織の文化の影響を受けやすいと思います。本記事では筆者のこれまでの開発経験をもとに「こういうプルリクエストがいいのでは？」という提案をしていきますが、もし「ウチのチームと考え方が全然違う！」という内容があれば、そちらの考え方を優先してもらっても大丈夫です。

ちょっと本文が長くなってしまったので、筆者の考える良いプルリクエストの条件を先に挙げておきます。

1. 1. なるべく小さいプルリクエストにすること
2. 2. プルリクエストのタイトルを具体的かつわかりやすく書くこと
3. 3. プルリクエストの説明を具体的かつわかりやすく書くこと
4. 4. 無関係なdiffを含めないこと
5. 5. ドラフト（WIP）のプルリクエストを早めに作成すること
6. 6. 無駄に細かいコミットを積み重ねないこと

具体的にどういうことなのかは、ここから下で詳しく説明していきます。

最初に検討したいのはプルリクエストの大きさです。

プルリクエストの大きさは変更されたファイルの数（File changed）や、追加・削除された行数で定量化できます。レビューする側の立場からすると、大きすぎるプルリクエストのコードレビューは時間がかかるので非常に気が重くなります。

絶対的な基準があるわけではありませんが、筆者は経験的に以下の範囲に収まっていれば許容範囲かなと考えています（注：筆者が携わっているのは主にRuby on Railsを使った開発プロジェクトです。使用する言語やフレームワークによって望ましい数字は前後すると思います）。

• ・File changedは30ファイル以下
• ・追加された行数は300行以下

 

もちろん、プルリクエストの内容によっては単純な修正でもdiffが大きくなったり、逆にdiff自体は小さいのにレビューにはとても苦労したりするケースがあるので、必ずしも「diffは小さい方が良い、大きいと悪い（小さいとレビューが楽、大きいとしんどい）」とは言えません。ですが、概してプルリクエストの大きさとレビューの大変さは比例します。

レビューを受ける側も、大きなプルリクエストほど指摘コメントがたくさん付くので、コードの修正やコメントのやりとりに苦労すると思います。diffが小さければそのぶんコメントの量も減るので、レビュー後の修正作業が楽になるはずです。

大きな修正はプルリクエストを分割すべし

しかし、大規模な新機能開発があったりすると、さすがに「diffは300行以下に抑えてください」とは言えません。こういう場合は、プルリクエストをいくつかのフェーズに分割しましょう。具体的には以下のような手順で開発します。

1. 1. mainブランチから開発用ブランチAを作成する
2. 2. 開発用ブランチAから、さらに開発用ブランチA-1を作成する
3. 3. 開発用ブランチA-1でキリのいいところまで開発する
4. 4. 開発用ブランチA-1から開発用ブランチAにマージするためのプルリクエストを作成する
5. 5. コードレビューが終わってapproveされたら、A-1をAにマージする
6. 6. 続いて開発用ブランチAから、開発用ブランチA-2を作成する
7. 7. （以後、必要に応じて同じ手順でA-2、A-3・・・を順に開発用ブランチAにマージする）
8. 8. 新機能の開発が一通り終わったら開発用ブランチAからmainブランチにマージするためのプルリクエストを作成する
9. 9. mainブランチへマージする前の最終チェック（コードレビュー）をしてもらう
10. 10. コードレビューが終わってapproveされたら、開発用ブランチAをmainにマージする

この手順を図で表すと、以下のようなイメージになります。

開発用ブランチA-1やA-2は適当な単位で分割して、なるべくdiffを小さくするのがポイントです（たとえばA-1は主に管理画面を開発し、A-2では主にエンドユーザー画面を開発する等）。

mainブランチへマージする直前のプルリクエスト（上の手順でいうと手順8で作成したプルリクエスト）のdiffはかなり大きくなってしまいますが、基本的にどのコードもレビュー済みなので、最後はざーっと見て気になる点がなければOK、という程度で終わらせても問題ありません。

これは言わば「プルリクエストの小口化」です。大きなプルリクエストを最後にどかん！とレビューしてもらうのではなく、キリのいいタイミングで「まだ続きがあるけど、いったんここでレビューお願いします！」というふうに、レビューしやすいサイズにプルリクエストを分割しましょう。

前回のコミットの記事では、突然発生した不具合の原因調査をするために直近のコミット一覧を見て怪しいコミットがないか調査する、という話を書きました。

これと同じことはプルリクエストにも言えます。つまり、突然発生した不具合の原因調査をするために、直近にマージされたプルリクエストの一覧を見て、怪しいプルリクエストがないか調査する場合があります。

このとき、プルリクエストのタイトルが具体的でわかりやすいものになっているほど、調査がしやすくなります。たとえば、「商品画面の修正」ではなく、「商品の詳細画面に各種SNSのシェアボタンを設置」と書いてあった方がより具体的で調査を進めやすくなる、といった具合です。

プルリクエストの入力フォームはタイトル欄と説明欄が分かれています。

タイトルだけでなく、説明欄も具体的かつ、わかりやすく書きましょう。たとえば説明欄には以下のような情報が載っていることが望ましいです。

• ・プルリクエストの概要（このプルリクエストはいったいどういうプルリクエストなのか）
• ・変更の発端となった開発チケットのURL
• ・UIに関連する変更であれば、画面のスクショ（複雑なUI制御を伴う場合は動画になっているとなお良い）

たとえば、「自分の日報に言及している日報を表示する」というタイトルのプルリクエストの説明欄は以下のように記述します。

## 概要
自分の日報のURLが他の日報の本文に含まれている場合、その日報を「この日報に言及している日報」として表示します。

## 開発チケットのURL
https://github.com/your/repository/issues/33

## スクリーンショット
<img width="707" alt="img" src="https://example.com/screenshot.png">

上の説明欄をGitHub上で表示すると以下のように表示されます。

また、必要に応じて以下のような情報も載せておくと、レビュアーに喜ばれるかもしれません。

• ・特に注意してレビューしてほしい部分（またはあまりちゃんと見なくても大丈夫な部分）
• ・環境変数の追加やデータ移行スクリプトの実行など、注意すべきリリース手順
• ・初めて導入した新しい技術や特殊なライブラリの存在（必要であればその技術の概要や注意点も書く）
• ・今回はやむを得ず後回しにした対応
• ・レビュアーの質問や指摘が予想される箇所に対する事前説明
• ・その他、レビュアーに伝えたい連絡事項

こうした情報の一番の目的はレビュアーに対する情報提供ですが、それだけでなく「大きなレベルのコミットメッセージ」として、将来役に立つ可能性があります。すなわち、数年後に「いつ、誰が、なぜ、こんなコードの変更を入れたのか？」を調査する過程で、未来の開発者があなたの作ったプルリクエストを見に来るかもしれない、ということです。

前回の記事で紹介したように、GitHubではコミットの詳細画面からそのコミットが含まれるプルリクエストにジャンプすることができます。

たとえばコミットメッセージにはあまり詳細な情報が書いてない場合でも、そのコミットが含まれるプルリクエストを検索したときに詳しい情報がプルリクエスト内に書かれていれば、未来の開発者の「いったいなぜ？」に対する答えになるかもしれません。実際、筆者も5年以上前に作成されたプルリクエストを調査して「よくわからない謎コード」が導入された理由を発見したことがあります。

コミットメッセージと同様に、プルリクエストもまたそのプログラムの歴史そのものになり、場合によっては古文書のような役割を担うことがある、ということを押さえておきましょう。

前回のコミットの記事では、変更の目的とコミットが1対1で紐付くようなコミットを作成すべし、という話を書きました。

これと同じことがプルリクエストにも言えます。つまり、ある新機能をリリースするためのプルリクエストを作成したのであれば、そのプルリクエストに含まれるdiffはすべてその新機能に関連するdiffのみで構成されるべき、ということです。「たまたま発見した全く無関係なファイルのインデント修正」はその新機能のプルリクエストに含めるのではなく、別のプルリクエストとして作成しましょう。

なぜなら、コミットと同じようにプルリクエストもまた、マージ済みの変更をまとめてrevertする際に使われることがあるからです。新機能導入の変更をrevertしたいのに、同じプルリクエストにそのdiffが含まれていたためにインデントの修正まで元に戻ってしまった、というような事態は極力避けたいです。ゆえに、プルリクエストは意味のある単位でまとめ、プルリクエストの主題に合致しない無関係な修正は、別のプルリクエストを作成するようにしてください。

開発タスクをアサインされてからプルリクエストを作成するまでの一番シンプルなワークフローは以下のようになると思います。

1. 1. mainブランチから開発用ブランチを作成する
2. 2. ローカル環境でコードを修正してコミット＆GitHubにpushする
3. 3. 開発が完了するまでコミット＆pushを繰り返す
4. 4. 最後まで開発したらプルリクエストを作成し、レビューを依頼する

上のワークフローでは一番最後にプルリクエストでは、一番最後にプルリクエストを作成しています。ですが、完成までに数日かかるような大きなプログラム修正を実施する場合は、早めにドラフトのプルリクエストを作るのがお勧めです。

具体的には以下のような手順になります（手順3に注目してください）。

1. 1. mainブランチから開発用ブランチを作成する
2. 2. ローカル環境でコードを修正してコミット＆GitHubにpushする
3. 3. 1回目のpushが終わった時点で、開発用ブランチからmainブランチにマージするためのプルリクエストをドラフトで作成する（←ここがポイント！）
4. 4. アサインされたタスクが完了するまでコミット＆pushを繰り返す
5. 5. 一通り完了したらプルリクエストをドラフトから通常のプルリクエストに変更し、レビューを依頼する（プルリクエストの説明もこのタイミングで丁寧に書く）

プルリクエストをドラフトで作成するためには、プルリクエスト作成用ボタンのプルダウンメニューから”Create draft pull request”を選択します。

ドラフト状態のプルリクエストには”Draft”のバッジがつきます。

ドラフトから通常のプルリクエストに変更する場合は”Ready for review”ボタンをクリックします。

ドラフトのプルリクエストを作成するメリットは、作業の進捗や途中経過が周りの人からわかりやすくなることです。たとえば、先輩プログラマがあなたのドラフトプルリクエストをチラッと覗いて、「そこはこういうふうに実装した方がいいよ」とアドバイスをくれるかもしれません。また、自分自身が現時点で自分がどんな修正を加えたのか振り返りたいときにも、ドラフトプルリクエストがあればさっとdiffを確認することができます。もちろん、ドラフトプルリクエストはあくまでドラフトなので、そのプルリクエストは完成版ではなく「現在作業中」であることも周りの開発者に伝わります。

なお、現場によってはドラフト状態のことをWIP（ウィップ、Work In Progress）と呼ぶこともあります。こちらの呼び方も知っておくと良いでしょう。

小規模な修正や簡単な不具合修正であれば1プルリクエストにつき1コミットで済むかもしれません。一方、ある程度の規模の仕様変更や新機能追加を行う場合は、以下のように複数件のコミットが1プルリクエストに含まれるはずです。

• ・○○機能用の管理画面を作成
• ・エンドユーザー画面に○○機能を追加
• ・テストコードを追加
• ・実行ボタンのpaddingを微調整
• ・未ログインユーザーのアクセス時に発生するエラーを修正

さらに、コードレビューで指摘があった場合は、以下のようなコミットがさらに追加されるケースもあるでしょう。

• ・eachではなくmapを使うように修正
• ・在庫が0の場合のテストを追加

個人的には1プルリクエスト内のコミット数がこれぐらいで収まっているなら許容範囲かなと思います。

ただし、コミット数があまりに多すぎる場合はちょっと問題です。前回の記事にも書いたとおり、あなたのコミットは将来他の開発者が「なんでこんなコードを書いたんだろう？」と疑問を覚えたときに調査の対象になる可能性があるからです。

このとき、git blameを使えば過去のコミットを遡って、ある変更が導入されたコミットを追跡することができます。たとえば、以下の画面操作ではLOGO_SIZEという定数が導入されたコミットに到達するまで、3回コミットをさかのぼっています。

こういったケースでは軽微な修正コミットが積み重なるほど、目的のコミットに到達するまでたくさんのコミットをさかのぼる必要が出てきます。

そこで、こうした問題を避けるために、無駄に細かいコミットを積み重ねないようにするためのTipsをいくつか紹介します。

gitではコミット時に--amendオプションを付けることで直前のコミットを修正することができます。--amendオプションの典型的な用途は直前に書き込んだコミットメッセージの修正です。

# amendオプションを付けて直前のコミットメッセージを修正する
git commit --amend -m "実行ボタンのpaddingを微調整"

ですが、筆者はたまにコミット履歴を減らすために--amendオプションを使います。

たとえば「○○機能用の管理画面を作成」というコミットを作成するために、「アレをして、コレをして、ソレをして全部完成したら最後にコミット」という手順にすると、「やっぱりアレが終わった時点にロールバックしたい」と思っても、その時点のコミットがないのでロールバックできません。しかし、「アレをしてコミット、コレをしてコミット、ソレをしてコミット」とその都度コミットしていくと、ただでさえ増えがちなコミット履歴がさらに増えてしまいます。

そこで活用したいのが--amendオプションです。--amendオプションを以下のような手順で使うと、コミットを1つだけに抑えることができます。

1. 1. アレを作り終えたらコミット（通常のコミット）
2. 2. コレを作り終えたら--amend付きでコミット（直前のコミットがアレ＋コレに変わる）
3. 3. ソレを作り終えたら--amend付きでコミット（直前のコミットがアレ＋コレ＋ソレに変わる）

こうするとコミット履歴は増やさずに、何かあればいつでも1つ前のステップにロールバックできる状態を作り出すことができます。

ただし、すでにGitHubにpush済みのコードを--amendで修正すると、そのままではpushできなくなります。これはamendを使うとコミットは増えないものの、コミットIDはamendするたびに変わってしまう（＝コミット履歴が改変される）ためです。もしpushしようとしてrejectされた場合は、--force-with-leaseオプションを付けてpushしてください。

# amendしたコミットをpushしようとするとrejectされる
$ git push
To github.com:JunichiIto/git-commit-sandbox.git
 ! [rejected]        initial -> initial (non-fast-forward)
error: failed to push some refs to 'github.com:JunichiIto/git-commit-sandbox.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. If you want to integrate the remote changes,
hint: use 'git pull' before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

# force-with-leaseオプションを付ければpushできる
$ git push --force-with-lease
Enumerating objects: 5, done.
Counting objects: 100% (5/5), done.
Delta compression using up to 8 threads
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 352 bytes | 352.00 KiB/s, done.
Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
To github.com:JunichiIto/git-commit-sandbox.git
 + c7c597e...6b123d5 initial -> initial (forced update)

試行錯誤しそうな予感がしたらブランチを分ける

たまに自分のローカル環境ではエラーにならず、CI（GitHub ActionsやCircleCI等）でしかエラーが発生しないことがあります。

CI上でしか発生しないエラーに対処するには、原因や解決策を探るためにCI上で試行錯誤するしかありません。しかし、そうするとコミット履歴が無駄に増えていってしまいます。こういうケースでは以下のような手順で試行錯誤用のブランチとプルリクエストを作るのがお勧めです。

1. 1. 試行錯誤用のブランチ＆プルリクエストを作る（プルリクエストを作成するとCIが動き出すプロジェクトを想定）
2. 2. 落ちるテストだけを実行対象にする
3. 3. エラーが解決するまで思う存分試行錯誤する
4. 4. 問題が解決したら、元のブランチで変更を適用する
5. 5. 試行錯誤用のブランチとプルリクエストは削除する

こうすればコミット履歴上はあたかも一発で問題を解決したかのように見せることができます。

ここではCIのエラー解決を例に挙げましたが、CIのエラーに限らず、「これは試行錯誤のコミットが増えそうだ」と思ったら試行錯誤専用のブランチを作ることを検討してみてください。

目に余る状態であればスカッシュする

gitには複数のコミットを1つにまとめるスカッシュ（squash）という機能があります。

「夢中で開発してたら、まとまりのないぐちゃぐちゃのコミット履歴ができあがってしまった💧」という場合は、スカッシュを使ってコミットをまとめるのも良いかもしれません。

ただし、スカッシュはコミット履歴の改変になるため、コンフリクトの解決手順が複雑になったりする場合があります。ですので、ある程度gitの知識がないと使いこなすのは難しいかもしれません。かく言う筆者も実は上手に使いこなせていないので、スカッシュを使うことは滅多にないです😅

いちおう、スカッシュという手段はあることは頭の片隅に置いておいて損はないですが、普段からなるべく無駄に細かいコミットを積み重ねないことを意識して、スカッシュに頼らなくて済むようにするのが良いと思います。

参考：「1プルリクエストにつき1コミットを厳守」というプロジェクトもある

なお、Ruby on Railsのようなオープンソースプロジェクトでは、積極的にスカッシュを使って「1プルリクエストにつき1コミット」を保つように求められるケースもあります。

プルリクエストは、1つのコミットにまとめておくことが望まれます。コミットを1つにまとめることで、新しい変更を安定版ブランチにバックポートしやすくなり、よくないコミットを取り消しやすくなり、Gitの履歴も多少追いやすくなります。Railsは巨大プロジェクトであり、不要なコミットが増えすぎると膨大なノイズが生じる可能性があります。
引用元: 「Ruby on Rails に貢献する方法 – Railsガイド」

1プルリクエストにつき1コミットを厳守するか、1プルリクエストにつき複数コミットを許容するかは、そのプロジェクトのルール次第です。筆者が参加しているプロジェクトではいずれも後者のスタイルを採用していますが、読者のみなさんの中には前者のスタイルで開発しているケースもあるかもしれません。その場合は「郷に入れば郷に従え」で、コミットをスカッシュするようにしてください。

プルリクエストを介したコメントのやりとりは通常テキストベースになると思います。ですが、プルリクエストの内容や規模によっては、以下のようにレビュアーと直接話す機会を設けるのも効果的です。

• ・プルリクエストの概要やレビューの観点を口頭でより詳しく説明する
• ・コードレビュー実施時に同席してリアルタイムに質疑応答する

特に、レビューする側もされる側も、テキストだけのやりとりになると、ニュアンスがうまく伝わらずに相手をいらつかせてしまうこともよくあります。「コメント欄がなんかイヤな空気になってきたな」とか「これはテキストでうまく説明しようとすると骨が折れそうだ」と思ったら、リアルタイムに話す機会を設けましょう。そうすればそれまでの問題があっさりと解決したりします。

というわけで、本記事では筆者の開発経験を交えながら良いプルリクエストと悪いプルリクエストについて議論してみました。

前回のコミットの記事もそうですが、コミットもプルリクエストの作成も日常的に行う作業でありながら、わざわざ「どんなコミットがいいのか？」「どんなプルリクエストがいいのか？」と議論する機会はあまりないと思います。コミットやプルリクエストを学ぶと言っても、せいぜいレビュー時に先輩から「こうした方がいいよ」と注意されて少しずつ正しいやり方を覚えました、というパターンが多いのではないでしょうか。

前回の記事と今回の記事では現場の暗黙知になりがちな「コミットとプルリクエストのお作法」について、筆者なりの視点でなるべく丁寧に説明してみました。冒頭の「おことわり」にも書いたように、組織の文化やルールによっては本記事の内容をそのまま採用できない部分があるかもしれませんが、良いコミットや良いプルリクエストをチーム内で考えるための叩き台として、本記事を活用してもらえると幸いです！