From 73c43846b62fa314c0f323c818a0ee35344b79ff Mon Sep 17 00:00:00 2001
From: Jeremy Bopp <jeremy@bopp.net>
Date: Thu, 15 Mar 2012 17:18:56 -0500
Subject: [PATCH 2/2] Process new git revisions all at once rather than per
 branch

* New revisions are parsed and stored exactly 1 time each
* Runs in a transaction to ensure DB integrity against concurrent updates
---
 app/models/repository/git.rb                       |  156 +++++++-------------
 lib/redmine/scm/adapters/git_adapter.rb            |   12 ++
 .../lib/redmine/scm/adapters/git_adapter_test.rb   |   12 ++
 test/unit/repository_git_test.rb                   |   30 ++--
 4 files changed, 94 insertions(+), 116 deletions(-)

diff --git a/app/models/repository/git.rb b/app/models/repository/git.rb
index 933b5e3..30934a8 100644
--- a/app/models/repository/git.rb
+++ b/app/models/repository/git.rb
@@ -79,6 +79,14 @@ class Repository::Git < Repository
     scm.tags
   end
 
+  def heads
+    h = {}
+    scm.heads.each do |revision, branch|
+      h[branch] = { 'last_scmid' => revision }
+    end
+    h
+  end
+
   def default_branch
     scm.default_branch
   rescue Exception => e
@@ -99,16 +107,16 @@ class Repository::Git < Repository
                 options = {:report_last_commit => extra_report_last_commit})
   end
 
-  # With SCMs that have a sequential commit numbering,
-  # such as Subversion and Mercurial,
-  # Redmine is able to be clever and only fetch changesets
-  # going forward from the most recent one it knows about.
+  # With SCMs that have a sequential commit numbering, such as Subversion and
+  # Mercurial, Redmine is able to be clever and only fetch changesets going
+  # forward from the most recent one it knows about.
   #
   # However, Git does not have a sequential commit numbering.
   #
-  # In order to fetch only new adding revisions,
-  # Redmine needs to parse revisions per branch.
-  # Branch "last_scmid" is for this requirement.
+  # In order to fetch only new revisions, Redmine needs to parse revisions
+  # introduced by the new heads of new and existing branches since the last time
+  # the branch heads were processed.  Branch "last_scmid" is for this
+  # requirement.
   #
   # In Git and Mercurial, revisions are not in date order.
   # Redmine Mercurial fixed issues.
@@ -118,117 +126,64 @@ class Repository::Git < Repository
   #      http://www.redmine.org/issues/3567
   #
   # Database revision column is text, so Redmine can not sort by revision.
-  # Mercurial has revision number, and revision number guarantees revision order.
-  # Redmine Mercurial model stored revisions ordered by database id to database.
-  # So, Redmine Mercurial model can use correct ordering revisions.
+  # Mercurial has revision number, and revision number guarantees revision
+  # order.  Redmine Mercurial model stored revisions ordered by database id to
+  # database.  So, Redmine Mercurial model can use correct ordering revisions.
   #
-  # Redmine Mercurial adapter uses "hg log -r 0:tip --limit 10"
-  # to get limited revisions from old to new.
-  # But, Git 1.7.3.4 does not support --reverse with -n or --skip.
+  # Redmine Mercurial adapter uses "hg log -r 0:tip --limit 10" to get limited
+  # revisions from old to new.  But, Git 1.7.3.4 does not support --reverse with
+  # -n or --skip.
   #
   # 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?
-    h1 = extra_info || {}
-    h  = h1.dup
-    h["branches"]       ||= {}
-    h["db_consistent"]  ||= {}
-    if changesets.count == 0
-      h["db_consistent"]["ordering"] = 1
-      merge_extra_info(h)
-      self.save
-    elsif ! h["db_consistent"].has_key?("ordering")
-      h["db_consistent"]["ordering"] = 0
+    transaction do
+      scm_heads = heads
+      return if scm_heads.nil? || scm_heads.empty?
+      h1 = extra_info || {}
+      h  = h1.dup
+      h["branches"]       ||= {}
+      h["db_consistent"]  ||= {}
+      if changesets.count == 0
+        h["db_consistent"]["ordering"] = 1
+      elsif ! h["db_consistent"].has_key?("ordering")
+        h["db_consistent"]["ordering"] = 0
+      end
+
+      begin
+        save_revisions(h["branches"], scm_heads)
+      rescue Redmine::Scm::Adapters::CommandFailed
+        raise ActiveRecord::Rollback, $!.message
+      end
+
+      h["branches"] = scm_heads
       merge_extra_info(h)
       self.save
     end
-    save_revisions(h, scm_brs)
   end
 
