Index: test/versioned_test.rb
===================================================================
--- test/versioned_test.rb	(revision 2511)
+++ test/versioned_test.rb	(working copy)
@@ -5,6 +5,137 @@
 class VersionedTest < Test::Unit::TestCase
   fixtures :pages, :page_versions, :locked_pages, :locked_pages_revisions, :authors, :landmarks, :landmark_versions

+  # Common setup for the multiple_rollbacks_and_saving_of_new_version tests.
+  #
+  # NOTE: This could probaly be DRYed up by removing the first five of the six blocks of code in this method
+  # and adding the relevant data into fixtures instead.
+  def common_setup(klass, version_column, property, values)
+    # Create a new model.
+    m = klass.create property => values[:first]
+    assert_equal values[:first], m.send(property)
+    assert_equal 1, m.send(version_column)
+    assert_equal 1, m.versions.size
+    assert_equal 1, m.versions.length
+    assert_equal 1, m.versions.count
+
+    # Update the name of the model.
+    m.send(property.to_s+"=", values[:second])
+    assert m.save!
+    assert_equal values[:second], m.send(property)
+    assert_equal 2, m.send(version_column)
+    assert_equal 1, m.versions.size
+    assert_equal 1, m.versions.length
+    assert_equal 2, m.versions.count
+    assert_equal 2, m.versions(true).size # Forcing reloading of the associated versions.
+    assert_equal 2, m.versions.length
+
+    # Roll back to the original version.
+    assert m.revert_to! 1
+    assert_equal values[:first], m.send(property)
+    assert_equal 1, m.send(version_column)
+    assert_equal 2, m.versions.size
+    assert_equal 2, m.versions.length
+    assert_equal 2, m.versions.count
+    assert_equal 2, m.versions(true).size # Forcing reloading of the associated versions.
+    assert_equal 2, m.versions.length
+
+    # Roll forward to the updated version.
+    assert m.revert_to! 2
+    assert_equal values[:second], m.send(property)
+    assert_equal 2, m.send(version_column)
+    assert_equal 2, m.versions.size
+    assert_equal 2, m.versions.length
+    assert_equal 2, m.versions.count
+    assert_equal 2, m.versions(true).size # Forcing reloading of the associated versions.
+    assert_equal 2, m.versions.length
+
+    # Roll back to the original version.
+    assert m.revert_to! 1
+    assert_equal values[:first], m.send(property)
+    assert_equal 1, m.send(version_column)
+    assert_equal 2, m.versions.size
+    assert_equal 2, m.versions.length
+    assert_equal 2, m.versions.count
+    assert_equal 2, m.versions(true).size # Forcing reloading of the associated versions.
+    assert_equal 2, m.versions.length
+
+    # Update the name of the model.
+    m.send(property.to_s+"=", values[:third])
+    assert m.save!
+    assert_equal values[:third], m.send(property)
+    assert_equal 2, m.send(version_column)
+    assert_equal 2, m.versions.size
+    assert_equal 2, m.versions.length
+    assert_equal 2, m.versions.count
+    assert_equal 2, m.versions(true).size # Forcing reloading of the associated versions.
+    assert_equal 2, m.versions.length
+
+    m
+  end
+
+  # This test exercises multiple rollbacks and saving of a new version from a state where higher version
+  # numbers exist in the versions table. The Color model uses ActiveRecord's optimistic locking.
+  def test_color_multiple_rollbacks_and_saving_of_new_version
+    values = {:first => 'Red', :second =>'Green', :third => 'Blue'}
+    c = common_setup(Color, :version, :name, values)
+# These assertions would pass before the patching of acts_as_versioned.rb
+#    assert_equal 3, c.versions.count
+#    assert_equal 3, c.versions(true).size # Forcing reloading of the associated versions.
+#    assert_equal 3, c.versions.length
+
+    assert_equal 1, c.versions[0].version
+    assert_equal 2, c.versions[1].version
+# This assertion would pass before the patching of acts_as_versioned.rb as we'd have two versions with the same version number.
+#    assert_equal 2, c.versions[2].version
+
+    # Let's revert back and forth and assert that we have the same version
+    assert_equal values[:third], c.name
+    c.revert_to! 1
+    assert_equal 1, c.version
+    c.revert_to! 2
+    assert_equal 2, c.version
+    assert_equal values[:third], c.name
+  end
+
+  # This test exercises multiple rollbacks and saving of a new version from a state where higher version
+  # numbers exist in the versions table. The Widget model does not use ActiveRecord's optimistic locking.
+  def test_widget_multiple_rollbacks_and_saving_of_new_version
+    values = {:first => 'Doohickey', :second =>'Thingamabob', :third => 'Whatchamacallit'}
+    w = common_setup(Widget, :version, :name, values)
+
+    # Widget versions are sorted in reverse order
+    assert_equal 1, w.versions[1].version
+    assert_equal 2, w.versions[0].version
+
+    # Let's revert back and forth and assert that we have the same version
+    assert_equal values[:third], w.name
+    w.revert_to! 1
+    assert_equal 1, w.version
+    w.revert_to! 2
+    assert_equal 2, w.version
+    assert_equal values[:third], w.name
+  end
+
+  # This test exercises multiple rollbacks and saving of a new version from a state where higher version
+  # numbers exist in the versions table. The LockedPage model uses ActiveRecord's optimistic locking.
+  def test_locked_page_multiple_rollbacks_and_saving_of_new_version
+    values = {:first => 'The best things in life are free',
+              :second =>'Do not try this at home',
+              :third => 'Objects in mirror are probably behind you'}
+    lp = common_setup(LockedPage, :lock_version, :title, values)
+
+    assert_equal 1, lp.versions[0].version
+    assert_equal 2, lp.versions[1].version
+
+    # Let's revert back and forth and assert that we have the same version
+    assert_equal values[:third], lp.title
+    lp.revert_to! 1
+    assert_equal 1, lp.lock_version
+    lp.revert_to! 2
+    assert_equal 2, lp.lock_version
+    assert_equal values[:third], lp.title
+  end
+
   def test_saves_versioned_copy
     p = Page.create :title => 'first title', :body => 'first body'
     assert !p.new_record?
