Fix lightweight tags not processed correctly by GitTagPushService

When we updated gitlab_git to 10.4.1, `tag.target` changed from pointing
to the sha of the tag to the sha of the commit the tag points to. The
problem is that only annotated tags have `object_sha`s, lightweight tags
don't (it's nil), so (only) in their case we still need to use
`tag.target`.

CHANGELOG.md

@@ -56,6 +56,7 @@ Please view this file on the master branch, on stable branches it's out of date.
   - Fix error in generating labels. !7055
   - Stop clearing the database cache on `rake cache:clear`. !7056
   - Only show register tab if signup enabled. !7058
+  - Fix lightweight tags not processed correctly by GitTagPushService
   - Expire and build repository cache after project import. !7064
   - Fix bug where labels would be assigned to issues that were moved. !7065
   - Fix reply-by-email not working due to queue name mismatch. !7068

Gemfile

@@ -51,7 +51,7 @@ gem 'browser', '~> 2.2'
 
 # Extracting information from a git repository
 # Provide access to Gitlab::Git library
-gem 'gitlab_git', '~> 10.6.8'
+gem 'gitlab_git', '~> 10.7.0'
 
 # LDAP Auth
 # GitLab fork with several improvements to original library. For full list of changes

Gemfile.lock

@@ -283,7 +283,7 @@ GEM
       mime-types (>= 1.16, < 3)
       posix-spawn (~> 0.3)
     gitlab-markup (1.5.0)
-    gitlab_git (10.6.8)
+    gitlab_git (10.7.0)
       activesupport (~> 4.0)
       charlock_holmes (~> 0.7.3)
       github-linguist (~> 4.7.0)
@@ -870,7 +870,7 @@ DEPENDENCIES
   github-linguist (~> 4.7.0)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-markup (~> 1.5.0)
-  gitlab_git (~> 10.6.8)
+  gitlab_git (~> 10.7.0)
   gitlab_omniauth-ldap (~> 1.2.1)
   gollum-lib (~> 4.2)
   gollum-rugged_adapter (~> 0.4.2)
@@ -998,4 +998,4 @@ DEPENDENCIES
   wikicloth (= 0.8.1)
 
 BUNDLED WITH
-   1.13.3
+   1.13.5

app/controllers/projects/tags_controller.rb

@@ -23,7 +23,7 @@ class Projects::TagsController < Projects::ApplicationController
     return render_404 unless @tag
 
     @release = @project.releases.find_or_initialize_by(tag: @tag.name)
-    @commit = @repository.commit(@tag.target)
+    @commit = @repository.commit(@tag.dereferenced_target)
   end
 
   def create

app/models/repository.rb

@@ -181,7 +181,7 @@ class Repository
     before_remove_branch
 
     branch = find_branch(branch_name)
-    oldrev = branch.try(:target).try(:id)
+    oldrev = branch.try(:dereferenced_target).try(:id)
     newrev = Gitlab::Git::BLANK_SHA
     ref    = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
 
@@ -297,10 +297,10 @@ class Repository
       # Rugged seems to throw a `ReferenceError` when given branch_names rather
       # than SHA-1 hashes
       number_commits_behind = raw_repository.
-        count_commits_between(branch.target.sha, root_ref_hash)
+        count_commits_between(branch.dereferenced_target.sha, root_ref_hash)
 
       number_commits_ahead = raw_repository.
-        count_commits_between(root_ref_hash, branch.target.sha)
+        count_commits_between(root_ref_hash, branch.dereferenced_target.sha)
 
       { behind: number_commits_behind, ahead: number_commits_ahead }
     end
@@ -682,11 +682,11 @@ class Repository
       branches.sort_by(&:name)
     when 'updated_desc'
       branches.sort do |a, b|
-        commit(b.target).committed_date <=> commit(a.target).committed_date
+        commit(b.dereferenced_target).committed_date <=> commit(a.dereferenced_target).committed_date
       end
     when 'updated_asc'
       branches.sort do |a, b|
