Ignore origin comment when checking for duplicates on Migration.copy

49ebe51 fixed copying migrations, but existing migrations would still
trigger warnings. The proper way to compare migrations is to ignore
origin lines - if migration is identical it means that we can
silently skip it, regardless where it comes from.

3 files changed, 58 insertions, 5 deletions

diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb
index 1d450da006..102a523744 100644
--- a/activerecord/lib/active_record/migration.rb
+++ b/activerecord/lib/active_record/migration.rb
@@ -461,11 +461,13 @@ module ActiveRecord
         source_migrations = ActiveRecord::Migrator.migrations(path)
 
         source_migrations.each do |migration|
-          source = File.read(migration.filename).chomp
+          source = File.read(migration.filename)
           source = "# This migration comes from #{name} (originally #{migration.version})\n#{source}"
 
           if duplicate = destination_migrations.detect { |m| m.name == migration.name }
-            options[:on_skip].call(name, migration) if File.read(duplicate.filename).chomp != source && options[:on_skip]
+            if options[:on_skip] && !migrations_identical?(File.read(duplicate.filename), source)
+              options[:on_skip].call(name, migration)
+            end
             next
           end
 
@@ -491,6 +493,22 @@ module ActiveRecord
         "%.3d" % number
       end
     end
+
+    def migrations_identical?(a, b)
+      # Due to a bug some of the migrations copied may not have origin comment,
+      # so we need to ignore it.
+      remove_origin_comment(a.chomp) == remove_origin_comment(b.chomp)
+    end
+    private :migrations_identical?
+
+    def remove_origin_comment(migration_source)
+      if migration_source =~ /^# This migration comes from/
+        migration_source = migration_source.to_a[1..-1].join
+      end
+
+      migration_source
+    end
+    private :remove_origin_comment
   end
 
   # MigrationProxy is used to defer loading of the actual migration classes
diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb
index 1997fc96ea..7c92db5def 100644
--- a/activerecord/test/cases/migration_test.rb
+++ b/activerecord/test/cases/migration_test.rb
@@ -2203,15 +2203,16 @@ if ActiveRecord::Base.connection.supports_migrations?
       @existing_migrations = Dir[@migrations_path + "/*.rb"]
 
       sources = ActiveSupport::OrderedHash.new
-      sources[:bukkits] = sources[:omg] = MIGRATIONS_ROOT + "/to_copy_with_timestamps"
+      sources[:bukkits] = MIGRATIONS_ROOT + "/to_copy_with_timestamps"
+      sources[:omg]     = MIGRATIONS_ROOT + "/to_copy_with_name_collision"
 
       skipped = []
       on_skip = Proc.new { |name, migration| skipped << "#{name} #{migration.name}" }
       copied = ActiveRecord::Migration.copy(@migrations_path, sources, :on_skip => on_skip)
       assert_equal 2, copied.length
 
-      assert_equal 2, skipped.length
-      assert_equal ["bukkits PeopleHaveHobbies", "bukkits PeopleHaveDescriptions"], skipped
+      assert_equal 1, skipped.length
+      assert_equal ["omg PeopleHaveHobbies"], skipped
     ensure
       clear
     end
@@ -2234,6 +2235,31 @@ if ActiveRecord::Base.connection.supports_migrations?
       clear
     end
 
+    def test_skip_ignores_origin_comment
+      ActiveRecord::Base.timestamped_migrations = false
+      @migrations_path = MIGRATIONS_ROOT + "/valid"
+      @existing_migrations = Dir[@migrations_path + "/*.rb"]
+
+      sources = ActiveSupport::OrderedHash.new
+      sources[:bukkits] = MIGRATIONS_ROOT + "/to_copy"
+
+      skipped = []
+      on_skip = Proc.new { |name, migration| skipped << "#{name} #{migration.name}" }
+      copied = ActiveRecord::Migration.copy(@migrations_path, sources, :on_skip => on_skip)
+
+      # remove origin comment
+      migration = @migrations_path + "/4_people_have_hobbies.rb"
+      migration_source = File.read(migration).to_a[1..-1].join
+      File.open(migration, "w") { |f| f.write migration_source }
+
+      ActiveRecord::Migration.copy(@migrations_path, sources, :on_skip => on_skip)
+
+      assert_equal 2, copied.length
+      assert_equal 0, skipped.length
+    ensure
+      clear
+    end
+
     def test_copying_migrations_to_non_existing_directory
       @migrations_path = MIGRATIONS_ROOT + "/non_existing"
       @existing_migrations = []
diff --git a/activerecord/test/migrations/to_copy_with_name_collision/1_people_have_hobbies.rb b/activerecord/test/migrations/to_copy_with_name_collision/1_people_have_hobbies.rb
new file mode 100644
index 0000000000..e438cf5999
--- /dev/null
+++ b/activerecord/test/migrations/to_copy_with_name_collision/1_people_have_hobbies.rb
@@ -0,0 +1,9 @@
+class PeopleHaveLastNames < ActiveRecord::Migration
+  def self.up
+    add_column "people", "hobbies", :string
+  end
+
+  def self.down
+    remove_column "people", "hobbies"
+  end
+end