OSSプロジェクトへのコントリビュートで避けるべき6つのこと：1. プルリクエストで複数のことをしない

結城です。

皆さんは「Hacktoberfest」をご存じでしょうか。OSSにコントリビュートする人を増やしたり、OSSへのコントリビューション自体を増やしたりすることを意図して、「どんなOSSプロジェクトでも、どんな内容でも構わないので、GitHub上でプルリクエストを4件作成したらイベントロゴ入りのTシャツが貰える」というルールで毎年開催されているイベントです。

今年の10月初頭、このイベントがきっかけで大量の迷惑なプルリクエスト（spam PR）が発生するという事件がありました¹。この件を承けて書かれた記事の1つとして、OSSプロジェクトへの望ましいコントリビューションの仕方について語った 6 Things to Avoid When Contributing to Open-Source Projects - Qvault という記事があります。こちらの記事で語られている「コントリビュートするときに避けるべき6つのこと（するべき6つのこと）」は、要約すると以下の通りです。

プルリクエストで複数のことをしない（プルリクエストはトピックごとに分ける）
一貫性を壊さない（コーディングスタイルを合わせる）
了承なしに作業を始めない（コンセンサスを得てから開発に着手する）
既知の問題や解決策があることについて報告しない（報告する前に、既存の報告や情報がないか探す）
途中の複数のコミットをそのままにしない（squashで1つのコミットにまとめる）
無意味な変更を提案しない（意義のある報告や提案をする）

それぞれは妥当な話なのですが、筆者はこの記事について、期待されているような効果はあまりなく、逆に予期しない萎縮効果を生んでしまうのではないか、と懸念しています。

この記事を書いた人は恐らく、spam PRをしてしまう人達に記事を読んでもらいたかったのでしょう。しかし、実際にspam PRをしていた人達にとっては、1から5のことに気をつける動機がありません²。それに対し、「まだコントリビュートをした経験はないが、OSSにコントリビュートしたいと思っている、向上心のある人」にとっては、この6箇条はむしろ、最初の一歩を踏み出しにくくさせるブレーキとなりえます。その結果としてOSSへの実際のコントリビュートが減ってしまっては、本末転倒です。

ということで、これから何回かのシリーズに分けて、先の記事の6箇条について、可能な限り具体的に・理由を示しながら「こういう考え方はよくない」「こう考えれば問題ない」と説明してみます。先の記事のような「べからず集」を見て萎縮してしまった心を解きほぐす一助となれば幸いです。

まず1つ目は、複数のトピックを含んだプルリクエストについてです。

1. プルリクエストで複数のことをしない（プルリクエストはトピックごとに分ける）

公開のOSSプロジェクトで問題の修正に取り組み始めてみると、「なんだかすごい物」と思っていたOSSのコードが、実は普通のコード……どころか、意外と粗だらけだということに気が付くことがあるかもしれません。

そう感じるのは、不思議なことではありません。というのも、有志の手により開発されているOSSは、「ものすごく技術力の高い人が作っている、完成度の高いソフトウェア」であるとは限らず、「切実にそれを必要としていた人がDIYの精神で作っている、間に合わせのソフトウェア」である場合も多々あるからです。もしかしたら、あなたの方がプロジェクトオーナーより技術力が高い場合すらあるかもしれません。

そういうときに、「この書き方は効率が悪いから駄目だと教わったやつだ」とか、「こういう設計は分かりにくいから避けろと言われたやつだ」といった要領で、本来やりたかったことと無関係ながらついでに直したい箇所が目につくこともあるでしょう。あるいは、「これを応用すればこういう便利な機能も追加できるな」と気が付いて、そっちにも手を出したくなるかもしれません。

ですが、当初の修正と無関係な変更まで「ついで」でやってしまうのは、プルリクエストでは避けた方がよいです。開発者側で変更内容を把握しやすくするために、プルリクエストは1回につき1つの趣旨に則った変更のみに留めましょう。

趣旨が異なる変更とは、どういうものか？

具体的にどういうものがそれにあたるのかを説明します。たとえば、以下のようなJavaScriptのコードがあったとします。

// リンク先のURL文字列を収集して返す
function getLinkURLs() {
  let links = document.querySelectorAll('a[href]');
  // ↓プロパティ名を間違えている（hre ではなく href が正しい）
  let urls = Array.from(links, link => link.hre);
  return urls;
}

// ページ内にある見出し（h1～h6）を収集して返す
function getHeadings() {
  // ↓h2とh3の間に「,」が抜けている（'h1,h2,h3,h4,h5,h6' が正しい）
  return document.querySelectorAll('h1,h2h3,h4,h5,h6');
}

見ての通り2箇所に間違いがありますが、こういう場面で「自分にも直せそう！」と思って両方を一度に変更してプルリクエストしてしまうのは良くないです。というのも、この2箇所の間違いは、表面的にはどちらも「誤記」ですが、それぞれ趣旨・性質が異なるからです。問題として報告するなら、この2つは

「リンク先のURLを収集する関数が、実際にはURLを返さない問題」
「見出しを収集する関数が、実際にはh2とh3を収集しない問題」

と、それぞれ別個に報告するのが適切です。

もし1つ目の問題の修正のためのプルリクエストに2つ目の問題の修正が混入していたら、開発者は「えっ、この変更はリンク先のURLと関係なさそうなんだけど……自分の知らない所で、これがリンク関係に影響してたの？」と混乱して、余計な調査に時間を使わないといけなくなります³。

また、後々後退バグが見つかったときの原因調査でも、このように複数の趣旨が混ざった変更があると、調査の妨げになりがちです。複数の趣旨が混ざったコミットやプルリクエストは、プルリクエストの時点でも問題を生むし、その後も問題を生むので、原則としては百害あって一利なしと言えるでしょう。

