コミットメッセージを書くときに気をつけていること

コミットメッセージに関するブログを読んだので、自分がコミットメッセージを書くときに気にしてることを書いておく。

chiastolite.hatenablog.com

sue445.hatenablog.com

基本的な話

まずは Gitのコミットメッセージの書き方 を読んでおく。

コミットメッセージは "将来の同僚のため" に書く

3ヶ月後にチームに入った人が理解できる内容 になっているのが理想。

チームメンバーが分かる内容で満足すると、暗黙的な業務知識に依存したコミットメッセージになっていることがあるので注意。

XXXを追加/変更/削除しました はダメ

油断するとこういうメッセージを書いてしまいがち。

git-diff の内容を日本語で書いただけだと意味がないので、コミットメッセージで付加価値がつくように気をつける。

コミットした後に見直す

コミットした後、git show @ を実行して、以下の点を確認する。

  1. diffの全てをコミットメッセージで説明できているか?
    • 関係ない変更が混ざっていないか?
  2. コミットメッセージを読んで「なぜ?」と自問する
    • 更に Why を深掘りして書けないか?

参考にしたURLを残す

自分が知らなかったことは、たぶん他メンバーも知らない可能性が高い。

コードを書いていて参考になったWebページがあったら、コミットメッセージに残しておく。

GitHubのURLを固定する

  1. https://github.com/rails/rails/blob/master/README.md
  2. https://github.com/rails/rails/blob/v6.0.0.rc2/README.md

1だとmasterブランチが更新されると内容が変わってしまう可能性があるので、2のようにタグの入ったURLにする。

ちなみに、GitHubでファイルを表示しているときに y を押すとURLが master から blob に変わります。*1

f:id:sinsoku:20190817013444p:plain
GitHub shortcut

URLを張りつつ、文面も引用してメッセージに含める

@sue445 さんのブログと半分同じなのですが、文面を引用して残すところが少し違うかな。

  • コミットメッセージ単体だと意味が分からなくなる事が多い
  • 自分がログを見ていたときに別ページを毎回開くのが面倒だった
  • リンク先が将来も残っているとは限らない
    • 突然リポジトリが消えてissueを読めなくなった経験があるので...*2

コミットメッセージは長くても良い

コミットメッセージが短くて不足しているより、多少冗長でも変更理由が書いてある方が嬉しい。

*1:下のヘルプ画像は ? で出ます

*2:https://tech.grooves.com/entry/2017/01/18/111900

Re:VIEWで技術書を書いているときにファイルを変更したら自動でビルドする

ファイルの変更を検知して、Re:VIEWのビルドを自動化したら便利だったのでブログに書いておく。

設定方法

vvakame/docker-reviewのdockerイメージを使ってビルドするスクリプト{project_root}/bin/build として作成しておきます。

#!/bin/sh

SHELL_PATH=$(cd $(dirname $0)/;pwd)
ROOT_PATH="${SHELL_PATH}/.."
IMAGE="vvakame/review:3.2"

docker run --rm -v ${ROOT_PATH}:/work -w /work ${IMAGE} /bin/sh -c "rake clean preproc pdf"

次にGemfileに以下の行を追加して、bundle install を実行する。

 source 'https://rubygems.org'
 
 gem 'rake'
 gem 'review', '3.2.0'
+
+ group :development do
+   gem 'listen', '~> 3.0'
+ end

あとはファイルの変更を検知して自動ビルドする {project_root}/bin/auto-build を作る。

#!ruby

require 'listen'

root_path = File.expand_path('..', __dir__)
patterns = [/\.re$/, /\.yml$/]

listener = Listen.to(root_path, only: patterns) do |modified, _added, _removed|
  next if modified.size.zero?

  result = system("#{__dir__}/build")
  # Macの人は以下をコメントアウトするとビルド後に通知できる
  # status = result ? 'success' : 'failed'
  # system("osascript -e 'display notification \"#{status}\" with title \"Re:VIEW build\"'")
end
listener.start
sleep

bin/auto-build を実行した後に xxx.re のファイルを変更すると自動でビルドされて、pdfが更新されます。

自動ビルドの前に原稿を書こう

現実逃避をしてる場合ではない。

ActiveDecoratorにプルリクを投げるまでにやったこと

怪しい挙動を見つけたきっかけ

テストコードで以下のようなことをしてました。

Rails.application.eager_load!

testでもproductionと同じ環境になるように事前に読み込んでいたのですが、テストを個別に実行するときにも全ファイルを読み込んでしまうため消したいと思っていました。

試しに消してCIを回してみると、一部のテストがfailした。

どんな問題が起きたか?

再現するコード例は以下の通り。

