From 713888609c339334473c8a39c703f6af78455cdb Mon Sep 17 00:00:00 2001
From: Jeremy Bopp <jeremy@bopp.net>
Date: Wed, 30 Nov 2011 14:57:33 -0600
Subject: [PATCH 2/3] Optimize Git operations for new branches

---
 app/models/repository/git.rb                       |   40 +++++++-------
 .../20111129232941_reformat_git_extra_info.rb      |   15 +++++
 lib/redmine/scm/adapters/git_adapter.rb            |   24 ++++++--
 .../lib/redmine/scm/adapters/git_adapter_test.rb   |   45 +++++++++++++++
 test/unit/repository_git_test.rb                   |   58 ++++++++++----------
 5 files changed, 127 insertions(+), 55 deletions(-)
 create mode 100644 db/migrate/20111129232941_reformat_git_extra_info.rb

diff --git a/app/models/repository/git.rb b/app/models/repository/git.rb
index 586a6e5..6d817fc 100644
--- a/app/models/repository/git.rb
+++ b/app/models/repository/git.rb
@@ -79,6 +79,10 @@ class Repository::Git < Repository
     scm.tags
   end
 
+  def head_revisions
+    scm.head_revisions
+  end
+
   def default_branch
     scm.default_branch
   rescue Exception => e
@@ -129,11 +133,10 @@ class Repository::Git < Repository
   # The repository can still be fully reloaded by calling #clear_changesets
   # before fetching changesets (eg. for offline resync)
   def fetch_changesets
-    scm_brs = branches
-    return if scm_brs.nil? || scm_brs.empty?
+    new_head_scmids = head_revisions
+    return if new_head_scmids.nil? || new_head_scmids.empty?
     h1 = extra_info || {}
     h  = h1.dup
-    h["branches"]       ||= {}
     h["db_consistent"]  ||= {}
     if changesets.count == 0
       h["db_consistent"]["ordering"] = 1
@@ -144,27 +147,24 @@ class Repository::Git < Repository
       merge_extra_info(h)
       self.save
     end
-    scm_brs.each do |br1|
-      br = br1.to_s
-      from_scmid = nil
-      from_scmid = h["branches"][br]["last_scmid"] if h["branches"][br]
-      h["branches"][br] ||= {}
-      scm.revisions('', from_scmid, br, {:reverse => true}) do |rev|
+    h["head_scmids"] ||= []
+    transaction do
+      scm.revisions('',
+                    h["head_scmids"], new_head_scmids,
+                    {:reverse => true}) do |rev|
         db_rev = find_changeset_by_name(rev.revision)
-        transaction do
-          if db_rev.nil?
-            db_saved_rev = save_revision(rev)
-            parents = {}
-            parents[db_saved_rev] = rev.parents unless rev.parents.nil?
-            parents.each do |ch, chparents|
-              ch.parents = chparents.collect{|rp| find_changeset_by_name(rp)}.compact
-            end
+        if db_rev.nil?
+          db_saved_rev = save_revision(rev)
+          unless rev.parents.nil?
+            db_saved_rev.parents =
+              rev.parents.collect{|rp| find_changeset_by_name(rp)}.compact
           end
-          h["branches"][br]["last_scmid"] = rev.scmid
-          merge_extra_info(h)
-          self.save
         end
       end
+      # Update saved head revisions.
+      h["head_scmids"] = new_head_scmids.dup
+      merge_extra_info(h)
+      self.save
     end
   end
 
diff --git a/db/migrate/20111129232941_reformat_git_extra_info.rb b/db/migrate/20111129232941_reformat_git_extra_info.rb
new file mode 100644
index 0000000..3f14e4d
--- /dev/null
+++ b/db/migrate/20111129232941_reformat_git_extra_info.rb
@@ -0,0 +1,15 @@
+class ReformatGitExtraInfo < ActiveRecord::Migration
+  def self.up
+    Repository.find(:all, :conditions => ['type = ?', 'Git']).each do |repo|
+      repo.extra_info['head_scmids'] =
+        repo.extra_info['branches'].map{|br, h| h['last_scmid']}
+      repo.extra_info.delete('branches')
+      repo.write_attribute(:extra_info, repo.extra_info)
+      repo.save
+    end
+  end
+
+  def self.down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
diff --git a/lib/redmine/scm/adapters/git_adapter.rb b/lib/redmine/scm/adapters/git_adapter.rb
index 18850fb..c70f6ca 100644
--- a/lib/redmine/scm/adapters/git_adapter.rb
+++ b/lib/redmine/scm/adapters/git_adapter.rb
@@ -102,6 +102,16 @@ module Redmine
           nil
         end
 
