Refactor compare logic for MR. Use satellites only for forks for better performance

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

4 files changed, 46 insertions, 22 deletions

diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb
new file mode 100644
index 00000000000..e4c3bb1507d
--- /dev/null
+++ b/app/services/compare_service.rb
@@ -0,0 +1,28 @@
+# Compare 2 branches for one repo or between repositories
+# and return Gitlab::CompareResult object that responds to commits and diffs
+class CompareService
+  def execute(current_user, source_project, source_branch, target_project, target_branch)
+    # Try to compare branches to get commits list and diffs
+    #
+    # Note: Use satellite only when need to compare between to repos
+    # because satellites are slower then operations on bare repo
+    if target_project == source_project
+      Gitlab::CompareResult.new(
+        Gitlab::Git::Compare.new(
+          target_project.repository.raw_repository,
+          target_branch,
+          source_branch,
+          10000
+        )
+      )
+    else
+      Gitlab::Satellite::CompareAction.new(
+        current_user,
+        target_project,
+        target_branch,
+        source_project,
+        source_branch
+      ).result
+    end
+  end
+end
diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb
index 81dd8887395..1475973e543 100644
--- a/app/services/merge_requests/build_service.rb
+++ b/app/services/merge_requests/build_service.rb
@@ -19,16 +19,15 @@ module MergeRequests
       # Generate suggested MR title based on source branch name
       merge_request.title = merge_request.source_branch.titleize.humanize
 
-      # Try to compare branches to get commits list and diffs
-      compare_action = Gitlab::Satellite::CompareAction.new(
+      compare_result = CompareService.new.execute(
         current_user,
+        merge_request.source_project,
+        merge_request.source_branch,
         merge_request.target_project,
         merge_request.target_branch,
-        merge_request.source_project,
-        merge_request.source_branch
       )
 
-      commits = compare_action.commits
+      commits = compare_result.commits
 
       # At this point we decide if merge request can be created
       # If we have at least one commit to merge -> creation allowed
@@ -38,7 +37,7 @@ module MergeRequests
         merge_request.compare_failed = false
 
         # Try to collect diff for merge request.
-        diffs = compare_action.diffs
+        diffs = compare_result.diffs
 
         if diffs.present?
           merge_request.compare_diffs = diffs
diff --git a/lib/gitlab/compare_result.rb b/lib/gitlab/compare_result.rb
new file mode 100644
index 00000000000..d72391dade5
--- /dev/null
+++ b/lib/gitlab/compare_result.rb
@@ -0,0 +1,9 @@
+module Gitlab
+  class CompareResult
+    attr_reader :commits, :diffs
+
+    def initialize(compare)
+      @commits, @diffs = compare.commits, compare.diffs
+    end
+  end
+end
diff --git a/lib/gitlab/satellite/compare_action.rb b/lib/gitlab/satellite/compare_action.rb
index e5b62576330..4905b146a55 100644
--- a/lib/gitlab/satellite/compare_action.rb
+++ b/lib/gitlab/satellite/compare_action.rb
@@ -10,28 +10,16 @@ module Gitlab
         @source_project, @source_branch = source_project, source_branch
       end
 
-      # Only show what is new in the source branch compared to the target branch, not the other way around.
-      # The line below with merge_base is equivalent to diff with three dots (git diff branch1...branch2)
-      # From the git documentation: "git diff A...B" is equivalent to "git diff $(git-merge-base A B) B"
-      def diffs
+      # Compare 2 repositories and return Gitlab::CompareResult object
+      def result
         in_locked_and_timed_satellite do |target_repo|
           prepare_satellite!(target_repo)
           update_satellite_source_and_target!(target_repo)
-          compare(target_repo).diffs
-        end
-      rescue Grit::Git::CommandFailed => ex
-        raise BranchesWithoutParent
-      end
 
-      # Retrieve an array of commits between the source and the target
-      def commits
-        in_locked_and_timed_satellite do |target_repo|
-          prepare_satellite!(target_repo)
-          update_satellite_source_and_target!(target_repo)
-          compare(target_repo).commits
+          Gitlab::CompareResult.new(compare(target_repo))
         end
       rescue Grit::Git::CommandFailed => ex
-        handle_exception(ex)
+        raise BranchesWithoutParent
       end
 
       private