-        commit(a.target).committed_date <=> commit(b.target).committed_date
+        commit(a.dereferenced_target).committed_date <=> commit(b.dereferenced_target).committed_date
       end
     else
       branches
@@ -861,7 +861,7 @@ class Repository
       branch = find_branch(ref)
 
       if branch
-        last_commit = branch.target
+        last_commit = branch.dereferenced_target
         index.read_tree(last_commit.raw_commit.tree)
         parents = [last_commit.sha]
       end
@@ -948,7 +948,7 @@ class Repository
   end
 
   def revert(user, commit, base_branch, revert_tree_id = nil)
-    source_sha = find_branch(base_branch).target.sha
+    source_sha = find_branch(base_branch).dereferenced_target.sha
     revert_tree_id ||= check_revert_content(commit, base_branch)
 
     return false unless revert_tree_id
@@ -965,7 +965,7 @@ class Repository
   end
 
   def cherry_pick(user, commit, base_branch, cherry_pick_tree_id = nil)
-    source_sha = find_branch(base_branch).target.sha
+    source_sha = find_branch(base_branch).dereferenced_target.sha
     cherry_pick_tree_id ||= check_cherry_pick_content(commit, base_branch)
 
     return false unless cherry_pick_tree_id
@@ -994,7 +994,7 @@ class Repository
   end
 
   def check_revert_content(commit, base_branch)
-    source_sha = find_branch(base_branch).target.sha
+    source_sha = find_branch(base_branch).dereferenced_target.sha
     args       = [commit.id, source_sha]
     args << { mainline: 1 } if commit.merge_commit?
 
@@ -1008,7 +1008,7 @@ class Repository
   end
 
   def check_cherry_pick_content(commit, base_branch)
-    source_sha = find_branch(base_branch).target.sha
+    source_sha = find_branch(base_branch).dereferenced_target.sha
     args       = [commit.id, source_sha]
     args << 1 if commit.merge_commit?
 
@@ -1081,7 +1081,7 @@ class Repository
     if rugged.lookup(newrev).parent_ids.empty? || target_branch.nil?
       oldrev = Gitlab::Git::BLANK_SHA
     else
-      oldrev = rugged.merge_base(newrev, target_branch.target.sha)
+      oldrev = rugged.merge_base(newrev, target_branch.dereferenced_target.sha)
     end
 
     GitHooksService.new.execute(current_user, path_to_repo, oldrev, newrev, ref) do
@@ -1141,7 +1141,7 @@ class Repository
   end
 
   def tags_sorted_by_committed_date
-    tags.sort_by { |tag| tag.target.committed_date }
+    tags.sort_by { |tag| tag.dereferenced_target.committed_date }
   end
 
   def keep_around_ref_name(sha)

app/services/delete_branch_service.rb

@@ -42,7 +42,7 @@ class DeleteBranchService < BaseService
     Gitlab::DataBuilder::Push.build(
       project,
       current_user,
-      branch.target.sha,
+      branch.dereferenced_target.sha,
       Gitlab::Git::BLANK_SHA,
       "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch.name}",
       [])

app/services/delete_tag_service.rb

@@ -36,7 +36,7 @@ class DeleteTagService < BaseService
     Gitlab::DataBuilder::Push.build(
       project,
       current_user,
-      tag.target.sha,
+      tag.dereferenced_target.sha,
       Gitlab::Git::BLANK_SHA,
       "#{Gitlab::Git::TAG_REF_PREFIX}#{tag.name}",
       [])

app/services/git_tag_push_service.rb

@@ -27,8 +27,8 @@ class GitTagPushService < BaseService
       tag_name = Gitlab::Git.ref_name(params[:ref])
       tag = project.repository.find_tag(tag_name)
 
-      if tag && tag.object_sha == params[:newrev]
-        commit = project.commit(tag.target)
+      if tag && tag.target == params[:newrev]
+        commit = project.commit(tag.dereferenced_target)
         commits = [commit].compact
         message = tag.message
       end

app/views/projects/branches/_branch.html.haml

