PRのマージ方法

[nmaya]

これまでの経緯

---

TeraTermProject/osdn-download#13

---

#136 (comment)

---

lng ファイルの１行目はコンフリクトすると思います。

悩ましいですね。並行作業していると history.html もコンフリクトしそうです。

みなさんどうしているのか分かりませんが、こんなことをしているのでしょうか？

• TeraTermProject/teraterm の main --fork--> 私の main
• 私の main ----> 私の branch1
• 私の main ----> 私の branch2
• 私の branch1 で作業
• 私の branch2 で作業
• 私の branch1 を PR
• branch1 の PR を取り込み（TeraTermProjectサイドで行う）
• branch1 に必要な修正があれば（TeraTermProjectサイドで行う）... (1)
• branch1 --merge--> main （TeraTermProjectサイドで行う）
• 私の main に TeraTermProject の main を持ってくる
• 私の branch2 の根本を↑の main に変更する（git rebase --onto で歴史改変）... (2)
  • コンフリクトが起きたら解消する
• 私の branch2 を PR
• branch2 の PR を取り込み（TeraTermProjectサイドで行う）
• 以下略

(1) がゼロならプロジェクト側は楽ですが、そう思い通りにはならないだろうと思います。
(2) はプロジェクト側で PR を取り込んでからプロジェクト側でできないことはないですが、「歴史改変は自分だけが触れる所で行うべし」というものだったと思うので、PRする側にやってもらったほうが間違いがないと思っています。

改版履歴

「LogDefaultPath に実在しないパスが指定されていて LogAutoStart=on でログ記録が開始できないとき、エラーメッセージを表示するようにした」
こんな感じでしょうか？

Originally posted by @nmaya in #157 (comment)

---

          history.html がコンフリクトしてしまいますね。

今回はGitHub上でmargeして、改版履歴を追加しました。

gitだけを使って、GitHubを使わないマージだと、
マージしたところに(コミットea20294)に改版履歴を混ぜこんで
pushするかなと思います。

クローズします。

Originally posted by @zmatsuo in #157 (comment)

---

CONTRIBUTING.md にプルリクエストの作成について簡単にかきました。
この通りでなければならないということではないですが、
標準的なやり方に従うのがわかりやすいのかなと思います。

プルリクエスト(≒作成したブランチ)をmainにマージするのに
プルリクエストの承認が使えそうです。

ブランチ保護ルールを作成する で、Require approvals を 2 に設定してみました。

_Originally posted by @zmatsuo in #177

[nmaya]

fork 先のブランチからの PR は、fork 元の main に対するマージになるようですね。

• branch1 の PR を取り込み（TeraTermProjectサイドで行う）
• branch1 に必要な修正があれば修正（TeraTermProjectサイドで行う）... (1)
• branch1 --merge--> main （TeraTermProjectサイドで行う）

これは不可能で、PR の内容に手直しが必要な場合は

• 直してもらってから PR をマージする
• マージしてから TeraTermProject/teraterm の main で直す

のどちらかになる、という理解でいいでしょうか？

[nmaya]

実例

PR #158 / issue #157

• fork 先のブランチ（sempreff:feature/NoticeWhenFailLogAutoStart）で修正
• GitHub上で TeraTermProject:main へマージ
• GitHub上で改版履歴を変更（コミット）

#157 (comment)

PR #165 / issue #163

• fork 先のブランチ（sempreff:refine/FillMissingMessageKeys）で修正
• zmatsuo さんの手元でマージ, コンフリクトを解消, push

#165 (comment)

PR #174 / issue #160

• fork 先のブランチ（sempreff:bugfix/AvoidCrashWhenHideWindow）で修正
• zmatsuo さんが（おそらく手元で）改版履歴を変更して commit
• zmatsuo さんが（おそらく手元から）push

#160 (comment)

[sempreff]

