Merge Request (Pull Request) の書き方

はじめに

チーム開発をする場合、Merge Request（MR）あるいはPull Requestを書いてコードレビューを行うことが多いでしょう。コードレビューに慣れていない場合、MRに何を書けばいいのかわからないことが多いと思います。
この記事では私のMRの書き方を紹介します。

なぜMRを書くのか

チーム開発でMRを用いるのは当たり前だと思いますが、なぜMRを書いてコードレビューを行うのかを考えてみるとどのようなMRを作成すべきかが見えてきます。
私の考えでは以下の四つの理由があると思います

1. コードの品質を担保するため

コードをデフォルトブランチにマージするという行為には、そのコードを今後保守し続けるという意味があります。別の言い方をすると、コードを一度マージしてしまうと、そのコードは他の大量のコードと同列に扱われます。

マージ前のコードはMRの修正範囲の中でしか使われていませんし、MRの作者がコードの変更内容について責任を持てます。一方でマージ後のコードはどこから参照されているか不明ですし、コードの内容を知っている人を特定するのは容易ではありません。¹ そのため、マージ前のコードを修正するのに比べて、マージ後のコードを修正するハードルはかなり大きくなります。

製品のコードベースの品質を維持するためには、MRを作成してマージ前に丁寧なレビューを行って品質の低いコードをマージしないことが重要です。品質にはいくつか構成要素がありますが、大きくは「外部品質」と「内部品質」に分かれます。

• 外部品質：外から見た振る舞いに関わる品質
  • 変更内容がテストされているか
  • 変更後の仕様に問題はないか
  • etc...
• 内部品質：振る舞いからはわからない設計や保守性に関わる品質
  • コードの設計に問題はないか
  • コードは開発チームのコーディングルールに従って書かれているか
  • ユニットテストが書かれているか
  • etc...

このうちMRでは最低限「内部品質」を担保する必要があります。さらにstagingブランチがなく、「MRのマージ＝本番環境へのデプロイ」となっている場合は「外部品質」についてもマージ前に担保する必要があります。²

MRを作成する際には、これらの品質が担保されているかレビュワーが判断しやすいように書き方を工夫しましょう。

2. コードを他のチームメンバーに読ませるため

MRをレビューすることで、自分の書いたコードを知っている人が増えます。
それにはコードの属人化を避ける効果や、メンバー同士のコーディングスキルを向上させる効果があります。

そのため、コードレビューはチームメンバー間で気軽に行う方が良いでしょう。
また、MRは（シニアエンジニアだけではなく）チームメンバーが読んで内容わかるように書くべきです。

3. コミットの粒度を揃えるため

Gitのコミットの粒度はひとによってバラバラです。
細かくコミットを分ける人もいれば、一通り実装し終わるまでコミットをしない人もいます。
コミットの粒度が大きすぎると「なぜこの修正が必要だったのか」が分かりにくいですし、
小さすぎると修正の全体像を把握しにくいでしょう。

MRを適切な粒度で作成し、マージ時にコミットをsquashすることでデフォルトブランチのコミットの粒度を揃えることができます。

4. なぜその変更が行われたかのドキュメントとするため

チームで開発するようなソフトウェアのコードベースは基本的に巨大で、
優れたエンジニアであっても一人がコード全体を把握することは不可能です。

開発者はコードの一部だけを読んで修正を行わなければなりません。
コードは複数のファイルに分割されてディレクトリが適切に分けられていますが、
目的の修正を行うためにどのファイルを読めばいいかというのはディレクトリだけからはなかなかわかりません。

そんな時には過去のMRを参考にすると機能ベースでどのようなファイルを修正すればいいのかがわかります。
すなわちMRはコードの修正内容のドキュメントとして読まれます。
その際にコードが何をしているのかは最悪コードを読めばわかりますが、コードが何のためにあるのかはコードだけからはわかりません。

MRにはそのコードを修正した理由や目的を明記しましょう。また、MRの目的が複数あるとその修正が何のために行われたかわかりにくくなります。目的ごとにMRを分けると良いでしょう。

MRのフォーマット

さて、MRのチームごとに定めたフォーマットに従って書くべきですが、もし決められたフォーマットがない場合、
私が最近よく使っている以下フォーマットを参考にしてみてください。

タイトル

チケット番号を含めてMRの内容を簡単に書きます。タイトルはそんなに重要ではないので一覧から内容が識別できればなんでもいいと思います。

例

• #12345 本の「いいね」apiの追加
• #12345 HogeControllerのリファクタリング

ブランチ名

${接頭辞}-${チケット番号}-${キーワード}

