diff options
-rw-r--r-- | app/services/compare_service.rb | 4 | ||||
-rw-r--r-- | changelogs/unreleased/sh-avoid-fetching-temp-refs-within-project.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/git/repository.rb | 32 | ||||
-rw-r--r-- | spec/controllers/projects/compare_controller_spec.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/git/repository_spec.rb | 64 | ||||
-rw-r--r-- | spec/services/compare_service_spec.rb | 13 |
6 files changed, 110 insertions, 10 deletions
diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb index 3adf8a0c1a1..3f0aedfbfb2 100644 --- a/app/services/compare_service.rb +++ b/app/services/compare_service.rb @@ -3,7 +3,7 @@ require 'securerandom' # Compare 2 refs for one repo or between repositories -# and return Gitlab::Git::Compare object that responds to commits and diffs +# and return Compare object that responds to commits and diffs class CompareService attr_reader :start_project, :start_ref_name @@ -15,7 +15,7 @@ class CompareService def execute(target_project, target_ref, base_sha: nil, straight: false) raw_compare = target_project.repository.compare_source_branch(target_ref, start_project.repository, start_ref_name, straight: straight) - return unless raw_compare + return unless raw_compare && raw_compare.base && raw_compare.head Compare.new(raw_compare, target_project, diff --git a/changelogs/unreleased/sh-avoid-fetching-temp-refs-within-project.yml b/changelogs/unreleased/sh-avoid-fetching-temp-refs-within-project.yml new file mode 100644 index 00000000000..7511543f7f6 --- /dev/null +++ b/changelogs/unreleased/sh-avoid-fetching-temp-refs-within-project.yml @@ -0,0 +1,5 @@ +--- +title: Don't create a temp reference for branch comparisons within project +merge_request: 24038 +author: +type: fixed diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index a22e3c4b9dd..c12cb6a6434 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -732,18 +732,29 @@ module Gitlab end def compare_source_branch(target_branch_name, source_repository, source_branch_name, straight:) + reachable_ref = + if source_repository == self + source_branch_name + else + # If a tmp ref was created before for a separate repo comparison (forks), + # we're able to short-circuit the tmp ref re-creation: + # 1. Take the SHA from the source repo + # 2. Read that in the current "target" repo + # 3. If that SHA is still known (readable), it means GC hasn't + # cleaned it up yet, so we can use it instead re-writing the tmp ref. + source_commit_id = source_repository.commit(source_branch_name)&.sha + commit(source_commit_id)&.sha if source_commit_id + end + + return compare(target_branch_name, reachable_ref, straight: straight) if reachable_ref + tmp_ref = "refs/tmp/#{SecureRandom.hex}" return unless fetch_source_branch!(source_repository, source_branch_name, tmp_ref) - Gitlab::Git::Compare.new( - self, - target_branch_name, - tmp_ref, - straight: straight - ) + compare(target_branch_name, tmp_ref, straight: straight) ensure - delete_refs(tmp_ref) + delete_refs(tmp_ref) if tmp_ref end def write_ref(ref_path, ref, old_ref: nil) @@ -999,6 +1010,13 @@ module Gitlab private + def compare(base_ref, head_ref, straight:) + Gitlab::Git::Compare.new(self, + base_ref, + head_ref, + straight: straight) + end + def empty_diff_stats Gitlab::Git::DiffStatsCollection.new([]) end diff --git a/spec/controllers/projects/compare_controller_spec.rb b/spec/controllers/projects/compare_controller_spec.rb index 1818809518d..92380a2bf09 100644 --- a/spec/controllers/projects/compare_controller_spec.rb +++ b/spec/controllers/projects/compare_controller_spec.rb @@ -82,7 +82,7 @@ describe Projects::CompareController do show_request expect(response).to be_success - expect(assigns(:diffs).diff_files.to_a).to eq([]) + expect(assigns(:diffs)).to eq([]) expect(assigns(:commits)).to eq([]) end end diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 778950c95e4..45fe5d72937 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -1966,6 +1966,70 @@ describe Gitlab::Git::Repository, :seed_helper do end end + describe '#compare_source_branch' do + let(:repository) { Gitlab::Git::Repository.new('default', TEST_GITATTRIBUTES_REPO_PATH, '', 'group/project') } + + context 'within same repository' do + it 'does not create a temp ref' do + expect(repository).not_to receive(:fetch_source_branch!) + expect(repository).not_to receive(:delete_refs) + + compare = repository.compare_source_branch('master', repository, 'feature', straight: false) + expect(compare).to be_a(Gitlab::Git::Compare) + expect(compare.commits.count).to be > 0 + end + + it 'returns empty commits when source ref does not exist' do + compare = repository.compare_source_branch('master', repository, 'non-existent-branch', straight: false) + + expect(compare.commits).to be_empty + end + end + + context 'with different repositories' do + context 'when ref is known by source repo, but not by target' do + before do + mutable_repository.write_ref('another-branch', 'feature') + end + + it 'creates temp ref' do + expect(repository).not_to receive(:fetch_source_branch!) + expect(repository).not_to receive(:delete_refs) + + compare = repository.compare_source_branch('master', mutable_repository, 'another-branch', straight: false) + expect(compare).to be_a(Gitlab::Git::Compare) + expect(compare.commits.count).to be > 0 + end + end + + context 'when ref is known by source and target repos' do + before do + mutable_repository.write_ref('another-branch', 'feature') + repository.write_ref('another-branch', 'feature') + end + + it 'does not create a temp ref' do + expect(repository).not_to receive(:fetch_source_branch!) + expect(repository).not_to receive(:delete_refs) + + compare = repository.compare_source_branch('master', mutable_repository, 'another-branch', straight: false) + expect(compare).to be_a(Gitlab::Git::Compare) + expect(compare.commits.count).to be > 0 + end + end + + context 'when ref is unknown by source repo' do + it 'returns nil when source ref does not exist' do + expect(repository).to receive(:fetch_source_branch!).and_call_original + expect(repository).to receive(:delete_refs).and_call_original + + compare = repository.compare_source_branch('master', mutable_repository, 'non-existent-branch', straight: false) + expect(compare).to be_nil + end + end + end + end + describe '#checksum' do it 'calculates the checksum for non-empty repo' do expect(repository.checksum).to eq '51d0a9662681f93e1fee547a6b7ba2bcaf716059' diff --git a/spec/services/compare_service_spec.rb b/spec/services/compare_service_spec.rb index 0e4ef69ec19..fadd43635a6 100644 --- a/spec/services/compare_service_spec.rb +++ b/spec/services/compare_service_spec.rb @@ -19,5 +19,18 @@ describe CompareService do it { expect(subject.diffs.size).to eq(3) } end + + context 'compare with target branch that does not exist' do + subject { service.execute(project, 'non-existent-ref') } + + it { expect(subject).to be_nil } + end + + context 'compare with source branch that does not exist' do + let(:service) { described_class.new(project, 'non-existent-branch') } + subject { service.execute(project, 'non-existent-ref') } + + it { expect(subject).to be_nil } + end end end |