• PR がそのままマージできない場合は、基本的に PR 側のブランチで整えてからマージするのが良いと思っています。
• lng ファイルの１行目のようにあらかじめコンフリクトが想定されるところは、一工夫要りますね。
• 改版履歴については更新が漏れておりまして、お手数をおかけしました。
  • allow users to select create backup or not #176 のようにコメントをいただけると助かります。（この後作業します）

[nmaya]

ブランチ保護ルールを作成する で、Require approvals を 2 に設定してみました。

#176 を approve しましたが、これが 1 とカウントされるので 2 には足りず、block 状態のままでした。

進まないので、#176 は強制的にマージしてみました。

[zmatsuo]

PR の内容に手直しが必要な場合は

• 直してもらってから PR をマージする
• マージしてから TeraTermProject/teraterm の main で直す
  のどちらかになる、という理解でいいでしょうか？

リポジトリのどこをとってもビルドできて動作するのが好ましいと思うので
マージ+手直しした物を main にプッシュするのが良いと思ったのですが、
誰の修正か分かりにくくなるので

• マージしてから TeraTermProject/teraterm の main で直す
  (ただしビルドできなかったりすることがあるかも)

が良いかなと思いはじめました。

PR 側のブランチで整えてからマージ

他のブランチがマージされた結果、
PRのブランチがコンフリクトしてマージできなくなると面倒ですね。
ソースだと変更箇所が近いことはなかなかないですけど
history.html が困りそうです。🤔

#176 を approve しましたが、これが 1 とカウントされるので 2 には足り
ず、block 状態のままでした。

1は少ないかなと2にしてみたのですが、
自分のPRが自分でapproveできないようなので、
2のままだと強制マージになりそうですね。
PRをapprove x2して自動マージってのができれば便利なんですよね。

[sempreff]

リポジトリのどこをとってもビルドできて動作するのが好ましい

賛成です。

2のままだと強制マージになりそう

2名が approve すれば強制マージしなくてよいという話で妥当だと思いますが、それが困難ということでしょうか。

[zmatsuo]

それが困難ということでしょうか。

自分(zmatsuo)が出したPRは自分で(zmatsuoが)approveできないので
+2になりにくいかなと思います。

リポジトリをまたぐと違うのかな?ということで
自分のリポジトリからPRを出してみました。#180

やはり自分のPRには自分でapproveできないようです。

プロジェクトメンバーだと、teratermの元リポジトリにそのままpushできるので
PRだして+1もらって強制マージというのは、
ちょっとこの修正見てほしいんだけど
という意味合いで使えるかなと思います。

[nmaya]

さらに修正が必要なマージの話

マージ+手直しした物を main にプッシュするのが良いと思ったのですが、
誰の修正か分かりにくくなるので

これは、外部のブランチを自分のところのブランチにして、修正して、main にマージすることを言っていますか？
だとしたら、これは GitHub の仕組みではできなそうです。#178 (comment)
git としてはできそうですが、ご指摘のとおり一部コミットがプロジェクト内の誰かになります。

マージしてから TeraTermProject/teraterm の main で直す

これだと、最終的に完成できるかわからないものが main に入るので、望ましいとは思いません。なんのためのブランチかという話になると思います。
「ブランチのauthorが複数人になる」ことよりも「完成していないもの（ビルドできないブランチ）がmainにマージされる」ことのほうが問題が大きいと思います。

そのままマージできない場合（conflict が起きる場合）の話

やはり外部ブランチ側で修正してもらうのが、最もスマートな気がします。
複数のブランチを同時に抱えているときは、プロジェクト側がmainに取り込むのを待たないと、別のブランチをmainに追従できません。
そこは待っていただくようになると思います。

ブランチ保護機能の話

2名が approve すれば強制マージしなくてよい

自分で自分の PR を approve できないので、私たちのコミットがプロテクトされて main に入りません。
2 になっていると、"Merge without waiting for requirements to be met (bypass branch protections)" にチェックを入れて強制的にマージを進めることになります。

[sempreff]

2名が approve すれば強制マージしなくてよい

