Merge branch 'ee-gitaly-local-branches' into 'master'

Gitaly local branches

See merge request !1751

app/models/repository.rb

@@ -649,22 +649,8 @@ class Repository
     "#{name}-#{highest_branch_id + 1}"
   end
 
-  # Remove archives older than 2 hours
   def branches_sorted_by(value)
-    case value
-    when 'name'
-      branches.sort_by(&:name)
-    when 'updated_desc'
-      branches.sort do |a, b|
-        commit(b.dereferenced_target).committed_date <=> commit(a.dereferenced_target).committed_date
-      end
-    when 'updated_asc'
-      branches.sort do |a, b|
-        commit(a.dereferenced_target).committed_date <=> commit(b.dereferenced_target).committed_date
-      end
-    else
-      branches
-    end
+    raw_repository.local_branches(sort_by: value)
   end
 
   def tags_sorted_by(value)

changelogs/unreleased/gitaly-local-branches.yml

+---
+title: Add suport for find_local_branches GRPC from Gitaly
+merge_request: 10059
+author:

lib/gitlab/git/branch.rb

 module Gitlab
   module Git
     class Branch < Ref
+      def initialize(repository, name, target)
+        if target.is_a?(Gitaly::FindLocalBranchResponse)
+          target = target_from_gitaly_local_branches_response(target)
+        end
+
+        super(repository, name, target)
+      end
+
+      def target_from_gitaly_local_branches_response(response)
+        # Git messages have no encoding enforcements. However, in the UI we only
+        # handle UTF-8, so basically we cross our fingers that the message force
+        # encoded to UTF-8 is readable.
+        message = response.commit_subject.dup.force_encoding('UTF-8')
+
+        # NOTE: For ease of parsing in Gitaly, we have only the subject of
+        # the commit and not the full message. This is ok, since all the
+        # code that uses `local_branches` only cares at most about the
+        # commit message.
+        # TODO: Once gitaly "takes over" Rugged consider separating the
+        # subject from the message to make it clearer when there's one
+        # available but not the other.
+        hash = {
+          id: response.commit_id,
+          message: message,
+          authored_date: Time.at(response.commit_author.date.seconds),
+          author_name: response.commit_author.name,
+          author_email: response.commit_author.email,
+          committed_date: Time.at(response.commit_committer.date.seconds),
+          committer_name: response.commit_committer.name,
+          committer_email: response.commit_committer.email
+        }
+
+        Gitlab::Git::Commit.decorate(hash)
+      end
     end
   end
 end

lib/gitlab/git/commit.rb

@@ -19,13 +19,7 @@ module Gitlab
       def ==(other)
         return false unless other.is_a?(Gitlab::Git::Commit)
 
-        methods = [:message, :parent_ids, :authored_date, :author_name,
-                   :author_email, :committed_date, :committer_name,
-                   :committer_email]
-
-        methods.all? do |method|
-          send(method) == other.send(method)
-        end
+        id && id == other.id
       end
 
       class << self
@@ -55,6 +49,7 @@ module Gitlab
         #   Commit.find(repo, 'master')
         #
         def find(repo, commit_id = "HEAD")
+          return commit_id if commit_id.is_a?(Gitlab::Git::Commit)
           return decorate(commit_id) if commit_id.is_a?(Rugged::Commit)
 
           obj = if commit_id.is_a?(String)

lib/gitlab/git/repository.rb

@@ -78,14 +78,16 @@ module Gitlab
       end
 
       # Returns an Array of Branches
-      def branches
-        rugged.branches.map do |rugged_ref|
+      def branches(filter: nil, sort_by: nil)
+        branches = rugged.branches.each(filter).map do |rugged_ref|
           begin
             Gitlab::Git::Branch.new(self, rugged_ref.name, rugged_ref.target)
           rescue Rugged::ReferenceError
             # Omit invalid branch
           end
-        end.compact.sort_by(&:name)
+        end.compact
+
+        sort_branches(branches, sort_by)
       end
 
       def reload_rugged
@@ -106,9 +108,15 @@ module Gitlab
         Gitlab::Git::Branch.new(self, rugged_ref.name, rugged_ref.target) if rugged_ref
       end
 
-      def local_branches
-        rugged.branches.each(:local).map do |branch|
-          Gitlab::Git::Branch.new(self, branch.name, branch.target)
+      def local_branches(sort_by: nil)
+        gitaly_migrate(:local_branches) do |is_enabled|
+          if is_enabled
+            gitaly_ref_client.local_branches(sort_by: sort_by).map do |gitaly_branch|
+              Gitlab::Git::Branch.new(self, gitaly_branch.name, gitaly_branch)
+            end
+          else
+            branches(filter: :local, sort_by: sort_by)
+          end
         end
       end
 