Index: test/schema.rb
===================================================================
--- test/schema.rb	(revision 2511)
+++ test/schema.rb	(working copy)
@@ -1,4 +1,15 @@
 ActiveRecord::Schema.define(:version => 0) do
+  create_table :colors, :force => true do |t|
+    t.column :lock_version, :integer
+    t.column :name, :string, :limit => 255
+  end
+
+  create_table :color_versions, :force => true do |t|
+    t.column :color_id, :integer
+    t.column :version, :integer
+    t.column :name, :string, :limit => 255
+  end
+
   create_table :pages, :force => true do |t|
     t.column :version, :integer
     t.column :title, :string, :limit => 255
Index: test/fixtures/color.rb
===================================================================
--- test/fixtures/color.rb	(revision 0)
+++ test/fixtures/color.rb	(revision 0)
@@ -0,0 +1,7 @@
+class Color < ActiveRecord::Base
+  acts_as_versioned :version_column => :lock_version
+
+  def version
+    lock_version
+  end
+end
Index: lib/acts_as_versioned.rb
===================================================================
--- lib/acts_as_versioned.rb	(revision 2511)
+++ lib/acts_as_versioned.rb	(working copy)
@@ -245,6 +245,7 @@

         # Saves a version of the model in the versioned table.  This is called in the after_save callback by default
         def save_version_on_create
+          clear_obsolete_higher_versions
           rev = self.class.versioned_class.new
           self.clone_versioned_model(self, rev)
           rev.version = send(self.class.version_column)
@@ -252,6 +253,42 @@
           rev.save
         end

+        # Clears obsolete higher versions. When using ActiveRecord's optimistic locking the versions table
+        # can end up with multiple records with the same version number if performing a revert_to to a previous
+        # version and then saving a new version. Clearing out higher versions before saving a new one will
+        # prevent this from happening.
+        #
+        # NOTE: I noticed that the first time this query is run for a new record, the query reads:
+        #   DELETE FROM thing_versions WHERE version >= 1 AND thing_id = 1
+        # This could potentially have bad consequences if someone has a Thing model, couldn't it? I don't see it
+        # in the source code of acts_as_versioned so I guess it is an ActiveRecord default?
+        #
+        # NOTE: This may break backwards compatibility if someone was relying on this feature. If this fix cannot
+        # be accepted, please consider changing the query to delete only the version with a version number equal
+        # to the current one. I.e. change ">=" to "=" in the SQL query.
+        #
+        # Here's an example.
+        #
+        # Current behavior with optimistic locking: When reverting back one or more versions and then saving a new
+        # version the end result will be that there will be two records with the same version number in the versions
+        # table. From this state, reverting back one version and forward one version might not be a deterministic
+        # operation depending on the database and sorting used as one or the other of the two records with the
+        # same version number might be selected for the new version.
+        #
+        # Current behavior without optimistic locking: When reverting back one or more versions and then saving a
+        # new version the end result will be that a new version with a version number one higher than the maximum
+        # version number in the versions table will be created. In this case, there will be no duplicate version
+        # numbers in the versions table, but the pre-save and post-save version numbers of the model being saved
+        # will differ by more than 1, i.e. a jump in version numbers will occur.
+        #
+        # Patched behavior with or without optimistic locking: When reverting back one or more versions and then
+        # saving a new version the end result will be that the new version will have a version number one higher
+        # than the previous version number and higher obsolete versions are gone.
+        def clear_obsolete_higher_versions
+          sql = "DELETE FROM #{self.class.versioned_table_name} WHERE version >= #{send(self.class.version_column)} AND #{self.class.versioned_foreign_key} = #{self.id}"
+          self.class.versioned_class.connection.execute sql
+        end
+
         # Clears old revisions if a limit is set with the :limit option in <tt>acts_as_versioned</tt>.
         # Override this method to set your own criteria for clearing old versions.
         def clear_old_versions
@@ -383,9 +420,19 @@
           end

           # Gets the next available version for the current record, or 1 for a new record
+          #
+          # The next available version is calculated as the lesser of the next available version in the model
+          # and the next available version in the versions table. The addition of checking the model and not
+          # only the versions table was made so that the proper version number would get calculated when using
+          # ActiveRecord's optimistic locking.
+          #
+          # NOTE: See the note in the clear_obsolete_versions methods. Either both this and that method needs
+          # to change, or none of them (but then there'd still be a defect).
           def next_version
             return 1 if new_record?
-            (versions.calculate(:max, :version) || 0) + 1
+            model_version = self.send("#{self.class.version_column}") + 1
+            versions_version = (versions.calculate(:max, :version) || 0) + 1
+            model_version < versions_version ? model_version : versions_version
           end

           # clears current changed attributes.  Called after save.