From c21ae07e331ca14605410555d0582f14cb661bb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Tue, 25 Jul 2017 16:48:17 -0400 Subject: Refactor Gitlab::Git::Commit to include a repository --- app/models/commit.rb | 3 +- app/models/merge_request_diff.rb | 4 +- app/models/repository.rb | 2 + lib/gitlab/cycle_analytics/plan_event_fetcher.rb | 2 +- lib/gitlab/git/blame.rb | 2 +- lib/gitlab/git/commit.rb | 26 +++++------ lib/gitlab/git/diff_collection.rb | 3 +- lib/gitlab/git/repository.rb | 2 +- lib/gitlab/gitaly_client/commit.rb | 14 ------ lib/gitlab/gitaly_client/commit_service.rb | 8 ++-- lib/gitlab/gitaly_client/ref_service.rb | 8 ++-- spec/lib/gitlab/git/commit_spec.rb | 52 +++++++++++----------- spec/lib/gitlab/git/repository_spec.rb | 19 +++++--- .../gitlab/gitaly_client/commit_service_spec.rb | 2 +- spec/models/commit_spec.rb | 2 - 15 files changed, 64 insertions(+), 85 deletions(-) delete mode 100644 lib/gitlab/gitaly_client/commit.rb diff --git a/app/models/commit.rb b/app/models/commit.rb index 7940733f557..96605c9168b 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -55,7 +55,8 @@ class Commit end def from_hash(hash, project) - new(Gitlab::Git::Commit.new(hash), project) + raw_commit = Gitlab::Git::Commit.new(project.repository.raw, hash) + new(raw_commit, project) end def valid_hash?(key) diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index d9d746ccf41..58050e1f438 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -282,9 +282,7 @@ class MergeRequestDiff < ActiveRecord::Base def load_commits commits = st_commits.presence || merge_request_diff_commits - commits.map do |commit| - Commit.new(Gitlab::Git::Commit.new(commit.to_hash), merge_request.source_project) - end + commits.map { |commit| Commit.from_hash(commit.to_hash, project) } end def save_diffs diff --git a/app/models/repository.rb b/app/models/repository.rb index f86a0869b01..3045db06af1 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -64,6 +64,8 @@ class Repository @raw_repository ||= initialize_raw_repository end + alias_method :raw, :raw_repository + # Return absolute path to repository def path_to_repo @path_to_repo ||= File.expand_path( diff --git a/lib/gitlab/cycle_analytics/plan_event_fetcher.rb b/lib/gitlab/cycle_analytics/plan_event_fetcher.rb index b260822788d..2479b4a7706 100644 --- a/lib/gitlab/cycle_analytics/plan_event_fetcher.rb +++ b/lib/gitlab/cycle_analytics/plan_event_fetcher.rb @@ -54,7 +54,7 @@ module Gitlab end def serialize_commit(event, commit, query) - commit = Commit.new(Gitlab::Git::Commit.new(commit.to_hash), @project) + commit = Commit.from_hash(commit.to_hash, @project) AnalyticsCommitSerializer.new(project: @project, total_time: event['total_time']).represent(commit) end diff --git a/lib/gitlab/git/blame.rb b/lib/gitlab/git/blame.rb index 8dbe25e55f6..31effdba292 100644 --- a/lib/gitlab/git/blame.rb +++ b/lib/gitlab/git/blame.rb @@ -16,7 +16,7 @@ module Gitlab def each @blames.each do |blame| yield( - Gitlab::Git::Commit.new(blame.commit), + Gitlab::Git::Commit.new(@repo, blame.commit), blame.line ) end diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb index 600d886e818..b08d7e8fec3 100644 --- a/lib/gitlab/git/commit.rb +++ b/lib/gitlab/git/commit.rb @@ -51,7 +51,7 @@ module Gitlab # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/321 def find(repo, commit_id = "HEAD") return commit_id if commit_id.is_a?(Gitlab::Git::Commit) - return decorate(commit_id) if commit_id.is_a?(Rugged::Commit) + return decorate(repo, commit_id) if commit_id.is_a?(Rugged::Commit) obj = if commit_id.is_a?(String) repo.rev_parse_target(commit_id) @@ -61,7 +61,7 @@ module Gitlab return nil unless obj.is_a?(Rugged::Commit) - decorate(obj) + decorate(repo, obj) rescue Rugged::ReferenceError, Rugged::InvalidError, Rugged::ObjectError, Gitlab::Git::Repository::NoRepository nil end @@ -102,7 +102,7 @@ module Gitlab if is_enabled repo.gitaly_commit_client.between(base, head) else - repo.rugged_commits_between(base, head).map { |c| decorate(c) } + repo.rugged_commits_between(base, head).map { |c| decorate(repo, c) } end end rescue Rugged::ReferenceError @@ -169,7 +169,7 @@ module Gitlab offset = actual_options[:skip] limit = actual_options[:max_count] walker.each(offset: offset, limit: limit) do |commit| - commits.push(decorate(commit)) + commits.push(decorate(repo, commit)) end walker.reset @@ -183,8 +183,8 @@ module Gitlab Gitlab::GitalyClient::CommitService.new(repo).find_all_commits(options) end - def decorate(commit, ref = nil) - Gitlab::Git::Commit.new(commit, ref) + def decorate(repository, commit, ref = nil) + Gitlab::Git::Commit.new(repository, commit, ref) end # Returns a diff object for the changes introduced by +rugged_commit+. @@ -221,7 +221,7 @@ module Gitlab end end - def initialize(raw_commit, head = nil) + def initialize(repository, raw_commit, head = nil) raise "Nil as raw commit passed" unless raw_commit case raw_commit @@ -229,12 +229,13 @@ module Gitlab init_from_hash(raw_commit) when Rugged::Commit init_from_rugged(raw_commit) - when Gitlab::GitalyClient::Commit + when Gitaly::GitCommit init_from_gitaly(raw_commit) else raise "Invalid raw commit type: #{raw_commit.class}" end + @repository = repository @head = head end @@ -309,14 +310,7 @@ module Gitlab end def parents - case raw_commit - when Rugged::Commit - raw_commit.parents.map { |c| Gitlab::Git::Commit.new(c) } - when Gitlab::GitalyClient::Commit - parent_ids.map { |oid| self.class.find(raw_commit.repository, oid) }.compact - else - raise NotImplementedError, "commit source doesn't support #parents" - end + parent_ids.map { |oid| self.class.find(@repository, oid) }.compact end # Get the gpg signature of this commit. diff --git a/lib/gitlab/git/diff_collection.rb b/lib/gitlab/git/diff_collection.rb index 87ed9c3ea26..6a601561c2a 100644 --- a/lib/gitlab/git/diff_collection.rb +++ b/lib/gitlab/git/diff_collection.rb @@ -28,7 +28,6 @@ module Gitlab @limits = self.class.collection_limits(options) @enforce_limits = !!options.fetch(:limits, true) @expanded = !!options.fetch(:expanded, true) - @from_gitaly = options.fetch(:from_gitaly, false) @line_count = 0 @byte_count = 0 @@ -44,7 +43,7 @@ module Gitlab return if @iterator.nil? Gitlab::GitalyClient.migrate(:commit_raw_diffs) do |is_enabled| - if is_enabled && @from_gitaly + if is_enabled && @iterator.is_a?(Gitlab::GitalyClient::DiffStitcher) each_gitaly_patch(&block) else each_rugged_patch(&block) diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index f6f9d49bf37..4162526be2b 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -314,7 +314,7 @@ module Gitlab options[:limit] ||= 0 options[:offset] ||= 0 - raw_log(options).map { |c| Commit.decorate(c) } + raw_log(options).map { |c| Commit.decorate(self, c) } end def count_commits(options) diff --git a/lib/gitlab/gitaly_client/commit.rb b/lib/gitlab/gitaly_client/commit.rb deleted file mode 100644 index 61fe462d762..00000000000 --- a/lib/gitlab/gitaly_client/commit.rb +++ /dev/null @@ -1,14 +0,0 @@ -module Gitlab - module GitalyClient - class Commit - attr_reader :repository, :gitaly_commit - - delegate :id, :subject, :body, :author, :committer, :parent_ids, to: :gitaly_commit - - def initialize(repository, gitaly_commit) - @repository = repository - @gitaly_commit = gitaly_commit - end - end - end -end diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index ac6817e6d0e..2a97a025e58 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -107,8 +107,7 @@ module Gitlab gitaly_commit = GitalyClient.call(@repository.storage, :commit_service, :last_commit_for_path, request).commit return unless gitaly_commit - commit = GitalyClient::Commit.new(@repository, gitaly_commit) - Gitlab::Git::Commit.new(commit) + Gitlab::Git::Commit.new(@repository, gitaly_commit) end def between(from, to) @@ -156,7 +155,7 @@ module Gitlab private def commit_diff_request_params(commit, options = {}) - parent_id = commit.parents[0]&.id || EMPTY_TREE_ID + parent_id = commit.parent_ids.first || EMPTY_TREE_ID { repository: @gitaly_repo, @@ -169,8 +168,7 @@ module Gitlab def consume_commits_response(response) response.flat_map do |message| message.commits.map do |gitaly_commit| - commit = GitalyClient::Commit.new(@repository, gitaly_commit) - Gitlab::Git::Commit.new(commit) + Gitlab::Git::Commit.new(@repository, gitaly_commit) end end end diff --git a/lib/gitlab/gitaly_client/ref_service.rb b/lib/gitlab/gitaly_client/ref_service.rb index b0f7548b7dc..919fb68b8c7 100644 --- a/lib/gitlab/gitaly_client/ref_service.rb +++ b/lib/gitlab/gitaly_client/ref_service.rb @@ -16,8 +16,7 @@ module Gitlab response.flat_map do |message| message.branches.map do |branch| - gitaly_commit = GitalyClient::Commit.new(@repository, branch.target) - target_commit = Gitlab::Git::Commit.decorate(gitaly_commit) + target_commit = Gitlab::Git::Commit.decorate(@repository, branch.target) Gitlab::Git::Branch.new(@repository, branch.name, branch.target.id, target_commit) end end @@ -102,8 +101,7 @@ module Gitlab response.flat_map do |message| message.tags.map do |gitaly_tag| if gitaly_tag.target_commit.present? - commit = GitalyClient::Commit.new(@repository, gitaly_tag.target_commit) - gitaly_commit = Gitlab::Git::Commit.new(commit) + gitaly_commit = Gitlab::Git::Commit.decorate(@repository, gitaly_tag.target_commit) end Gitlab::Git::Tag.new( @@ -141,7 +139,7 @@ module Gitlab committer_email: response.commit_committer.email.dup } - Gitlab::Git::Commit.decorate(hash) + Gitlab::Git::Commit.decorate(@repository, hash) end end end diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb index 26d7a364f5b..4a7e5f87ff9 100644 --- a/spec/lib/gitlab/git/commit_spec.rb +++ b/spec/lib/gitlab/git/commit_spec.rb @@ -2,7 +2,7 @@ require "spec_helper" describe Gitlab::Git::Commit, seed_helper: true do let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH) } - let(:commit) { Gitlab::Git::Commit.find(repository, SeedRepo::Commit::ID) } + let(:commit) { described_class.find(repository, SeedRepo::Commit::ID) } let(:rugged_commit) do repository.rugged.lookup(SeedRepo::Commit::ID) end @@ -24,7 +24,7 @@ describe Gitlab::Git::Commit, seed_helper: true do } @parents = [repo.head.target] - @gitlab_parents = @parents.map { |c| Gitlab::Git::Commit.decorate(c) } + @gitlab_parents = @parents.map { |c| described_class.decorate(repository, c) } @tree = @parents.first.tree sha = Rugged::Commit.create( @@ -38,7 +38,7 @@ describe Gitlab::Git::Commit, seed_helper: true do ) @raw_commit = repo.lookup(sha) - @commit = Gitlab::Git::Commit.new(@raw_commit) + @commit = described_class.new(repository, @raw_commit) end it { expect(@commit.short_id).to eq(@raw_commit.oid[0..10]) } @@ -91,7 +91,7 @@ describe Gitlab::Git::Commit, seed_helper: true do committer: committer ) end - let(:commit) { described_class.new(Gitlab::GitalyClient::Commit.new(repository, gitaly_commit)) } + let(:commit) { described_class.new(repository, gitaly_commit) } it { expect(commit.short_id).to eq(id[0..10]) } it { expect(commit.id).to eq(id) } @@ -113,45 +113,45 @@ describe Gitlab::Git::Commit, seed_helper: true do context 'Class methods' do describe '.find' do it "should return first head commit if without params" do - expect(Gitlab::Git::Commit.last(repository).id).to eq( + expect(described_class.last(repository).id).to eq( repository.rugged.head.target.oid ) end it "should return valid commit" do - expect(Gitlab::Git::Commit.find(repository, SeedRepo::Commit::ID)).to be_valid_commit + expect(described_class.find(repository, SeedRepo::Commit::ID)).to be_valid_commit end it "should return valid commit for tag" do - expect(Gitlab::Git::Commit.find(repository, 'v1.0.0').id).to eq('6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') + expect(described_class.find(repository, 'v1.0.0').id).to eq('6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') end it "should return nil for non-commit ids" do blob = Gitlab::Git::Blob.find(repository, SeedRepo::Commit::ID, "files/ruby/popen.rb") - expect(Gitlab::Git::Commit.find(repository, blob.id)).to be_nil + expect(described_class.find(repository, blob.id)).to be_nil end it "should return nil for parent of non-commit object" do blob = Gitlab::Git::Blob.find(repository, SeedRepo::Commit::ID, "files/ruby/popen.rb") - expect(Gitlab::Git::Commit.find(repository, "#{blob.id}^")).to be_nil + expect(described_class.find(repository, "#{blob.id}^")).to be_nil end it "should return nil for nonexisting ids" do - expect(Gitlab::Git::Commit.find(repository, "+123_4532530XYZ")).to be_nil + expect(described_class.find(repository, "+123_4532530XYZ")).to be_nil end context 'with broken repo' do let(:repository) { Gitlab::Git::Repository.new('default', TEST_BROKEN_REPO_PATH) } it 'returns nil' do - expect(Gitlab::Git::Commit.find(repository, SeedRepo::Commit::ID)).to be_nil + expect(described_class.find(repository, SeedRepo::Commit::ID)).to be_nil end end end describe '.last_for_path' do context 'no path' do - subject { Gitlab::Git::Commit.last_for_path(repository, 'master') } + subject { described_class.last_for_path(repository, 'master') } describe '#id' do subject { super().id } @@ -160,7 +160,7 @@ describe Gitlab::Git::Commit, seed_helper: true do end context 'path' do - subject { Gitlab::Git::Commit.last_for_path(repository, 'master', 'files/ruby') } + subject { described_class.last_for_path(repository, 'master', 'files/ruby') } describe '#id' do subject { super().id } @@ -169,7 +169,7 @@ describe Gitlab::Git::Commit, seed_helper: true do end context 'ref + path' do - subject { Gitlab::Git::Commit.last_for_path(repository, SeedRepo::Commit::ID, 'encoding') } + subject { described_class.last_for_path(repository, SeedRepo::Commit::ID, 'encoding') } describe '#id' do subject { super().id } @@ -181,7 +181,7 @@ describe Gitlab::Git::Commit, seed_helper: true do describe '.where' do context 'path is empty string' do subject do - commits = Gitlab::Git::Commit.where( + commits = described_class.where( repo: repository, ref: 'master', path: '', @@ -199,7 +199,7 @@ describe Gitlab::Git::Commit, seed_helper: true do context 'path is nil' do subject do - commits = Gitlab::Git::Commit.where( + commits = described_class.where( repo: repository, ref: 'master', path: nil, @@ -217,7 +217,7 @@ describe Gitlab::Git::Commit, seed_helper: true do context 'ref is branch name' do subject do - commits = Gitlab::Git::Commit.where( + commits = described_class.where( repo: repository, ref: 'master', path: 'files', @@ -237,7 +237,7 @@ describe Gitlab::Git::Commit, seed_helper: true do context 'ref is commit id' do subject do - commits = Gitlab::Git::Commit.where( + commits = described_class.where( repo: repository, ref: "874797c3a73b60d2187ed6e2fcabd289ff75171e", path: 'files', @@ -257,7 +257,7 @@ describe Gitlab::Git::Commit, seed_helper: true do context 'ref is tag' do subject do - commits = Gitlab::Git::Commit.where( + commits = described_class.where( repo: repository, ref: 'v1.0.0', path: 'files', @@ -278,7 +278,7 @@ describe Gitlab::Git::Commit, seed_helper: true do describe '.between' do subject do - commits = Gitlab::Git::Commit.between(repository, SeedRepo::Commit::PARENT_ID, SeedRepo::Commit::ID) + commits = described_class.between(repository, SeedRepo::Commit::PARENT_ID, SeedRepo::Commit::ID) commits.map { |c| c.id } end @@ -294,12 +294,12 @@ describe Gitlab::Git::Commit, seed_helper: true do it 'should return a return a collection of commits' do commits = described_class.find_all(repository) - expect(commits).to all( be_a_kind_of(Gitlab::Git::Commit) ) + expect(commits).to all( be_a_kind_of(described_class) ) end context 'max_count' do subject do - commits = Gitlab::Git::Commit.find_all( + commits = described_class.find_all( repository, max_count: 50 ) @@ -322,7 +322,7 @@ describe Gitlab::Git::Commit, seed_helper: true do context 'ref + max_count + skip' do subject do - commits = Gitlab::Git::Commit.find_all( + commits = described_class.find_all( repository, ref: 'master', max_count: 50, @@ -374,7 +374,7 @@ describe Gitlab::Git::Commit, seed_helper: true do end describe '#init_from_rugged' do - let(:gitlab_commit) { Gitlab::Git::Commit.new(rugged_commit) } + let(:gitlab_commit) { described_class.new(repository, rugged_commit) } subject { gitlab_commit } describe '#id' do @@ -384,7 +384,7 @@ describe Gitlab::Git::Commit, seed_helper: true do end describe '#init_from_hash' do - let(:commit) { Gitlab::Git::Commit.new(sample_commit_hash) } + let(:commit) { described_class.new(repository, sample_commit_hash) } subject { commit } describe '#id' do @@ -451,7 +451,7 @@ describe Gitlab::Git::Commit, seed_helper: true do end describe '#ref_names' do - let(:commit) { Gitlab::Git::Commit.find(repository, 'master') } + let(:commit) { described_class.find(repository, 'master') } subject { commit.ref_names(repository) } it 'has 1 element' do diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 20d6b58d6d1..23a77865aff 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -505,17 +505,22 @@ describe Gitlab::Git::Repository, seed_helper: true do end describe "#log" do - commit_with_old_name = nil - commit_with_new_name = nil - rename_commit = nil + let(:commit_with_old_name) do + Gitlab::Git::Commit.decorate(repository, @commit_with_old_name_id) + end + let(:commit_with_new_name) do + Gitlab::Git::Commit.decorate(repository, @commit_with_new_name_id) + end + let(:rename_commit) do + Gitlab::Git::Commit.decorate(repository, @rename_commit_id) + end before(:context) do # Add new commits so that there's a renamed file in the commit history repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH).rugged - - commit_with_old_name = Gitlab::Git::Commit.decorate(new_commit_edit_old_file(repo)) - rename_commit = Gitlab::Git::Commit.decorate(new_commit_move_file(repo)) - commit_with_new_name = Gitlab::Git::Commit.decorate(new_commit_edit_new_file(repo)) + @commit_with_old_name_id = new_commit_edit_old_file(repo) + @rename_commit_id = new_commit_move_file(repo) + @commit_with_new_name_id = new_commit_edit_new_file(repo) end after(:context) do diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb index d71e0f84c65..a6c48c178b3 100644 --- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb @@ -30,7 +30,7 @@ describe Gitlab::GitalyClient::CommitService do context 'when a commit does not have a parent' do it 'sends an RPC request with empty tree ref as left commit' do - initial_commit = project.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863') + initial_commit = project.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863').raw request = Gitaly::CommitDiffRequest.new( repository: repository_message, left_commit_id: '4b825dc642cb6eb9a060e54bf8d69288fbee4904', diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index 08693b5da33..1d38f8e5a6d 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -33,7 +33,6 @@ describe Commit do describe '#to_reference' do let(:project) { create(:project, :repository, path: 'sample-project') } - let(:commit) { project.commit } it 'returns a String reference to the object' do expect(commit.to_reference).to eq commit.id @@ -47,7 +46,6 @@ describe Commit do describe '#reference_link_text' do let(:project) { create(:project, :repository, path: 'sample-project') } - let(:commit) { project.commit } it 'returns a String reference to the object' do expect(commit.reference_link_text).to eq commit.short_id -- cgit v1.2.1