Merge branch 'fix/use-new-gitaly-commit-diff-rpc' into 'master'

Use the new Gitaly CommitDiff RPC

See merge request !11826

Gemfile

@@ -368,7 +368,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
 

Gemfile.lock

@@ -272,7 +272,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)
@@ -933,7 +933,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)

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
+    # 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
   end
 
   def raw_deltas

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

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)

lib/gitlab/gitaly_client/diff.rb

+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

lib/gitlab/gitaly_client/diff_stitcher.rb

+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

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

spec/lib/gitlab/gitaly_client/diff_spec.rb

+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

spec/lib/gitlab/gitaly_client/diff_stitcher_spec.rb

+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