@@ -1204,6 +1212,23 @@ module Gitlab
         diff.each_patch
       end
 
+      def sort_branches(branches, sort_by)
+        case sort_by
+        when 'name'
+          branches.sort_by(&:name)
+        when 'updated_desc'
+          branches.sort do |a, b|
+            b.dereferenced_target.committed_date <=> a.dereferenced_target.committed_date
+          end
+        when 'updated_asc'
+          branches.sort do |a, b|
+            a.dereferenced_target.committed_date <=> b.dereferenced_target.committed_date
+          end
+        else
+          branches
+        end
+      end
+
       def gitaly_ref_client
         @gitaly_ref_client ||= Gitlab::GitalyClient::Ref.new(self)
       end

lib/gitlab/gitaly_client/ref.rb

@@ -44,6 +44,12 @@ module Gitlab
         branch_names.count
       end
 
+      def local_branches(sort_by: nil)
+        request = Gitaly::FindLocalBranchesRequest.new(repository: @gitaly_repo)
+        request.sort_by = sort_by_param(sort_by) if sort_by
+        consume_branches_response(stub.find_local_branches(request))
+      end
+
       private
 
       def consume_refs_response(response, prefix:)
@@ -51,6 +57,16 @@ module Gitlab
           r.names.map { |name| name.sub(/\A#{Regexp.escape(prefix)}/, '') }
         end
       end
+
+      def sort_by_param(sort_by)
+        enum_value = Gitaly::FindLocalBranchesRequest::SortBy.resolve(sort_by.upcase.to_sym)
+        raise ArgumentError, "Invalid sort_by key `#{sort_by}`" unless enum_value
+        enum_value
+      end
+
+      def consume_branches_response(response)
+        response.flat_map { |r| r.branches }
+      end
     end
   end
 end

spec/features/protected_tags_spec.rb

 require 'spec_helper'
-Dir["./spec/features/protected_tags/*.rb"].sort.each { |f| require f }
 
 feature 'Projected Tags', feature: true, js: true do
   let(:user) { create(:user, :admin) }

spec/lib/gitlab/git/branch_spec.rb

@@ -7,6 +7,51 @@ describe Gitlab::Git::Branch, seed_helper: true do
 
   it { is_expected.to be_kind_of Array }
 
+  describe 'initialize' do
+    let(:commit_id) { 'f00' }
+    let(:commit_subject) { "My commit".force_encoding('ASCII-8BIT') }
+    let(:committer) do
+      Gitaly::FindLocalBranchCommitAuthor.new(
+        name: generate(:name),
+        email: generate(:email),
+        date: Google::Protobuf::Timestamp.new(seconds: 123)
+      )
+    end
+    let(:author) do
+      Gitaly::FindLocalBranchCommitAuthor.new(
+        name: generate(:name),
+        email: generate(:email),
+        date: Google::Protobuf::Timestamp.new(seconds: 456)
+      )
+    end
+    let(:gitaly_branch) do
+      Gitaly::FindLocalBranchResponse.new(
+        name: 'foo', commit_id: commit_id, commit_subject: commit_subject,
+        commit_author: author, commit_committer: committer
+      )
+    end
+    let(:attributes) do
+      {
+        id: commit_id,
+        message: commit_subject,
+        authored_date: Time.at(author.date.seconds),
+        author_name: author.name,
+        author_email: author.email,
+        committed_date: Time.at(committer.date.seconds),
+        committer_name: committer.name,
+        committer_email: committer.email
+      }
+    end
+    let(:branch) { described_class.new(repository, 'foo', gitaly_branch) }
+
+    it 'parses Gitaly::FindLocalBranchResponse correctly' do
+      expect(Gitlab::Git::Commit).to receive(:decorate).
+        with(hash_including(attributes)).and_call_original
+
+      expect(branch.dereferenced_target.message.encoding).to be(Encoding::UTF_8)
+    end
+  end
+
   describe '#size' do
     subject { super().size }
     it { is_expected.to eq(SeedRepo::Repo::BRANCHES.size) }

spec/lib/gitlab/git/repository_spec.rb

@@ -1080,7 +1080,9 @@ describe Gitlab::Git::Repository, seed_helper: true do
       ref = double()
       allow(ref).to receive(:name) { 'bad-branch' }
       allow(ref).to receive(:target) { raise Rugged::ReferenceError }
-      allow(repository.rugged).to receive(:branches) { [ref] }
+      branches = double()
+      allow(branches).to receive(:each) { [ref].each }
+      allow(repository.rugged).to receive(:branches) { branches }
     end
 
     it 'should return empty branches' do
@@ -1264,7 +1266,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
   describe '#local_branches' do
     before(:all) do
-      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH)
+      @repo = Gitlab::Git::Repository.new('default', File.join(TEST_MUTABLE_REPO_PATH, '.git'))
     end
 
     after(:all) do
@@ -1279,6 +1281,28 @@ describe Gitlab::Git::Repository, seed_helper: true do
       expect(@repo.local_branches.any? { |branch| branch.name == 'remote_branch' }).to eq(false)
       expect(@repo.local_branches.any? { |branch| branch.name == 'local_branch' }).to eq(true)
     end
+
+    context 'with gitaly enabled' do
+      before { stub_gitaly }
+
+      it 'gets the branches from GitalyClient' do
+        expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:local_branches).
+          and_return([])
+        @repo.local_branches
+      end
+
+      it 'wraps GRPC not found' do
+        expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:local_branches).
+          and_raise(GRPC::NotFound)
+        expect { @repo.local_branches }.to raise_error(Gitlab::Git::Repository::NoRepository)
+      end
+
+      it 'wraps GRPC exceptions' do
+        expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:local_branches).
+          and_raise(GRPC::Unknown)
+        expect { @repo.local_branches }.to raise_error(Gitlab::Git::CommandError)
+      end
+    end
   end
 
   def create_remote_branch(remote_name, branch_name, source_branch_name)

