Merge branch 'feature/migrate-load-blame-to-gitaly' into 'master'

Migrate blame loading to Gitaly

Closes gitaly#421

See merge request !13155

Gemfile

@@ -391,7 +391,7 @@ gem 'vmstat', '~> 2.3.0'
 gem 'sys-filesystem', '~> 1.1.6'
 
 # Gitaly GRPC client
-gem 'gitaly', '~> 0.23.0'
+gem 'gitaly', '~> 0.24.0'
 
 gem 'toml-rb', '~> 0.3.15', require: false
 

Gemfile.lock

@@ -269,7 +269,7 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     gherkin-ruby (0.3.2)
-    gitaly (0.23.0)
+    gitaly (0.24.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.0)
     github-linguist (4.7.6)
@@ -974,7 +974,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.2.0)
-  gitaly (~> 0.23.0)
+  gitaly (~> 0.24.0)
   github-linguist (~> 4.7.0)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-markup (~> 1.5.1)

lib/gitlab/git/blame.rb

-# Gitaly note: JV: needs 1 RPC for #load_blame.
-
 module Gitlab
   module Git
     class Blame
@@ -26,15 +24,29 @@ module Gitlab
 
       private
 
-      # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/376
       def load_blame
-        cmd = %W(#{Gitlab.config.git.bin_path} --git-dir=#{@repo.path} blame -p #{@sha} -- #{@path})
-        # Read in binary mode to ensure ASCII-8BIT
-        raw_output = IO.popen(cmd, 'rb') {|io| io.read }
+        raw_output = @repo.gitaly_migrate(:blame) do |is_enabled|
+          if is_enabled
+            load_blame_by_gitaly
+          else
+            load_blame_by_shelling_out
+          end
+        end
+
         output = encode_utf8(raw_output)
         process_raw_blame output
       end
 
+      def load_blame_by_gitaly
+        @repo.gitaly_commit_client.raw_blame(@sha, @path)
+      end
+
+      def load_blame_by_shelling_out
+        cmd = %W(#{Gitlab.config.git.bin_path} --git-dir=#{@repo.path} blame -p #{@sha} -- #{@path})
+        # Read in binary mode to ensure ASCII-8BIT
+        IO.popen(cmd, 'rb') {|io| io.read }
+      end
+
       def process_raw_blame(output)
         lines, final = [], []
         info, commits = {}, {}

lib/gitlab/git/repository.rb

@@ -683,6 +683,14 @@ module Gitlab
         @gitaly_repository_client ||= Gitlab::GitalyClient::RepositoryService.new(self)
       end
 
+      def gitaly_migrate(method, &block)
+        Gitlab::GitalyClient.migrate(method, &block)
+      rescue GRPC::NotFound => e
+        raise NoRepository.new(e)
+      rescue GRPC::BadStatus => e
+        raise CommandError.new(e)
+      end
+
       private
 
       # Gitaly note: JV: Trying to get rid of the 'filter' option so we can implement this with 'git'.
@@ -1017,14 +1025,6 @@ module Gitlab
 
         raw_output.to_i
       end
-
-      def gitaly_migrate(method, &block)
-        Gitlab::GitalyClient.migrate(method, &block)
-      rescue GRPC::NotFound => e
-        raise NoRepository.new(e)
-      rescue GRPC::BadStatus => e
-        raise CommandError.new(e)
-      end
     end
   end
 end

lib/gitlab/gitaly_client/commit_service.rb

@@ -128,6 +128,17 @@ module Gitlab
         response.languages.map { |l| { value: l.share.round(2), label: l.name, color: l.color, highlight: l.color } }
       end
 
+      def raw_blame(revision, path)
+        request = Gitaly::RawBlameRequest.new(
+          repository: @gitaly_repo,
+          revision: revision,
+          path: path
+        )
+
+        response = GitalyClient.call(@repository.storage, :commit_service, :raw_blame, request)
+        response.reduce("") { |memo, msg| memo << msg.data }
+      end
+
       private
 
       def commit_diff_request_params(commit, options = {})

spec/lib/gitlab/git/blame_spec.rb

@@ -7,63 +7,73 @@ describe Gitlab::Git::Blame, seed_helper: true do
     Gitlab::Git::Blame.new(repository, SeedRepo::Commit::ID, "CONTRIBUTING.md")
   end
 
-  context "each count" do
-    it do
-      data = []
-      blame.each do |commit, line|
-        data << {
-          commit: commit,
-          line: line
-        }
-      end
-
-      expect(data.size).to eq(95)
-      expect(data.first[:commit]).to be_kind_of(Gitlab::Git::Commit)
-      expect(data.first[:line]).to eq("# Contribute to GitLab")
-      expect(data.first[:line]).to be_utf8
-    end
-  end
+  shared_examples 'blaming a file' do
+    context "each count" do
+      it do
+        data = []
+        blame.each do |commit, line|
+          data << {
+            commit: commit,
+            line: line
+          }
+        end
 
-  context "ISO-8859 encoding" do
-    let(:blame) do
-      Gitlab::Git::Blame.new(repository, SeedRepo::EncodingCommit::ID, "encoding/iso8859.txt")
+        expect(data.size).to eq(95)
+        expect(data.first[:commit]).to be_kind_of(Gitlab::Git::Commit)
+        expect(data.first[:line]).to eq("# Contribute to GitLab")
+        expect(data.first[:line]).to be_utf8
+      end
     end
 
-    it 'converts to UTF-8' do
-      data = []
-      blame.each do |commit, line|
-        data << {
-          commit: commit,
-          line: line
-        }
+    context "ISO-8859 encoding" do
+      let(:blame) do
+        Gitlab::Git::Blame.new(repository, SeedRepo::EncodingCommit::ID, "encoding/iso8859.txt")
       end
 
-      expect(data.size).to eq(1)
-      expect(data.first[:commit]).to be_kind_of(Gitlab::Git::Commit)
-      expect(data.first[:line]).to eq("Ä ü")
-      expect(data.first[:line]).to be_utf8
-    end
-  end
+      it 'converts to UTF-8' do
+        data = []
+        blame.each do |commit, line|
+          data << {
+            commit: commit,
+            line: line
+          }
+        end
 
-  context "unknown encoding" do
-    let(:blame) do
-      Gitlab::Git::Blame.new(repository, SeedRepo::EncodingCommit::ID, "encoding/iso8859.txt")
+        expect(data.size).to eq(1)
+        expect(data.first[:commit]).to be_kind_of(Gitlab::Git::Commit)
+        expect(data.first[:line]).to eq("Ä ü")
+        expect(data.first[:line]).to be_utf8
+      end
     end
 
-    it 'converts to UTF-8' do
-      expect(CharlockHolmes::EncodingDetector).to receive(:detect).and_return(nil)
-      data = []
-      blame.each do |commit, line|
-        data << {
+    context "unknown encoding" do
+      let(:blame) do
+        Gitlab::Git::Blame.new(repository, SeedRepo::EncodingCommit::ID, "encoding/iso8859.txt")
+      end
+
+      it 'converts to UTF-8' do
+        expect(CharlockHolmes::EncodingDetector).to receive(:detect).and_return(nil)
+        data = []
+        blame.each do |commit, line|
+          data << {
             commit: commit,
             line: line
-        }
-      end
+          }
+        end
 
-      expect(data.size).to eq(1)
-      expect(data.first[:commit]).to be_kind_of(Gitlab::Git::Commit)
-      expect(data.first[:line]).to eq(" ")
-      expect(data.first[:line]).to be_utf8
+        expect(data.size).to eq(1)
+        expect(data.first[:commit]).to be_kind_of(Gitlab::Git::Commit)
+        expect(data.first[:line]).to eq(" ")
+        expect(data.first[:line]).to be_utf8
+      end
     end
   end
+
+  context 'when Gitaly blame feature is enabled' do
+    it_behaves_like 'blaming a file'
+  end
+
+  context 'when Gitaly blame feature is disabled', skip_gitaly_mock: true do
+    it_behaves_like 'blaming a file'
+  end
 end