form = Form::User.new
ActiveDecorator::Decorator.instance.decorate(form)
form.is_a?(UserDecorator)  #=> true

Form::UserDecorator を定義してないときに、なぜか UserDecorator が使われてしまう。

コードを読んで発生箇所を突き止める

ActiveDecorator のコード内で Object.const_get を使っている箇所が UserDecorator を返していた。

そして、モジュールの自動読み込みに関する処理は ActiveSupport に定義されている。

ここまで調べて、自動読み込み周りはどう直すべきかよく分からなかった。

OSSパッチ会で松田さんに聞く

ちょうど問題を見つけた2日後にパッチ会があったので、直接聞いてみることにした。

blog.agile.esm.co.jp

a_matsuda さんに聞いてみたところ、以下のことが分かった。

  • 最近、そのあたりのコードを変えた
    • v1.2.0、v1.3.0(最新)で挙動を確認してみて欲しい
  • 意図した挙動ではない(バグっぽい)

動作確認してみると、 v1.2.0 では問題が起きず v1.3.0 で問題が起きること が判明した!

再現するテストケースを書く

v1.2.0 では問題ない事が分かったら、すぐ amatsuda/active_decorator@73dcdd6 が原因だと分かった。

問題を再現させるテストケースを書き、意図通りに失敗させて、 73dcdd6 をリバートしてテストが通ることを確認した。

英語のコミットログを書く

自分が最初に書いた英語のコミットログがこれ。

Fix a wrong decoration for nested classes

There is a problem that unintended decorators are used for nested classes.
This commit reverts 73dcdd67ae146ee9e76b1da9ae9e9dbd55529193 to fix the problem.

英語が苦手なので、OSSパッチ会に参加していた @yahonda さんにレビューしてもらいました。フィードバックは以下の通り。

  • wrongunintended が何か分からない
    • actual と expected を書いた方が良い
  • 細かいけど a wrong decoration の a は要らない
  • unintended より unexpected の方が良い

レビュー頂いて気づいたけど、英語の問題の前にコミットログの内容が足りていなかった...。

直したコミットログ

どういう問題が起きていて、何を期待しているかが分かるようにコミットログを直してプルリクを作成した。

Fix wrong decoration for nested classes

ActiveDecorator decorates incorrectly for the nested class that do not define decorator.

comic = Foo::Comic.new
ActiveDecorator::Decorator.instance.decorate(comic)
comic.is_a?(ComicDecorator)  #=> true

If `Foo::ComicDecorator` is not defined, it should not use `ComicDecorator` instead of `Foo::ComicDecorator`.
The cause of the problem is 73dcdd6, so we will revert it.

github.com

まとめ

OSSパッチ会のおかげでプルリク作るところまで出来た。ありがたい。

ActiveRecordのissueで再現コードを書くときに発行されたSQLを確認する方法

rails/rails#36895 の挙動を確認してみようかなーと思ったけど、発行されるSQLを確認する方法が分からなかったので調べた。

Rails本体のテストコードだと assert_no_queries が使えるけど、これは外で使えないので。

動いたコード

assert_no_queriesの定義されている場所を git grep 'def assert_no_queries' すると、実装はrails/activerecord/test/cases/test_case.rb に書かれていた。

必要なのはSQLCounterのクラスぐらいなので、これを再現コードにコピペする。

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"
  gem "rails", "5.2.3"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

puts
puts "Ruby = #{RUBY_VERSION}"
puts "ActiveRecord.version = #{ActiveRecord.version}"
puts

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table(:users, force: true)
  create_table(:posts, force: true) do |t|
    t.references :user
    t.timestamps
  end
end

class User < ActiveRecord::Base
  has_many :posts
  has_many :new_posts,
    -> { where("posts.created_at > ?", 1.week.ago) },
   class_name: "Post"
end

class Post < ActiveRecord::Base
  belongs_to :user
end

require "active_support/notifications"