-  def save_revisions(h, scm_brs)
-    # Remember what revisions we already processed (in any branches)
-    all_revisions = []
-    scm_brs.each do |br1|
-      br = br1.to_s
-      last_revision = nil
-      from_scmid = nil
-      from_scmid = h["branches"][br]["last_scmid"] if h["branches"][br]
-      h["branches"][br] ||= {}
-
-      revisions = scm.revisions('', from_scmid, br, {:reverse => true})
-      next if revisions.blank?
-
-      # Remember the last commit id here, before we start removing revisions from the array.
-      # We'll do that for optimization, but it also means, that we may lose even all revisions.
-      last_revision  = revisions.last
-
-      # remove revisions that we have already processed (possibly in other branches)
-      revisions.reject!{|r| all_revisions.include?(r.scmid)}
-      # add revisions that we are to parse now to 'all processed revisions'
-      # (this equals to a union, because we executed diff above)
-      all_revisions += revisions.map{|r| r.scmid}
-
-      # Make the search for existing revisions in the database in a more sufficient manner
-      # This is replacing the one-after-one queries.
-      # Find all revisions, that are in the database, and then remove them from the revision array.
-      # Then later we won't need any conditions for db existence.
-      # Query for several revisions at once, and remove them from the revisions array, if they are there.
-      # Do this in chunks, to avoid eventual memory problems (in case of tens of thousands of commits).
-      # If there are no revisions (because the original code's algoritm filtered them),
-      # then this part will be stepped over.
-      # We make queries, just if there is any revision.
-      limit = 100
-      offset = 0
-      revisions_copy = revisions.clone # revisions will change
-      while offset < revisions_copy.size
-        recent_changesets_slice = changesets.find(
-                                     :all,
-                                     :conditions => [
-                                        'scmid IN (?)',
-                                        revisions_copy.slice(offset, limit).map{|x| x.scmid}
-                                      ]
-                                    )
-        # Subtract revisions that redmine already knows about
-        recent_revisions = recent_changesets_slice.map{|c| c.scmid}
-        revisions.reject!{|r| recent_revisions.include?(r.scmid)}
-        offset += limit
-      end
-
-      revisions.each do |rev|
-        transaction do
-          # There is no search in the db for this revision, because above we ensured,
-          # that it's not in the db.
-          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
-          # saving the last scmid was moved from here, because we won't come in here,
-          # if the revision was already added for another branch
-        end
-      end
-
-      # save the data about the last revision for this branch
-      unless last_revision.nil?
-        h["branches"][br]["last_scmid"] = last_revision.scmid
-        merge_extra_info(h)
-        self.save
-      end
+  def save_revisions(old_scm_heads, new_scm_heads)
+    scm.revisions(
+      '', nil, nil,
+      :reverse => true,
+      :includes => heads_from_branches_hash(new_scm_heads),
+      :excludes => heads_from_branches_hash(old_scm_heads)
+    ) do |revision|
+      save_revision(revision)
     end
   end
   private :save_revisions
 
   def save_revision(rev)
+    parents = (rev.parents || []).collect{|rp| find_changeset_by_name(rp)}.compact
     changeset = Changeset.new(
               :repository   => self,
               :revision     => rev.identifier,
               :scmid        => rev.scmid,
               :committer    => rev.author,
               :committed_on => rev.time,
-              :comments     => rev.message
+              :comments     => rev.message,
+              :parents      => parents
               )
     if changeset.save
       rev.paths.each do |file|
@@ -242,11 +197,8 @@ class Repository::Git < Repository
   end
   private :save_revision
 
-  def heads_from_branches_hash
-    h1 = extra_info || {}
-    h  = h1.dup
-    h["branches"] ||= {}
-    h['branches'].map{|br, hs| hs['last_scmid']}
+  def heads_from_branches_hash(branches)
+    branches.map{|br, hs| hs['last_scmid']}
   end
 
   def latest_changesets(path,rev,limit=10)
diff --git a/lib/redmine/scm/adapters/git_adapter.rb b/lib/redmine/scm/adapters/git_adapter.rb
index 301b6e7..302f13c 100644
--- a/lib/redmine/scm/adapters/git_adapter.rb
+++ b/lib/redmine/scm/adapters/git_adapter.rb
@@ -107,6 +107,18 @@ module Redmine
           nil
         end
 
+        def heads
+          return @heads if @heads
+          cmd_args = %w|show-ref --heads|
+          @heads = scm_cmd(cmd_args) do |io|
+            io.map do |line|
+              line.chomp.match('(.*?)\s+refs/heads/(.*)')[1, 2]
+            end.sort
+          end
+        rescue ScmCommandAborted
+          []
+        end
+
         def default_branch
           bras = self.branches
           return nil if bras.nil?
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 2cdb14f..600da7f 100644
--- a/test/unit/lib/redmine/scm/adapters/git_adapter_test.rb
+++ b/test/unit/lib/redmine/scm/adapters/git_adapter_test.rb
@@ -105,6 +105,18 @@ begin
             ], @adapter.tags
       end
 