• 接頭辞はレポジトリのルールに従いましょう。よくあるのはfeature, bug, refactor, ticket, wipなどです。
• チケット番号はMRに紐づくチケットの番号を記載します
• キーワードはチケットに複数のMRが紐づく場合に、MRを識別するためにキーワードのリストをハイフン区切りで書きます。連番は何を意味するかわからないので避けた方が良いです。ブランチ名は実は日本語や絵文字が使えますが、そういったブランチ名はCI泣かせなのでやめてあげてください。

例

• feature-12345-add-hoge-api
• bug-12345-fix-npe

本文

MRの変更内容を書きます。私はよく以下の見出しを使います。

• チケット
• 概要
• 変更内容
• テスト
• TODO

チケット

MRに紐づくチケットへの参照を書きます。チケットとMRは1:nの関係にあるべきです。
チケット管理にGitHubやGitLabのIssue機能を使う場合MRに書いておけば勝手にチケット側にもリンクされるので便利です。

概要

このMRの目的や変更内容を簡潔に記述します。一つのMRには一つの目的があるべきです。このセクションを書いてみて目的が一つに絞れない場合、目的ごとにMRを分けるべきです。また、ここではIssueではなく解決策を記述すべきです。

良い例

• 本に対して「いいね」をつけるAPIと「いいね数」を返すAPIを追加します。
• リファクタリングのためにHogeControllerの仕様化テストを追加します

悪い例

• XXの場合にエラーとなる不具合を修正します
  • 理由：どう修正したのかわかりません。不具合の内容はチケットに書いてあるはずです。
• ユーザ退会処理を実装します
  • 理由：変更内容が抽象的すぎます。MRが大きくなりレビューができない可能性があります。
• FugaControllerのリファクタリングを行い、POST /hoge/ APIを追加します。
  • 理由：リファクタリングと仕様変更はMRを分けるべきです。

変更内容

編集した各ファイルごとにどのような変更を行ったのかを記述します。ここに変更内容を書くことで、不要なファイルを変更していないかをチェックできます。不具合修正の場合は影響範囲調査をどうやってどうやって行なったのか書いておくと良いでしょう。

例

• Controller.java
  • GET /books/${bookId}/likesを追加
    • 本のいいね数を返します
  • PUT /book/${bookId}/likesを追加
    • 本にいいねを送ります
    • ログインしていない場合はUnauthorizedExceptionを発生します
• LikesDao.java
  • int countLikes(int bookId)メソッドを追加
  • void addLike(int bookId, int userId)メソッドを追加
• LikesService.java
  • int countLikes(Book book)を追加
  • void addLike(Book book, User user)を追加

テスト

マージ前に行うテストをタスクリストの形で記述します。テストケースを一つ一つ記述する必要はなく、テスト観点ごとに書くと良いでしょう。詳細なテストケースが必要な場合はテストケースを別途作成してリンクを貼ります。
レビュワーはテスターやCIが実施したテスト結果を確認してタスクリストにチェックをつけます。

例

• ControllerTest
  • GET /books/${bookId}/likesはいいね数を返す
  • PUT /book/${bookId}/likesはいいねを追加する
• LikesDaoTest
  • countLikes(bookId)はLIKESテーブルの指定したbookIdを持つ行数を返す
  • addLike(bookId, userId)はLIKESテーブルにInsertする
• LikesServiceTest
  • countLikes(book)はLikesDaoのcountLikes(book.id)を呼び出す
  • addLike(book,user)はLikesDaoのaddLike(book.id, user.id)を呼び出す

TODO

マージ後に行わなければならないことをタスクリストの形で記述し、実施したらチェックをつけます。
チケット側でタスク管理を行なっている場合は不要なこともあります。

例

• トピックブランチを削除する
• 評価環境でテストケースを評価する
• wikiの更新

おわりに

• MRはコードメンテナに「私のコードをマージしてください」というお願いです。メンテナの要求する品質が担保できていることを示しましょう。
• MRは同僚に「こんなコード書いたんだけどかっこよくない？」と自慢するものです。チームメンバー同士でレビューしあいましょう。
• MRは未来の自分や同僚に「ここ修正するときはここに気をつける必要があるよ」と託すものです。自分の書いたコードであっても三ヶ月もすれば何やってたか忘れるものです。コンテキストなしに読めるように書きましょう。

MRの本文を書くという行為はなかなか面倒ですが、適切なフォーマットを決めて書いていくとコードのセルフレビューが行えて、変更箇所の不足やテスト観点の不足に気づくことができます。

ぜひこの記事を参考に自分のMRの書き方を工夫してみてください。

1. チームメンバーの異動や退職等で担当者が存在しない場合もよくあります。 ↩

2. もちろん、stagingブランチがある場合でも、マージ後の修正はコストが大きいのでバグはマージ前に潰した方が良いです。 ↩