"approve できるメンバーの数(仮にxとします)" - "自分(通常は1)" < Require approvalsの数
になるようになっていればよいという理解ですが、x は 2 なのでしょうか。
（自分の理解では 3 だという認識です。現時点では。）

対応としては

• メンバのマージは PR を経由しないで行う運用にする
• approve のためのアカウントを追加する(x を増やす)
• Require approvals を下げる

などの案があると思います。（外野から口出しするのもおこがましいと思います。すみません。）

[sempreff]

#180 を見ていたところ、自分にも Approve が選択できるようです。てっきり Organization のメンバに限定なのだろうと思い込んでおりました。試しに Approve を選択して Submit rewview してみてもよいでしょうか？

[sempreff]

試しに Approve を選択して submit review してみましたが、2 のまま変わらず、でした。駄コメントを付けてしまって失礼しました。

[zmatsuo]

これは GitHub の仕組みではできなそうです。#178 (comment)
git としてはできそうですが、ご指摘のとおり一部コミットがプロジェクト内の誰かになります。

webeditorを使うと簡単かもしれないです。

#156 のconflictで、

This branch has conflicts that must be resolved
Use the web editor or the to resolve conflicts.
Conflicting files

と表示されていて、
web editorを開くと

こんな感じになっています。
これぐらいのconflictならブラウザ上で直してマージしてよさそうに思います。

ソース修正がconflictしてよくわからないときは修正をお願いするのがよさそう。

マージしてから TeraTermProject/teraterm の main で直す

これだと、最終的に完成できるかわからないものが main に入るので、望ましいとは思いません。なんのためのブランチかという話になると思います。

いまいちですかね。

やはり外部ブランチ側で修正してもらうのが、最もスマートな気がします。

そうですね。
でも conflict は発生してしまう(特にhisotry.htmlとかですね)と予想されます。

複数のブランチを同時に抱えているときは、プロジェクト側がmainに取り込むのを待たないと、別のブランチをmainに追従できません。
そこは待っていただくようになると思います。

あるブランチ(≒PR)をプロジェクトがmainにmergeしないと
別のブランチ(≒PR)はmainが確定しないのでmainにmergeできない
ということでしょうか?

1つのブランチに最後まで(mainにマージまで)集中していていることもあります。
でも、たいてい複数のことを並列にやっていて、複数のブランチが同時に進んでいて、
そのうち1つがいちだんらくしたらmainにマージしていく。
特に複数人でやっているとそんな使い方になるのかなと思います。

history.html を web editorで直せそう、ぐらいのconflictなら
メンバで修正してmergeする、難しい場合は修正をお願いする
という運用でどうでしょうか?

自分で自分の PR を approve できないので、私たちのコミットがプロテクトされて main に入りません。
2 になっていると、"Merge without waiting for requirements to be met (bypass branch protections)" にチェックを入れて強制的にマージを進めることになります。

approveが1になれば、メンバーなら強制的にマージで良いかなと思いますがどうでしょう?
「この修正は入れたほうがよい」ならmainに入れてしまうので、
メンバが(まつおが)PRにするのは「この修正はちょっと迷う、相談したいな」という
内容になると思います。

Require approvals を下げる

1 にするといまひとつチェックがあまいのではと思うので 2 が妥当かなと考えました。

x は 2 なのでしょうか。

どれぐらいPRのチェックに時間が割けるかはいろいろで
現在は x は 2 なのかな、です。

PR(やIssue)にTera Termのメンバではない方が👍をつけていたり
これはいいとか、ここ直したら、というコメントを付けていると
それはとっても参考になりそうに思います。

[zmatsuo]

試しに Approve を選択して submit review してみましたが、2 のまま変わらず、でした。

ブラウザからみると次のようになっています。

2のまま変わらない(設定してあっても自動マージには進まない)ですが、
コメントからはこれでよいが伝わるのでとても参考になります。😀

[sempreff]

「auto merge されない PR」を「レビュー依頼」として使うのは良くないと思います。
また、「通らない前提の auto merge を設定する」のも良くないと思います。

[nmaya]