spec/lib/gitlab/gitaly_client/ref_spec.rb

@@ -38,4 +38,27 @@ describe Gitlab::GitalyClient::Ref do
       client.default_branch_name
     end
   end
+
+  describe '#local_branches' do
+    it 'sends a find_local_branches message' do
+      expect_any_instance_of(Gitaly::Ref::Stub).
+        to receive(:find_local_branches).with(gitaly_request_with_repo_path(repo_path)).
+          and_return([])
+
+      client.local_branches
+    end
+
+    it 'parses and sends the sort parameter' do
+      expect_any_instance_of(Gitaly::Ref::Stub).
+        to receive(:find_local_branches).
+          with(gitaly_request_with_params(sort_by: :UPDATED_DESC)).
+          and_return([])
+
+      client.local_branches(sort_by: 'updated_desc')
+    end
+
+    it 'raises an argument error if an invalid sort_by parameter is passed' do
+      expect { client.local_branches(sort_by: 'invalid_sort') }.to raise_error(ArgumentError)
+    end
+  end
 end

spec/support/matchers/gitaly_matchers.rb

 RSpec::Matchers.define :gitaly_request_with_repo_path do |path|
   match { |actual| actual.repository.path == path }
 end
+
+RSpec::Matchers.define :gitaly_request_with_params do |params|
+  match do |actual|
+    params.reduce(true) { |r, (key, val)| r && actual.send(key) == val }
+  end
+end

spec/support/protected_branches/access_control_ce_shared_examples.rb

