Merge branch 'impprove-compare-logic' into 'master'

Impprove compare logic

* Use single class CompareService for collecting commits and diffs at Compare, MR#new, MR#create pages
* Don't use satellites for compare inside one repo

See merge request !1001

Gemfile.lock

@@ -179,7 +179,7 @@ GEM
       mime-types (~> 1.19)
     gitlab_emoji (0.0.1.1)
       emoji (~> 1.0.1)
-    gitlab_git (6.0.1)
+    gitlab_git (6.1.0)
       activesupport (~> 4.0)
       charlock_holmes (~> 0.6)
       gitlab-grit (~> 2.6)
@@ -244,7 +244,7 @@ GEM
       json (~> 1.8)
       multi_xml (>= 0.5.2)
     httpauth (0.2.0)
-    i18n (0.6.9)
+    i18n (0.6.11)
     ice_nine (0.10.0)
     jasmine (2.0.2)
       jasmine-core (~> 2.0.0)

app/controllers/projects/compare_controller.rb

@@ -8,14 +8,21 @@ class Projects::CompareController < Projects::ApplicationController
   end
 
   def show
-    compare = Gitlab::Git::Compare.new(@repository.raw_repository, params[:from], params[:to], MergeRequestDiff::COMMITS_SAFE_SIZE)
+    base_ref = params[:from]
+    head_ref = params[:to]
 
-    @commits       = compare.commits
-    @commit        = compare.commit
-    @diffs         = compare.diffs
-    @refs_are_same = compare.same
-    @line_notes    = []
-    @diff_timeout  = compare.timeout
+    compare_result = CompareService.new.execute(
+      current_user,
+      @project,
+      head_ref,
+      @project,
+      base_ref
+    )
+
+    @commits = compare_result.commits
+    @diffs = compare_result.diffs
+    @commit = @commits.last
+    @line_notes = []
   end
 
   def create

app/models/merge_request_diff.rb

@@ -83,11 +83,7 @@ class MergeRequestDiff < ActiveRecord::Base
   # Collect array of Git::Commit objects
   # between target and source branches
   def unmerged_commits
-    commits = if merge_request.for_fork?
-                compare_action.commits
-              else
-                repository.commits_between(target_branch, source_branch)
-              end
+    commits = compare_result.commits
 
     if commits.present?
       commits = Commit.decorate(commits).
@@ -147,12 +143,7 @@ class MergeRequestDiff < ActiveRecord::Base
   # Collect array of Git::Diff objects
   # between target and source branches
   def unmerged_diffs
-    diffs = if merge_request.for_fork?
-              compare_action.diffs
-            else
-              Gitlab::Git::Diff.between(repository, source_branch, target_branch)
-            end
-
+    diffs = compare_result.diffs
     diffs ||= []
     diffs
   rescue Gitlab::Git::Diff::TimeoutError => ex
@@ -166,13 +157,13 @@ class MergeRequestDiff < ActiveRecord::Base
 
   private
 
-  def compare_action
-    Gitlab::Satellite::CompareAction.new(
+  def compare_result
+    @compare_result ||= CompareService.new.execute(
       merge_request.author,
+      merge_request.source_project,
+      merge_request.source_branch,
       merge_request.target_project,
       merge_request.target_branch,
-      merge_request.source_project,
-      merge_request.source_branch
     )
   end
 end

app/services/compare_service.rb

+# 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,
+        )
+      )
+    else
+      Gitlab::Satellite::CompareAction.new(
+        current_user,
+        target_project,
+        target_branch,
+        source_project,
+        source_branch
+      ).result
+    end
+  end
+end

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

app/views/notify/repository_push_email.html.haml

@@ -25,6 +25,4 @@
     %br
 
 - if @compare.timeout
-  %h5 To prevent performance issues changes are hidden
-- elsif @compare.commits_over_limit?
-  %h5 Changes are not shown due to large amount of commits
+  %h5 Huge diff. To prevent performance issues changes are hidden

app/views/notify/repository_push_email.text.haml

@@ -23,5 +23,3 @@ Changes:
 \
 - if @compare.timeout
   Huge diff. To prevent performance issues it was hidden
-- elsif @compare.commits_over_limit?
-  Changes are not shown due to large amount of commits

app/workers/emails_on_push_worker.rb

@@ -13,7 +13,7 @@ class EmailsOnPushWorker
       return true
     end
 
-    compare = Gitlab::Git::Compare.new(project.repository.raw_repository, before_sha, after_sha, MergeRequestDiff::COMMITS_SAFE_SIZE)
+    compare = Gitlab::Git::Compare.new(project.repository.raw_repository, before_sha, after_sha)
 
     # Do not send emails if git compare failed
     return false unless compare && compare.commits.present?

lib/api/entities.rb

@@ -201,13 +201,13 @@ module API
 
     class Compare < Grape::Entity
       expose :commit, using: Entities::RepoCommit do |compare, options|
-        if compare.commit
-          Commit.new compare.commit
-        end
+        Commit.decorate(compare.commits).last
       end
+
       expose :commits, using: Entities::RepoCommit do |compare, options|
-        Commit.decorate compare.commits
+        Commit.decorate(compare.commits)
       end
+
       expose :diffs, using: Entities::RepoDiff do |compare, options|
         compare.diffs
       end

lib/api/repositories.rb

@@ -147,7 +147,7 @@ module API
       get ':id/repository/compare' do
         authorize! :download_code, user_project
         required_attributes! [:from, :to]
-        compare = Gitlab::Git::Compare.new(user_project.repository.raw_repository, params[:from], params[:to], MergeRequestDiff::COMMITS_SAFE_SIZE)
+        compare = Gitlab::Git::Compare.new(user_project.repository.raw_repository, params[:from], params[:to])
         present compare, with: Entities::Compare
       end
 

lib/gitlab/compare_result.rb

+module Gitlab
+  class CompareResult
+    attr_reader :commits, :diffs
+
+    def initialize(compare)
+      @commits, @diffs = compare.commits, compare.diffs
+    end
+  end
+end

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
@@ -45,7 +33,11 @@ module Gitlab
       end
 
       def compare(repo)
-        @compare ||= Gitlab::Git::Compare.new(Gitlab::Git::Repository.new(repo.path), "origin/#{@target_branch}", "source/#{@source_branch}", 10000)
+        @compare ||= Gitlab::Git::Compare.new(
+          Gitlab::Git::Repository.new(repo.path),
+          "origin/#{@target_branch}",
+          "source/#{@source_branch}"
+        )
       end
     end
   end