Merge branch...

Merge branch '17073-tagscontroller-index-is-terrible-response-time-goes-up-to-5-seconds' into 'master'

Update to gitlab_git 10.4.1 and take advantage of preserved Ref objects

See merge request !5536

CHANGELOG

@@ -10,6 +10,7 @@ v 8.11.0 (unreleased)
   - Limit git rev-list output count to one in forced push check
   - Clean up unused routes (Josef Strzibny)
   - Add green outline to New Branch button. !5447 (winniehell)
+  - Update to gitlab_git 10.4.1 and take advantage of preserved Ref objects
   - Retrieve rendered HTML from cache in one request
   - Fix renaming repository when name contains invalid chararacters under project settings
   - Nokogiri's various parsing methods are now instrumented

Gemfile

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

Gemfile.lock

@@ -278,7 +278,7 @@ GEM
       diff-lcs (~> 1.1)
       mime-types (>= 1.16, < 3)
       posix-spawn (~> 0.3)
-    gitlab_git (10.3.2)
+    gitlab_git (10.4.1)
       activesupport (~> 4.0)
       charlock_holmes (~> 0.7.3)
       github-linguist (~> 4.7.0)
@@ -870,7 +870,7 @@ DEPENDENCIES
   github-linguist (~> 4.7.0)
   github-markup (~> 1.4)
   gitlab-flowdock-git-hook (~> 1.0.1)
-  gitlab_git (~> 10.3.2)
+  gitlab_git (~> 10.4.1)
   gitlab_meta (= 7.0)
   gitlab_omniauth-ldap (~> 1.2.1)
   gollum-lib (~> 4.2)

app/models/repository.rb

@@ -70,7 +70,12 @@ class Repository
 
   def commit(ref = 'HEAD')
     return nil unless exists?
-    commit = Gitlab::Git::Commit.find(raw_repository, ref)
+    commit =
+      if ref.is_a?(Gitlab::Git::Commit)
+        ref
+      else
+        Gitlab::Git::Commit.find(raw_repository, ref)
+      end
     commit = ::Commit.new(commit, @project) if commit
     commit
   rescue Rugged::OdbError
@@ -158,7 +163,7 @@ class Repository
     before_remove_branch
 
     branch = find_branch(branch_name)
-    oldrev = branch.try(:target)
+    oldrev = branch.try(:target).try(:id)
     newrev = Gitlab::Git::BLANK_SHA
     ref    = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
 
@@ -259,10 +264,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, root_ref_hash)
+        count_commits_between(branch.target.sha, root_ref_hash)
 
       number_commits_ahead = raw_repository.
-        count_commits_between(root_ref_hash, branch.target)
+        count_commits_between(root_ref_hash, branch.target.sha)
 
       { behind: number_commits_behind, ahead: number_commits_ahead }
     end
@@ -688,9 +693,7 @@ class Repository
   end
 
   def local_branches
-    @local_branches ||= rugged.branches.each(:local).map do |branch|
-      Gitlab::Git::Branch.new(branch.name, branch.target)
-    end
+    @local_branches ||= raw_repository.local_branches
   end
 
   alias_method :branches, :local_branches
@@ -831,7 +834,7 @@ class Repository
   end
 
   def revert(user, commit, base_branch, revert_tree_id = nil)
-    source_sha = find_branch(base_branch).target
+    source_sha = find_branch(base_branch).target.sha
     revert_tree_id ||= check_revert_content(commit, base_branch)
 
     return false unless revert_tree_id
@@ -848,7 +851,7 @@ class Repository
   end
 
   def cherry_pick(user, commit, base_branch, cherry_pick_tree_id = nil)
-    source_sha = find_branch(base_branch).target
+    source_sha = find_branch(base_branch).target.sha
     cherry_pick_tree_id ||= check_cherry_pick_content(commit, base_branch)
 
     return false unless cherry_pick_tree_id
@@ -869,7 +872,7 @@ class Repository
   end
 
   def check_revert_content(commit, base_branch)