別の変更に依存する変更、ある変更の前提になる変更

「ある変更をするために、それが依存することになる別の変更もしたい」というケースもよくあります。たとえば以下の要領です。

MacBook ProのTouch Barで機能を呼び出せるようにしたい。（本来やりたい変更）
- そもそもmacOSでの動作に対応していない。そのため、macOSに対応させる所からやらないといけない。（依存する変更）
Windows 10の新機能と連係して動作するようにしたい。（本来やりたい変更）
- その新機能を使うには、依存ライブラリのバージョンを上げないといけない。依存ライブラリの新バージョンは旧バージョンと互換性が無いため、ライブラリを呼び出している部分を広範囲に渡って書き換えなければいけない。（依存する変更）

このようなケースでも、まず最初に「依存する変更」を単独のプルリクエストで行うのがおすすめです。本来やりたかったこと（Touch Bar対応やWindows 10の新機能との連携）が実現されないとしても、前提となる変更自体で得られるものがある場合、それらは本質的には「趣旨の異なる変更」と見なせるからです。

複数の変更を一度にしてもよい場合

コードの見た目の変更

元々直したかった問題を直す過程で、その変更の範囲に自然に含まれる変更であれば、一度に行っても問題ないと言える場合はあります。たとえば、以下のようなコードがあったとしましょう。

function getLinkURLs() {
  let links = document.querySelectorAll('a[href]');
  // ↓プロパティ名を間違えている（hre ではなく href が正しい）
  // ↓この行だけインデントが揃っていない
    let urls = Array.from(links, link => link.hre);
  return urls;
}

この場合、「プロパティ名の間違いを直す」趣旨の変更の一環でインデントも揃えてしまう、ということはよく行われます。動作を変えるために変更しないといけなかった行については、動作を変えない範囲の変更であればついでに行っていい、というのが一般的な慣習だと言っていいでしょう。

ただし、単に「同じ行なら複数の変更をまとめてやってしまってもよい」わけではない、ということには注意してください。1行の中であっても、趣旨の異なる変更は別々のプルリクエストに分けることが望ましいです。

複数の技術レイヤーにまたがる、同じ趣旨の変更

細かいレベルでは別々の変更と言えるけれども、大局的には1つの趣旨にまとめられる、という場合には、1つのプルリクエストにしてよい場合もあります。たとえば、「Firefox 60までにしか対応していなかったFirefox用アドオンを、Firefox 68に対応させるため」という名目で以下の2つの変更を1つのプルリクエストにまとめるのは、問題無いと判断してもらえるかもしれません。

Firefox 68で廃止されたJavaScriptのメソッドを使っていた部分を、Firefox 68以降でも動作する書き方に修正する。
Firefox 68で廃止されたCSSのプロパティを使っていた部分を、Firefox 68以降でも有効な書き方に修正する。

筆者の個人的な感覚では、この2つをまとめてプルリクエストすることは問題無いと感じます。しかし、プロジェクトオーナーによっては異なる判断をするかもしれません。心配な場合は、プルリクエストのコメントの中で「変更ごとに分けた方がよいでしょうか？（Should I separate this for each change?）」と尋ねてみるか、最初から分けてプルリクエストすることをおすすめします。

まとめ

以上、「複数の事を1つのプルリクエストにしない（プルリクエストはトピックごとに分ける）」という原則について、実例を挙げて解説してみました。

この解説は、OSSへのコントリビューションを増やすことを意図した取り組みであるOSS Gateで開催しているワークショップの中で得られた知見をまとめた本、「これでできる！はじめてのOSSフィードバックガイド」の一部を抜粋・再編集した物です。本編ではこのほかにも、問題の報告の仕方やありがちなミス、フィードバック初心者の方が戸惑いがちな点について、なるべく具体例を示しながら、幅広く解説してみています。リンク先では原稿の全文を公開していますが、手元に置いて参照しやすい形式での販売も行っていますので、読書スタイルに合った形式で参照して頂ければ幸いです。

OSS Gateでは、新型コロナウィルスの感染拡大防止の観点から、現在は東京地域を主体としたワークショップをオンライン（Discord）で開催しています。次回開催予定は10月31日（土曜）10:30からで、ビギナー（ワークショップで初めてのフィードバックを体験してみたい人）・サポーター（ビギナーにアドバイスする人）のどちらも参加者を募集中です。ご都合の付く方はぜひエントリーしてみて下さい。

また、当社では企業内での研修としてのOSS Gateワークショップの開催も承っています（例：アカツキさまでの事例）。会社としてOSSへの関わりを増やしていきたいとお考えの企業のご担当者さまは、お問い合わせフォームからご連絡頂けましたら幸いです。

事件の概要はGIGAZINEの記事にまとめられています。記事によると、前年までも迷惑なプルリクエストあったようですが、今年はそれと比べものにならないレベルで迷惑行為が多発した模様です。そのためHacktoberfestは運用ルールが変更され、現在は「イベントへの協賛を明示的に示したOSSプロジェクトに対してプルリクエストを行い、そのプルリクエストに承認のラベルが付けられた」状態のみカウントされるようになっています。 ↩
なぜなら、彼らの動機は「Tシャツを貰うこと」にあり、それを達成するのに最も簡単な方法としてspam PRを選んだだけに過ぎません。1から5のことを守らないとTシャツを貰えないのなら、コストパフォーマンスが悪いので去るだけでしょう。 ↩
開発者は今あるコードのすべてを把握できているとは限りません。変更に変更を重ねた結果、自分の意識していないところでの変更が別の所に影響するようになってしまっている、ということはよくあります。そのため、常に変更の副作用を疑う癖が付いているのです。 ↩

ククログ