Backport #5702: backport r33935, r33987 (fix private constant)

• Status: Closed
• Priority: Normal
• Assignee: -

[mame (Yusuke Endoh)]

r33935 を ruby_1_9_3 にバックポート希望です。
([ruby-list:48558] のバグ修正)
誰かレビュー頼みます。

--
Yusuke Endoh [e-mail]

[kosaki (Motohiro KOSAKI)]

---

Backport #5702: backport r33935
http://redmine.ruby-lang.org/issues/5702

Author: Yusuke Endoh
Status: Open
Priority: Normal
Assignee: Yusuke Endoh
Category:
Target version:

r33886 を ruby_1_9_3 にバックポート希望です。
([ruby-list:48558] のバグ修正)
誰かレビュー頼みます。

一瞬レビューしようと思ったけど、これは無理。あきらめた。
コアなコミッタの降臨を期待します

ところでバックポートチケットに修正内容書かずにレビューワーほいほい
しようとするライフハックはどこ由来なんでしょう。正直僕はあんまり
うれしくないなあ・・・

あからさまに関係ないのみいちいちコミットを探して関係無いことを

確認しないといけないから

[mame (Yusuke Endoh)]

遠藤です。

2011年12月4日3:19 KOSAKI Motohiro [e-mail]:

r33886 を ruby_1_9_3 にバックポート希望です。
([ruby-list:48558] のバグ修正)
誰かレビュー頼みます。

一瞬レビューしようと思ったけど、これは無理。あきらめた。
コアなコミッタの降臨を期待します

ぎゃー、r33935 の間違いです。(タイトルの方が正しい)

ところでバックポートチケットに修正内容書かずにレビューワーほいほい
しようとするライフハックはどこ由来なんでしょう。正直僕はあんまり
うれしくないなあ・・・

テスト以外は 1 行パッチなので、ML 番号を書いておけば
十分だと思ってしまいました。

あと正直なところ、バックポートに関しては、されないなら
されないで別にいいや、という気持ちもあります。チケット
作るのも面倒でしょうがなかったのにがんばったという感じ
なので、細かいこと言わないでというか、過剰に期待しない
でほしい。チケット作る気持ちすらなくなっちゃう。

--
Yusuke Endoh [e-mail]

[nagachika (Tomoyuki Chikanaga)]

• File backport_5702.patch backport_5702.patch added

近永です。

そんなにコアじゃないですがレビューしました。

• continue するよりは NameError 発生を else 節にしたほうが見通しが良いかと思います。好みの問題のような気もしますけど。
• また NameError 発生時に、1つ以上の constant の flag を修正済みの場合に rb_clear_cache_by_class() が呼ばれていないことに気がつきました
  というわけで追加の変更を添付します。

あと kosaki さんがおっしゃってたのは、差分の具体的な解説ではなくて(それがあったほうが良いのはもちろんでしょうけど)、「これは○○の件についてのbackport」と一言あると、日常的にチェックしてる人ならだいたいどの修正のことか見当がつくので自分が読めるかどうかすぐ判断ができていいということかなーって思います。この件なら「private constant のアレ」くらいで充分かと。違ってたらごめんなさい。

[nagachika (Tomoyuki Chikanaga)]

すみません、先程わたしが添付したパッチをあてると、なぜか Syck のテストが1つ Failure になってしまいました。後程調べるのでとりあえずこのパッチは差し戻させてください。

[nagachika (Tomoyuki Chikanaga)]

一度 make clean したら発生しなくなりました。
おさわがせしました。

[naruse (Yui NARUSE)]

2011年12月5日12:39 Yusuke Endoh [e-mail]:

ただ、バックポートチケットを書く人に過剰な期待をしないでという気持ち
には変わりがありません。ある程度真面目に書かないといけないとなると、
面倒で後回しにして、そのまま忘れることうけあいです。
でも、いっぱいレビューしてくれてる (し、この先もしてくれそうな)
kosaki さんが、「解読不能なバックポート依頼が来ると、他のレビューも
する気もなくなる」というなら、それはとてもまずいので、どうしても
バックポートしてほしいものだけを真面目にチケット化するようにします。

チケット作るなら簡単に内容を〜というは同意するのですが、
（というかコミットログコピペでいいはずではある）
そもそもバックポートチケットって受益者が作るべきなんじゃないかなぁとは思ったりも。

--
NARUSE, Yui [e-mail]

[kosaki (Motohiro KOSAKI)]

言い訳にしか聞こえませんが私が望んでいるのは「〜の件」と１行書いてほしい
だけで詳細な説明は不要だと思います。どうせみんなcommit log 読んでるんだし。
ChangeLogをカット＆ペーストとかでも十分。さすがにリビジョン番号は暗記できない
からつらいなーとかそういう要望でした。

書いた後に気づいたけど、このメール内容がなるせさんのメールとまるまる一緒だ。
すいません、破棄してください。無駄にトラフィック増やしてすいません