+      def test_heads
+        assert_equal  [
+              ["1ca7f5ed374f3cb31a93ae5215c2e25cc6ec5127", "latin-1-path-encoding"],
+              ["2a682156a3b6e77a8bf9cd4590e8db757f3c6c78", "issue-8857"],
+              ["67e7792ce20ccae2e4bb73eed09bb397819c8834", "test-latin-1"],
+              ["83ca5fd546063a3c7dc2e568ba3355661a9e2b2c", "master"],
+              ["83ca5fd546063a3c7dc2e568ba3355661a9e2b2c", "master-20120212"],
+              ["fba357b886984ee71185ad2065e65fc0417d9b92", "test_branch"]
+            ], @adapter.heads
+      end
+
+
       def test_revisions_master_all
         revs1 = []
         @adapter.revisions('', nil, "master",{}) do |rev|
diff --git a/test/unit/repository_git_test.rb b/test/unit/repository_git_test.rb
index bc1e3a6..3dfe20e 100644
--- a/test/unit/repository_git_test.rb
+++ b/test/unit/repository_git_test.rb
@@ -146,12 +146,16 @@ class RepositoryGitTest < ActiveSupport::TestCase
       h = @repository.extra_info.dup
       h["branches"]["master"]["last_scmid"] =
             "4a07fe31bffcf2888791f3e6cbc9c4545cefe3e8"
+      h["branches"]["master-20120212"]["last_scmid"] =
+            "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"]
+      assert_equal "4a07fe31bffcf2888791f3e6cbc9c4545cefe3e8",
+                    extra_info_db_1["master-20120212"]["last_scmid"]
 
       @repository.fetch_changesets
       @project.reload
@@ -264,12 +268,16 @@ class RepositoryGitTest < ActiveSupport::TestCase
       h = @repository.extra_info.dup
       h["branches"]["master"]["last_scmid"] =
             "4a07fe31bffcf2888791f3e6cbc9c4545cefe3e8"
+      h["branches"]["master-20120212"]["last_scmid"] =
+            "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"]
+      assert_equal "4a07fe31bffcf2888791f3e6cbc9c4545cefe3e8",
+                    extra_info_db_1["master-20120212"]["last_scmid"]
 
       @repository.fetch_changesets
       assert_equal NUM_REV, @repository.changesets.count
@@ -277,19 +285,13 @@ class RepositoryGitTest < ActiveSupport::TestCase
     end
 
     def test_heads_from_branches_hash
-      assert_nil @repository.extra_info
-      assert_equal 0, @repository.changesets.count
-      assert_equal [], @repository.heads_from_branches_hash
       h = {}
-      h["branches"] = {}
-      h["branches"]["test1"] = {}
-      h["branches"]["test1"]["last_scmid"] = "1234abcd"
-      h["branches"]["test2"] = {}
-      h["branches"]["test2"]["last_scmid"] = "abcd1234"
-      @repository.merge_extra_info(h)
-      @repository.save
-      @project.reload
-      assert_equal ["1234abcd", "abcd1234"], @repository.heads_from_branches_hash.sort
+      assert_equal [], @repository.heads_from_branches_hash(h)
+      h["test1"] = {}
+      h["test1"]["last_scmid"] = "1234abcd"
+      h["test2"] = {}
+      h["test2"]["last_scmid"] = "abcd1234"
+      assert_equal ["1234abcd", "abcd1234"], @repository.heads_from_branches_hash(h).sort
     end
 
     def test_latest_changesets
@@ -511,7 +513,7 @@ class RepositoryGitTest < ActiveSupport::TestCase
       @repository.fetch_changesets
       @project.reload
       assert_equal NUM_REV, @repository.changesets.count
-      %w|95488a44bc25f7d1f97d775a31359539ff333a63 95488a44b|.each do |r1|
+      %w|7234cb2750b63f47bff735edc50a1c0a433c2518 7234cb27|.each do |r1|
         changeset = @repository.find_changeset_by_name(r1)
         assert_nil changeset.previous
       end
@@ -535,7 +537,7 @@ class RepositoryGitTest < ActiveSupport::TestCase
       @repository.fetch_changesets
       @project.reload
       assert_equal NUM_REV, @repository.changesets.count
-      %w|67e7792ce20ccae2e4bb73eed09bb397819c8834 67e7792ce20cca|.each do |r1|
+      %w|2a682156a3b6e77a8bf9cd4590e8db757f3c6c78 2a682156|.each do |r1|
         changeset = @repository.find_changeset_by_name(r1)
         assert_nil changeset.next
       end
-- 
1.7.5.4

