diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index bee3d56076c..e2b178314c0 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/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 diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index b8f36a2c958..7946d8e123e 100644 --- a/app/models/merge_request_diff.rb +++ b/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 diff --git a/app/models/repository.rb b/app/models/repository.rb index 73a9e269a65..ced68b9d274 100644 --- a/app/models/repository.rb +++ b/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) diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb index 5e8fafca98c..4367cb5f615 100644 --- a/app/services/compare_service.rb +++ b/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 + attr_reader :source_project, :source_sha - source_sha = source_commit.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 - 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" - ) + 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, diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb index 36c8b8ff575..a7d267cd6b4 100644 --- a/app/services/git_operation_service.rb +++ b/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 diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index bebfca7537b..a52a94c5ffa 100644 --- a/app/services/merge_requests/build_service.rb +++ b/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, ) diff --git a/app/workers/emails_on_push_worker.rb b/app/workers/emails_on_push_worker.rb index b9cd49985dc..d4c3f14ec06 100644 --- a/app/workers/emails_on_push_worker.rb +++ b/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 diff --git a/spec/services/compare_service_spec.rb b/spec/services/compare_service_spec.rb index 3760f19aaa2..0a7fc58523f 100644 --- a/spec/services/compare_service_spec.rb +++ b/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