From e363fbf71a7874de2352740b3f33350e5ec4cf54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Tue, 25 Jul 2017 17:26:52 -0400 Subject: Move `deltas` and `diff_from_parents` logic to Gitlab::Git::Commit This helps keep the abstraction layers simpler, and also keep the interface of those methods consistent, in case of implementation changes. --- app/models/commit.rb | 14 +----- lib/gitlab/git/commit.rb | 58 +++++++++++++--------- lib/gitlab/git/commit_stats.rb | 2 +- lib/gitlab/gitaly_client/commit_service.rb | 7 ++- ...rialize_merge_request_diffs_and_commits_spec.rb | 4 +- spec/lib/gitlab/git/repository_spec.rb | 2 +- .../gitlab/gitaly_client/commit_service_spec.rb | 12 +---- 7 files changed, 46 insertions(+), 53 deletions(-) diff --git a/app/models/commit.rb b/app/models/commit.rb index 96605c9168b..638fddc5d3d 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -321,21 +321,11 @@ class Commit end def raw_diffs(*args) - if Gitlab::GitalyClient.feature_enabled?(:commit_raw_diffs) - Gitlab::GitalyClient::CommitService.new(project.repository).diff_from_parent(self, *args) - else - raw.diffs(*args) - end + raw.diffs(*args) end def raw_deltas - @deltas ||= Gitlab::GitalyClient.migrate(:commit_deltas) do |is_enabled| - if is_enabled - Gitlab::GitalyClient::CommitService.new(project.repository).commit_deltas(self) - else - raw.deltas - end - end + @deltas ||= raw.deltas end def diffs(diff_options = nil) diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb index b08d7e8fec3..c3eb3fc44f5 100644 --- a/lib/gitlab/git/commit.rb +++ b/lib/gitlab/git/commit.rb @@ -187,25 +187,6 @@ module Gitlab Gitlab::Git::Commit.new(repository, commit, ref) end - # Returns a diff object for the changes introduced by +rugged_commit+. - # If +rugged_commit+ doesn't have a parent, then the diff is between - # this commit and an empty repo. See Repository#diff for the keys - # allowed in the +options+ hash. - def diff_from_parent(rugged_commit, options = {}) - options ||= {} - break_rewrites = options[:break_rewrites] - actual_options = Gitlab::Git::Diff.filter_diff_options(options) - - diff = if rugged_commit.parents.empty? - rugged_commit.diff(actual_options.merge(reverse: true)) - else - rugged_commit.parents[0].diff(rugged_commit, actual_options) - end - - diff.find_similar!(break_rewrites: break_rewrites) - diff - end - # Returns the `Rugged` sorting type constant for one or more given # sort types. Valid keys are `:none`, `:topo`, and `:date`, or an array # containing more than one of them. `:date` uses a combination of date and @@ -270,19 +251,50 @@ module Gitlab # # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/324 def to_diff - diff_from_parent.patch + rugged_diff_from_parent.patch end # Returns a diff object for the changes from this commit's first parent. # If there is no parent, then the diff is between this commit and an - # empty repo. See Repository#diff for keys allowed in the +options+ + # empty repo. See Repository#diff for keys allowed in the +options+ # hash. def diff_from_parent(options = {}) - Commit.diff_from_parent(raw_commit, options) + Gitlab::GitalyClient.migrate(:commit_raw_diffs) do |is_enabled| + if is_enabled + @repository.gitaly_commit_client.diff_from_parent(self, options) + else + rugged_diff_from_parent(options) + end + end + end + + def rugged_diff_from_parent(options = {}) + options ||= {} + break_rewrites = options[:break_rewrites] + actual_options = Gitlab::Git::Diff.filter_diff_options(options) + + diff = if raw_commit.parents.empty? + raw_commit.diff(actual_options.merge(reverse: true)) + else + raw_commit.parents[0].diff(raw_commit, actual_options) + end + + diff.find_similar!(break_rewrites: break_rewrites) + diff end def deltas - @deltas ||= diff_from_parent.each_delta.map { |d| Gitlab::Git::Diff.new(d) } + @deltas ||= begin + deltas = Gitlab::GitalyClient.migrate(:commit_deltas) do |is_enabled| + if is_enabled + @repository.gitaly_commit_client.commit_deltas(self) + else + rugged_diff_from_parent.each_delta + end + end + + deltas.map { |delta| Gitlab::Git::Diff.new(delta) } + end end def has_zero_stats? diff --git a/lib/gitlab/git/commit_stats.rb b/lib/gitlab/git/commit_stats.rb index 57c29ad112c..00acb4763e9 100644 --- a/lib/gitlab/git/commit_stats.rb +++ b/lib/gitlab/git/commit_stats.rb @@ -16,7 +16,7 @@ module Gitlab @deletions = 0 @total = 0 - diff = commit.diff_from_parent + diff = commit.rugged_diff_from_parent diff.each_patch do |p| # TODO: Use the new Rugged convenience methods when they're released diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index 2a97a025e58..793de65595f 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -29,15 +29,14 @@ module Gitlab request = Gitaly::CommitDiffRequest.new(request_params) response = GitalyClient.call(@repository.storage, :diff_service, :commit_diff, request) - Gitlab::Git::DiffCollection.new(GitalyClient::DiffStitcher.new(response), options.merge(from_gitaly: true)) + GitalyClient::DiffStitcher.new(response) end def commit_deltas(commit) request = Gitaly::CommitDeltaRequest.new(commit_diff_request_params(commit)) response = GitalyClient.call(@repository.storage, :diff_service, :commit_delta, request) - response.flat_map do |msg| - msg.deltas.map { |d| Gitlab::Git::Diff.new(d) } - end + + response.flat_map { |msg| msg.deltas } end def tree_entry(ref, path, limit = nil) diff --git a/spec/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits_spec.rb b/spec/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits_spec.rb index 18843cbe992..f4dfa53f050 100644 --- a/spec/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits_spec.rb +++ b/spec/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits_spec.rb @@ -170,7 +170,7 @@ describe Gitlab::BackgroundMigration::DeserializeMergeRequestDiffsAndCommits do context 'when the merge request diffs are Rugged::Patch instances' do let(:commits) { merge_request_diff.commits.map(&:to_hash) } let(:first_commit) { merge_request.project.repository.commit(merge_request_diff.head_commit_sha) } - let(:diffs) { first_commit.diff_from_parent.patches } + let(:diffs) { first_commit.rugged_diff_from_parent.patches } let(:expected_diffs) { [] } include_examples 'updated MR diff' @@ -179,7 +179,7 @@ describe Gitlab::BackgroundMigration::DeserializeMergeRequestDiffsAndCommits do context 'when the merge request diffs are Rugged::Diff::Delta instances' do let(:commits) { merge_request_diff.commits.map(&:to_hash) } let(:first_commit) { merge_request.project.repository.commit(merge_request_diff.head_commit_sha) } - let(:diffs) { first_commit.diff_from_parent.deltas } + let(:diffs) { first_commit.rugged_diff_from_parent.deltas } let(:expected_diffs) { [] } include_examples 'updated MR diff' diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 23a77865aff..858616117d5 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -759,7 +759,7 @@ describe Gitlab::Git::Repository, seed_helper: true do let(:options) { { ref: 'master', path: ['PROCESS.md', 'README.md'] } } def commit_files(commit) - commit.diff_from_parent.deltas.flat_map do |delta| + commit.rugged_diff_from_parent.deltas.flat_map do |delta| [delta.old_file[:path], delta.new_file[:path]].uniq.compact end end diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb index a6c48c178b3..f4a715e43d6 100644 --- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb @@ -46,18 +46,10 @@ describe Gitlab::GitalyClient::CommitService do end end - it 'returns a Gitlab::Git::DiffCollection' do + it 'returns a Gitlab::GitalyClient::DiffStitcher' do ret = client.diff_from_parent(commit) - expect(ret).to be_kind_of(Gitlab::Git::DiffCollection) - end - - it 'passes options to Gitlab::Git::DiffCollection' do - options = { max_files: 31, max_lines: 13, from_gitaly: true } - - expect(Gitlab::Git::DiffCollection).to receive(:new).with(kind_of(Enumerable), options) - - client.diff_from_parent(commit, options) + expect(ret).to be_kind_of(Gitlab::GitalyClient::DiffStitcher) end end -- cgit v1.2.1