-    source_sha = find_branch(base_branch).target
+    source_sha = find_branch(base_branch).target.sha
     args       = [commit.id, source_sha]
     args << { mainline: 1 } if commit.merge_commit?
 
@@ -883,7 +886,7 @@ class Repository
   end
 
   def check_cherry_pick_content(commit, base_branch)
-    source_sha = find_branch(base_branch).target
+    source_sha = find_branch(base_branch).target.sha
     args       = [commit.id, source_sha]
     args << 1 if commit.merge_commit?
 
@@ -974,7 +977,7 @@ class Repository
     was_empty = empty?
 
     if !was_empty && target_branch
-      oldrev = target_branch.target
+      oldrev = target_branch.target.id
     end
 
     # Make commit
@@ -994,7 +997,7 @@ class Repository
         after_create_branch
       else
         # Update head
-        current_head = find_branch(branch).target
+        current_head = find_branch(branch).target.id
 
         # Make sure target branch was not changed during pre-receive hook
         if current_head == oldrev
@@ -1052,7 +1055,7 @@ class Repository
   end
 
   def tags_sorted_by_committed_date
-    tags.sort_by { |tag| commit(tag.target).committed_date }
+    tags.sort_by { |tag| tag.target.committed_date }
   end
 
   def keep_around_ref_name(sha)

app/services/delete_branch_service.rb

@@ -40,6 +40,6 @@ class DeleteBranchService < BaseService
 
   def build_push_data(branch)
     Gitlab::PushDataBuilder
-      .build(project, current_user, branch.target, Gitlab::Git::BLANK_SHA, "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch.name}", [])
+      .build(project, current_user, branch.target.sha, Gitlab::Git::BLANK_SHA, "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch.name}", [])
   end
 end

app/services/delete_tag_service.rb

@@ -34,6 +34,6 @@ class DeleteTagService < BaseService
 
   def build_push_data(tag)
     Gitlab::PushDataBuilder
-      .build(project, current_user, tag.target, Gitlab::Git::BLANK_SHA, "#{Gitlab::Git::TAG_REF_PREFIX}#{tag.name}", [])
+      .build(project, current_user, tag.target.sha, Gitlab::Git::BLANK_SHA, "#{Gitlab::Git::TAG_REF_PREFIX}#{tag.name}", [])
   end
 end

app/services/git_tag_push_service.rb

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

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

 .branch-commit
-  = link_to commit.short_id, namespace_project_commit_path(project.namespace, project, commit), class: "commit-id monospace"
+  = link_to commit.short_id, namespace_project_commit_path(project.namespace, project, commit.id), class: "commit-id monospace"
   ·
   %span.str-truncated
     = link_to_gfm commit.title, namespace_project_commit_path(project.namespace, project, commit.id), class: "commit-row-message"

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

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

spec/controllers/projects/commit_controller_spec.rb

@@ -24,15 +24,6 @@ describe Projects::CommitController do
       get :show, params.merge(extra_params)
     end
 
-    let(:project) { create(:project) }
-
-    before do
-      user = create(:user)
-      project.team << [user, :master]
-
-      sign_in(user)
-    end
-
     context 'with valid id' do
       it 'responds with 200' do
         go(id: commit.id)

spec/models/repository_spec.rb

@@ -50,8 +50,9 @@ describe Repository, models: true do
           double_first = double(committed_date: Time.now)
           double_last = double(committed_date: Time.now - 1.second)
 
-          allow(repository).to receive(:commit).with(tag_a.target).and_return(double_first)
-          allow(repository).to receive(:commit).with(tag_b.target).and_return(double_last)
+          allow(tag_a).to receive(:target).and_return(double_first)
+          allow(tag_b).to receive(:target).and_return(double_last)
+          allow(repository).to receive(:tags).and_return([tag_a, tag_b])
         end
 
         it { is_expected.to eq(['v1.0.0', 'v1.1.0']) }