approve の使い方（GitHubの機能）について

自分にも Approve が選択できるようです。

確認したところ、メンバーでなくても approve できる設定になっていました。

試しに Approve を選択して submit review してみましたが、2 のまま変わらず、でした。

ブラウザからみると次のようになっています。

試しに approve してみたところ、緑のチェックが付きました。
@sempreff さんの approve は灰色のチェックのようです。
これでまだブロックされているので、数が足りないのだと思います。メンバーの数だけカウントされるのかもしれません。

approve と merge のルール（設定値）について

@zmatsuo (誰でも approve できる && +2で自動マージする) という状態は大丈夫でしょうか？

• 「プロジェクト外の誰かが approve できる」のは、それでもよいと思います。
• 「+2 にならないとブロックされる」は、現状で不便に思います。また、@sempreff さんのコメントにあるように、異常検知を無視するのが常態化するのはよくないと思います。
• 「特定の条件で自動マージする」は、マージできると思う基準が私と松尾さんでも異なると思うので、手動で行ったほうがよいと思います。

merge のルール（運用）について

私の中では「十分な修正が行われている」=「mainにmergeできる」=「approveできる」です。
「十分な修正が行われている/いない」と「conflictがある/ない」を別物として考えています。

webeditorを使うと簡単かもしれないです。

「耳がそろった」PR で conflict が起きた場合、webeditorを使ったら「ブランチで修正」のち「mainにマージ」が行われるならよいと思います。

conflict が起こらなかった場合に「conflictが起きてないからapproveしよう」というのは「なし」で、「耳がそろっていない」PR（例: ドキュメントが不足している）かどうかを確認する必要があると考えています。

複数のブランチを同時に抱えているときは、プロジェクト側がmainに取り込むのを待たないと、別のブランチをmainに追従できません。
そこは待っていただくようになると思います。

あるブランチ(≒PR)をプロジェクトがmainにmergeしないと
別のブランチ(≒PR)はmainが確定しないのでmainにmergeできない
ということでしょうか?

その理解です。

history.html を web editorで直せそう、ぐらいのconflictなら
メンバで修正してmergeする、難しい場合は修正をお願いするという運用でどうでしょうか?

• 「十分な修正が行われていない」→修正をお願いする
• 「history.html を web editorで直せそう、ぐらいのconflict」→「web editor」がブランチを修正するものなら、メンバで修正してmergeする

でよいと思います。

コメントが書ける箇所

あちこちにコメントを書けるので、相談したいときどこに書くか迷いそうです。

• issue（PR #180は issue がない PR でした）
• PR の Conversation
• PR の Files changed で特定のファイルへのコメント
• commit で特定のファイルへのコメント

issueが一番上位（たいていbranchを切る根拠になるので、issueがbranchよりも上）にあると思うので、なるべくissueに書くようにしています。

[sempreff]

history.html については .gitattributes に history.html merge=union しておく案はいかがでしょう。
（lng は衝突する頻度も低いと想定されるので都度 PR側が pull して merge 可能な状態を保つことでよいと思っています。）

[zmatsuo]

「auto merge されない PR」を「レビュー依頼」として使うのは良くないと思います。

zmatsuoやnmayaはPull Request レビュー (≒レビュー依頼) を使えばいいのかな?
okとなったらそのままマージできるのでしょうか。
次に修正する時につかってみます。

誰でも approve できる && +2で自動マージするという状態は大丈夫でしょうか？

approveの2以外はデフォルトだったとおもいます。
プロジェクト外の誰かが approve できない設定がどれかわからなかったです。

「+2 にならないとブロックされる」は、現状で不便に思います

2なのは(たとえばnmayaさんと私の)2人がapproveとしたらマージしてよい
というのを考えていました。

現在は自動マージはOFFです。Settings の General の中にあります。(自動マージを管理する)

「十分な修正が行われていない」→修正をお願いする
「history.html を web editorで直せそう、ぐらいのconflict」→「web editor」がブランチを修正するものなら、メンバで修正してmergeする