[shyouhei (Shyouhei Urabe)]

On 2011年12月05日 14:25, NARUSE, Yui wrote:

2011年12月5日12:39 Yusuke Endoh [e-mail]:

ただ、バックポートチケットを書く人に過剰な期待をしないでという気持ち
には変わりがありません。ある程度真面目に書かないといけないとなると、
面倒で後回しにして、そのまま忘れることうけあいです。
でも、いっぱいレビューしてくれてる (し、この先もしてくれそうな)
kosaki さんが、「解読不能なバックポート依頼が来ると、他のレビューも
する気もなくなる」というなら、それはとてもまずいので、どうしても
バックポートしてほしいものだけを真面目にチケット化するようにします。

チケット作るなら簡単に内容を〜というは同意するのですが、
（というかコミットログコピペでいいはずではある）
そもそもバックポートチケットって受益者が作るべきなんじゃないかなぁとは思ったりも。

受益者というかfirst reporterは当然リリース済みの1.9.3に対してレポート
してくるわけで、それをこちらの都合でtrunkで修正しておいてからもう一回
チケット切り直せと言い出すのはさすがに違うんじゃないですか。

[kosaki (Motohiro KOSAKI)]

チケット作るなら簡単に内容を〜というは同意するのですが、
（というかコミットログコピペでいいはずではある）
そもそもバックポートチケットって受益者が作るべきなんじゃないかなぁとは思ったりも。

受益者というかfirst reporterは当然リリース済みの1.9.3に対してレポート
してくるわけで、それをこちらの都合でtrunkで修正しておいてからもう一回
チケット切り直せと言い出すのはさすがに違うんじゃないですか。

今回の件に限っていうと、ちょっとこのコメントははずしているような。
first reporterがチケットを切っている場合は、そのチケットがそのまま
バックポートチケットに流用されるのでどの話かわからない問題は発生してなくて、
これが起きるのは元チケットがないとき、つまり

１）first reporterがチケットを切らなかった
２）開発者が自分でバグを見つけて、いきなりコミットした

の２択なのです。今回の遠藤さんのケースは（１）ですね。MLで質問が
きて、チケット作成をすっ飛ばしていきなりコミット。

[shyouhei (Shyouhei Urabe)]

On 2011年12月05日 16:17, KOSAKI Motohiro wrote:

今回の件に限っていうと、ちょっとこのコメントははずしているような。

そうかもしれません。ごめんなさい。私がいいたかった事はまた別の機会にします。

[mame (Yusuke Endoh)]

遠藤です。

本題の方、時間があいてしまってすみません。

2011年12月5日10:14 Tomoyuki Chikanaga [e-mail]:

そんなにコアじゃないですがレビューしました。

• continue するよりは NameError 発生を else 節にしたほうが見通しが良いかと思います。好みの問題のような気もしますけど。
• また NameError 発生時に、1つ以上の constant の flag を修正済みの場合に rb_clear_cache_by_class() が呼ばれていないことに気がつきました
  というわけで追加の変更を添付します。

今更ですがパッチ見ました。いいと思います。
make check も問題ありませんでした。
コミットをお願いしてもいいでしょうか。

--
Yusuke Endoh [e-mail]

[nagachika (Tomoyuki Chikanaga)]

ありがとうございます。
テストを追加して r33987 でコミットしました。

[kosaki (Motohiro KOSAKI)]

• Subject changed from backport r33935 to backport r33935, r33987 (fix private constant)

[naruse (Yui NARUSE)]

• Status changed from Open to Closed

r34538 で backport しました。

[naruse (Yui NARUSE)]

• Status changed from Closed to Open
• Assignee deleted (mame (Yusuke Endoh))

r34538 でテストに失敗するようになったので revert しました

[nagachika (Tomoyuki Chikanaga)]

• File bug5702.patch bug5702.patch added

rb_to_id() の引数が trunk で変更されていたので SEGV していました。修正したバックポート用パッチを添付します。

[naruse (Yui NARUSE)]

• Status changed from Open to Closed
• % Done changed from 0 to 100

This issue was solved with changeset r34579.
Yusuke, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

---

merge revision(s) 33935,33936,33987: [Backport #5702]

* variable.c (set_const_visibility): Module#private_constant has
  changed the visibility of only the first argument.  Now it changes
  all of them.  [ruby-list:48558]

* test/ruby/test_module.rb: add a test for above.

* variable.c (set_const_visibility): print a warning when no argument
  is passwd to Module#private_constant.  [ruby-list:48558]

* vm_method.c (set_method_visibility): ditto for
  Module#private_class_method.

* variable.c (set_const_visibility): clear inine-cache when constant's
  visibility is modified. [ruby-dev:44929]

* test/ruby/test_module.rb (test_private_constants_clear_inlinecache):
  add test for it.