# refs: activerecord/test/cases/test_case.rb
class SQLCounter
  class << self
    attr_accessor :ignored_sql, :log, :log_all
    def clear_log; self.log = []; self.log_all = []; end
  end

  clear_log

  self.ignored_sql = [/^PRAGMA/, /^SELECT currval/, /^SELECT CAST/, /^SELECT @@IDENTITY/, /^SELECT @@ROWCOUNT/, /^SAVEPOINT/, /^ROLLBACK TO SAVEPOINT/, /^RELEASE SAVEPOINT/, /^SHOW max_identifier_length/, /^BEGIN/, /^COMMIT/]

  # FIXME: this needs to be refactored so specific database can add their own
  # ignored SQL, or better yet, use a different notification for the queries
  # instead examining the SQL content.
  oracle_ignored     = [/^select .*nextval/i, /^SAVEPOINT/, /^ROLLBACK TO/, /^\s*select .* from all_triggers/im, /^\s*select .* from all_constraints/im, /^\s*select .* from all_tab_cols/im, /^\s*select .* from all_sequences/im]
  mysql_ignored      = [/^SHOW FULL TABLES/i, /^SHOW FULL FIELDS/, /^SHOW CREATE TABLE /i, /^SHOW VARIABLES /, /^\s*SELECT (?:column_name|table_name)\b.*\bFROM information_schema\.(?:key_column_usage|tables)\b/im]
  postgresql_ignored = [/^\s*select\b.*\bfrom\b.*pg_namespace\b/im, /^\s*select tablename\b.*from pg_tables\b/im, /^\s*select\b.*\battname\b.*\bfrom\b.*\bpg_attribute\b/im, /^SHOW search_path/i, /^\s*SELECT\b.*::regtype::oid\b/im]
  sqlite3_ignored =    [/^\s*SELECT name\b.*\bFROM sqlite_master/im, /^\s*SELECT sql\b.*\bFROM sqlite_master/im]

  [oracle_ignored, mysql_ignored, postgresql_ignored, sqlite3_ignored].each do |db_ignored_sql|
    ignored_sql.concat db_ignored_sql
  end

  attr_reader :ignore

  def initialize(ignore = Regexp.union(self.class.ignored_sql))
    @ignore = ignore
  end

  def call(name, start, finish, message_id, values)
    return if values[:cached]

    sql = values[:sql]
    self.class.log_all << sql
    self.class.log << sql unless ignore.match?(sql)
  end
end

ActiveSupport::Notifications.subscribe("sql.active_record", SQLCounter.new)

def capture_sql
  SQLCounter.clear_log
  yield
  SQLCounter.log.dup
end

require "active_support/testing/time_helpers"

class BugTest < Minitest::Test
  include ActiveSupport::Testing::TimeHelpers

  def test_to_reproduce
    travel_to Time.new(2019, 8, 10)
    User.create

    log = capture_sql { User.includes(:new_posts).to_a }
    assert_equal 'SELECT "users".* FROM "users"', log[0]
    assert_equal 'SELECT "posts".* FROM "posts" WHERE (posts.created_at > \'2019-08-02 15:00:00\') AND "posts"."user_id" = ?', log[1]
  end
end

includes に関する挙動は to_sql だと確認できないので、そういうバグを踏んだときに使えるようにメモしておく。

Railsのissueに書いてある再現コードを使ってgit bisectする方法

備忘録。

具体的な例

先日に登録されたissueを例にしてみる。

github.com

再現コードを少し書き換える

v5.2.3と6.0.0.rc2でsqlite3の依存周りでエラーが出たので、Gemfileのあたりを少しだけ弄った。

# frozen_string_literal: true

require "bundler/inline"

gemfile = File.read(File.expand_path("Gemfile", __dir__)).strip
version = gemfile.lines.find { |line| line.include?('sqlite3') }
  .then { |sqlite3_line| sqlite3_line.strip.split(',').last.delete('"') }

gemfile(true) do
  source "https://rubygems.org"
  gem "rails", path: "."
  gem 'sqlite3', version
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :articles, force: true do |t|
    t.string :title
    t.string :category
  end
end

class Article < ActiveRecord::Base
end

puts
puts "ActiveRecord.version = #{ActiveRecord.version}"
puts

class BugTest < Minitest::Test
  def test_to_group_by_count_with_order
    article = Article.create(title: 'foo', category: 'foo')

    assert_equal ({"foo"=>1}), Article.group(:category).order('count_articles_id').count('`articles`.`id`')
  end
end

実際の手順

$ git clone https://github.com/rails/rails.git
$ cd rails
$ vim
(上記の再現コードを issue.rb として保存)
$ git bisect start v6.0.0.rc2 v5.2.3
$ git bisect run ruby issue.rb
.
.
.
(コーヒーでも飲みながら待つ)
.
.
.
Finished in 0.008043s, 124.3317 runs/s, 0.0000 assertions/s.
1 runs, 0 assertions, 0 failures, 1 errors, 0 skips
c9e4c848eeeb8999b778fa1ae52185ca5537fffe is the first bad commit
commit c9e4c848eeeb8999b778fa1ae52185ca5537fffe
Author: Ryuta Kamizono <kamipo@gmail.com>
Date:   Sat Apr 6 13:04:06 2019 +0900

    Don't repeat same expression in SELECT and GROUP BY clauses

    This refactors `execute_grouped_calculation` and slightly changes
    generated GROUP BY queries, since I'd not prefer to repeat same
    expression in SELECT and GROUP BY clauses.

    Before:

    ```
    SELECT COUNT(*) AS count_all, "topics"."author_name" AS topics_author_name, COALESCE(type, title) AS coalesce_type_title FROM "topics" GROUP BY "topics"."author_name", COALESCE(type, title)
    ```

    After:

    ```
    SELECT COUNT(*) AS count_all, "topics"."author_name" AS topics_author_name, COALESCE(type, title) AS coalesce_type_title FROM "topics" GROUP BY topics_author_name, coalesce_type_title
    ```

    Although we generally don't guarantee to support Arel node constructed
    by user itself, this also fixes #24207.

 .../lib/active_record/relation/calculations.rb     | 46 ++++++++++------------
 1 file changed, 20 insertions(+), 26 deletions(-)