でよいと思います。

よいと思います。
了解です。

コメントが書ける箇所
あちこちにコメントを書けるので、相談したいときどこに書くか迷いそうです。
issue（PR #180 は issue がない PR でした）

PR と issue で2重になるので PRを作ったときはそれだけでよいかなと思います。
@sempreff さんの修正はPR + issueですね。
それより下位のコメントなどはコード内についての細かな会話には便利そうです。

[zmatsuo]

Pull Request レビュー出しました。 #184

Reviewers のところに
At least 2 approving reviews are required to merge this pull request.

と出ているので二人のレビューが必要そうですね。

[nmaya]

現在は自動マージはOFFです。

了解です。

zmatsuoやnmayaはPull Request レビュー (≒レビュー依頼) を使えばいいのかな?
二人のレビューが必要そうですね。

• マージに承認を必要としなくする=「Require a pull request before merging の Require approvals」を off にする
• レビューしてほしいときは issue に書いたりレビュー依頼を使う

でどうでしょうか？

[nmaya]

PR と issue で2重になるので PRを作ったときはそれだけでよいかなと思います。
@sempreff さんの修正はPR + issueですね。
それより下位のコメントなどはコード内についての細かな会話には便利そうです。

私は「問題」でも「タスク」でも issue を切ります。
「問題」であれば、「想定する動き」「実際の動き」「どこが問題か」などを説明する issue を切ります。気づいた人が外部か内部かは関係なく issue が発生します。
「タスク」であれば、思いついた作業を忘れない（TODOリストとして使う）ために issue を切ります。
issue の一覧を見ることで「なにが作業途中なのか」をすべて見渡すことができます。（条件から is:issue を削除れば pr も出てくるのですが）
また、この順番だと commit ログに issue 番号を書くことができます。

このような関係になると思っています。
issue <--------------+*2
 |      PR*3 <---+*4 |
 |       |       |   |
 +-*1-> branch   |   |
         +------ commit
                  +---- changed files
*1 issue の修正のために branch を作る
*2 コミットログで言及
*3 main へのマージのために PR を作る
*4 PR をマージしたとき、どのPRによってマージされたか記録される
※作業後に削除したときブランチは消える

このように追えます。
あるファイルの全体
  https://github.com/TeraTermProject/teraterm/blob/main/teraterm/teraterm/vtwin.cpp
そのファイルの修正履歴
  https://github.com/TeraTermProject/teraterm/commits/main/teraterm/teraterm/vtwin.cpp
そのファイルの行ごとの最終修正
  https://github.com/TeraTermProject/teraterm/blame/main/teraterm/teraterm/vtwin.cpp
この修正の詳細が知りたい
  https://github.com/TeraTermProject/teraterm/blame/9fb201093f07a0842ff49d545a595b855337dca8/teraterm/teraterm/vtwin.cpp#L483
  このコミットが最終コミット
    https://github.com/TeraTermProject/teraterm/commit/6491bf2eabfdd895a0700ec99b40a140da831e28
  このPRでマージされた
    https://github.com/TeraTermProject/teraterm/pull/174

https://github.com/TeraTermProject/teraterm/wiki/Commit-rule
(2)チケットに関連するコミットはチケット番号を記載する
コミットメッセージにチケットへのリンクが自動的に張られるからである。

svn と ticket のときの記述ですが、私はこの作業手順で続けるつもりでいます。

もし issue がないと、branch の作成と commit が先に行われるので、状況説明を commit のログに書くことになります。
commit ログを大雑把に書いて PR のコメントに書くこともできますが、commit ログに PR 番号を書けないため、上記のルールを踏襲できません。

PR -----+
 |      |
branch  |
 +---- commit
        +---- changed files
※作業後に削除したときブランチは消える

git では branch = PR が修正の固まりで、個別の commit にそれほど大きな意味がないのかもしれません。
ファイルの修正履歴を眺めたときに、なんのために修正されてきたのか分かるためには、マージのコミットが大事そうです。
「あとから遡って見たときにわかりやすいよう、どのように記録しておくのがよいか工夫したい」というのが狙いです。

