diff --git a/config/feature_flags/development/changes_batch_commits.yml b/config/feature_flags/development/changes_batch_commits.yml
new file mode 100644
index 0000000000000000000000000000000000000000..2b276759fb1fa347a36db0b4a6619c44b1484850
--- /dev/null
+++ b/config/feature_flags/development/changes_batch_commits.yml
@@ -0,0 +1,8 @@
+---
+name: changes_batch_commits
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/66731
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/336992
+milestone: '14.2'
+type: development
+group: group::gitaly
+default_enabled: false
diff --git a/ee/spec/lib/gitlab/git_access_spec.rb b/ee/spec/lib/gitlab/git_access_spec.rb
index 4a00c4b282e4f7df2ca2428bcc4e317f9636c31b..aa0fae0ac2201d122a3f80328c5f00d81bd3380c 100644
--- a/ee/spec/lib/gitlab/git_access_spec.rb
+++ b/ee/spec/lib/gitlab/git_access_spec.rb
@@ -77,7 +77,7 @@ def download_ability
end
it 'allows githook for new branch with an old bad commit' do
- bad_commit = double("Commit", safe_message: 'Some change').as_null_object
+ bad_commit = double("Commit", safe_message: 'Some change', id: end_sha).as_null_object
ref_object = double(name: 'heads/master')
allow(bad_commit).to receive(:refs).and_return([ref_object])
allow_next_instance_of(Repository) do |instance|
@@ -91,7 +91,7 @@ def download_ability
end
it 'allows githook for any change with an old bad commit' do
- bad_commit = double("Commit", safe_message: 'Some change').as_null_object
+ bad_commit = double("Commit", safe_message: 'Some change', id: end_sha).as_null_object
ref_object = double(name: 'heads/master')
allow(bad_commit).to receive(:refs).and_return([ref_object])
allow(project.repository).to receive(:commits_between).and_return([bad_commit])
@@ -103,7 +103,7 @@ def download_ability
end
it 'does not allow any change from Web UI with bad commit' do
- bad_commit = double("Commit", safe_message: 'Some change').as_null_object
+ bad_commit = double("Commit", safe_message: 'Some change', id: end_sha).as_null_object
# We use tmp ref a a temporary for Web UI commiting
ref_object = double(name: 'refs/tmp')
allow(bad_commit).to receive(:refs).and_return([ref_object])
diff --git a/lib/gitlab/checks/changes_access.rb b/lib/gitlab/checks/changes_access.rb
index 4e8b293a3e629282401697942641b5b6d4aa05bf..3ec3cdafd7c7ac5ff78ba252a2f92cee0dc01943 100644
--- a/lib/gitlab/checks/changes_access.rb
+++ b/lib/gitlab/checks/changes_access.rb
@@ -29,11 +29,48 @@ def validate!
true
end
+ # All commits which have been newly introduced via any of the given
+ # changes. This set may also contain commits which are not referenced by
+ # any of the new revisions.
+ def commits
+ newrevs = @changes.map do |change|
+ newrev = change[:newrev]
+ newrev unless newrev.blank? || Gitlab::Git.blank_ref?(newrev)
+ end.compact
+
+ return [] if newrevs.empty?
+
+ @commits ||= project.repository.new_commits(newrevs)
+ end
+
+ # All commits which have been newly introduced via the given revision.
+ def commits_for(newrev)
+ commits_by_id = commits.index_by(&:id)
+
+ result = []
+ pending = [newrev]
+
+ # We go up the parent chain of our newrev and collect all commits which
+ # are new. In case a commit's ID cannot be found in the set of new
+ # commits, then it must already be a preexisting commit.
+ pending.each do |rev|
+ commit = commits_by_id[rev]
+ next if commit.nil?
+
+ pending.push(*commit.parent_ids)
+ result << commit
+ end
+
+ result
+ end
+
protected
def single_access_checks!
# Iterate over all changes to find if user allowed all of them to be applied
changes.each do |change|
+ commits = Gitlab::Lazy.new { commits_for(change[:newrev]) } if Feature.enabled?(:changes_batch_commits)
+
# If user does not have access to make at least one change, cancel all
# push by allowing the exception to bubble up
Checks::SingleChangeAccess.new(
@@ -41,7 +78,8 @@ def single_access_checks!
user_access: user_access,
project: project,
protocol: protocol,
- logger: logger
+ logger: logger,
+ commits: commits
).validate!
end
end
diff --git a/lib/gitlab/checks/single_change_access.rb b/lib/gitlab/checks/single_change_access.rb
index 280b2dd25e23e50babf7514fec91437d63017dfa..2fd48dfbfe2c9b957c37961c894f5413de35a946 100644
--- a/lib/gitlab/checks/single_change_access.rb
+++ b/lib/gitlab/checks/single_change_access.rb
@@ -11,7 +11,7 @@ class SingleChangeAccess
def initialize(
change, user_access:, project:,
- protocol:, logger:
+ protocol:, logger:, commits: nil
)
@oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
@branch_name = Gitlab::Git.branch_name(@ref)
@@ -19,6 +19,7 @@ def initialize(
@user_access = user_access
@project = project
@protocol = protocol
+ @commits = commits
@logger = logger
@logger.append_message("Running checks for ref: #{@branch_name || @tag_name}")
diff --git a/spec/lib/gitlab/checks/changes_access_spec.rb b/spec/lib/gitlab/checks/changes_access_spec.rb
index a46732f8255250cc8e90b7bf302faca6914f89e9..1e053d25123fbc95352404b858dc948fb993ec1a 100644
--- a/spec/lib/gitlab/checks/changes_access_spec.rb
+++ b/spec/lib/gitlab/checks/changes_access_spec.rb
@@ -3,40 +3,169 @@
require 'spec_helper'
RSpec.describe Gitlab::Checks::ChangesAccess do
+ include_context 'changes access checks context'
+
+ subject { changes_access }
+
describe '#validate!' do
- include_context 'changes access checks context'
+ shared_examples '#validate!' do
+ before do
+ allow(project).to receive(:lfs_enabled?).and_return(true)
+ end
- before do
- allow(project).to receive(:lfs_enabled?).and_return(true)
- end
+ context 'without failed checks' do
+ it "doesn't raise an error" do
+ expect { subject.validate! }.not_to raise_error
+ end
- subject { changes_access }
+ it 'calls lfs checks' do
+ expect_next_instance_of(Gitlab::Checks::LfsCheck) do |instance|
+ expect(instance).to receive(:validate!)
+ end
- context 'without failed checks' do
- it "doesn't raise an error" do
- expect { subject.validate! }.not_to raise_error
+ subject.validate!
+ end
end
- it 'calls lfs checks' do
- expect_next_instance_of(Gitlab::Checks::LfsCheck) do |instance|
- expect(instance).to receive(:validate!)
+ context 'when time limit was reached' do
+ it 'raises a TimeoutError' do
+ logger = Gitlab::Checks::TimedLogger.new(start_time: timeout.ago, timeout: timeout)
+ access = described_class.new(changes,
+ project: project,
+ user_access: user_access,
+ protocol: protocol,
+ logger: logger)
+
+ expect { access.validate! }.to raise_error(Gitlab::Checks::TimedLogger::TimeoutError)
end
+ end
+ end
+
+ context 'with batched commits enabled' do
+ before do
+ stub_feature_flags(changes_batch_commits: true)
+ end
+
+ it_behaves_like '#validate!'
+ end
+
+ context 'with batched commits disabled' do
+ before do
+ stub_feature_flags(changes_batch_commits: false)
+ end
+
+ it_behaves_like '#validate!'
+ end
+ end
+
+ describe '#commits' do
+ it 'calls #new_commits' do
+ expect(project.repository).to receive(:new_commits).and_return([])
+
+ expect(subject.commits).to eq([])
+ end
+
+ context 'when changes contain empty revisions' do
+ let(:changes) { [{ newrev: newrev }, { newrev: '' }, { newrev: Gitlab::Git::BLANK_SHA }] }
+ let(:expected_commit) { instance_double(Commit) }
+
+ it 'returns only commits with non empty revisions' do
+ expect(project.repository).to receive(:new_commits).with([newrev]) { [expected_commit] }
+ expect(subject.commits).to eq([expected_commit])
+ end
+ end
+ end
- subject.validate!
+ describe '#commits_for' do
+ let(:new_commits) { [] }
+ let(:expected_commits) { [] }
+
+ shared_examples 'a listing of new commits' do
+ it 'returns expected commits' do
+ expect(subject).to receive(:commits).and_return(new_commits)
+
+ expect(subject.commits_for(newrev)).to eq(expected_commits)
+ end
+ end
+
+ context 'with no commits' do
+ it_behaves_like 'a listing of new commits'
+ end
+
+ context 'with unrelated commits' do
+ let(:new_commits) { [create_commit('1234', %w[1111 2222])] }
+
+ it_behaves_like 'a listing of new commits'
+ end
+
+ context 'with single related commit' do
+ let(:new_commits) { [create_commit(newrev, %w[1111 2222])] }
+ let(:expected_commits) { new_commits }
+
+ it_behaves_like 'a listing of new commits'
+ end
+
+ context 'with single related and unrelated commit' do
+ let(:new_commits) do
+ [
+ create_commit(newrev, %w[1111 2222]),
+ create_commit('abcd', %w[1111 2222])
+ ]
end
+
+ let(:expected_commits) do
+ [create_commit(newrev, %w[1111 2222])]
+ end
+
+ it_behaves_like 'a listing of new commits'
end
- context 'when time limit was reached' do
- it 'raises a TimeoutError' do
- logger = Gitlab::Checks::TimedLogger.new(start_time: timeout.ago, timeout: timeout)
- access = described_class.new(changes,
- project: project,
- user_access: user_access,
- protocol: protocol,
- logger: logger)
+ context 'with multiple related commits' do
+ let(:new_commits) do
+ [
+ create_commit(newrev, %w[1111]),
+ create_commit('1111', %w[2222]),
+ create_commit('abcd', [])
+ ]
+ end
- expect { access.validate! }.to raise_error(Gitlab::Checks::TimedLogger::TimeoutError)
+ let(:expected_commits) do
+ [
+ create_commit(newrev, %w[1111]),
+ create_commit('1111', %w[2222])
+ ]
end
+
+ it_behaves_like 'a listing of new commits'
end
+
+ context 'with merge commits' do
+ let(:new_commits) do
+ [
+ create_commit(newrev, %w[1111 2222 3333]),
+ create_commit('1111', []),
+ create_commit('3333', %w[4444]),
+ create_commit('4444', [])
+ ]
+ end
+
+ let(:expected_commits) do
+ [
+ create_commit(newrev, %w[1111 2222 3333]),
+ create_commit('1111', []),
+ create_commit('3333', %w[4444]),
+ create_commit('4444', [])
+ ]
+ end
+
+ it_behaves_like 'a listing of new commits'
+ end
+ end
+
+ def create_commit(id, parent_ids)
+ Gitlab::Git::Commit.new(project.repository, {
+ id: id,
+ parent_ids: parent_ids
+ })
end
end
diff --git a/spec/lib/gitlab/checks/single_change_access_spec.rb b/spec/lib/gitlab/checks/single_change_access_spec.rb
index 8b235005b3eeaaa515647248df00f1a7351a5c92..e81e4951539616f5f560a363f9e593e45b7fc408 100644
--- a/spec/lib/gitlab/checks/single_change_access_spec.rb
+++ b/spec/lib/gitlab/checks/single_change_access_spec.rb
@@ -58,5 +58,52 @@
expect { access.validate! }.to raise_error(Gitlab::Checks::TimedLogger::TimeoutError)
end
end
+
+ describe '#commits' do
+ let(:expected_commits) { [Gitlab::Git::Commit.new(project.repository, { id: "1234" })] }
+
+ let(:access) do
+ described_class.new(changes,
+ project: project,
+ user_access: user_access,
+ protocol: protocol,
+ logger: logger,
+ commits: provided_commits)
+ end
+
+ shared_examples '#commits' do
+ it 'returns expected commits' do
+ expect(access.commits).to eq(expected_commits)
+ end
+
+ it 'returns expected commits on repeated calls' do
+ expect(access.commits).to eq(expected_commits)
+ expect(access.commits).to eq(expected_commits)
+ end
+ end
+
+ context 'with provided commits' do
+ let(:provided_commits) { expected_commits }
+
+ before do
+ expect(project.repository).not_to receive(:new_commits)
+ end
+
+ it_behaves_like '#commits'
+ end
+
+ context 'without provided commits' do
+ let(:provided_commits) { nil }
+
+ before do
+ expect(project.repository)
+ .to receive(:new_commits)
+ .once
+ .and_return(expected_commits)
+ end
+
+ it_behaves_like '#commits'
+ end
+ end
end
end