bisect run success

なるほど、こうやってコミットを探すのか。

追記

bisect が終わった後に元のブランチに戻るには git bisect reset を実行する。

minitestで書かれたActiveRecordのテストで特定のケースだけを実行する方法

自分用の備忘録。

特定のファイルを実行する方法

$ cd activerecord
$ ARCONN=postgresql ruby \
  -I"lib:test" \
  test/cases/adapters/postgresql/range_test.rb

特定のテストケースを実行する方法

$ cd activerecord
$ ARCONN=postgresql ruby \
  -I"lib:test" \
  test/cases/adapters/postgresql/range_test.rb \
  -n test_endless_range_values

DMMを退職しました

退職エントリを書くつもりはあまり無かったけど、はてブに退職エントリが多くあがっていて書く気力が高まったので書いてみた。

DMMに入社した理由

去年の5月に入社しました。

  • 大きな企業の便利な社内ツール・サービスを見たい
  • 色んな種類のサービスの裏側を見たい
  • Railsアプリをゼロから作る機会
    • 仕事で rails new できる
    • プロジェクト初期からレビューできる

あたりに魅力を感じて内定を承諾した。

DMMでやっていたこと

とあるサービスのリプレイス案件*1のサーバサイドエンジニアを担当していた。

  • 既存のデプロイ方法の修正
  • AWSアーキテクチャ設計
  • APIや管理画面をRailsで実装
  • RubyRailsに関するレビュー(他メンバーがあまりRailsに慣れていなかった)
  • Terraformを使ったインフラ業
  • 死活監視の設定
  • タスク管理

この案件、入社して2ヶ月でデスマの気配を感じていた。

そして、9月くらいにデスマを避けられないと感じていた。

リリース前は死ぬほど大変だったけど、なんとか生きてリリース日を乗り越えることが出来て良かった。
デスマを予測できていたのに、どうにも出来なくて自分のスキル不足を痛感した案件だった。

プロジェクト管理に関しては 言いたい事が山程ある けど、これは綺麗に書ける気がしないので控えておく。*2

リリース後

リリース後は落ち着いたのでふりかえりをしたり、開発の進め方を変えたりしていた。 そして、役割がスクラムマスターになったのでスクラムマスター業をしていた。

あと、隙間時間で他チームのRailsアプリのレビューを勝手にしていた。 パフォーマンス改善や脆弱性を指摘できたので、少しは貢献できていたと思う。

辞めた理由

色々とあるけど、あまり書くと残っている方々に迷惑がかかるので、当たり障りのない範囲だけ書いておく。

  • 自分のやりたい事は終わった
    • 大きな企業の社内サービス、ツールの状況がわかった
    • Railsでゼロからアプリを作ってリリースまで終わった
  • 査定結果が年収640万だった
    • 年収交渉したが、事業部で昇給するのは無理だった
    • 異動を含めた交渉もしたが、無理だった
  • DMMに残りたい理由が特になかった

DMMは売上のある事業をいくつも持っているし、多くのエンジニアを抱えていてポテンシャルは高いと思う。 ただ、制度や組織体制には微妙な点(=改善できそうな点)が非常に多い。

こういう点を直すのには興味あって、横断的な部署に異動して技術で会社を改善していきたいな...と半年前くらいから1on1で異動の打診はしていた。 ただ、需要がないのか、自分の技術不足なのか無理だった。

それ以外で特にDMMでやりたい事もなかったので、退職することにした。

次にやること

去年の開発で疲れたので、当分はフリーランスで週1だけ仕事して、残りはOSSにパッチでも投げる生活をしようかなと思っています。

あと、作ってみたい個人サービスのネタがあるので、プロトタイプを作ってみる予定。

*1:残念ながらアダルト関連ではない

*2:興味ある人はオフラインで聞いてください