[nmaya]

history.html については .gitattributes に history.html merge=union しておく案はいかがでしょう。

この設定でconflictが起きづらくなるなら有用な設定だと思います。
@zmatsuo いかがでしょうか？

[zmatsuo]

commit ログに PR 番号を書けないため、上記のルールを踏襲できません。

commit してから PR 出すので、書けないですね。

0098cde は手もとで修正を始めて PR にしたコミットです。
コミットからどの PR かは、web だと Commit に main(#153) とあって、
わかるようになっているようです。
手もとのローカルリポジトリではわからないです。

いただいた PR をマージする時に、
OSDN の svn と同じように "PR #180" などと入れると
ローカルリポジトリでも分かるようになって便利に使えるかもです。

[zmatsuo]

gitattributes でこういうことができるんですね。
random order とあるのがどうなるのかな?です。

入れてみて、いただいているPRをマージしてみます。

[zmatsuo]

マージに承認を必要としなくする=「Require a pull request before merging の Require approvals」を off にする

そうしましょう。off にしました。(デフォルトに戻しました。)

[zmatsuo]

#156 をマージしました。

GitHubが自動生成するメッセージに PR番号が自動で入っていました。

0098cde は手もとで修正を始めて PR にしたコミットです。

ログにPR番号が入っていないのはwebインターフェイスを使わずに
手もとのツールでマージしてmainにpushしたかもしれません。

#171 をみると "This branch has conflicts that must be resolved" 表示が出ています。
.gitattributes の書きかた間違っている?

[sempreff]

手元の git では #178 (comment) のようにファイル名だけにしてあげたら効きました。github の auto merge がどう働くかまでは試せていませんが、一度 パス名を抜いて ファイル名だけにしてみてはいかがでしょうか。

[nmaya]

一度 パス名を抜いて ファイル名だけにしてみてはいかがでしょうか。

変更してみました。 f5969c7

[zmatsuo]

#171 は "This branch has conflicts that must be resolved" ですね。
効かないのかな・・。

[sempreff]

もしかすると PR側に必要なのかもしれないですね。＞ .gitattributes
#171 は main に追随しておきました。

[sempreff]

171 は期待通り普通にマージできて、#185 はコンフリクトと出ています。
残念ながら .gitattributes は効かないように見えますね。
（これが確認したくて 185 を出した、という側面もあったのですが…）

追記：
似たようなお悩みの issue を見つけました。
isaacs/github#487
https://github.com/orgs/community/discussions/9288

[zmatsuo]

#164 をマージしました。
その際、webeditor を使ってコンフリクトを修正したところ
Shift_JISのhistroy.htmlが UTF-8 になってしまった(#193)ため
ローカルで修正してpushし直しました。

histroy.html の文字コードが Shift_JIS なのは、
html help の入力文字コードが Shift_JIS だからです。

webeditor 要注意です。

[nmaya]

Shift_JISのhistroy.htmlが UTF-8 になってしまった

了解しました。頭の隅に入れておきます。

[zmatsuo]

CONTRIBUTING.md を追加しました。

ほとんど使ったことがないですが
「レビューを使って 内容の確認や追加の修正をお願いする…」と
入れました。

[nmaya]

マージのときに、意図せずソースの文字コードが Shift_JIS から UTF-8 に変更されました。
今後も起きるかもしれないので、経緯を記録しておきます。

• #197_ssh_further_auth ブランチの 8e4c0527ed196932664ce6f99c8f6c90b454bb2c を main にマージしようとしたところ、conflict が起きた
• web editor を使用して編集して解決した。このとき Shift_JIS のファイルが UTF-8 として保存された
  • main から #197_ssh_further_auth へマージ 320e266 ... ここで変化
    • 
  • main へマージ 9a54759
• #197_ssh_further_auth ブランチ で作業中に warning が起きているのに気づいて文字コードを Shift_JIS に変更して保存し、コミットした 5979e82