-- commit = @repository.commit(branch.target)
+- commit = @repository.commit(branch.dereferenced_target)
 - bar_graph_width_factor = @max_commits > 0 ? 100.0/@max_commits : 0
 - diverging_commit_counts = @repository.diverging_commit_counts(branch)
 - number_commits_behind = diverging_commit_counts[:behind]

app/views/projects/issues/_related_branches.html.haml

@@ -4,7 +4,7 @@
   %ul.unstyled-list.related-merge-requests
     - @related_branches.each do |branch|
       %li
-        - target = @project.repository.find_branch(branch).target
+        - target = @project.repository.find_branch(branch).dereferenced_target
         - pipeline = @project.pipeline_for(branch, target.sha) if target
         - if pipeline
           %span.related-branch-ci-status

app/views/projects/tags/_tag.html.haml

-- commit = @repository.commit(tag.target)
+- commit = @repository.commit(tag.dereferenced_target)
 - release = @releases.find { |release| release.tag == tag.name }
 %li
   %div

lib/api/entities.rb

@@ -138,7 +138,7 @@ module API
       expose :name
 
       expose :commit do |repo_branch, options|
-        options[:project].repository.commit(repo_branch.target)
+        options[:project].repository.commit(repo_branch.dereferenced_target)
       end
 
       expose :protected do |repo_branch, options|
@@ -523,7 +523,7 @@ module API
       expose :name, :message
 
       expose :commit do |repo_tag, options|
-        options[:project].repository.commit(repo_tag.target)
+        options[:project].repository.commit(repo_tag.dereferenced_target)
       end
 
       expose :release, using: Entities::Release do |repo_tag, options|

lib/gitlab/data_builder/push.rb

@@ -83,7 +83,7 @@ module Gitlab
           tag = repository.find_tag(tag_name)
 
           if tag
-            commit = repository.commit(tag.target)
+            commit = repository.commit(tag.dereferenced_target)
             commit.try(:sha)
           end
         else

spec/finders/branches_finder_spec.rb

@@ -21,7 +21,7 @@ describe BranchesFinder do
         result = branches_finder.execute
 
         recently_updated_branch = repository.branches.max do |a, b|
-          repository.commit(a.target).committed_date <=> repository.commit(b.target).committed_date
+          repository.commit(a.dereferenced_target).committed_date <=> repository.commit(b.dereferenced_target).committed_date
         end
 
         expect(result.first.name).to eq(recently_updated_branch.name)

spec/finders/tags_finder_spec.rb

@@ -20,7 +20,7 @@ describe TagsFinder do
 
         result = tags_finder.execute
         recently_updated_tag = repository.tags.max do |a, b|
-          repository.commit(a.target).committed_date <=> repository.commit(b.target).committed_date
+          repository.commit(a.dereferenced_target).committed_date <=> repository.commit(b.dereferenced_target).committed_date
         end
 
         expect(result.first.name).to eq(recently_updated_tag.name)

spec/models/repository_spec.rb

@@ -68,8 +68,8 @@ describe Repository, models: true do
           double_first = double(committed_date: Time.now)
           double_last = double(committed_date: Time.now - 1.second)
 
-          allow(tag_a).to receive(:target).and_return(double_first)
-          allow(tag_b).to receive(:target).and_return(double_last)
+          allow(tag_a).to receive(:dereferenced_target).and_return(double_first)
+          allow(tag_b).to receive(:dereferenced_target).and_return(double_last)
           allow(repository).to receive(:tags).and_return([tag_a, tag_b])
         end
 
@@ -83,8 +83,8 @@ describe Repository, models: true do
           double_first = double(committed_date: Time.now - 1.second)
           double_last = double(committed_date: Time.now)
 
-          allow(tag_a).to receive(:target).and_return(double_last)
-          allow(tag_b).to receive(:target).and_return(double_first)
+          allow(tag_a).to receive(:dereferenced_target).and_return(double_last)
+          allow(tag_b).to receive(:dereferenced_target).and_return(double_first)
           allow(repository).to receive(:tags).and_return([tag_a, tag_b])
         end
 
