Incorporate Gitaly's local_branches operation into repo code

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

@@ -49,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

@@ -80,14 +80,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
@@ -108,9 +110,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
@@ -1202,6 +1210,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/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

@@ -26,6 +26,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
     context 'with gitaly enabled' do
       before { stub_gitaly }
+      after { Gitlab::GitalyClient.clear_stubs! }
       it 'gets the branch name from GitalyClient' do
         expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:default_branch_name)
@@ -120,6 +121,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
     context 'with gitaly enabled' do
       before { stub_gitaly }
+      after { Gitlab::GitalyClient.clear_stubs! }
       it 'gets the branch names from GitalyClient' do
         expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:branch_names)
@@ -157,6 +159,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
     context 'with gitaly enabled' do
       before { stub_gitaly }
+      after { Gitlab::GitalyClient.clear_stubs! }
       it 'gets the tag names from GitalyClient' do
         expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:tag_names)
@@ -1080,7 +1083,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 +1269,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 +1284,29 @@ 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 }
+      after { Gitlab::GitalyClient.clear_stubs! }
+
+      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

@@ -9,6 +9,13 @@ describe Gitlab::GitalyClient::Ref do
     allow(Gitlab.config.gitaly).to receive(:enabled).and_return(true)
   end
+  after do
+    # When we say `expect_any_instance_of(Gitaly::Ref::Stub)` a double is created,
+    # and because GitalyClient shares stubs these will get passed from example to
+    # example, which will cause an error, so we clean the stubs after each example.
+    Gitlab::GitalyClient.clear_stubs!
+  end
+
   describe '#branch_names' do
     it 'sends a find_all_branch_names message' do
       expect_any_instance_of(Gitaly::Ref::Stub).
@@ -38,4 +45,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