From 44f89eafc08a7967544429a3f930354a5f9bbbaf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 15 Apr 2016 12:15:52 +0200 Subject: [PATCH 1/2] Use Rugged's TagCollection#create instead of gitlab-shell's Repository#add_tag for better performance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- CHANGELOG | 1 + app/models/repository.rb | 11 +++++-- app/services/create_tag_service.rb | 44 ++++++++++++-------------- features/steps/project/commits/tags.rb | 4 +-- lib/gitlab/backend/shell.rb | 18 ----------- spec/models/repository_spec.rb | 11 ++++--- spec/requests/api/tags_spec.rb | 4 +-- 7 files changed, 41 insertions(+), 52 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 4c5c2b42dfa..577a3643d97 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -13,6 +13,7 @@ v 8.8.0 (unreleased) - Allow "NEWS" and "CHANGES" as alternative names for CHANGELOG. !3768 (Connor Shea) - Added button to toggle whitespaces changes on diff view - Backport GitLab Enterprise support from EE + - Create tags using Rugged for performance reasons. !3745 - Files over 5MB can only be viewed in their raw form, files over 1MB without highlighting !3718 - Add support for supressing text diffs using .gitattributes on the default branch (Matt Oakes) - Added multiple colors for labels in dropdowns when dups happen. diff --git a/app/models/repository.rb b/app/models/repository.rb index b4319297e43..c4e38980a83 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -146,10 +146,17 @@ class Repository find_branch(branch_name) end - def add_tag(tag_name, ref, message = nil) + def add_tag(user, tag_name, ref, message = nil) before_push_tag - gitlab_shell.add_tag(path_with_namespace, tag_name, ref, message) + options = { message: message, tagger: user_to_committer(user) } if message + + tag = rugged.tags.create(tag_name, ref, options) + if tag.annotated? + Gitlab::Git::Tag.new(tag_name, ref, tag.annotation.message) + else + Gitlab::Git::Tag.new(tag_name, ref) + end end def rm_branch(user, branch_name) diff --git a/app/services/create_tag_service.rb b/app/services/create_tag_service.rb index 55985380d31..775f9db2a46 100644 --- a/app/services/create_tag_service.rb +++ b/app/services/create_tag_service.rb @@ -8,32 +8,28 @@ class CreateTagService < BaseService end repository = project.repository - existing_tag = repository.find_tag(tag_name) - if existing_tag - return error('Tag already exists') - end - message.strip! if message - - repository.add_tag(tag_name, ref, message) - new_tag = repository.find_tag(tag_name) - - if new_tag - push_data = create_push_data(project, current_user, new_tag) - EventCreateService.new.push(project, current_user, push_data) - project.execute_hooks(push_data.dup, :tag_push_hooks) - project.execute_services(push_data.dup, :tag_push_hooks) - CreateCommitBuildsService.new.execute(project, current_user, push_data) - - if release_description - CreateReleaseService.new(@project, @current_user). - execute(tag_name, release_description) - end - - success(new_tag) - else - error('Invalid reference name') + begin + new_tag = repository.add_tag(current_user, tag_name, ref, message) + rescue Rugged::TagError + return error("Tag #{tag_name} already exists") + rescue Rugged::ReferenceError + return error("Target #{ref} is invalid") end + + push_data = create_push_data(project, current_user, new_tag) + + EventCreateService.new.push(project, current_user, push_data) + project.execute_hooks(push_data.dup, :tag_push_hooks) + project.execute_services(push_data.dup, :tag_push_hooks) + CreateCommitBuildsService.new.execute(project, current_user, push_data) + + if release_description + CreateReleaseService.new(@project, @current_user). + execute(tag_name, release_description) + end + + success(new_tag) end def success(branch) diff --git a/features/steps/project/commits/tags.rb b/features/steps/project/commits/tags.rb index eff4234a44a..912ba580efd 100644 --- a/features/steps/project/commits/tags.rb +++ b/features/steps/project/commits/tags.rb @@ -57,11 +57,11 @@ class Spinach::Features::ProjectCommitsTags < Spinach::FeatureSteps end step 'I should see new an error that tag ref is invalid' do - expect(page).to have_content 'Invalid reference name' + expect(page).to have_content 'Target foo is invalid' end step 'I should see new an error that tag already exists' do - expect(page).to have_content 'Tag already exists' + expect(page).to have_content 'Tag v1.0.0 already exists' end step "I visit tag 'v1.1.0' page" do diff --git a/lib/gitlab/backend/shell.rb b/lib/gitlab/backend/shell.rb index 5e2fb863a8f..132f9cd1966 100644 --- a/lib/gitlab/backend/shell.rb +++ b/lib/gitlab/backend/shell.rb @@ -79,24 +79,6 @@ module Gitlab 'rm-project', "#{name}.git"]) end - # Add repository tag from passed ref - # - # path - project path with namespace - # tag_name - new tag name - # ref - HEAD for new tag - # message - optional message for tag (annotated tag) - # - # Ex. - # add_tag("gitlab/gitlab-ci", "v4.0", "master") - # add_tag("gitlab/gitlab-ci", "v4.0", "master", "message") - # - def add_tag(path, tag_name, ref, message = nil) - cmd = %W(#{gitlab_shell_path}/bin/gitlab-projects create-tag #{path}.git - #{tag_name} #{ref}) - cmd << message unless message.nil? || message.empty? - Gitlab::Utils.system_silent(cmd) - end - # Gc repository # # path - project path with namespace diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 397bb5a8028..5cdf644a0e1 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -859,12 +859,15 @@ describe Repository, models: true do describe '#add_tag' do it 'adds a tag' do + user = build_stubbed(:user) expect(repository).to receive(:before_push_tag) + expect(repository.rugged.tags).to receive(:create). + with('8.5', 'master', + hash_including(message: 'foo', + tagger: hash_including(name: user.name, email: user.email))). + and_call_original - expect_any_instance_of(Gitlab::Shell).to receive(:add_tag). - with(repository.path_with_namespace, '8.5', 'master', 'foo') - - repository.add_tag('8.5', 'master', 'foo') + repository.add_tag(user, '8.5', 'master', 'foo') end end diff --git a/spec/requests/api/tags_spec.rb b/spec/requests/api/tags_spec.rb index edcb2bedbf7..12e170b232f 100644 --- a/spec/requests/api/tags_spec.rb +++ b/spec/requests/api/tags_spec.rb @@ -147,7 +147,7 @@ describe API::API, api: true do tag_name: 'v8.0.0', ref: 'master' expect(response.status).to eq(400) - expect(json_response['message']).to eq('Tag already exists') + expect(json_response['message']).to eq('Tag v8.0.0 already exists') end it 'should return 400 if ref name is invalid' do @@ -155,7 +155,7 @@ describe API::API, api: true do tag_name: 'mytag', ref: 'foo' expect(response.status).to eq(400) - expect(json_response['message']).to eq('Invalid reference name') + expect(json_response['message']).to eq('Target foo is invalid') end end From 209f2f1e6fd861dd7bb6a73389400b4bb266d26d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 25 Apr 2016 16:31:45 +0200 Subject: [PATCH 2/2] Use a similar approach to branch creation for tag creation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/models/repository.rb | 17 ++++---- app/services/create_branch_service.rb | 5 --- app/services/create_tag_service.rb | 46 +++++++------------- spec/models/repository_spec.rb | 32 ++++++++++---- spec/services/create_tag_service_spec.rb | 53 ++++++++++++++++++++++++ 5 files changed, 101 insertions(+), 52 deletions(-) create mode 100644 spec/services/create_tag_service_spec.rb diff --git a/app/models/repository.rb b/app/models/repository.rb index c4e38980a83..d5d70f41fd3 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -146,17 +146,20 @@ class Repository find_branch(branch_name) end - def add_tag(user, tag_name, ref, message = nil) - before_push_tag + def add_tag(user, tag_name, target, message = nil) + oldrev = Gitlab::Git::BLANK_SHA + ref = Gitlab::Git::TAG_REF_PREFIX + tag_name + target = commit(target).try(:id) + + return false unless target options = { message: message, tagger: user_to_committer(user) } if message - tag = rugged.tags.create(tag_name, ref, options) - if tag.annotated? - Gitlab::Git::Tag.new(tag_name, ref, tag.annotation.message) - else - Gitlab::Git::Tag.new(tag_name, ref) + GitHooksService.new.execute(user, path_to_repo, oldrev, target, ref) do + rugged.tags.create(tag_name, target, options) end + + find_tag(tag_name) end def rm_branch(user, branch_name) diff --git a/app/services/create_branch_service.rb b/app/services/create_branch_service.rb index 707c2f7ff85..9f4481a8153 100644 --- a/app/services/create_branch_service.rb +++ b/app/services/create_branch_service.rb @@ -43,9 +43,4 @@ class CreateBranchService < BaseService out[:branch] = branch out end - - def build_push_data(project, user, branch) - Gitlab::PushDataBuilder. - build(project, user, Gitlab::Git::BLANK_SHA, branch.target, "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch.name}", []) - end end diff --git a/app/services/create_tag_service.rb b/app/services/create_tag_service.rb index 775f9db2a46..91ed0e354d0 100644 --- a/app/services/create_tag_service.rb +++ b/app/services/create_tag_service.rb @@ -1,46 +1,30 @@ require_relative 'base_service' class CreateTagService < BaseService - def execute(tag_name, ref, message, release_description = nil) + def execute(tag_name, target, message, release_description = nil) valid_tag = Gitlab::GitRefValidator.validate(tag_name) - if valid_tag == false - return error('Tag name invalid') - end + return error('Tag name invalid') unless valid_tag repository = project.repository message.strip! if message + + new_tag = nil begin - new_tag = repository.add_tag(current_user, tag_name, ref, message) + new_tag = repository.add_tag(current_user, tag_name, target, message) rescue Rugged::TagError return error("Tag #{tag_name} already exists") - rescue Rugged::ReferenceError - return error("Target #{ref} is invalid") + rescue GitHooksService::PreReceiveError + return error('Tag creation was rejected by Git hook') end - push_data = create_push_data(project, current_user, new_tag) - - EventCreateService.new.push(project, current_user, push_data) - project.execute_hooks(push_data.dup, :tag_push_hooks) - project.execute_services(push_data.dup, :tag_push_hooks) - CreateCommitBuildsService.new.execute(project, current_user, push_data) - - if release_description - CreateReleaseService.new(@project, @current_user). - execute(tag_name, release_description) + if new_tag + if release_description + CreateReleaseService.new(@project, @current_user). + execute(tag_name, release_description) + end + success.merge(tag: new_tag) + else + error("Target #{target} is invalid") end - - success(new_tag) - end - - def success(branch) - out = super() - out[:tag] = branch - out - end - - def create_push_data(project, user, tag) - commits = [project.commit(tag.target)].compact - Gitlab::PushDataBuilder. - build(project, user, Gitlab::Git::BLANK_SHA, tag.target, "#{Gitlab::Git::TAG_REF_PREFIX}#{tag.name}", commits, tag.message) end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 5cdf644a0e1..b3359a42237 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -858,16 +858,30 @@ describe Repository, models: true do end describe '#add_tag' do - it 'adds a tag' do - user = build_stubbed(:user) - expect(repository).to receive(:before_push_tag) - expect(repository.rugged.tags).to receive(:create). - with('8.5', 'master', - hash_including(message: 'foo', - tagger: hash_including(name: user.name, email: user.email))). - and_call_original + context 'with a valid target' do + let(:user) { build_stubbed(:user) } - repository.add_tag(user, '8.5', 'master', 'foo') + it 'creates the tag using rugged' do + expect(repository.rugged.tags).to receive(:create). + with('8.5', repository.commit('master').id, + hash_including(message: 'foo', + tagger: hash_including(name: user.name, email: user.email))). + and_call_original + + repository.add_tag(user, '8.5', 'master', 'foo') + end + + it 'returns a Gitlab::Git::Tag object' do + tag = repository.add_tag(user, '8.5', 'master', 'foo') + + expect(tag).to be_a(Gitlab::Git::Tag) + end + end + + context 'with an invalid target' do + it 'returns false' do + expect(repository.add_tag(user, '8.5', 'bar', 'foo')).to be false + end end end diff --git a/spec/services/create_tag_service_spec.rb b/spec/services/create_tag_service_spec.rb new file mode 100644 index 00000000000..91f9e663b66 --- /dev/null +++ b/spec/services/create_tag_service_spec.rb @@ -0,0 +1,53 @@ +require 'spec_helper' + +describe CreateTagService, services: true do + let(:project) { create(:project) } + let(:repository) { project.repository } + let(:user) { create(:user) } + let(:service) { described_class.new(project, user) } + + describe '#execute' do + it 'creates the tag and returns success' do + response = service.execute('v42.42.42', 'master', 'Foo') + + expect(response[:status]).to eq(:success) + expect(response[:tag]).to be_a Gitlab::Git::Tag + expect(response[:tag].name).to eq('v42.42.42') + end + + context 'when target is invalid' do + it 'returns an error' do + response = service.execute('v1.1.0', 'foo', 'Foo') + + expect(response).to eq(status: :error, + message: 'Target foo is invalid') + end + end + + context 'when tag already exists' do + it 'returns an error' do + expect(repository).to receive(:add_tag). + with(user, 'v1.1.0', 'master', 'Foo'). + and_raise(Rugged::TagError) + + response = service.execute('v1.1.0', 'master', 'Foo') + + expect(response).to eq(status: :error, + message: 'Tag v1.1.0 already exists') + end + end + + context 'when pre-receive hook fails' do + it 'returns an error' do + expect(repository).to receive(:add_tag). + with(user, 'v1.1.0', 'master', 'Foo'). + and_raise(GitHooksService::PreReceiveError) + + response = service.execute('v1.1.0', 'master', 'Foo') + + expect(response).to eq(status: :error, + message: 'Tag creation was rejected by Git hook') + end + end + end +end