Pull Requestを分割する技術 〜レビューで疲弊しない/させないために〜

何故 Pull Request(以下、PR) を分割するのか？

• レビュアーを疲弊させないため
• 自分が疲弊しないため
• レビューの精度/速度を上げるため

PRを分割しないと、何が起きるのか

時折、それなりに規模の大きな機能を実装する必要がでてきたり、当初はそこまででもないと思っていたが、いざ実装し始めてみたら色々必要になり、結果大規模な作業内容になった、というケースが出てくる。

そうした状況で、諸々の作業内容をまとめて一つのPRとして作成すると、一旦どんな問題が起こるか。
以下、レビュアー/レビュイーの視点からまとめてみる。

レビュアーの視点から

1. 追加/変更ファイル数が多い

• 単純に圧倒される
• ファイル変更に伴う依存関係のチェックが大変
• どこからどこまで変更が入ったのか、影響範囲がどの程度か、把握が大変

2. diffの量が数百行有って辛い

• 単純に死ぬほど見づらい
• diffが多いと目が滑る
• 変更箇所の影響範囲を考え始めると、要求される脳内メモリ容量が膨大になる
• 変更内容によっては修正前後のコード表示位置が離れる場合もあり（関数をたくさん追加したとか）、比較しにくいことが有る

3. 体力消耗と、それに伴い時間がかかる

• レビュー時に把握/確認しておくべき情報が多いので、どうしても体力を消耗する
• 体力を消耗しながらのレビューになるので、時間がかかる
• 体力消耗により見落としも発生しやすい
• 時間が掛かる分、レビュアー自身の作業リソースも逼迫する

レビュイーの視点から

1. レビュー対応が大変

• 規模が大きい分、よほど上手に実装できていない限り、一度のレビュー指摘の数も多くなる
  • PR３〜4個分のレビュー指摘が一度に振ってくる感じ
• レビュアーが変更内容を把握しきれていない場合、依存関係や構造、設計意図に対する質問/確認を都度回答する必要がある

2. レビュー結果が返ってくるまでに時間がかかり、タスクがなかなか消化できない

• レビュアーも消耗するので、返ってくるまでに時間がかかる
• 必然、自分のタスクもなかなか消化できず、進捗報告の時に肩身の狭い思いをする

PRを分割すると何が嬉しいのか

各論としては、大体前節の裏返しとなる。
レビュアーは

• 見るべきdiffの量が減る
• 一度に考える量が減る
• 体力も消耗しない
• 細かいところまで、余裕を持って確認できる

レビュイーは

• 一度に対応すべきレビュー指摘の量が減る
• 必要なやりとりの量が減る
• レビュー結果が早く返ってくるので、タスク消化しやすい

といった恩恵が得られる。
結果、冒頭に記載した

• レビュアーを疲弊させない
• 自分が疲弊しない
• レビューの精度/速度を上げる

という効果を得ることができる。

PRを分割するためのテクニック

では、どうやってPRを分割すれば良いのか。
やたらめったらPRを細かくすれば良いというものではなく、レビュアーにとっても、PRの内容としても、意味のあるまとまりとして整理されていなければならない。

以降では、個人的に意識/実践している具体的なtipsを紹介していく。

タスクに含まれている作業を洗い出しておく

「機能Xを実装する」というタスクを進めるとして、そのために必要な内容を事前に整理し、その結果を基にPRを作成する。
例えば「Webサイトの更新情報をTwitterに自動連携する」という機能だった場合

1. Twitterに任意の文字列を投稿する機能
2. Webサイトの更新情報を、一つのデータとしてまとめる機能
3. 2.の機能で生成したデータを、SNSシェア用の文字列に変換する機能
4. 3.の機能で生成した文字列を、1.の機能に渡す手続き

が必要そうだ。考え出すと他にもどんどん出てくるかもしれないが、設計や分析は主眼ではないため、↑が要素の全てということにする。

こうして事前にサブタスクを分類したら、それらに対応したブランチを作成し、それぞれ作業が終わったタイミングでPRを作成する。
結果、

• タスクX実装/Twitter文字列投稿機能
• タスクX実装/更新情報データ生成機能
• タスクX実装/更新データの文字列変換機能
• タスクX実装/更新データ文字列のTwitter投稿

というようなブランチと、それらに対応するPRがそれぞれ出来上がる。
こうすると、作業者も作業の見通しが立てやすいだけでなく、一つあたりのPRサイズも抑えられ、レビュアーも各機能にのみ集中してレビューできる。

PRのマージ先を、機械的に開発用ブランチに設定しない

PRを作る時、develop等の開発用ブランチをマージ先に指定することが多いと思う。
実際、1つのPRで十分完結する内容で、かつそれが疲弊しない程度の小規模であれば問題無い。

しかし、PRの分割を行うということはそれだけ細かくブランチを分割するということ。
この時、機械的にマージ先を開発用ブランチに設定してしまうと、後続のブランチになるほどdiffが大きくなってしまい、当初の目的を達成できない。

diffを必要最小限に抑えるため、PRのマージ先は「ベースとなったブランチ」にすること。

• developから作成したブランチAに対するPRの場合
  • マージ先はdevelop
• ブランチAから作成したブランチBに対するPRの場合
  • マージ先はブランチA
