From 03440eed20cc36a2f9836dc078d2101849e11319 Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Fri, 28 Jul 2017 14:16:26 +0200 Subject: [PATCH] Migrate blame loading to Gitaly Closes gitaly#421 --- GITALY_SERVER_VERSION | 2 +- Gemfile | 2 +- Gemfile.lock | 4 +- lib/gitlab/git/blame.rb | 24 +++-- lib/gitlab/git/repository.rb | 16 +-- lib/gitlab/gitaly_client/commit_service.rb | 11 ++ spec/lib/gitlab/git/blame_spec.rb | 116 +++++++++++---------- 7 files changed, 104 insertions(+), 71 deletions(-) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 4e8f395fa5e..1b58cc10180 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -0.26.0 +0.27.0 diff --git a/Gemfile b/Gemfile index 403b104a9d6..34f231a04b8 100644 --- a/Gemfile +++ b/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 diff --git a/Gemfile.lock b/Gemfile.lock index 9f90965a567..a17aa3ee1ed 100644 --- a/Gemfile.lock +++ b/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) @@ -978,7 +978,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) diff --git a/lib/gitlab/git/blame.rb b/lib/gitlab/git/blame.rb index 0deaab01b5b..8dbe25e55f6 100644 --- a/lib/gitlab/git/blame.rb +++ b/lib/gitlab/git/blame.rb @@ -1,5 +1,3 @@ -# 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 = {}, {} diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index ffe2c8b91bb..1c3beb5e834 100644 --- a/lib/gitlab/git/repository.rb +++ b/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 diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index b1424a458e9..1ae13677b42 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/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 = {}) diff --git a/spec/lib/gitlab/git/blame_spec.rb b/spec/lib/gitlab/git/blame_spec.rb index 66c016d14b3..800c245b130 100644 --- a/spec/lib/gitlab/git/blame_spec.rb +++ b/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 - - context "ISO-8859 encoding" do - let(:blame) do - Gitlab::Git::Blame.new(repository, SeedRepo::EncodingCommit::ID, "encoding/iso8859.txt") - end - - it 'converts to UTF-8' do - data = [] - blame.each do |commit, line| - data << { - commit: commit, - line: line - } - 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 - - 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 << { + shared_examples 'blaming a file' do + 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 + + 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 + it 'converts to UTF-8' do + data = [] + blame.each do |commit, line| + data << { + commit: commit, + line: line + } + 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 + + 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 + + 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