diff options
author | Ahmad Sherif <me@ahmadsherif.com> | 2017-05-05 16:55:12 +0200 |
---|---|---|
committer | Ahmad Sherif <me@ahmadsherif.com> | 2017-05-08 01:29:39 +0200 |
commit | 413beaea5664ab0a9fde1686f07c8c0496229fb4 (patch) | |
tree | d05d477b7922dba6b567fbe7ab5eca9c78e12686 | |
parent | 8b9cd3c072768ca810d2b33009e35d93a05e417f (diff) | |
download | gitlab-ce-fix/support-commit-deltas-only-for-gitaly.tar.gz |
Add support for deltas_only under Gitalyfix/support-commit-deltas-only-for-gitaly
Closes gitaly#199
-rw-r--r-- | Gemfile | 2 | ||||
-rw-r--r-- | Gemfile.lock | 4 | ||||
-rw-r--r-- | app/models/commit.rb | 8 | ||||
-rw-r--r-- | lib/gitlab/git/diff.rb | 20 | ||||
-rw-r--r-- | lib/gitlab/git/diff_collection.rb | 10 | ||||
-rw-r--r-- | lib/gitlab/gitaly_client/commit.rb | 54 | ||||
-rw-r--r-- | spec/lib/gitlab/gitaly_client/commit_spec.rb | 11 | ||||
-rw-r--r-- | spec/models/commit_spec.rb | 28 |
8 files changed, 63 insertions, 74 deletions
@@ -367,6 +367,6 @@ gem 'vmstat', '~> 2.3.0' gem 'sys-filesystem', '~> 1.1.6' # Gitaly GRPC client -gem 'gitaly', '~> 0.6.0' +gem 'gitaly', '~> 0.7.0' gem 'toml-rb', '~> 0.3.15', require: false diff --git a/Gemfile.lock b/Gemfile.lock index c0c56aa9602..bc14f76b907 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -263,7 +263,7 @@ GEM po_to_json (>= 1.0.0) rails (>= 3.2.0) gherkin-ruby (0.3.2) - gitaly (0.6.0) + gitaly (0.7.0) google-protobuf (~> 3.1) grpc (~> 1.0) github-linguist (4.7.6) @@ -922,7 +922,7 @@ DEPENDENCIES gettext (~> 3.2.2) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.2.0) - gitaly (~> 0.6.0) + gitaly (~> 0.7.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 9359b323ed4..05ff8da02a7 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -326,11 +326,9 @@ class Commit end def raw_diffs(*args) - use_gitaly = Gitlab::GitalyClient.feature_enabled?(:commit_raw_diffs) - deltas_only = args.last.is_a?(Hash) && args.last[:deltas_only] - - if use_gitaly && !deltas_only - Gitlab::GitalyClient::Commit.diff_from_parent(self, *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 diff --git a/lib/gitlab/git/diff.rb b/lib/gitlab/git/diff.rb index 019be151353..31d1b66b4f7 100644 --- a/lib/gitlab/git/diff.rb +++ b/lib/gitlab/git/diff.rb @@ -183,6 +183,8 @@ module Gitlab when Gitaly::CommitDiffResponse init_from_gitaly(raw_diff) prune_diff_if_eligible(collapse) + when Gitaly::CommitDelta + init_from_gitaly(raw_diff) when nil raise "Nil as raw diff passed" else @@ -278,15 +280,15 @@ module Gitlab end end - def init_from_gitaly(diff_msg) - @diff = diff_msg.raw_chunks.join - @new_path = encode!(diff_msg.to_path.dup) - @old_path = encode!(diff_msg.from_path.dup) - @a_mode = diff_msg.old_mode.to_s(8) - @b_mode = diff_msg.new_mode.to_s(8) - @new_file = diff_msg.from_id == BLANK_SHA - @renamed_file = diff_msg.from_path != diff_msg.to_path - @deleted_file = diff_msg.to_id == BLANK_SHA + 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 end def prune_diff_if_eligible(collapse = false) diff --git a/lib/gitlab/git/diff_collection.rb b/lib/gitlab/git/diff_collection.rb index 4e45ec7c174..38d502081f5 100644 --- a/lib/gitlab/git/diff_collection.rb +++ b/lib/gitlab/git/diff_collection.rb @@ -82,10 +82,14 @@ module Gitlab end def each_delta - @iterator.each_delta.with_index do |delta, i| - diff = Gitlab::Git::Diff.new(delta) + Gitlab::GitalyClient.migrate(:commit_raw_diffs) do |is_enabled| + enum = is_enabled ? @iterator.each : @iterator.each_delta - yield @array[i] = diff + enum.with_index do |delta, i| + diff = Gitlab::Git::Diff.new(delta) + + yield @array[i] = diff + end end end diff --git a/lib/gitlab/gitaly_client/commit.rb b/lib/gitlab/gitaly_client/commit.rb index 0b001a9903d..5b1005c5f45 100644 --- a/lib/gitlab/gitaly_client/commit.rb +++ b/lib/gitlab/gitaly_client/commit.rb @@ -5,40 +5,52 @@ module Gitlab # See http://stackoverflow.com/a/40884093/1856239 and https://github.com/git/git/blob/3ad8b5bf26362ac67c9020bf8c30eee54a84f56d/cache.h#L1011-L1012 EMPTY_TREE_ID = '4b825dc642cb6eb9a060e54bf8d69288fbee4904'.freeze - attr_accessor :stub - def initialize(repository) @gitaly_repo = repository.gitaly_repository - @stub = Gitaly::Commit::Stub.new(nil, nil, channel_override: repository.gitaly_channel) + @repository = repository end def is_ancestor(ancestor_id, child_id) + stub = Gitaly::Commit::Stub.new(nil, nil, channel_override: @repository.gitaly_channel) request = Gitaly::CommitIsAncestorRequest.new( repository: @gitaly_repo, ancestor_id: ancestor_id, child_id: child_id ) - @stub.commit_is_ancestor(request).value + stub.commit_is_ancestor(request).value + end + + def diff_from_parent(commit, options = {}) + stub = Gitaly::Diff::Stub.new(nil, nil, channel_override: @repository.gitaly_channel) + parent = commit.parents[0] + parent_id = parent ? parent.id : EMPTY_TREE_ID + request_params = { + repository: @gitaly_repo, + left_commit_id: parent_id, + right_commit_id: commit.id, + paths: options.fetch(:paths, []), + } + + iterator = if options[:deltas_only] + deltas_iterator(stub, request_params) + else + request_params[:ignore_whitespace_change] = options.fetch(:ignore_whitespace_change, false) + diffs_iterator(stub, request_params) + end + + Gitlab::Git::DiffCollection.new(iterator, options) + end + + private + + def deltas_iterator(stub, request_params) + response = stub.commit_delta(Gitaly::CommitDeltaRequest.new(request_params)) + response.flat_map(&:deltas) end - class << self - def diff_from_parent(commit, options = {}) - repository = commit.project.repository - gitaly_repo = repository.gitaly_repository - stub = Gitaly::Diff::Stub.new(nil, nil, channel_override: repository.gitaly_channel) - parent = commit.parents[0] - parent_id = parent ? parent.id : EMPTY_TREE_ID - request = Gitaly::CommitDiffRequest.new( - repository: gitaly_repo, - left_commit_id: parent_id, - right_commit_id: commit.id, - ignore_whitespace_change: options.fetch(:ignore_whitespace_change, false), - paths: options.fetch(:paths, []), - ) - - Gitlab::Git::DiffCollection.new(stub.commit_diff(request), options) - end + def diffs_iterator(stub, request_params) + stub.commit_diff(Gitaly::CommitDiffRequest.new(request_params)) end end end diff --git a/spec/lib/gitlab/gitaly_client/commit_spec.rb b/spec/lib/gitlab/gitaly_client/commit_spec.rb index abe08ccdfa1..89833f5d9bc 100644 --- a/spec/lib/gitlab/gitaly_client/commit_spec.rb +++ b/spec/lib/gitlab/gitaly_client/commit_spec.rb @@ -4,7 +4,8 @@ describe Gitlab::GitalyClient::Commit do describe '.diff_from_parent' do let(:diff_stub) { double('Gitaly::Diff::Stub') } let(:project) { create(:project, :repository) } - let(:repository_message) { project.repository.gitaly_repository } + let(:repository) { project.repository } + let(:repository_message) { repository.gitaly_repository } let(:commit) { project.commit('913c66a37b4a45b9769037c55c2d238bd0942d2e') } context 'when a commit has a parent' do @@ -17,7 +18,7 @@ describe Gitlab::GitalyClient::Commit do expect_any_instance_of(Gitaly::Diff::Stub).to receive(:commit_diff).with(request) - described_class.diff_from_parent(commit) + described_class.new(repository).diff_from_parent(commit) end end @@ -32,12 +33,12 @@ describe Gitlab::GitalyClient::Commit do expect_any_instance_of(Gitaly::Diff::Stub).to receive(:commit_diff).with(request) - described_class.diff_from_parent(initial_commit) + described_class.new(repository).diff_from_parent(initial_commit) end end it 'returns a Gitlab::Git::DiffCollection' do - ret = described_class.diff_from_parent(commit) + ret = described_class.new(repository).diff_from_parent(commit) expect(ret).to be_kind_of(Gitlab::Git::DiffCollection) end @@ -47,7 +48,7 @@ describe Gitlab::GitalyClient::Commit do expect(Gitlab::Git::DiffCollection).to receive(:new).with(kind_of(Enumerable), options) - described_class.diff_from_parent(commit, options) + described_class.new(repository).diff_from_parent(commit, options) end end end diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index 852889d4540..72f83d63224 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -388,32 +388,4 @@ eos expect(described_class.valid_hash?('a' * 41)).to be false end end - - describe '#raw_diffs' do - context 'Gitaly commit_raw_diffs feature enabled' do - before do - allow(Gitlab::GitalyClient).to receive(:feature_enabled?).with(:commit_raw_diffs).and_return(true) - end - - context 'when a truthy deltas_only is not passed to args' do - it 'fetches diffs from Gitaly server' do - expect(Gitlab::GitalyClient::Commit).to receive(:diff_from_parent). - with(commit) - - commit.raw_diffs - end - end - - context 'when a truthy deltas_only is passed to args' do - it 'fetches diffs using Rugged' do - opts = { deltas_only: true } - - expect(Gitlab::GitalyClient::Commit).not_to receive(:diff_from_parent) - expect(commit.raw).to receive(:diffs).with(opts) - - commit.raw_diffs(opts) - end - end - end - end end |