• ブランチAから作成したブランチCに対するPRの場合
  • マージ先はブランチA
• ブランチBから作成したブランチDに対するPRの場合
  • マージ先はブランチB

リファクタ/変更/追加でPRを分ける

事前にサブタスクを整理しておいたとしても、実装を進めている内に
「この部分、設計が汚くて利用しづらいからリファクタしよう」
「この部分、今回の対応で変更しよう」
「今回の対応でこのコード再利用したいから、共通モジュール作ろう」
と、色々な作業が途中で発生すると思う。
余裕や作業フローの問題で、事前のサブタスク整理自体うまくできなければ尚の事。

そういう場合には、見つかった作業内容をリファクタ/変更/追加/削除に分類して

• ひたすらリファクタのコミットを積み重ねるブランチ
• リファクタの内容を基に、変更を行うブランチ
• 処理の共通化のため、共通モジュールの抽出を行うブランチ
• 整理の結果、不要になった部分を削除するブランチ

をそれぞれ作成し、それぞれでPRを作成する。
一般に、リファクタリングの時は「実装の帽子をリファクタリングの帽子に被りなおす」と言われるが、ブランチを分けることで強制的に帽子が切り替わる。
また、内容に応じてブランチを分けることで「今、自分は何をしているか/何を目指しているか」が明確になる。

レビューの際にもPRのスコープがリファクタリング/変更/追加/削除とそれぞれ明確になるため、レビューがしやすい。

なお、これらのブランチは順不同かつ複数回作成することも有る。
例えば、

1. リファクタブランチ
2. 変更ブランチ
3. リファクタブランチ
4. 追加ブランチ
5. 削除ブランチ

等。この場合、追加作業の前にリファクタした方が良い箇所が有ることに、変更後に気づいたケース。

git reset を活用する（PR作成前のみ）

作業内容でPRを分けるといっても、結構進んだ後になって「あ、これもやらなきゃ」と気づくことは多い。
↑のように全て整頓した状態で作業できていれば良いが、実際にはそうはいかないことも多い。割と前段の方でやっておいた方が良い内容に、後段になってから気づくことも結構有る。

そういう場合には、最初の状態まで一気に git reset --mixed でコミットを巻き戻してしまう。
その後、変更内容を確認して

• リファクタのdiff
• 変更のdiff
• 追加のdiff
• 削除のdiff

とdiffの内容を整理し、改めてブランチとコミットを組み立て直す。
手間はかかるものの、全容を把握した上でコミットを組み立て直すので、キレイなPR分割を実現しやすい。

そこそこ大きなタスクだと、おおよそ1〜2回程度は git reset を使っている。
サブタスクを事前に抽出しきれていれば良いものの、リファクタ系は中々難しいので、中規模程度のタスクでもたまに使う。
小規模は、大体作業開始する前にほぼ内容が見えていたり、小規模すぎてPR分割の恩恵がほぼゼロだったりするので、まず使わない。

なお、既にPUSH済みのブランチについては、諦めて新たにPRを積み上げること。
あくまで、PR作成前のローカル作業中の段階でのお話。

PRで表示されるdiffをイメージする

ちょっとふわっとした内容になるが、すべての基本になるのがコレ。

• 「これもリファクタだけど、機能XのリファクタPRなのに機能Yのリファクタが混ざったらビックリするかな」
  • →「機能Xのリファクタと機能Yのリファクタで、PRを分けよう」
• 「機能Yを実装するにあたってZという関数を作ったけど、結構Zの実装とテストが大きくなっちゃった。Zの追加diffが大きくなりそうだな」
  • →「Zの実装/テストで一つのPRにして、それに積み上げる形で機能YのPRを作ろう」

etc...

シチュエーションとそれに対する解法は色々と考えられるが、詰まる所 「PRを見る人のことを考える」 に尽きると思われる。

ここまで紹介したテクニックの目的は「diffを必要最低限にする」こと。
「diffを必要最低限にする」目的は、「diffを見る人＝レビュアーの負担を抑えること」である。
レビュアーの負担を抑えるためには、「どんなdiffだとレビュアーが辛く感じるか」「どんなdiffだとレビュアーは見易いと感じるか」を考えながら、期待する内容のdiffが組み上がるようにして、コミットを積み上げなくてはならない。

可読性の高いコードを書くのにある程度の知識と経験が要求されるのと同じことで、 「可読性の高いPR」 を作るためにも、ある程度のテクニック知識と経験値が必要になるだろう。
この辺りは、各人でどんどんクソPRを作って読んで、「読みやすいdiff」「読みにくいdiff」に対する嗅覚を高められるよう、そして期待通りのdiffを作り上げられるように努めてもらうしかないと思う。

まとめ

• PRは小さくまとめて作る
  • その方が読む側も書く側も疲弊しない
  • 結局、その方が早くて正確
• そのためのテクニックを模索しよう
  • PRを小さくまとめて作る技術は、そのまま作業内容を整理するテクニックにもつながる
  • 副作用として、事前見積や工数把握の精度も向上させられるかも？
• 読む人のことを考えよう
  • コードを書く時と同じぐらい、diffの組み立てにも気を配ろう
  • 可読性の高いコードを書くのと同様、経験が必要。クソPRを作ったり読んだりしたら、どうしたら可読性を上げられるか考えてみよう