2015年2月26日
プルリクエストをより使いこなす
(2015-01-25)by Atlassian Developers
本記 事は、原著者の許諾のもとに翻訳・掲載しております。
Gitを使用している人であれば、プルリクエストには馴染みがあるでしょう。これは、分散バージョン管理システムが世に出始めてから、何らかの形で使われています。BitbucketやGitHubのように凝ったWebユーザインターフェイスが構築される前は、プルリクエストは単純に電子メールベースで行われており、Aliceのリポジトリから変更をプルするように依頼していました。プルリクエストを受けた側がこの変更を妥当だと判断すれば、いくつかのコマンドを実行しmasterブランチに変更をプルするという流れです。
$ git remote add alice git://bitbucket.org/alice/bleak.git
$ git checkout master
$ git pull alice master
もちろん、手あたり次第Aliceの変更をmasterにプルすることは、 得策 ではありません。masterには、顧客に納品する段階のコードが書かれているべきですから、何がマージされるのかをしっかりと把握しておきたいところです。ですから、masterに直接変更をプルするよりも、いったん異なるブランチにプルし、内容を確認してからマージする方が、間違いがありません。
$ git fetch alice
$ git diff master...alice/master
git diffの後ろに“トリプルドット”を入力した構文を使うことで、alice/masterの最新のコミットとローカルのmasterブランチにあるマージベース(共通の祖先)との差分を表示することができます。これにより、Aliceがプルして欲しいすべての変更を効率よく表示できます。
git diff master…alice/masterは、git diff A Bと同等のコマンドです。
プルリクエストを受けた側が、加えられた変更個所を確認する手段として、これは一見妥当な方法に思えます。実際、この記事を書いている時点では、ほとんどのGitホスティングサービスがプルリクエストの差分を表示するアルゴリズムをこのように実装しているようでした。
しかし、“トリプルドット”を使った差分表示のアプローチでは、プルリクエストの差分を表示させるのにいくつかの問題が生じます。というのも、実際のプロジェクトではmasterブランチは、どのfeatureブランチとも内容が大きく異なります。開発者は、それぞれのブランチで作業を行いmasterにマージしていきます。つまりmasterも更新されていくので、featureブランチの最新のコミットとマージベースを表示するシンプルなgit diffでは、最新のブランチ同士の差が適切に表示されないのです。表示されるのは、ブランチを分岐した時点でのmasterのバージョンとfeatureブランチの最新のコミットとの差になります。
“トリプルドット”を使ったgit diff master…alice/masterでは、masterに加えられた変更が考慮されません。
では、プルリスクエストでこれらの変更の差分が表示されないことが、なぜ問題になるのでしょう。それには2つの理由があります。
マージコンフリクト
1つ目は、よくありがちな マージコンフリクト です。featureブランチにあるファイルに変更を加え、同時にmasterにも変更が加わっている場合、git diffではfeatureブランチに加わった変更しか表示されません。一方、git mergeでは、エラーがはじき出され、作業しているファイル全体にコンフリクトマーカーが追加されます。これはマージしようとしているブランチとの間でコンフリクトが起きている、またはGitでさえもマージする方法が見つけられないということを表しています。
マージコンフリクトの解決は面倒ですが、バージン管理システムではよくあることです。少なくともファイルレベルのロックをサポートしないバージョン管理システムに関しては、それ自体が問題です。
しかし、 “トリプルドット”を使ったgit diffで生じる2つ目の問題に比べれば、マージコンフリクトの問題はまだマシです。その2つ目の問題とは、 論理的な コンフリクトです。これはマージする際にはエラーにならいのですが、コードベースに発見しづらいバグが出てしまいます。
問題なくマージされる論理的コンフリクト
もし複数の開発者が同じファイルに対して別のブランチを作成し、それぞれに変更を加えたとしたら、多少厄介なことになるかもしれません。それぞれの変更が単独では機能していて、 コンフリクトもなく 適切にマージされているように見える場合でも、それらが統合された時に論理的なバグが生じることがあります。
いくつか状況は考えられますが、よくあるシナリオとしては、2人以上の開発者が偶然同じバグに気付き、それぞれのブランチを作成して修正してしまうというケースです。下記のような、航空券の代金を計算するJavascriptを例にとって考えてみましょう。
// flat fees and taxes
var customsFee = 5.5;
var immigrationFee = 7;
var federalTransportTax = .025;
function calculateAirfare(baseFare) {
var fare = baseFare;
fare += immigrationFee;
fare *= (1 + federalTransportTax);
return fare;
}
このコードには、税関手数料が含まれていないという明白なバグがあります。
2人の開発者、AliceとBobがこのバグに気付き、それぞれがブランチを作成し修正を行いました。
AliceはimmigrationFeeの前の行にcustomsFeeを追加しています。
function calculateAirfare(baseFare) {
var fare = baseFare;
+++ fare += customsFee; // Fixed it! Phew. Glad we didn't ship that! - Alice
fare += immigrationFee;
fare *= (1 + federalTransportTax);
return fare;
}
一方BobはimmigrationFeeの後ろの行に似たような修正を入れます。
function calculateAirfare(baseFare) {
var fare = baseFare;
fare += immigrationFee;
+++ fare += customsFee; // Fixed it! Gee, lucky I caught that one. - Bob
fare *= (1 + federalTransportTax);
return fare;
}
それぞれのブランチで異なる行が追加されたので、これらのブランチは両方とも問題なく順にmasterへとマージされます。しかしそうすると、masterには 両方の行 が追加されることになり、税関手数料が顧客にダブルでチャージされてしまうという深刻なバグが発生してしまいます。
function calculateAirfare(baseFare) {
var fare = baseFare;
fare += customsFee; // Fixed it! Phew. Glad we didn't ship that! - Alice
fare += immigrationFee;
fare += customsFee; // Fixed it! Gee, lucky I caught that one. - Bob
fare *= (1 + federalTransportTax);
return fare;
}
(もちろんこれはわざとらしい例かもしれません。しかし重複したコードやロジックは、 “go to fail”文の重複 のようにかなり深刻な問題を起こし得ます。心当たりはありませんか?)
先にAliceのプルリクエストをmasterにマージしたと仮定しましょう。“トリプルドット”を使ったgit diffでブランチの最新のコミットと共通の祖先との差分を表示した場合、Bobのプルリクエストはこのようになります。
function calculateAirfare(baseFare) {
var fare = baseFare;
fare += immigrationFee;
+++ fare += customsFee; // Fixed it! Gee, lucky I caught that one. - Bob
fare *= (1 + federalTransportTax);
return fare;
}
これは祖先に対しての差分をレビューしているので、マージボタンをクリックしても差し迫った問題になるような警告は出ません。
プルリクエストで本当に見たい内容は、Bobのブランチをマージした時masterがどのように変更されるかです。
function calculateAirfare(baseFare) {
var fare = baseFare;
fare += customsFee; // Fixed it! Phew. Glad we didn't ship that! - Alice
fare += immigrationFee;
+++ fare += customsFee; // Fixed it! Gee, lucky I caught that one. - Bob
fare *= (1 + federalTransportTax);
return fare;
}
この差分にはちゃんと問題が表示されています。これならプルリクエストのレビューワーはおそらく重複した行を発見し、コードを再修正するようBobに知らせることができるでしょう。このようにしてmasterに(ひいては製品版に)深刻なバグが発生するのを防ぐことができます。
このようなことからBitbucketやStashの中ではプルリクエストの差分を実装することにしました。プルリクエストを表示すると、マージコミット後の結果をプレビューすることができます。これは実際にマージコミットを水面下で作成することで、マージする2つのターゲットの差分を表示させています
Dを マージコミット とした場合“git diff C D”で2つのブランチの全ての差分が表示されます。
いくつかのホスティングプロバイダに同じリポジトリをプッシュしたので、ご興味があればどうぞ。実際に活用されているいろいろなdiffのアルゴリズムを見ることができます。
GitHubを使った“トリプルドット”diff のプルリクエスト
Bitbucketを使った“マージコミット”diff のプルリクエスト
GitLabを使った“トリプルドット”diff のプルリクエスト
BitbucketとStashで使われている“マージコミット”のdiffは、マージした時に適用される 実際の 変更を表示します。問題点としては実装しづらいということと、実行するのにコストがかかるということです。
ターゲットの移動
1つ目のコストの問題ですが、この時点でマージコミットDが実際はまだ存在しておらず、マージコミットを作成するのは比較的費用のかかるプロセスだということです。2つ目の問題である実装に関しては、Dを作成すればそれで終わりではないということです。このマージコミットの親であるBとCがいつ変更されるか分かりません。この場合、親に対してプルリクエストのスコープ範囲の変更をかけることで、プルリクエストがマージされた時に適用される差分が効率よく更新されます。masterのようにビジーなブランチをターゲットにしたプルリクエストであれば、頻繁にスコープの設定が変更される可能性が高いのです。
どちらの ブランチが更新されても、いつでもマージコミットは作成されます。
実際、誰かがブランチを、masterもしくはfeatureブランチに対してプッシュしたりマージしたりする度に、BitbucketやStashは正確な差分を表示するために新しいマージを計算する必要がでてくる可能性があります。
マージコンフリクトの対処
プルリクエストの差分を表示させるためのマージの問題では、時としてマージコンフリクトに対処する必要があります。Gitサーバは無人の状態で運用されるため、周りにマージコンフリクトを解決できる人はいないでしょう。そのため、事態は少しばかり複雑になってきますが、実際には好都合なことが分かります。BitbucketやStashでは、コンフリクトマーカーをマージコミットDの一部として コミットします 。そしてプルリクエストのコンフリクトの内容を差分の中にマークアップします。
BitbucketとStashの差分では、追加された行が緑、削除された行が赤、コンフリクトしている行がオレンジで表示されます。
こうすることで、プルリクエストがコンフリクトしていることを事前に検出することができるだけでなく、コンフリクの解決法についてもレビューワーに検討させることができます。コンフリクトには少なくとも2人が関係しているので、これを解決するのは、プルリクエストが適切な場と言えるでしょう。
さらなる複雑性やかかるコストに関わらず、StashやBitbucketで取ったアプローチが最も正確かつ効果的にプルリクエストの差分を表示できると思います。もし質問やフィードバックがあるようなら、ぜひコメントやTwitterで教えてください。GitやBitbucket、Stashの更新もたまにつぶやいているので、もしよかったら私のアカウント( @kannonboy )もフォローしてください。
株式会社リクルート プロダクト統括本部 プロダクト開発統括室 グループマネジャー 株式会社ニジボックス デベロップメント室 室長 Node.js 日本ユーザーグループ代表
- Twitter: @yosuke_furukawa
- Github: yosuke-furukawa