Merge branch 'use-rugged-to-create-tag' into 'master'

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

This was originally opened at !1757 by @pcarranza but I changed it to use Rugged instead of gitlab_git, following @DouweM's request.

Once this is merged, https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/26 will be mergeable too.

See merge request !3745

9 files changed, 105 insertions, 67 deletions

diff --git a/CHANGELOG b/CHANGELOG
index cc6588c6cbd..b2f5c283a1a 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 ddffc1f110a..7aebfe279fb 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -148,10 +148,20 @@ class Repository
     find_branch(branch_name)
   end
 
-  def add_tag(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
+
+    GitHooksService.new.execute(user, path_to_repo, oldrev, target, ref) do
+      rugged.tags.create(tag_name, target, options)
+    end
 
-    gitlab_shell.add_tag(path_with_namespace, tag_name, ref, message)
+    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 55985380d31..91ed0e354d0 100644
--- a/app/services/create_tag_service.rb
+++ b/app/services/create_tag_service.rb
@@ -1,50 +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
-    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)
+    new_tag = nil
+    begin
+      new_tag = repository.add_tag(current_user, tag_name, target, message)
+    rescue Rugged::TagError
+      return error("Tag #{tag_name} already exists")
+    rescue GitHooksService::PreReceiveError
+      return error('Tag creation was rejected by Git hook')
+    end
 
     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)
+      success.merge(tag: new_tag)
     else
-      error('Invalid reference name')
+      error("Target #{target} is invalid")
     end
   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/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 204c9eb20f3..34a13f9b5c9 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -858,13 +858,30 @@ describe Repository, models: true do
   end
 
   describe '#add_tag' do
-    it 'adds a tag' do
-      expect(repository).to receive(:before_push_tag)
+    context 'with a valid target' do
+      let(:user) { build_stubbed(:user) }
 
-      expect_any_instance_of(Gitlab::Shell).to receive(:add_tag).
-        with(repository.path_with_namespace, '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('8.5', 'master', 'foo')
+        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/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
 
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