+RSpec.shared_examples "protected branches > access control > CE" do
+  ProtectedBranch::PushAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)|
+    it "allows creating protected branches that #{access_type_name} can push to" do
+      visit namespace_project_protected_branches_path(project.namespace, project)
+
+      set_protected_branch_name('master')
+
+      within('.new_protected_branch') do
+        allowed_to_push_button = find(".js-allowed-to-push")
+
+        unless allowed_to_push_button.text == access_type_name
+          allowed_to_push_button.trigger('click')
+          within(".dropdown.open .dropdown-menu") { click_on access_type_name }
+        end
+      end
+
+      click_on "Protect"
+
+      expect(ProtectedBranch.count).to eq(1)
+      expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).to eq([access_type_id])
+    end
+
+    it "allows updating protected branches so that #{access_type_name} can push to them" do
+      visit namespace_project_protected_branches_path(project.namespace, project)
+
+      set_protected_branch_name('master')
+
+      click_on "Protect"
+
+      expect(ProtectedBranch.count).to eq(1)
+
+      within(".protected-branches-list") do
+        find(".js-allowed-to-push").click
+        
+        within('.js-allowed-to-push-container') do
+          expect(first("li")).to have_content("Roles")
+          click_on access_type_name
+        end
+      end
+
+      wait_for_ajax
+
+      expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).to include(access_type_id)
+    end
+  end
+
+  ProtectedBranch::MergeAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)|
+    it "allows creating protected branches that #{access_type_name} can merge to" do
+      visit namespace_project_protected_branches_path(project.namespace, project)
+
+      set_protected_branch_name('master')
+
+      within('.new_protected_branch') do
+        allowed_to_merge_button = find(".js-allowed-to-merge")
+
+        unless allowed_to_merge_button.text == access_type_name
+          allowed_to_merge_button.click
+          within(".dropdown.open .dropdown-menu") { click_on access_type_name }
+        end
+      end
+
+      click_on "Protect"
+
+      expect(ProtectedBranch.count).to eq(1)
+      expect(ProtectedBranch.last.merge_access_levels.map(&:access_level)).to eq([access_type_id])
+    end
+
+    it "allows updating protected branches so that #{access_type_name} can merge to them" do
+      visit namespace_project_protected_branches_path(project.namespace, project)
+
+      set_protected_branch_name('master')
+
+      click_on "Protect"
+
+      expect(ProtectedBranch.count).to eq(1)
+
+      within(".protected-branches-list") do
+        find(".js-allowed-to-merge").click
+
+        within('.js-allowed-to-merge-container') do
+          expect(first("li")).to have_content("Roles")
+          click_on access_type_name
+        end
+      end
+
+      wait_for_ajax
+
+      expect(ProtectedBranch.last.merge_access_levels.map(&:access_level)).to include(access_type_id)
+    end
+  end
+end

spec/support/protected_branches/access_control_ee_shared_examples.rb

