From c13f712c77e0837760e79293b6fb41c734741e77 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Fri, 18 Aug 2017 17:43:23 -0700 Subject: [PATCH 1/6] implement Repository#== so that with_repo_branch_commit can properly short-circuit --- app/models/repository.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/models/repository.rb b/app/models/repository.rb index c1e4fcf94a4..1c3080c0efd 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -60,6 +60,12 @@ class Repository @project = project end + def equals(other) + @disk_path == other.disk_path + end + + alias_method :==, :equals + def raw_repository return nil unless full_path From 00b440443808fba04fc55f7e77c61f79e29c21ea Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Mon, 21 Aug 2017 15:20:44 -0700 Subject: [PATCH 2/6] skip the branch fetch if we already have the sha --- app/models/repository.rb | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 1c3080c0efd..61fab9764e8 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -991,23 +991,27 @@ class Repository end def with_repo_branch_commit(start_repository, start_branch_name) - return yield(nil) if start_repository.empty_repo? + tmp_ref = nil + return yield nil if start_repository.empty_repo? - branch_name_or_sha = + branch_commit = if start_repository == self - start_branch_name + commit(start_branch_name) else - tmp_ref = fetch_ref( - start_repository.path_to_repo, - "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}", - "refs/tmp/#{SecureRandom.hex}/head" - ) + sha = start_repository.find_branch(start_branch_name).target + commit(sha) || + begin + tmp_ref = fetch_ref( + start_repository.path_to_repo, + "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}", + "refs/tmp/#{SecureRandom.hex}/head" + ) - start_repository.commit(start_branch_name).sha + commit(start_repository.commit(start_branch_name).sha) + end end - yield(commit(branch_name_or_sha)) - + yield branch_commit ensure rugged.references.delete(tmp_ref) if tmp_ref end From 6d9fb6b0b779d7065f0c36ec15a42d0d459abbeb Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 29 Aug 2017 21:11:38 +0800 Subject: [PATCH 3/6] It doesn't seem that rubocop is complaining for me --- app/models/repository.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 0f67395b88b..9fe39172b19 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -60,12 +60,10 @@ class Repository @project = project end - def equals(other) + def ==(other) @disk_path == other.disk_path end - alias_method :==, :equals - def raw_repository return nil unless full_path From e6630d7f7276dc5cec2a02e32dc74a0a060886ab Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 29 Aug 2017 22:42:02 +0800 Subject: [PATCH 4/6] Make sure inspect doesn't generate crazy string --- app/models/repository.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/models/repository.rb b/app/models/repository.rb index 9fe39172b19..04a76c365be 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -79,6 +79,10 @@ class Repository ) end + def inspect + "#<#{self.class.name}:#{@disk_path}>" + end + # # Git repository can contains some hidden refs like: # /refs/notes/* From 9d3ee1ff139650e064a41fd90d34cd3f558c1099 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 29 Aug 2017 22:42:34 +0800 Subject: [PATCH 5/6] Further break with_repo_branch_commit into parts So it's more clear what could happen. Also add more tests about the behaviour. --- app/models/repository.rb | 43 +++++++++++++++++++--------------- spec/models/repository_spec.rb | 36 +++++++++++++++++++++++++++- 2 files changed, 59 insertions(+), 20 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 04a76c365be..93017dfedf9 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -1000,29 +1000,22 @@ class Repository end def with_repo_branch_commit(start_repository, start_branch_name) - tmp_ref = nil return yield nil if start_repository.empty_repo? - branch_commit = - if start_repository == self - commit(start_branch_name) + if start_repository == self + yield commit(start_branch_name) + else + sha = start_repository.commit(start_branch_name).sha + + if branch_commit = commit(sha) + yield branch_commit else - sha = start_repository.find_branch(start_branch_name).target - commit(sha) || - begin - tmp_ref = fetch_ref( - start_repository.path_to_repo, - "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}", - "refs/tmp/#{SecureRandom.hex}/head" - ) - - commit(start_repository.commit(start_branch_name).sha) - end + with_repo_tmp_commit( + start_repository, start_branch_name, sha) do |tmp_commit| + yield tmp_commit + end end - - yield branch_commit - ensure - rugged.references.delete(tmp_ref) if tmp_ref + end end def add_remote(name, url) @@ -1231,4 +1224,16 @@ class Repository .commits_by_message(query, revision: ref, path: path, limit: limit, offset: offset) .map { |c| commit(c) } end + + def with_repo_tmp_commit(start_repository, start_branch_name, sha) + tmp_ref = fetch_ref( + start_repository.path_to_repo, + "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}", + "refs/tmp/#{SecureRandom.hex}/head" + ) + + yield commit(sha) + ensure + rugged.references.delete(tmp_ref) if tmp_ref + end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 462e92b8b62..8d9a86d952b 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -923,13 +923,16 @@ describe Repository, models: true do describe '#update_branch_with_hooks' do let(:old_rev) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } # git rev-parse feature let(:new_rev) { 'a74ae73c1ccde9b974a70e82b901588071dc142a' } # commit whose parent is old_rev + let(:updating_ref) { 'refs/heads/feature' } + let(:target_project) { project } + let(:target_repository) { target_project.repository } context 'when pre hooks were successful' do before do service = Gitlab::Git::HooksService.new expect(Gitlab::Git::HooksService).to receive(:new).and_return(service) expect(service).to receive(:execute) - .with(committer, repository, old_rev, new_rev, 'refs/heads/feature') + .with(committer, target_repository, old_rev, new_rev, updating_ref) .and_yield(service).and_return(true) end @@ -960,6 +963,37 @@ describe Repository, models: true do expect(repository.find_branch('feature').dereferenced_target.id).to eq(new_rev) end end + + context 'when target project does not have the commit' do + let(:target_project) { create(:project, :empty_repo) } + let(:old_rev) { Gitlab::Git::BLANK_SHA } + let(:new_rev) { project.commit('feature').sha } + let(:updating_ref) { 'refs/heads/master' } + + it 'fetch_ref and create the branch' do + expect(target_project.repository).to receive(:fetch_ref) + .and_call_original + + GitOperationService.new(committer, target_repository) + .with_branch( + 'master', + start_project: project, + start_branch_name: 'feature') { new_rev } + + expect(target_repository.branch_names).to contain_exactly('master') + end + end + + context 'when target project already has the commit' do + let(:target_project) { create(:project, :repository) } + + it 'does not fetch_ref and just pass the commit' do + expect(target_repository).not_to receive(:fetch_ref) + + GitOperationService.new(committer, target_repository) + .with_branch('feature', start_project: project) { new_rev } + end + end end context 'when temporary ref failed to be created from other project' do From 7fba1e85fdd7a09b07d799712c738bbf6a0b1da7 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 30 Aug 2017 19:07:58 +0800 Subject: [PATCH 6/6] Add changelog entry --- changelogs/unreleased/perf-slow-issuable.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelogs/unreleased/perf-slow-issuable.yml diff --git a/changelogs/unreleased/perf-slow-issuable.yml b/changelogs/unreleased/perf-slow-issuable.yml new file mode 100644 index 00000000000..29d15be1401 --- /dev/null +++ b/changelogs/unreleased/perf-slow-issuable.yml @@ -0,0 +1,6 @@ +--- +title: Fix repository equality check and avoid fetching ref if the commit is already + available. This affects merge request creation performance +merge_request: 13685 +author: +type: other