@@ -632,9 +632,9 @@ describe Repository, models: true do
 
       context "when the branch wasn't empty" do
         it 'updates the head' do
-          expect(repository.find_branch('feature').target.id).to eq(old_rev)
+          expect(repository.find_branch('feature').dereferenced_target.id).to eq(old_rev)
           repository.update_branch_with_hooks(user, 'feature') { new_rev }
-          expect(repository.find_branch('feature').target.id).to eq(new_rev)
+          expect(repository.find_branch('feature').dereferenced_target.id).to eq(new_rev)
         end
       end
     end
@@ -659,7 +659,7 @@ describe Repository, models: true do
     context 'when the update would remove commits from the target branch' do
       it 'raises an exception' do
         branch = 'master'
-        old_rev = repository.find_branch(branch).target.sha
+        old_rev = repository.find_branch(branch).dereferenced_target.sha
 
         # The 'master' branch is NOT an ancestor of new_rev.
         expect(repository.rugged.merge_base(old_rev, new_rev)).not_to eq(old_rev)

spec/services/git_tag_push_service_spec.rb

@@ -37,65 +37,138 @@ describe GitTagPushService, services: true do
   end
 
   describe "Git Tag Push Data" do
-    before do
-      service.execute
-      @push_data = service.push_data
-      @tag_name = Gitlab::Git.ref_name(ref)
-      @tag = project.repository.find_tag(@tag_name)
-      @commit = project.commit(@tag.target)
-    end
-
     subject { @push_data }
+    let(:tag) { project.repository.find_tag(tag_name) }
+    let(:commit) { tag.dereferenced_target }
 
-    it { is_expected.to include(object_kind: 'tag_push') }
-    it { is_expected.to include(ref: ref) }
-    it { is_expected.to include(before: oldrev) }
-    it { is_expected.to include(after: newrev) }
-    it { is_expected.to include(message: @tag.message) }
-    it { is_expected.to include(user_id: user.id) }
-    it { is_expected.to include(user_name: user.name) }
-    it { is_expected.to include(project_id: project.id) }
-
-    context "with repository data" do
-      subject { @push_data[:repository] }
-
-      it { is_expected.to include(name: project.name) }
-      it { is_expected.to include(url: project.url_to_repo) }
-      it { is_expected.to include(description: project.description) }
-      it { is_expected.to include(homepage: project.web_url) }
-    end
+    context 'annotated tag' do
+      let(:tag_name) { Gitlab::Git.ref_name(ref) }
 
-    context "with commits" do
-      subject { @push_data[:commits] }
+      before do
+        service.execute
+        @push_data = service.push_data
+      end
 
-      it { is_expected.to be_an(Array) }
-      it 'has 1 element' do
-        expect(subject.size).to eq(1)
+      it { is_expected.to include(object_kind: 'tag_push') }
+      it { is_expected.to include(ref: ref) }
+      it { is_expected.to include(before: oldrev) }
+      it { is_expected.to include(after: newrev) }
+      it { is_expected.to include(message: tag.message) }
+      it { is_expected.to include(user_id: user.id) }
+      it { is_expected.to include(user_name: user.name) }
+      it { is_expected.to include(project_id: project.id) }
+
+      context "with repository data" do
+        subject { @push_data[:repository] }
+
+        it { is_expected.to include(name: project.name) }
+        it { is_expected.to include(url: project.url_to_repo) }
+        it { is_expected.to include(description: project.description) }
+        it { is_expected.to include(homepage: project.web_url) }
       end
 
