Introduce Repository#with_tmp_ref which we need

commits from the other repository. We'll cleanup
the tmp ref after we're done with our business.

app/controllers/projects/compare_controller.rb

@@ -37,7 +37,8 @@ class Projects::CompareController < Projects::ApplicationController
   end
   def define_diff_vars
-    @compare = CompareService.new.execute(@project, @head_ref, @project, @start_ref)
+    @compare = CompareService.new(@project, @head_ref).
+      execute(@project, @start_ref)
     if @compare
       @commits = @compare.commits

app/models/merge_request_diff.rb

@@ -169,7 +169,8 @@ class MergeRequestDiff < ActiveRecord::Base
     # When compare merge request versions we want diff A..B instead of A...B
     # so we handle cases when user does squash and rebase of the commits between versions.
     # For this reason we set straight to true by default.
-    CompareService.new.execute(project, head_commit_sha, project, sha, straight: straight)
+    CompareService.new(project, head_commit_sha).
+      execute(project, sha, straight: straight)
   end
   def commits_count

app/models/repository.rb

@@ -1099,6 +1099,21 @@ class Repository
     Gitlab::Popen.popen(args, path_to_repo).first.lines.map(&:strip)
   end
+  def with_tmp_ref(source_repository, source_branch_name)
+    random_string = SecureRandom.hex
+
+    fetch_ref(
+      source_repository.path_to_repo,
+      "#{Gitlab::Git::BRANCH_REF_PREFIX}#{source_branch_name}",
+      "refs/tmp/#{random_string}/head"
+    )
+
+    yield
+
+  ensure
+    FileUtils.rm_rf("#{path_to_repo}/refs/tmp/#{random_string}")
+  end
+
   def fetch_ref(source_path, source_ref, target_ref)
     args = %W(#{Gitlab.config.git.bin_path} fetch --no-tags -f #{source_path} #{source_ref}:#{target_ref})
     Gitlab::Popen.popen(args, path_to_repo)

app/services/compare_service.rb

@@ -3,23 +3,29 @@ require 'securerandom'
 # Compare 2 branches for one repo or between repositories
 # and return Gitlab::Git::Compare object that responds to commits and diffs
 class CompareService
-  def execute(source_project, source_branch, target_project, target_branch, straight: false)
-    source_commit = source_project.commit(source_branch)
-    return unless source_commit
-    source_sha = source_commit.sha
-    # If compare with other project we need to fetch ref first
-    unless target_project == source_project
-      random_string = SecureRandom.hex
-      target_project.repository.fetch_ref(
-        source_project.repository.path_to_repo,
-        "refs/heads/#{source_branch}",
-        "refs/tmp/#{random_string}/head"
-      )
+  attr_reader :source_project, :source_sha
+  def initialize(new_source_project, source_branch)
+    @source_project = new_source_project
+    @source_sha = new_source_project.commit(source_branch).try(:sha)
+  end
+  def execute(target_project, target_branch, straight: false)
+    return unless source_sha
+    # If compare with other project we need to fetch ref first
+    if target_project == source_project
+      compare(target_project, target_branch, straight)
+    else
+      target_project.repository.with_tmp_ref(source_project, source_branch) do
+        compare(target_project, target_branch, straight)
+      end
     end
+  end
+
+  private
+  def compare(target_project, target_branch, straight)
     raw_compare = Gitlab::Git::Compare.new(
       target_project.repository.raw_repository,
       target_branch,

app/services/git_operation_service.rb

@@ -38,15 +38,14 @@ GitOperationService = Struct.new(:user, :repository) do
       branch_name, source_branch_name, source_project)
     update_branch_with_hooks(branch_name) do |ref|
-      if repository.project != source_project
-        repository.fetch_ref(
-          source_project.repository.path_to_repo,
-          "#{Gitlab::Git::BRANCH_REF_PREFIX}#{source_branch_name}",
-          "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch_name}"
-        )
+      if repository.project == source_project
+        yield(ref)
+      else
+        repository.with_tmp_ref(
+          source_project.repository, source_branch_name) do
+            yield(ref)
+          end
       end
-
-      yield(ref)
     end
   end

app/services/merge_requests/build_service.rb

@@ -16,9 +16,10 @@ module MergeRequests
       messages = validate_branches(merge_request)
       return build_failed(merge_request, messages) unless messages.empty?
-      compare = CompareService.new.execute(
+      compare = CompareService.new(
         merge_request.source_project,
-        merge_request.source_branch,
+        merge_request.source_branch
+      ).execute(
         merge_request.target_project,
         merge_request.target_branch,
       )

app/workers/emails_on_push_worker.rb

@@ -33,13 +33,15 @@ class EmailsOnPushWorker
     reverse_compare = false
     if action == :push
-      compare = CompareService.new.execute(project, after_sha, project, before_sha)
+      compare = CompareService.new(project, after_sha).
+        execute(project, before_sha)
       diff_refs = compare.diff_refs
       return false if compare.same
       if compare.commits.empty?
-        compare = CompareService.new.execute(project, before_sha, project, after_sha)
+        compare = CompareService.new(project, before_sha).
+          execute(project, after_sha)
         diff_refs = compare.diff_refs
         reverse_compare = true

spec/services/compare_service_spec.rb

@@ -3,17 +3,17 @@ require 'spec_helper'
 describe CompareService, services: true do
   let(:project) { create(:project) }
   let(:user) { create(:user) }
-  let(:service) { described_class.new }
+  let(:service) { described_class.new(project, 'feature') }
   describe '#execute' do
     context 'compare with base, like feature...fix' do
-      subject { service.execute(project, 'feature', project, 'fix', straight: false) }
+      subject { service.execute(project, 'fix', straight: false) }
       it { expect(subject.diffs.size).to eq(1) }
     end
     context 'straight compare, like feature..fix' do
-      subject { service.execute(project, 'feature', project, 'fix', straight: true) }
+      subject { service.execute(project, 'fix', straight: true) }
       it { expect(subject.diffs.size).to eq(3) }
     end