Use Rugged's TagCollection#create instead of gitlab-shell's Repository#add_tag for better performance

Signed-off-by: Rémy Coutable <remy@rymai.me>

7 files changed, 38 insertions, 49 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
+    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
 
-    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)
+    push_data = create_push_data(project, current_user, new_tag)
 
-      if release_description
-        CreateReleaseService.new(@project, @current_user).
-          execute(tag_name, release_description)
-      end
+    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)
 
-      success(new_tag)
-    else
-      error('Invalid reference name')
+    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