+        def head_revisions
+          return @head_revisions if @head_revisions
+          cmd_args = %w|show-ref --heads|
+          @head_revisions = scm_cmd(*cmd_args) do |io|
+            io.map{|line| line.match('(.*?)\s+refs/heads/.*')[1]}.sort!
+          end
+        rescue ScmCommandAborted
+          nil
+        end
+
         def default_branch
           bras = self.branches
           return nil if bras.nil?
@@ -187,17 +197,19 @@ module Redmine
           nil
         end
 
-        def revisions(path, identifier_from, identifier_to, options={})
+        def revisions(path, excludes, includes, options={})
+          # Ensure that includes and excludes are lists (possibly empty).
+          includes = [includes].compact.flatten
+          excludes = [excludes].compact.flatten
+
           revs = Revisions.new
           cmd_args = %w|log --no-color --encoding=UTF-8 --raw --date=iso --pretty=fuller --parents|
           cmd_args << "--reverse" if options[:reverse]
           cmd_args << "--all" if options[:all]
           cmd_args << "-n" << "#{options[:limit].to_i}" if options[:limit]
-          from_to = ""
-          from_to << "#{identifier_from}.." if identifier_from
-          from_to << "#{identifier_to}" if identifier_to
-          cmd_args << from_to if !from_to.empty?
           cmd_args << "--since=#{options[:since].strftime("%Y-%m-%d %H:%M:%S")}" if options[:since]
+          cmd_args.push(*includes) unless includes.empty?
+          cmd_args.push("--not", *excludes) unless excludes.empty?
           cmd_args << "--" << scm_iconv(@path_encoding, 'UTF-8', path) if path && !path.empty?
 
           scm_cmd *cmd_args do |io|
@@ -284,7 +296,7 @@ module Redmine
           end
           revs
         rescue ScmCommandAborted => e
-          logger.error("git log #{from_to.to_s} error: #{e.message}")
+          logger.error("git log error: #{e.message}")
           revs
         end
 
diff --git a/test/unit/lib/redmine/scm/adapters/git_adapter_test.rb b/test/unit/lib/redmine/scm/adapters/git_adapter_test.rb
index c5da973..8750223 100644
--- a/test/unit/lib/redmine/scm/adapters/git_adapter_test.rb
+++ b/test/unit/lib/redmine/scm/adapters/git_adapter_test.rb
@@ -87,6 +87,15 @@ begin
             ], @adapter.tags
       end
 
+      def test_head_revisions
+        assert_equal  [
+              "1ca7f5ed374f3cb31a93ae5215c2e25cc6ec5127",
+              "67e7792ce20ccae2e4bb73eed09bb397819c8834",
+              "83ca5fd546063a3c7dc2e568ba3355661a9e2b2c",
+              "fba357b886984ee71185ad2065e65fc0417d9b92"
+            ], @adapter.head_revisions
+      end
+
       def test_getting_all_revisions
         assert_equal 21, @adapter.revisions('',nil,nil,:all => true).length
       end
@@ -95,6 +104,42 @@ begin
         assert_equal 1, @adapter.revisions('','899a15d^','899a15d').length
       end
 
