From 4d1e987ec3263feda7a2f3469e31f5839e25731b Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Tue, 30 May 2017 21:30:05 +0200 Subject: [PATCH] Use the new Gitaly CommitDiff RPC --- Gemfile | 2 +- Gemfile.lock | 4 +- app/models/commit.rb | 11 ++-- lib/gitlab/git/diff.rb | 20 +++---- lib/gitlab/gitaly_client/commit.rb | 2 +- lib/gitlab/gitaly_client/diff.rb | 21 +++++++ lib/gitlab/gitaly_client/diff_stitcher.rb | 31 ++++++++++ spec/lib/gitlab/git/diff_spec.rb | 10 ++-- spec/lib/gitlab/gitaly_client/diff_spec.rb | 30 ++++++++++ .../gitaly_client/diff_stitcher_spec.rb | 59 +++++++++++++++++++ 10 files changed, 166 insertions(+), 24 deletions(-) create mode 100644 lib/gitlab/gitaly_client/diff.rb create mode 100644 lib/gitlab/gitaly_client/diff_stitcher.rb create mode 100644 spec/lib/gitlab/gitaly_client/diff_spec.rb create mode 100644 spec/lib/gitlab/gitaly_client/diff_stitcher_spec.rb diff --git a/Gemfile b/Gemfile index dce2e4ba94e..b8911c46bdc 100644 --- a/Gemfile +++ b/Gemfile @@ -367,7 +367,7 @@ gem 'vmstat', '~> 2.3.0' gem 'sys-filesystem', '~> 1.1.6' # Gitaly GRPC client -gem 'gitaly', '~> 0.7.0' +gem 'gitaly', '~> 0.8.0' gem 'toml-rb', '~> 0.3.15', require: false diff --git a/Gemfile.lock b/Gemfile.lock index f0728a358fa..4b2fb1b4732 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -267,7 +267,7 @@ GEM po_to_json (>= 1.0.0) rails (>= 3.2.0) gherkin-ruby (0.3.2) - gitaly (0.7.0) + gitaly (0.8.0) google-protobuf (~> 3.1) grpc (~> 1.0) github-linguist (4.7.6) @@ -927,7 +927,7 @@ DEPENDENCIES gettext (~> 3.2.2) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.2.0) - gitaly (~> 0.7.0) + gitaly (~> 0.8.0) github-linguist (~> 4.7.0) gitlab-flowdock-git-hook (~> 1.0.1) gitlab-markup (~> 1.5.1) diff --git a/app/models/commit.rb b/app/models/commit.rb index dbc0a22829e..c1c1f282db1 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -326,11 +326,12 @@ class Commit end def raw_diffs(*args) - if Gitlab::GitalyClient.feature_enabled?(:commit_raw_diffs) - Gitlab::GitalyClient::Commit.new(project.repository).diff_from_parent(self, *args) - else - raw.diffs(*args) - end + # Uncomment when https://gitlab.com/gitlab-org/gitaly/merge_requests/170 is merged + # if Gitlab::GitalyClient.feature_enabled?(:commit_raw_diffs) + # Gitlab::GitalyClient::Commit.new(project.repository).diff_from_parent(self, *args) + # else + raw.diffs(*args) + # end end def raw_deltas diff --git a/lib/gitlab/git/diff.rb b/lib/gitlab/git/diff.rb index ccccca96595..636cac3259d 100644 --- a/lib/gitlab/git/diff.rb +++ b/lib/gitlab/git/diff.rb @@ -189,7 +189,7 @@ module Gitlab prune_diff_if_eligible when Rugged::Patch, Rugged::Diff::Delta init_from_rugged(raw_diff) - when Gitaly::CommitDiffResponse + when Gitlab::GitalyClient::Diff init_from_gitaly(raw_diff) prune_diff_if_eligible when Gitaly::CommitDelta @@ -290,15 +290,15 @@ module Gitlab end end - def init_from_gitaly(msg) - @diff = msg.raw_chunks.join if msg.respond_to?(:raw_chunks) - @new_path = encode!(msg.to_path.dup) - @old_path = encode!(msg.from_path.dup) - @a_mode = msg.old_mode.to_s(8) - @b_mode = msg.new_mode.to_s(8) - @new_file = msg.from_id == BLANK_SHA - @renamed_file = msg.from_path != msg.to_path - @deleted_file = msg.to_id == BLANK_SHA + def init_from_gitaly(diff) + @diff = diff.patch if diff.respond_to?(:patch) + @new_path = encode!(diff.to_path.dup) + @old_path = encode!(diff.from_path.dup) + @a_mode = diff.old_mode.to_s(8) + @b_mode = diff.new_mode.to_s(8) + @new_file = diff.from_id == BLANK_SHA + @renamed_file = diff.from_path != diff.to_path + @deleted_file = diff.to_id == BLANK_SHA end def prune_diff_if_eligible diff --git a/lib/gitlab/gitaly_client/commit.rb b/lib/gitlab/gitaly_client/commit.rb index 4491903d788..ba3da781dad 100644 --- a/lib/gitlab/gitaly_client/commit.rb +++ b/lib/gitlab/gitaly_client/commit.rb @@ -26,7 +26,7 @@ module Gitlab request_params[:ignore_whitespace_change] = options.fetch(:ignore_whitespace_change, false) response = diff_service_stub.commit_diff(Gitaly::CommitDiffRequest.new(request_params)) - Gitlab::Git::DiffCollection.new(response, options) + Gitlab::Git::DiffCollection.new(GitalyClient::DiffStitcher.new(response), options) end def commit_deltas(commit) diff --git a/lib/gitlab/gitaly_client/diff.rb b/lib/gitlab/gitaly_client/diff.rb new file mode 100644 index 00000000000..1e117b7e74a --- /dev/null +++ b/lib/gitlab/gitaly_client/diff.rb @@ -0,0 +1,21 @@ +module Gitlab + module GitalyClient + class Diff + FIELDS = %i(from_path to_path old_mode new_mode from_id to_id patch).freeze + + attr_accessor(*FIELDS) + + def initialize(params) + params.each do |key, val| + public_send(:"#{key}=", val) + end + end + + def ==(other) + FIELDS.all? do |field| + public_send(field) == other.public_send(field) + end + end + end + end +end diff --git a/lib/gitlab/gitaly_client/diff_stitcher.rb b/lib/gitlab/gitaly_client/diff_stitcher.rb new file mode 100644 index 00000000000..d84e8d752dc --- /dev/null +++ b/lib/gitlab/gitaly_client/diff_stitcher.rb @@ -0,0 +1,31 @@ +module Gitlab + module GitalyClient + class DiffStitcher + include Enumerable + + def initialize(rpc_response) + @rpc_response = rpc_response + end + + def each + current_diff = nil + + @rpc_response.each do |diff_msg| + if current_diff.nil? + diff_params = diff_msg.to_h.slice(*GitalyClient::Diff::FIELDS) + diff_params[:patch] = diff_msg.raw_patch_data + + current_diff = GitalyClient::Diff.new(diff_params) + else + current_diff.patch += diff_msg.raw_patch_data + end + + if diff_msg.end_of_patch + yield current_diff + current_diff = nil + end + end + end + end + end +end diff --git a/spec/lib/gitlab/git/diff_spec.rb b/spec/lib/gitlab/git/diff_spec.rb index 8e24168ad71..9c2e8a298c6 100644 --- a/spec/lib/gitlab/git/diff_spec.rb +++ b/spec/lib/gitlab/git/diff_spec.rb @@ -110,23 +110,23 @@ EOT end end - context 'using a Gitaly::CommitDiffResponse' do + context 'using a GitalyClient::Diff' do let(:diff) do described_class.new( - Gitaly::CommitDiffResponse.new( + Gitlab::GitalyClient::Diff.new( to_path: ".gitmodules", from_path: ".gitmodules", old_mode: 0100644, new_mode: 0100644, from_id: '357406f3075a57708d0163752905cc1576fceacc', to_id: '8e5177d718c561d36efde08bad36b43687ee6bf0', - raw_chunks: raw_chunks + patch: raw_patch ) ) end context 'with a small diff' do - let(:raw_chunks) { [@raw_diff_hash[:diff]] } + let(:raw_patch) { @raw_diff_hash[:diff] } it 'initializes the diff' do expect(diff.to_hash).to eq(@raw_diff_hash) @@ -138,7 +138,7 @@ EOT end context 'using a diff that is too large' do - let(:raw_chunks) { ['a' * 204800] } + let(:raw_patch) { 'a' * 204800 } it 'prunes the diff' do expect(diff.diff).to be_empty diff --git a/spec/lib/gitlab/gitaly_client/diff_spec.rb b/spec/lib/gitlab/gitaly_client/diff_spec.rb new file mode 100644 index 00000000000..2960c9a79ad --- /dev/null +++ b/spec/lib/gitlab/gitaly_client/diff_spec.rb @@ -0,0 +1,30 @@ +require 'spec_helper' + +describe Gitlab::GitalyClient::Diff, lib: true do + let(:diff_fields) do + { + to_path: ".gitmodules", + from_path: ".gitmodules", + old_mode: 0100644, + new_mode: 0100644, + from_id: '357406f3075a57708d0163752905cc1576fceacc', + to_id: '8e5177d718c561d36efde08bad36b43687ee6bf0', + patch: 'a' * 100 + } + end + + subject { described_class.new(diff_fields) } + + it { is_expected.to respond_to(:from_path) } + it { is_expected.to respond_to(:to_path) } + it { is_expected.to respond_to(:old_mode) } + it { is_expected.to respond_to(:new_mode) } + it { is_expected.to respond_to(:from_id) } + it { is_expected.to respond_to(:to_id) } + it { is_expected.to respond_to(:patch) } + + describe '#==' do + it { expect(subject).to eq(described_class.new(diff_fields)) } + it { expect(subject).not_to eq(described_class.new(diff_fields.merge(patch: 'a'))) } + end +end diff --git a/spec/lib/gitlab/gitaly_client/diff_stitcher_spec.rb b/spec/lib/gitlab/gitaly_client/diff_stitcher_spec.rb new file mode 100644 index 00000000000..07650013052 --- /dev/null +++ b/spec/lib/gitlab/gitaly_client/diff_stitcher_spec.rb @@ -0,0 +1,59 @@ +require 'spec_helper' + +describe Gitlab::GitalyClient::DiffStitcher, lib: true do + describe 'enumeration' do + it 'combines segregated diff messages together' do + diff_1 = OpenStruct.new( + to_path: ".gitmodules", + from_path: ".gitmodules", + old_mode: 0100644, + new_mode: 0100644, + from_id: '357406f3075a57708d0163752905cc1576fceacc', + to_id: '8e5177d718c561d36efde08bad36b43687ee6bf0', + patch: 'a' * 100 + ) + diff_2 = OpenStruct.new( + to_path: ".gitignore", + from_path: ".gitignore", + old_mode: 0100644, + new_mode: 0100644, + from_id: '357406f3075a57708d0163752905cc1576fceacc', + to_id: '8e5177d718c561d36efde08bad36b43687ee6bf0', + patch: 'a' * 200 + ) + diff_3 = OpenStruct.new( + to_path: "README", + from_path: "README", + old_mode: 0100644, + new_mode: 0100644, + from_id: '357406f3075a57708d0163752905cc1576fceacc', + to_id: '8e5177d718c561d36efde08bad36b43687ee6bf0', + patch: 'a' * 100 + ) + + msg_1 = OpenStruct.new(diff_1.to_h.except(:patch)) + msg_1.raw_patch_data = diff_1.patch + msg_1.end_of_patch = true + + msg_2 = OpenStruct.new(diff_2.to_h.except(:patch)) + msg_2.raw_patch_data = diff_2.patch[0..100] + msg_2.end_of_patch = false + + msg_3 = OpenStruct.new(raw_patch_data: diff_2.patch[101..-1], end_of_patch: true) + + msg_4 = OpenStruct.new(diff_3.to_h.except(:patch)) + msg_4.raw_patch_data = diff_3.patch + msg_4.end_of_patch = true + + diff_msgs = [msg_1, msg_2, msg_3, msg_4] + + expected_diffs = [ + Gitlab::GitalyClient::Diff.new(diff_1.to_h), + Gitlab::GitalyClient::Diff.new(diff_2.to_h), + Gitlab::GitalyClient::Diff.new(diff_3.to_h) + ] + + expect(described_class.new(diff_msgs).to_a).to eq(expected_diffs) + end + end +end