+RSpec.shared_examples "protected branches > access control > EE" do
+  [['merge', ProtectedBranch::MergeAccessLevel], ['push', ProtectedBranch::PushAccessLevel]].each do |git_operation, access_level_class|
+    # Need to set a default for the `git_operation` access level that _isn't_ being tested
+    other_git_operation = git_operation == 'merge' ? 'push' : 'merge'
+    roles = git_operation == 'merge' ? access_level_class.human_access_levels : access_level_class.human_access_levels.except(0)
+
+    let(:users) { create_list(:user, 5) }
+    let(:groups) { create_list(:group, 5) }
+
+    before do
+      users.each { |user| project.team << [user, :developer] }
+      groups.each { |group| project.project_group_links.create(group: group, group_access: Gitlab::Access::DEVELOPER) }
+    end
+
+    it "allows creating protected branches that roles, users, and groups can #{git_operation} to" do
+      visit namespace_project_protected_branches_path(project.namespace, project)
+
+      set_protected_branch_name('master')
+      set_allowed_to(git_operation, users.map(&:name))
+      set_allowed_to(git_operation, groups.map(&:name))
+      set_allowed_to(git_operation, roles.values)
+      set_allowed_to(other_git_operation)
+
+      click_on "Protect"
+
+      within(".protected-branches-list") { expect(page).to have_content('master') }
+      expect(ProtectedBranch.count).to eq(1)
+      roles.each { |(access_type_id, _)| expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:access_level)).to include(access_type_id) }
+      users.each { |user| expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:user_id)).to include(user.id) }
+      groups.each { |group| expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:group_id)).to include(group.id) }
+    end
+
+    it "allows updating protected branches so that roles and users can #{git_operation} to it" do
+      visit namespace_project_protected_branches_path(project.namespace, project)
+      set_protected_branch_name('master')
+      set_allowed_to('merge')
+      set_allowed_to('push')
+
+      click_on "Protect"
+
+      set_allowed_to(git_operation, users.map(&:name), form: ".js-protected-branch-edit-form")
+      set_allowed_to(git_operation, groups.map(&:name), form: ".js-protected-branch-edit-form")
+      set_allowed_to(git_operation, roles.values, form: ".js-protected-branch-edit-form")
+
+      wait_for_ajax
+
+      expect(ProtectedBranch.count).to eq(1)
+      roles.each { |(access_type_id, _)| expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:access_level)).to include(access_type_id) }
+      users.each { |user| expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:user_id)).to include(user.id) }
+      groups.each { |group| expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:group_id)).to include(group.id) }
+    end
+
+    it "allows updating protected branches so that roles and users cannot #{git_operation} to it" do
+      visit namespace_project_protected_branches_path(project.namespace, project)
+      set_protected_branch_name('master')
+
+      users.each { |user| set_allowed_to(git_operation, user.name) }
+      roles.each { |(_, access_type_name)| set_allowed_to(git_operation, access_type_name) }
+      groups.each { |group| set_allowed_to(git_operation, group.name) }
+      set_allowed_to(other_git_operation)
+
+      click_on "Protect"
+
+      users.each { |user| set_allowed_to(git_operation, user.name, form: ".js-protected-branch-edit-form") }
+      groups.each { |group| set_allowed_to(git_operation, group.name, form: ".js-protected-branch-edit-form") }
+      roles.each { |(_, access_type_name)| set_allowed_to(git_operation, access_type_name, form: ".js-protected-branch-edit-form") }
+
+      wait_for_ajax
+
+      expect(ProtectedBranch.count).to eq(1)
+      expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym)).to be_empty
+    end
+
+    it "prepends selected users that can #{git_operation} to" do
+      users = create_list(:user, 21)
+      users.each { |user| project.team << [user, :developer] }
+
+      visit namespace_project_protected_branches_path(project.namespace, project)
+
+      # Create Protected Branch
+      set_protected_branch_name('master')
+      set_allowed_to(git_operation, roles.values)
+      set_allowed_to(other_git_operation)
+
+      click_on 'Protect'
+
+      # Update Protected Branch
+      within(".protected-branches-list") do
+        find(".js-allowed-to-#{git_operation}").click
+        find(".dropdown-input-field").set(users.last.name) # Find a user that is not loaded
+
+        expect(page).to have_selector('.dropdown-header', count: 3)
+
+        %w{Roles Groups Users}.each_with_index do |header, index|
+          expect(all('.dropdown-header')[index]).to have_content(header)
+        end
+
+        wait_for_ajax
+        click_on users.last.name
+        find(".js-allowed-to-#{git_operation}").click # close
+      end
+      wait_for_ajax
+
+      # Verify the user is appended in the dropdown
+      find(".protected-branches-list .js-allowed-to-#{git_operation}").click
+      expect(page).to have_selector '.dropdown-content .is-active', text: users.last.name
+
+      expect(ProtectedBranch.count).to eq(1)
+      roles.each { |(access_type_id, _)| expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:access_level)).to include(access_type_id) }
+      expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:user_id)).to include(users.last.id)
+    end
+  end
+
+  context 'When updating a protected branch' do
+    it 'discards other roles when choosing "No one"' do
+      roles = ProtectedBranch::PushAccessLevel.human_access_levels.except(0)
+      visit namespace_project_protected_branches_path(project.namespace, project)
+      set_protected_branch_name('fix')
+      set_allowed_to('merge')
+      set_allowed_to('push', roles.values)
+      click_on "Protect"
+      wait_for_ajax
+
+      roles.each do |(access_type_id, _)|
+        expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).to include(access_type_id)
+      end
+      expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).not_to include(0)
+
+      set_allowed_to('push', 'No one', form: '.js-protected-branch-edit-form')
+
+      wait_for_ajax
+
+      roles.each do |(access_type_id, _)|
+        expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).not_to include(access_type_id)
+      end
+      expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).to include(0)
+    end
+  end
+
+  context 'When creating a protected branch' do
+    it 'discards other roles when choosing "No one"' do
+      roles = ProtectedBranch::PushAccessLevel.human_access_levels.except(0)
+      visit namespace_project_protected_branches_path(project.namespace, project)
+      set_protected_branch_name('master')
+      set_allowed_to('merge')
+      set_allowed_to('push', ProtectedBranch::PushAccessLevel.human_access_levels.values) # Last item (No one) should deselect the other ones
+      click_on "Protect"
+      wait_for_ajax
+
+      roles.each do |(access_type_id, _)|
+        expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).not_to include(access_type_id)
+      end
+      expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).to include(0)
+    end
+  end
+end

spec/tasks/gitlab/backup_rake_spec.rb

@@ -251,6 +251,10 @@ describe 'gitlab:app namespace rake task' do
         FileUtils.rm_rf('tmp/tests/default_storage')
         FileUtils.rm_rf('tmp/tests/custom_storage')
         FileUtils.rm(@backup_tar)
+
+        # We unstub the storages to be able to reconfigure the actual Gitaly channels
+        allow(Gitlab.config.repositories).to receive(:storages).and_call_original
+        Gitlab::GitalyClient.configure_channels
       end
 
       it 'includes repositories in all repository storages' do