-      context "the commit" do
-        subject { @push_data[:commits].first }
-
-        it { is_expected.to include(id: @commit.id) }
-        it { is_expected.to include(message: @commit.safe_message) }
-        it { is_expected.to include(timestamp: @commit.date.xmlschema) }
-        it do
-          is_expected.to include(
-            url: [
-             Gitlab.config.gitlab.url,
-             project.namespace.to_param,
-             project.to_param,
-             'commit',
-             @commit.id
-            ].join('/')
-          )
+      context "with commits" do
+        subject { @push_data[:commits] }
+
+        it { is_expected.to be_an(Array) }
+        it 'has 1 element' do
+          expect(subject.size).to eq(1)
+        end
+
+        context "the commit" do
+          subject { @push_data[:commits].first }
+
+          it { is_expected.to include(id: commit.id) }
+          it { is_expected.to include(message: commit.safe_message) }
+          it { is_expected.to include(timestamp: commit.date.xmlschema) }
+          it do
+            is_expected.to include(
+              url: [
+               Gitlab.config.gitlab.url,
+               project.namespace.to_param,
+               project.to_param,
+               'commit',
+               commit.id
+              ].join('/')
+            )
+          end
+
+          context "with a author" do
+            subject { @push_data[:commits].first[:author] }
+
+            it { is_expected.to include(name: commit.author_name) }
+            it { is_expected.to include(email: commit.author_email) }
+          end
         end
+      end
+    end
 
-        context "with a author" do
-          subject { @push_data[:commits].first[:author] }
+    context 'lightweight tag' do
+      let(:tag_name) { 'light-tag' }
+      let(:newrev) { '5937ac0a7beb003549fc5fd26fc247adbce4a52e' }
+      let(:ref) { "refs/tags/light-tag" }
+
+      before do
+        # Create the lightweight tag
+        project.repository.raw_repository.rugged.tags.create(tag_name, newrev)
+
+        # Clear tag list cache
+        project.repository.expire_tags_cache
+
+        service.execute
+        @push_data = service.push_data
+      end
+
+      it { is_expected.to include(object_kind: 'tag_push') }
+      it { is_expected.to include(ref: ref) }
+      it { is_expected.to include(before: oldrev) }
+      it { is_expected.to include(after: newrev) }
+      it { is_expected.to include(message: tag.message) }
+      it { is_expected.to include(user_id: user.id) }
+      it { is_expected.to include(user_name: user.name) }
+      it { is_expected.to include(project_id: project.id) }
+
+      context "with repository data" do
+        subject { @push_data[:repository] }
+
+        it { is_expected.to include(name: project.name) }
+        it { is_expected.to include(url: project.url_to_repo) }
+        it { is_expected.to include(description: project.description) }
+        it { is_expected.to include(homepage: project.web_url) }
+      end
+
+      context "with commits" do
+        subject { @push_data[:commits] }
+
+        it { is_expected.to be_an(Array) }
+        it 'has 1 element' do
+          expect(subject.size).to eq(1)
+        end
 
-          it { is_expected.to include(name: @commit.author_name) }
-          it { is_expected.to include(email: @commit.author_email) }
+        context "the commit" do
+          subject { @push_data[:commits].first }
+
+          it { is_expected.to include(id: commit.id) }
+          it { is_expected.to include(message: commit.safe_message) }
+          it { is_expected.to include(timestamp: commit.date.xmlschema) }
+          it do
+            is_expected.to include(
+              url: [
+               Gitlab.config.gitlab.url,
+               project.namespace.to_param,
+               project.to_param,
+               'commit',
+               commit.id
+              ].join('/')
+            )
+          end
+
+          context "with a author" do
+            subject { @push_data[:commits].first[:author] }
+
+            it { is_expected.to include(name: commit.author_name) }
+            it { is_expected.to include(email: commit.author_email) }
+          end
         end
       end
     end

spec/views/projects/issues/_related_branches.html.haml_spec.rb

@@ -5,7 +5,7 @@ describe 'projects/issues/_related_branches' do
 
   let(:project) { create(:project) }
   let(:branch) { project.repository.find_branch('feature') }
-  let!(:pipeline) { create(:ci_pipeline, project: project, sha: branch.target.id, ref: 'feature') }
+  let!(:pipeline) { create(:ci_pipeline, project: project, sha: branch.dereferenced_target.id, ref: 'feature') }
 
   before do
     assign(:project, project)