+      def test_getting_multiple_named_revisions
+        revs = @adapter.revisions('',nil,['4f26664'])
+        assert_equal 13, revs.length
+        rev_ids = revs.map{|rev| rev.identifier}
+        assert rev_ids.include?("4f26664364207fa8b1af9f8722647ab2d4ac5d43"),
+             "Expected #{rev_ids.inspect} to include \"4f26664364207fa8b1af9f8722647ab2d4ac5d43\""
+
+        revs = @adapter.revisions('',nil,['4f26664', '57ca437'])
+        assert_equal 15, revs.length
+        rev_ids = revs.map{|rev| rev.identifier}
+        assert rev_ids.include?("4f26664364207fa8b1af9f8722647ab2d4ac5d43"),
+             "Expected #{rev_ids.inspect} to include \"4f26664364207fa8b1af9f8722647ab2d4ac5d43\""
+        assert rev_ids.include?("57ca437c0acbbcb749821fdf3726a1367056d364"),
+             "Expected #{rev_ids.inspect} to include \"57ca437c0acbbcb749821fdf3726a1367056d364\""
+      end
+
+      def test_excluding_multiple_revisions
+        revs = @adapter.revisions('',['32ae898'],'4f26664')
+        assert_equal 2, revs.length
+        rev_ids = revs.map{|rev| rev.identifier}
+        assert rev_ids.include?("4f26664364207fa8b1af9f8722647ab2d4ac5d43"),
+             "Expected #{rev_ids.inspect} to include \"4f26664364207fa8b1af9f8722647ab2d4ac5d43\""
+        assert rev_ids.include?("deff712f05a90d96edbd70facc47d944be5897e3"),
+             "Expected #{rev_ids.inspect} to include \"deff712f05a90d96edbd70facc47d944be5897e3\""
+
+        revs = @adapter.revisions('',['713f494', '4a07fe3'],'4f26664')
+        assert_equal 5, revs.length
+        rev_ids = revs.map{|rev| rev.identifier}
+        assert rev_ids.include?("4f26664364207fa8b1af9f8722647ab2d4ac5d43"),
+             "Expected #{rev_ids.inspect} to include \"4f26664364207fa8b1af9f8722647ab2d4ac5d43\""
+        assert rev_ids.include?("deff712f05a90d96edbd70facc47d944be5897e3"),
+             "Expected #{rev_ids.inspect} to include \"deff712f05a90d96edbd70facc47d944be5897e3\""
+        assert rev_ids.include?("7e61ac704deecde634b51e59daa8110435dcb3da"),
+             "Expected #{rev_ids.inspect} to include \"7e61ac704deecde634b51e59daa8110435dcb3da\""
+      end
+
       def test_revisions_reverse
         revs1 = @adapter.revisions('',nil,nil,{:all => true, :reverse => true })
         assert_equal 21, revs1.length
diff --git a/test/unit/repository_git_test.rb b/test/unit/repository_git_test.rb
index ed049b3..02a230d 100644
--- a/test/unit/repository_git_test.rb
+++ b/test/unit/repository_git_test.rb
@@ -87,7 +87,7 @@ class RepositoryGitTest < ActiveSupport::TestCase
       assert_equal "README", change.path
       assert_equal "A", change.action
 
-      assert_equal 4, @repository.extra_info["branches"].size
+      assert_equal 4, @repository.extra_info["head_scmids"].size
     end
 
     def test_fetch_changesets_incremental
@@ -96,12 +96,12 @@ class RepositoryGitTest < ActiveSupport::TestCase
       @project.reload
       assert_equal NUM_REV, @repository.changesets.count
       assert_equal 33, @repository.changes.count
-      extra_info_db = @repository.extra_info["branches"]
+      extra_info_db = @repository.extra_info["head_scmids"]
       assert_equal 4, extra_info_db.size