@@ -64,8 +65,9 @@ describe Repository, models: true do
           double_first = double(committed_date: Time.now - 1.second)
           double_last = double(committed_date: Time.now)
 
-          allow(repository).to receive(:commit).with(tag_a.target).and_return(double_last)
-          allow(repository).to receive(:commit).with(tag_b.target).and_return(double_first)
+          allow(tag_a).to receive(:target).and_return(double_last)
+          allow(tag_b).to receive(:target).and_return(double_first)
+          allow(repository).to receive(:tags).and_return([tag_a, tag_b])
         end
 
         it { is_expected.to eq(['v1.1.0', 'v1.0.0']) }
@@ -381,9 +383,13 @@ describe Repository, models: true do
   end
 
   describe '#rm_branch' do
+    let(:old_rev) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } # git rev-parse feature
+    let(:blank_sha) { '0000000000000000000000000000000000000000' }
+
     context 'when pre hooks were successful' do
       it 'should run without errors' do
-        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, nil])
+        expect_any_instance_of(GitHooksService).to receive(:execute).
+          with(user, project.repository.path_to_repo, old_rev, blank_sha, 'refs/heads/feature')
 
         expect { repository.rm_branch(user, 'feature') }.not_to raise_error
       end
@@ -418,10 +424,13 @@ describe Repository, models: true do
   end
 
   describe '#commit_with_hooks' do
+    let(:old_rev) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } # git rev-parse feature
+
     context 'when pre hooks were successful' do
       before do
         expect_any_instance_of(GitHooksService).to receive(:execute).
-          and_return(true)
+          with(user, repository.path_to_repo, old_rev, sample_commit.id, 'refs/heads/feature').
+          and_yield.and_return(true)
       end
 
       it 'should run without errors' do
@@ -435,6 +444,14 @@ describe Repository, models: true do
 
         repository.commit_with_hooks(user, 'feature') { sample_commit.id }
       end
+
+      context "when the branch wasn't empty" do
+        it 'updates the head' do
+          expect(repository.find_branch('feature').target.id).to eq(old_rev)
+          repository.commit_with_hooks(user, 'feature') { sample_commit.id }
+          expect(repository.find_branch('feature').target.id).to eq(sample_commit.id)
+        end
+      end
     end
 
     context 'when pre hooks failed' do
@@ -1198,17 +1215,6 @@ describe Repository, models: true do
     end
   end
 
-  describe '#local_branches' do
-    it 'returns the local branches' do
-      masterrev = repository.find_branch('master').target
-      create_remote_branch('joe', 'remote_branch', masterrev)
-      repository.add_branch(user, 'local_branch', masterrev)
-
-      expect(repository.local_branches.any? { |branch| branch.name == 'remote_branch' }).to eq(false)
-      expect(repository.local_branches.any? { |branch| branch.name == 'local_branch' }).to eq(true)
-    end
-  end
-
   describe "#keep_around" do
     it "does not fail if we attempt to reference bad commit" do
       expect(repository.kept_around?('abc1234')).to be_falsey
@@ -1236,9 +1242,4 @@ describe Repository, models: true do
       File.delete(path)
     end
   end
-
-  def create_remote_branch(remote_name, branch_name, target)
-    rugged = repository.rugged
-    rugged.references.create("refs/remotes/#{remote_name}/#{branch_name}", target)
-  end
 end

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

+require 'spec_helper'
+
+describe 'projects/issues/_related_branches' do
+  include Devise::TestHelpers
+
+  let(:project) { create(:project) }
+  let(:branch) { project.repository.find_branch('feature') }
+  let!(:pipeline) { create(:ci_pipeline, project: project, sha: branch.target.id, ref: 'feature') }
+
+  before do
+    assign(:project, project)
+    assign(:related_branches, ['feature'])
+
+    render
+  end
+
+  it 'shows the related branches with their build status' do
+    expect(rendered).to match('feature')
+    expect(rendered).to have_css('.related-branch-ci-status')
+  end
+end