[インデックス 14228] ファイルの概要
このコミットは、Goプロジェクトのコードレビューダッシュボードツールにおいて、「NOT LGTM」というレビューコマンドの認識ロジックを修正するものです。具体的には、レビューメッセージの最初の行にのみ「NOT LGTM」が存在する場合に限り、そのコマンドを有効と判断するように変更されました。これにより、メールの「トップポスティング」慣習によってレビューコマンドが意図せず本文中に含まれてしまう問題を回避します。
コミット
commit b506c3e17aad13ec64ff24d86ec034259bd224d1
Author: David Symonds <dsymonds@golang.org>
Date: Mon Oct 29 22:03:58 2012 +1100
misc/dashboard/codereview: only accept "NOT LGTM" on the first line of a message.
Too many people quote entire emails and put their reply at the top ("top posting"),
so we shouldn't recognise review commands anywhere in the review text.
R=golang-dev, bradfitz
CC=golang-dev
https://golang.org/cl/6815048
GitHub上でのコミットページへのリンク
https://github.com/golang/go/commit/b506c3e17aad13ec64ff24d86ec034259bd224d1
元コミット内容
misc/dashboard/codereview ツールにおいて、「NOT LGTM」というレビューコマンドをメッセージの最初の行でのみ受け入れるように変更します。これは、多くの人がメール全体を引用し、その返信を冒頭に書く「トップポスティング」を行うため、レビューテキストのどこかにレビューコマンドが認識されるべきではないという問題に対処するためです。
変更の背景
この変更の背景には、Goプロジェクトが当時利用していたコードレビューシステム（おそらくGoogleの内部ツールであるRietveldやGerritのカスタマイズ版）の運用上の課題がありました。当時のコードレビューは、メール通知と連携しており、レビューコメントや承認/非承認のステータスがメールの本文を通じてやり取りされることが一般的でした。
問題となったのは、メールの返信スタイルの一つである「トップポスティング」です。これは、元のメールの全文を引用した上で、その引用の冒頭に自分の返信を書くという慣習です。このスタイルが広まっていたため、レビュー担当者が「NOT LGTM」のようなレビューコマンドを意図して書いたわけではないにもかかわらず、過去のメールの引用部分にその文字列が含まれてしまい、システムが誤って「NOT LGTM」と認識してしまう事態が発生していました。
このような誤認識は、コードレビューのステータスを不正確にし、開発ワークフローに混乱をもたらす可能性がありました。例えば、実際には承認されていない変更が誤って「非承認」とマークされたり、その逆の状況が発生したりすることが考えられます。このコミットは、このような誤認識を防ぎ、レビューコマンドの解釈をより厳密にすることで、コードレビュープロセスの信頼性を向上させることを目的としています。
前提知識の解説
LGTM / NOT LGTM
「LGTM」は "Looks Good To Me" の略で、ソフトウェア開発におけるコードレビューで使われる一般的なフレーズです。これは、レビュー担当者が提出されたコードをレビューし、問題がないと判断した場合に承認を示すために使用されます。多くのコードレビューシステムでは、このフレーズを認識して、コードの承認ステータスを更新する機能を持っています。
「NOT LGTM」は「LGTM」の反対で、レビュー担当者がコードに問題があると判断し、承認できない場合に非承認を示すために使用されます。このフレーズもまた、システムによって認識され、コードの非承認ステータスを更新するために利用されます。
これらのフレーズは、特にGoogleのコードレビュー文化から広まったとされており、RietveldやGerritといったツールで広く採用されています。
コードレビューシステム (golang.org/cl)
golang.org/cl は、Goプロジェクトがコードレビューに利用しているシステムへのリンクです。これはGoogleが開発したRietveldというコードレビューツール、またはその進化形であるGerritをベースにカスタマイズされたものである可能性が高いです。これらのシステムは、パッチセットのアップロード、コメントの追加、LGTM/NOT LGTMなどのステータス変更、そして最終的な変更の承認とマージといった一連のコードレビュープロセスをサポートします。メール通知と密接に連携しており、レビューのやり取りがメールで行われることも一般的です。
トップポスティング (Top Posting)
トップポスティングとは、電子メールの返信スタイルの一つで、元のメールの全文または一部を引用した上で、その引用の冒頭に自分の返信やコメントを記述する形式を指します。
例：
（ここにあなたの返信）
> （元のメールの引用）
このスタイルは、特にメーリングリストやフォーラムなど、多くの人が参加するコミュニケーションで、文脈を保持しつつ迅速に返信するために用いられることがあります。しかし、引用部分が長くなると、本当に伝えたいメッセージが埋もれてしまったり、過去のやり取りが何度も繰り返されたりする原因となることもあります。
このコミットの文脈では、過去のメールの引用部分に「NOT LGTM」という文字列が含まれてしまうことで、コードレビューシステムが誤ってそのコマンドを認識してしまうという問題を引き起こしていました。
appengine.Context
appengine.Context は、Google App Engine（GAE）で実行されるGoアプリケーションにおいて、リクエスト固有のコンテキストを管理するためのインターフェースです。GAEは、Googleが提供するPaaS（Platform as a Service）であり、ウェブアプリケーションやモバイルバックエンドを構築・デプロイするためのプラットフォームです。
appengine.Context は、現在のリクエストに関する情報（例えば、ユーザー情報、データストアへのアクセス、ログ記録、キャッシュなど）を提供し、GAEのサービスと連携するために使用されます。このコミットのコードが misc/dashboard/codereview/dashboard/cl.go にあることから、このコードレビューダッシュボードがGoogle App Engine上で動作していたことが示唆されます。
技術的詳細
このコミットは、Go言語で書かれたコードレビューダッシュボードのバックエンドロジックの一部を変更しています。変更の核心は、レビューコメントのテキストから「NOT LGTM」という文字列を検出する方法にあります。
元のコードでは、strings.Contains(msg.Text, "NOT LGTM") を使用していました。ここで msg.Text はレビューコメントの全文を表します。このため、コメントのどこかに「NOT LGTM」という文字列が含まれていれば、それがレビューコマンドとして認識されていました。
変更後のコードでは、strings.Contains(line, "NOT LGTM") となっています。これは、レビューコメントの全文ではなく、おそらくコメントの各行を個別に処理し、その「行」の中に「NOT LGTM」が含まれているかをチェックするように変更されたことを示唆しています。コミットメッセージの「only accept "NOT LGTM" on the first line of a message」という説明から、この line 変数にはレビューコメントの最初の行が格納されるように、その前の処理が変更されたか、あるいは updateCL 関数内でコメントが複数行に分割され、最初の行のみが line に渡されるようになったと推測できます。
この変更により、メールのトップポスティングによって引用された過去のメッセージ中に「NOT LGTM」が含まれていても、それがレビューコマンドとして誤認識されることがなくなります。システムは、レビュー担当者が意図的にコメントの冒頭に「NOT LGTM」と記述した場合にのみ、そのコマンドを有効と判断するようになります。
updateCL 関数は、おそらく特定の変更リスト（Change List, CL）のステータスを更新する役割を担っています。この関数内で、レビューコメントを解析し、「LGTM」や「NOT LGTM」といったコマンドを検出して、CLの承認/非承認ステータスを管理していると考えられます。lgtm と notLGTM は、おそらく各レビュー担当者からのLGTM/NOT LGTMステータスを追跡するためのマップ（map[string]bool）であると推測されます。
コアとなるコードの変更箇所
diff --git a/misc/dashboard/codereview/dashboard/cl.go b/misc/dashboard/codereview/dashboard/cl.go
index 4187ca6855..dce2744257 100644
--- a/misc/dashboard/codereview/dashboard/cl.go
+++ b/misc/dashboard/codereview/dashboard/cl.go
@@ -389,7 +389,7 @@ func updateCL(c appengine.Context, n string) error {
t\tlgtm[s] = true
t\tdelete(notLGTM, s) // "LGTM" overrules previous "NOT LGTM"
t}
- t\tif strings.Contains(msg.Text, "NOT LGTM") {
+ t\tif strings.Contains(line, "NOT LGTM") {
t\t\tnotLGTM[s] = true
t\t\tdelete(lgtm, s) // "NOT LGTM" overrules previous "LGTM"
t}
コアとなるコードの解説
変更は misc/dashboard/codereview/dashboard/cl.go ファイルの updateCL 関数内で行われています。
元のコード:
if strings.Contains(msg.Text, "NOT LGTM") {
notLGTM[s] = true
delete(lgtm, s) // "NOT LGTM" overrules previous "LGTM"
}
この行では、msg.Text という変数（おそらくレビューコメントの全文を保持）の中に「NOT LGTM」という文字列が含まれているかどうかを strings.Contains 関数でチェックしていました。このため、コメントのどの部分に「NOT LGTM」があっても、それが検出されていました。
変更後のコード:
if strings.Contains(line, "NOT LGTM") {
notLGTM[s] = true
delete(lgtm, s) // "NOT LGTM" overrules previous "LGTM"
}
変更点としては、msg.Text が line に置き換えられています。この line 変数は、コミットメッセージの意図から、レビューコメントの「最初の行」を指すものと推測されます。これにより、「NOT LGTM」の検出はコメントの最初の行に限定され、メールの引用部分など、意図しない場所に含まれる「NOT LGTM」が誤ってコマンドとして認識されることを防ぎます。
この変更は、コードレビューシステムの堅牢性を高め、レビューコマンドの解釈における曖昧さを排除することを目的としています。
関連リンク
- https://golang.org/cl/6815048 - このコミットに対応するGoコードレビューシステム上の変更リスト（Change List）のリンク。
参考にした情報源リンク
- Google App Engine (GAE)
- Rietveld (Googleのコードレビューツール、現在はGerritなどに移行)
- Gerrit Code Review
- Top-posting - Wikipedia
- LGTM - Urban Dictionary
- Go言語 strings.Contains 関数 (Go言語の標準ライブラリに関する情報)
[インデックス 14228] ファイルの概要
このコミットは、Goプロジェクトのコードレビューダッシュボードツールにおいて、「NOT LGTM」というレビューコマンドの認識ロジックを修正するものです。具体的には、レビューメッセージの最初の行にのみ「NOT LGTM」が存在する場合に限り、そのコマンドを有効と判断するように変更されました。これにより、メールの「トップポスティング」慣習によってレビューコマンドが意図せず本文中に含まれてしまう問題を回避します。
コミット
commit b506c3e17aad13ec64ff24d86ec034259bd224d1
Author: David Symonds <dsymonds@golang.org>
Date: Mon Oct 29 22:03:58 2012 +1100
misc/dashboard/codereview: only accept "NOT LGTM" on the first line of a message.
Too many people quote entire emails and put their reply at the top ("top posting"),
so we shouldn't recognise review commands anywhere in the review text.
R=golang-dev, bradfitz
CC=golang-dev
https://golang.org/cl/6815048
GitHub上でのコミットページへのリンク
https://github.com/golang/go/commit/b506c3e17aad13ec64ff24d86ec034259bd224d1
元コミット内容
misc/dashboard/codereview ツールにおいて、「NOT LGTM」というレビューコマンドをメッセージの最初の行でのみ受け入れるように変更します。これは、多くの人がメール全体を引用し、その返信を冒頭に書く「トップポスティング」を行うため、レビューテキストのどこかにレビューコマンドが認識されるべきではないという問題に対処するためです。
変更の背景
この変更の背景には、Goプロジェクトが当時利用していたコードレビューシステム（おそらくGoogleの内部ツールであるRietveldやGerritのカスタマイズ版）の運用上の課題がありました。当時のコードレビューは、メール通知と連携しており、レビューコメントや承認/非承認のステータスがメールの本文を通じてやり取りされることが一般的でした。
問題となったのは、メールの返信スタイルの一つである「トップポスティング」です。これは、元のメールの全文を引用した上で、その引用の冒頭に自分の返信を書くという慣習です。このスタイルが広まっていたため、レビュー担当者が「NOT LGTM」のようなレビューコマンドを意図して書いたわけではないにもかかわらず、過去のメールの引用部分にその文字列が含まれてしまい、システムが誤って「NOT LGTM」と認識してしまう事態が発生していました。
このような誤認識は、コードレビューのステータスを不正確にし、開発ワークフローに混乱をもたらす可能性がありました。例えば、実際には承認されていない変更が誤って「非承認」とマークされたり、その逆の状況が発生したりすることが考えられます。このコミットは、このような誤認識を防ぎ、レビューコマンドの解釈をより厳密にすることで、コードレビュープロセスの信頼性を向上させることを目的としています。
前提知識の解説
LGTM / NOT LGTM
「LGTM」は "Looks Good To Me" の略で、ソフトウェア開発におけるコードレビューで使われる一般的なフレーズです。これは、レビュー担当者が提出されたコードをレビューし、問題がないと判断した場合に承認を示すために使用されます。多くのコードレビューシステムでは、このフレーズを認識して、コードの承認ステータスを更新する機能を持っています。
「NOT LGTM」は「LGTM」の反対で、レビュー担当者がコードに問題があると判断し、承認できない場合に非承認を示すために使用されます。このフレーズもまた、システムによって認識され、コードの非承認ステータスを更新するために利用されます。
これらのフレーズは、特にGoogleのコードレビュー文化から広まったとされており、RietveldやGerritといったツールで広く採用されています。
コードレビューシステム (golang.org/cl)
golang.org/cl は、Goプロジェクトがコードレビューに利用しているシステムへのリンクです。これはGoogleが開発したRietveldというコードレビューツール、またはその進化形であるGerritをベースにカスタマイズされたものである可能性が高いです。これらのシステムは、パッチセットのアップロード、コメントの追加、LGTM/NOT LGTMなどのステータス変更、そして最終的な変更の承認とマージといった一連のコードレビュープロセスをサポートします。メール通知と密接に連携しており、レビューのやり取りがメールで行われることも一般的です。
トップポスティング (Top Posting)
トップポスティングとは、電子メールの返信スタイルの一つで、元のメールの全文または一部を引用した上で、その引用の冒頭に自分の返信やコメントを記述する形式を指します。
例：
（ここにあなたの返信）
> （元のメールの引用）
このスタイルは、特にメーリングリストやフォーラムなど、多くの人が参加するコミュニケーションで、文脈を保持しつつ迅速に返信するために用いられることがあります。しかし、引用部分が長くなると、本当に伝えたいメッセージが埋もれてしまったり、過去のやり取りが何度も繰り返されたりする原因となることもあります。
このコミットの文脈では、過去のメールの引用部分に「NOT LGTM」という文字列が含まれてしまうことで、コードレビューシステムが誤ってそのコマンドを認識してしまうという問題を引き起こしていました。
appengine.Context
appengine.Context は、Google App Engine（GAE）で実行されるGoアプリケーションにおいて、リクエスト固有のコンテキストを管理するためのインターフェースです。GAEは、Googleが提供するPaaS（Platform as a Service）であり、ウェブアプリケーションやモバイルバックエンドを構築・デプロイするためのプラットフォームです。
appengine.Context は、現在のリクエストに関する情報（例えば、ユーザー情報、データストアへのアクセス、ログ記録、キャッシュなど）を提供し、GAEのサービスと連携するために使用されます。このコミットのコードが misc/dashboard/codereview/dashboard/cl.go にあることから、このコードレビューダッシュボードがGoogle App Engine上で動作していたことが示唆されます。
技術的詳細
このコミットは、Go言語で書かれたコードレビューダッシュボードのバックエンドロジックの一部を変更しています。変更の核心は、レビューコメントのテキストから「NOT LGTM」という文字列を検出する方法にあります。
元のコードでは、strings.Contains(msg.Text, "NOT LGTM") を使用していました。ここで msg.Text はレビューコメントの全文を表します。このため、コメントのどこかに「NOT LGTM」という文字列が含まれていれば、それがレビューコマンドとして認識されていました。
変更後のコードでは、strings.Contains(line, "NOT LGTM") となっています。これは、レビューコメントの全文ではなく、おそらくコメントの各行を個別に処理し、その「行」の中に「NOT LGTM」が含まれているかをチェックするように変更されたことを示唆しています。コミットメッセージの「only accept "NOT LGTM" on the first line of a message」という説明から、この line 変数にはレビューコメントの最初の行が格納されるように、その前の処理が変更されたか、あるいは updateCL 関数内でコメントが複数行に分割され、最初の行のみが line に渡されるようになったと推測できます。
この変更により、メールのトップポスティングによって引用された過去のメッセージ中に「NOT LGTM」が含まれていても、それがレビューコマンドとして誤認識されることがなくなります。システムは、レビュー担当者が意図的にコメントの冒頭に「NOT LGTM」と記述した場合にのみ、そのコマンドを有効と判断するようになります。
updateCL 関数は、おそらく特定の変更リスト（Change List, CL）のステータスを更新する役割を担っています。この関数内で、レビューコメントを解析し、「LGTM」や「NOT LGTM」といったコマンドを検出して、CLの承認/非承認ステータスを管理していると考えられます。lgtm と notLGTM は、おそらく各レビュー担当者からのLGTM/NOT LGTMステータスを追跡するためのマップ（map[string]bool）であると推測されます。
コアとなるコードの変更箇所
diff --git a/misc/dashboard/codereview/dashboard/cl.go b/misc/dashboard/codereview/dashboard/cl.go
index 4187ca6855..dce2744257 100644
--- a/misc/dashboard/codereview/dashboard/cl.go
+++ b/misc/dashboard/codereview/dashboard/cl.go
@@ -389,7 +389,7 @@ func updateCL(c appengine.Context, n string) error {
t\tlgtm[s] = true
t\tdelete(notLGTM, s) // "LGTM" overrules previous "NOT LGTM"
t}
- t\tif strings.Contains(msg.Text, "NOT LGTM") {
+ t\tif strings.Contains(line, "NOT LGTM") {
t\t\tnotLGTM[s] = true
t\t\tdelete(lgtm, s) // "NOT LGTM" overrules previous "LGTM"
t}
コアとなるコードの解説
変更は misc/dashboard/codereview/dashboard/cl.go ファイルの updateCL 関数内で行われています。
元のコード:
if strings.Contains(msg.Text, "NOT LGTM") {
notLGTM[s] = true
delete(lgtm, s) // "NOT LGTM" overrules previous "LGTM"
}
この行では、msg.Text という変数（おそらくレビューコメントの全文を保持）の中に「NOT LGTM」という文字列が含まれているかどうかを strings.Contains 関数でチェックしていました。このため、コメントのどの部分に「NOT LGTM」があっても、それが検出されていました。
変更後のコード:
if strings.Contains(line, "NOT LGTM") {
notLGTM[s] = true
delete(lgtm, s) // "NOT LGTM" overrules previous "LGTM"
}
変更点としては、msg.Text が line に置き換えられています。この line 変数は、コミットメッセージの意図から、レビューコメントの「最初の行」を指すものと推測されます。これにより、「NOT LGTM」の検出はコメントの最初の行に限定され、メールの引用部分など、意図しない場所に含まれる「NOT LGTM」が誤ってコマンドとして認識されることを防ぎます。
この変更は、コードレビューシステムの堅牢性を高め、レビューコマンドの解釈における曖昧さを排除することを目的としています。
関連リンク
- https://golang.org/cl/6815048 - このコミットに対応するGoコードレビューシステム上の変更リスト（Change List）のリンク。
参考にした情報源リンク
- Google App Engine (GAE)
- Rietveld (Googleのコードレビューツール、現在はGerritなどに移行)
- Gerrit Code Review
- Top-posting - Wikipedia
- LGTM - Urban Dictionary
- Go言語 strings.Contains 関数 (Go言語の標準ライブラリに関する情報)