-      assert_equal "1ca7f5ed374f3cb31a93ae5215c2e25cc6ec5127",
-                    extra_info_db["latin-1-path-encoding"]["last_scmid"]
-      assert_equal "83ca5fd546063a3c7dc2e568ba3355661a9e2b2c",
-                    extra_info_db["master"]["last_scmid"]
+      assert extra_info_db.include?("1ca7f5ed374f3cb31a93ae5215c2e25cc6ec5127"),
+             "Expected #{extra_info_db.inspect} to include \"1ca7f5ed374f3cb31a93ae5215c2e25cc6ec5127\""
+      assert extra_info_db.include?("83ca5fd546063a3c7dc2e568ba3355661a9e2b2c"),
+             "Expected #{extra_info_db.inspect} to include \"83ca5fd546063a3c7dc2e568ba3355661a9e2b2c\""
 
       del_revs = [
           "83ca5fd546063a3c7dc2e568ba3355661a9e2b2c",
@@ -118,14 +118,14 @@ class RepositoryGitTest < ActiveSupport::TestCase
       cs1 = @repository.changesets
       assert_equal 15, cs1.count
       h = @repository.extra_info.dup
-      h["branches"]["master"]["last_scmid"] =
-            "4a07fe31bffcf2888791f3e6cbc9c4545cefe3e8"
+      h["head_scmids"].delete("83ca5fd546063a3c7dc2e568ba3355661a9e2b2c")
+      h["head_scmids"] << "4a07fe31bffcf2888791f3e6cbc9c4545cefe3e8"
       @repository.merge_extra_info(h)
       @repository.save
       @project.reload
-      extra_info_db_1 = @repository.extra_info["branches"]
-      assert_equal "4a07fe31bffcf2888791f3e6cbc9c4545cefe3e8",
-                    extra_info_db_1["master"]["last_scmid"]
+      extra_info_db_1 = @repository.extra_info["head_scmids"]
+      assert extra_info_db_1.include?("4a07fe31bffcf2888791f3e6cbc9c4545cefe3e8"),
+             "Expected #{extra_info_db_1.inspect} to include \"4a07fe31bffcf2888791f3e6cbc9c4545cefe3e8\""
 
       @repository.fetch_changesets
       @project.reload
@@ -136,13 +136,14 @@ class RepositoryGitTest < ActiveSupport::TestCase
       assert_equal 0, @repository.changesets.count
       @repository.fetch_changesets
       @project.reload
-      assert_equal NUM_REV, @repository.changesets.count
-      extra_info_db = @repository.extra_info["branches"]
+      assert_equal 21, @repository.changesets.count
+      assert_equal 33, @repository.changes.count
+      extra_info_db = @repository.extra_info["head_scmids"]
       assert_equal 4, extra_info_db.size
-      assert_equal "1ca7f5ed374f3cb31a93ae5215c2e25cc6ec5127",
-                    extra_info_db["latin-1-path-encoding"]["last_scmid"]
-      assert_equal "83ca5fd546063a3c7dc2e568ba3355661a9e2b2c",
-                    extra_info_db["master"]["last_scmid"]
+      assert extra_info_db.include?("1ca7f5ed374f3cb31a93ae5215c2e25cc6ec5127"),
+             "Expected #{extra_info_db.inspect} to include \"1ca7f5ed374f3cb31a93ae5215c2e25cc6ec5127\""
+      assert extra_info_db.include?("83ca5fd546063a3c7dc2e568ba3355661a9e2b2c"),
+             "Expected #{extra_info_db.inspect} to include \"83ca5fd546063a3c7dc2e568ba3355661a9e2b2c\""
 
       del_revs = [
           "83ca5fd546063a3c7dc2e568ba3355661a9e2b2c",
@@ -159,14 +160,14 @@ class RepositoryGitTest < ActiveSupport::TestCase
       cs1 = @repository.changesets
       assert_equal 15, cs1.count
       h = @repository.extra_info.dup
-      h["branches"]["master"]["last_scmid"] =
-            "abcd1234efgh"
+      h["head_scmids"].delete("83ca5fd546063a3c7dc2e568ba3355661a9e2b2c")
+      h["head_scmids"] << "abcd1234efgh"
       @repository.merge_extra_info(h)
       @repository.save
       @project.reload
-      extra_info_db_1 = @repository.extra_info["branches"]
-      assert_equal "abcd1234efgh",
-                    extra_info_db_1["master"]["last_scmid"]
+      extra_info_db_1 = @repository.extra_info["head_scmids"]
+      assert extra_info_db_1.include?("abcd1234efgh"),
+             "Expected #{extra_info_db_1.inspect} to include \"abcd1234efgh\""
 
       @repository.fetch_changesets
       @project.reload
@@ -230,14 +231,14 @@ class RepositoryGitTest < ActiveSupport::TestCase
       assert_equal 15, cs1.count
       assert_equal 0, @repository.extra_info["db_consistent"]["ordering"]
       h = @repository.extra_info.dup
-      h["branches"]["master"]["last_scmid"] =
-            "4a07fe31bffcf2888791f3e6cbc9c4545cefe3e8"
+      h["head_scmids"].delete("83ca5fd546063a3c7dc2e568ba3355661a9e2b2c")
+      h["head_scmids"] << "4a07fe31bffcf2888791f3e6cbc9c4545cefe3e8"
       @repository.merge_extra_info(h)
       @repository.save
       @project.reload
-      extra_info_db_1 = @repository.extra_info["branches"]
-      assert_equal "4a07fe31bffcf2888791f3e6cbc9c4545cefe3e8",
-                    extra_info_db_1["master"]["last_scmid"]
+      extra_info_db_1 = @repository.extra_info["head_scmids"]
+      assert extra_info_db_1.include?("4a07fe31bffcf2888791f3e6cbc9c4545cefe3e8"),
+             "Expected #{extra_info_db_1.inspect} to include \"4a07fe31bffcf2888791f3e6cbc9c4545cefe3e8\""
 
       @repository.fetch_changesets
       assert_equal NUM_REV, @repository.changesets.count
@@ -486,8 +487,7 @@ class RepositoryGitTest < ActiveSupport::TestCase
       assert_equal 0, @repository.changesets.count
       @repository.fetch_changesets
       @project.reload
-      assert_equal NUM_REV, @repository.changesets.count
-      %w|67e7792ce20ccae2e4bb73eed09bb397819c8834 67e7792ce20cca|.each do |r1|
+      %w|1ca7f5ed374f3cb31a93ae5215c2e25cc6ec5127 1ca7f5ed374f3c|.each do |r1|
         changeset = @repository.find_changeset_by_name(r1)
         assert_nil changeset.next
       end
-- 
1.7.5.4

