diff --git a/app/models/commit_collection.rb b/app/models/commit_collection.rb index a9a2e9c81eb5be1e445aaaaee8bfeb74f93d12b3..52524456439ae9247c78b12a7a35c9bfed46ab64 100644 --- a/app/models/commit_collection.rb +++ b/app/models/commit_collection.rb @@ -28,10 +28,43 @@ class CommitCollection def without_merge_commits strong_memoize(:without_merge_commits) do - commits.reject(&:merge_commit?) + # `#enrich!` the collection to ensure all commits contain + # the necessary parent data + enrich!.commits.reject(&:merge_commit?) end end + def unenriched + commits.reject(&:gitaly_commit?) + end + + def fully_enriched? + unenriched.empty? + end + + # Batch load any commits that are not backed by full gitaly data, and + # replace them in the collection. + def enrich! + # A project is needed in order to fetch data from gitaly. Projects + # can be absent from commits in certain rare situations (like when + # viewing a MR of a deleted fork). In these cases, assume that the + # enriched data is not needed. + return self if project.blank? || fully_enriched? + + # Batch load full Commits from the repository + # and map to a Hash of id => Commit + replacements = Hash[unenriched.map do |c| + [c.id, Commit.lazy(project, c.id)] + end.compact] + + # Replace the commits, keeping the same order + @commits = @commits.map do |c| + replacements.fetch(c.id, c) + end + + self + end + # Sets the pipeline status for every commit. # # Setting this status ahead of time removes the need for running a query for diff --git a/changelogs/unreleased/58805-allow-incomplete-commit-data-to-be-fetched-from-collection.yml b/changelogs/unreleased/58805-allow-incomplete-commit-data-to-be-fetched-from-collection.yml new file mode 100644 index 0000000000000000000000000000000000000000..4377ebfdbdf165518ac2ac0addefef77e2ec3f3d --- /dev/null +++ b/changelogs/unreleased/58805-allow-incomplete-commit-data-to-be-fetched-from-collection.yml @@ -0,0 +1,5 @@ +--- +title: Fix merge commits being used as default squash commit messages +merge_request: 26445 +author: +type: fixed diff --git a/ee/changelogs/unreleased/58805-allow-incomplete-commit-data-to-be-fetched-from-collection-ee.yml b/ee/changelogs/unreleased/58805-allow-incomplete-commit-data-to-be-fetched-from-collection-ee.yml new file mode 100644 index 0000000000000000000000000000000000000000..d2303ad032fdc4c33775a88cfa74c8feab3a1ac8 --- /dev/null +++ b/ee/changelogs/unreleased/58805-allow-incomplete-commit-data-to-be-fetched-from-collection-ee.yml @@ -0,0 +1,5 @@ +--- +title: Fix authors of merge commits being excluded from approving an MR +merge_request: 10359 +author: +type: fixed diff --git a/ee/spec/models/approvable_for_rule_spec.rb b/ee/spec/models/approvable_for_rule_spec.rb index 6711aeda5c18c929ff06e573ef9051797cb15f41..a8797d469ada58ce062dc3af81a5ab04e0e7d642 100644 --- a/ee/spec/models/approvable_for_rule_spec.rb +++ b/ee/spec/models/approvable_for_rule_spec.rb @@ -60,7 +60,7 @@ describe ApprovableForRule do end context 'when user is committer' do - let(:user) { create(:user, email: merge_request.commits.first.committer_email) } + let(:user) { create(:user, email: merge_request.commits.without_merge_commits.first.committer_email) } before do project.add_developer(user) @@ -90,7 +90,7 @@ describe ApprovableForRule do end it 'returns false when user is a committer' do - user = create(:user, email: merge_request.commits.first.committer_email) + user = create(:user, email: merge_request.commits.without_merge_commits.first.committer_email) project.add_developer(user) create(:approver, target: merge_request, user: user) diff --git a/ee/spec/models/approvable_spec.rb b/ee/spec/models/approvable_spec.rb index 13d14e51c7940c950e73c9f929057b3ee1119b9b..0b6569c047014d8fe1f31536e11e832d4adb4cff 100644 --- a/ee/spec/models/approvable_spec.rb +++ b/ee/spec/models/approvable_spec.rb @@ -79,7 +79,7 @@ describe Approvable do end context 'when user is committer' do - let(:user) { create(:user, email: merge_request.commits.first.committer_email) } + let(:user) { create(:user, email: merge_request.commits.without_merge_commits.first.committer_email) } before do project.add_developer(user) @@ -109,7 +109,7 @@ describe Approvable do end it 'returns false when user is a committer' do - user = create(:user, email: merge_request.commits.first.committer_email) + user = create(:user, email: merge_request.commits.without_merge_commits.first.committer_email) project.add_developer(user) create(:approver, target: merge_request, user: user) diff --git a/ee/spec/models/visible_approvable_for_rule_spec.rb b/ee/spec/models/visible_approvable_for_rule_spec.rb index 7ffb9c4a857c8d3cf67d1fde6169d6a4cf5ce3da..051aa07f1246532e64e48b9bec4e91ef4b8d54fb 100644 --- a/ee/spec/models/visible_approvable_for_rule_spec.rb +++ b/ee/spec/models/visible_approvable_for_rule_spec.rb @@ -85,7 +85,7 @@ describe VisibleApprovableForRule do end context 'when committer is approver' do - let(:approver) { create(:user, email: resource.commits.first.committer_email) } + let(:approver) { create(:user, email: resource.commits.without_merge_commits.first.committer_email) } it_behaves_like 'able to exclude authors' end diff --git a/ee/spec/models/visible_approvable_spec.rb b/ee/spec/models/visible_approvable_spec.rb index 88f3e466ed01d321d44972997822cd00a8699f36..d4db55a8ee954a485b8a629f9affdb115b416e69 100644 --- a/ee/spec/models/visible_approvable_spec.rb +++ b/ee/spec/models/visible_approvable_spec.rb @@ -69,7 +69,7 @@ describe VisibleApprovable do end context 'when committer is approver' do - let(:user) { create(:user, email: resource.commits.first.committer_email) } + let(:user) { create(:user, email: resource.commits.without_merge_commits.first.committer_email) } let!(:committer_approver) { create(:approver, target: project, user: user) } before do diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb index 88ff9fbceb40be08cf15bde92a5a59e42de72c53..8fac3621df9c8ffe85d579eb157c38687a44195d 100644 --- a/lib/gitlab/git/commit.rb +++ b/lib/gitlab/git/commit.rb @@ -318,6 +318,10 @@ module Gitlab parent_ids.size > 1 end + def gitaly_commit? + raw_commit.is_a?(Gitaly::GitCommit) + end + def tree_entry(path) return unless path.present? @@ -340,7 +344,7 @@ module Gitlab end def to_gitaly_commit - return raw_commit if raw_commit.is_a?(Gitaly::GitCommit) + return raw_commit if gitaly_commit? message_split = raw_commit.message.split("\n", 2) Gitaly::GitCommit.new( diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb index 3fb41a626b29b347c9869f238871e12915e7647e..4a4ac833e39fdc1b43e1cd5c8e4cff23b5128369 100644 --- a/spec/lib/gitlab/git/commit_spec.rb +++ b/spec/lib/gitlab/git/commit_spec.rb @@ -537,6 +537,18 @@ describe Gitlab::Git::Commit, :seed_helper do end end + describe '#gitaly_commit?' do + context 'when the commit data comes from gitaly' do + it { expect(commit.gitaly_commit?).to eq(true) } + end + + context 'when the commit data comes from a Hash' do + let(:commit) { described_class.new(repository, sample_commit_hash) } + + it { expect(commit.gitaly_commit?).to eq(false) } + end + end + describe '#has_zero_stats?' do it { expect(commit.has_zero_stats?).to eq(false) } end diff --git a/spec/models/commit_collection_spec.rb b/spec/models/commit_collection_spec.rb index 0f5d03ff458caa6b3cb2c4a89ba36d6a4564e3c4..30c504ebea8812bfd71cea0bc8548e4bec9cc841 100644 --- a/spec/models/commit_collection_spec.rb +++ b/spec/models/commit_collection_spec.rb @@ -37,12 +37,92 @@ describe CommitCollection do describe '#without_merge_commits' do it 'returns all commits except merge commits' do + merge_commit = project.commit("60ecb67744cb56576c30214ff52294f8ce2def98") + expect(merge_commit).to receive(:merge_commit?).and_return(true) + collection = described_class.new(project, [ - build(:commit), - build(:commit, :merge_commit) + commit, + merge_commit ]) - expect(collection.without_merge_commits.size).to eq(1) + expect(collection.without_merge_commits).to contain_exactly(commit) + end + end + + describe 'enrichment methods' do + let(:gitaly_commit) { commit } + let(:hash_commit) { Commit.from_hash(gitaly_commit.to_hash, project) } + + describe '#unenriched' do + it 'returns all commits that are not backed by gitaly data' do + collection = described_class.new(project, [gitaly_commit, hash_commit]) + + expect(collection.unenriched).to contain_exactly(hash_commit) + end + end + + describe '#fully_enriched?' do + it 'returns true when all commits are backed by gitaly data' do + collection = described_class.new(project, [gitaly_commit, gitaly_commit]) + + expect(collection.fully_enriched?).to eq(true) + end + + it 'returns false when any commits are not backed by gitaly data' do + collection = described_class.new(project, [gitaly_commit, hash_commit]) + + expect(collection.fully_enriched?).to eq(false) + end + + it 'returns true when the collection is empty' do + collection = described_class.new(project, []) + + expect(collection.fully_enriched?).to eq(true) + end + end + + describe '#enrich!' do + it 'replaces commits in the collection with those backed by gitaly data' do + collection = described_class.new(project, [hash_commit]) + + collection.enrich! + + new_commit = collection.commits.first + expect(new_commit.id).to eq(hash_commit.id) + expect(hash_commit.gitaly_commit?).to eq(false) + expect(new_commit.gitaly_commit?).to eq(true) + end + + it 'maintains the original order of the commits' do + gitaly_commits = [gitaly_commit] * 3 + hash_commits = [hash_commit] * 3 + # Interleave the gitaly and hash commits together + original_commits = gitaly_commits.zip(hash_commits).flatten + collection = described_class.new(project, original_commits) + + collection.enrich! + + original_commits.each_with_index do |original_commit, i| + new_commit = collection.commits[i] + expect(original_commit.id).to eq(new_commit.id) + end + end + + it 'fetches data if there are unenriched commits' do + collection = described_class.new(project, [hash_commit]) + + expect(Commit).to receive(:lazy).exactly(:once) + + collection.enrich! + end + + it 'does not fetch data if all commits are enriched' do + collection = described_class.new(project, [gitaly_commit]) + + expect(Commit).not_to receive(:lazy) + + collection.enrich! + end end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 48b78aa21157a5f2da34e35eea5377f2ada797ef..c1c4c2410d345b45872d8cd0496d3f579d96a056 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -84,32 +84,27 @@ describe MergeRequest do describe '#default_squash_commit_message' do let(:project) { subject.project } - - def commit_collection(commit_hashes) - raw_commits = commit_hashes.map { |raw| Commit.from_hash(raw, project) } - - CommitCollection.new(project, raw_commits) - end + let(:is_multiline) { -> (c) { c.description.present? } } + let(:multiline_commits) { subject.commits.select(&is_multiline) } + let(:singleline_commits) { subject.commits.reject(&is_multiline) } it 'returns the oldest multiline commit message' do - commits = commit_collection([ - { message: 'Singleline', parent_ids: [] }, - { message: "Second multiline\nCommit message", parent_ids: [] }, - { message: "First multiline\nCommit message", parent_ids: [] } - ]) - - expect(subject).to receive(:commits).and_return(commits) - - expect(subject.default_squash_commit_message).to eq("First multiline\nCommit message") + expect(subject.default_squash_commit_message).to eq(multiline_commits.last.message) end it 'returns the merge request title if there are no multiline commits' do - commits = commit_collection([ - { message: 'Singleline', parent_ids: [] } - ]) + expect(subject).to receive(:commits).and_return( + CommitCollection.new(project, singleline_commits) + ) + + expect(subject.default_squash_commit_message).to eq(subject.title) + end - expect(subject).to receive(:commits).and_return(commits) + it 'does not return commit messages from multiline merge commits' do + collection = CommitCollection.new(project, multiline_commits).enrich! + expect(collection.commits).to all( receive(:merge_commit?).and_return(true) ) + expect(subject).to receive(:commits).and_return(collection) expect(subject.default_squash_commit_message).to eq(subject.title) end end @@ -1044,7 +1039,7 @@ describe MergeRequest do describe '#commit_authors' do it 'returns all the authors of every commit in the merge request' do - users = subject.commits.map(&:author_email).uniq.map do |email| + users = subject.commits.without_merge_commits.map(&:author_email).uniq.map do |email| create(:user, email: email) end @@ -1058,7 +1053,7 @@ describe MergeRequest do describe '#authors' do it 'returns a list with all the commit authors in the merge request and author' do - users = subject.commits.map(&:author_email).uniq.map do |email| + users = subject.commits.without_merge_commits.map(&:author_email).uniq.map do |email| create(:user, email: email) end diff --git a/spec/serializers/merge_request_widget_entity_spec.rb b/spec/serializers/merge_request_widget_entity_spec.rb index 4dbd79f2fc0a4fb06f31690881f839fa4e278813..727fd8951f29af0d90bd0cc0f611b316a15b9877 100644 --- a/spec/serializers/merge_request_widget_entity_spec.rb +++ b/spec/serializers/merge_request_widget_entity_spec.rb @@ -279,13 +279,18 @@ describe MergeRequestWidgetEntity do end describe 'commits_without_merge_commits' do + def find_matching_commit(short_id) + resource.commits.find { |c| c.short_id == short_id } + end + it 'should not include merge commits' do - # Mock all but the first 5 commits to be merge commits - resource.commits.each_with_index do |commit, i| - expect(commit).to receive(:merge_commit?).at_least(:once).and_return(i > 4) - end + commits_in_widget = subject[:commits_without_merge_commits] - expect(subject[:commits_without_merge_commits].size).to eq(5) + expect(commits_in_widget.length).to be < resource.commits.length + expect(commits_in_widget.length).to eq(resource.commits.without_merge_commits.length) + commits_in_widget.each do |c| + expect(find_matching_commit(c[:short_id]).merge_commit?).to eq(false) + end end end end