diff --git a/app/models/repository.rb b/app/models/repository.rb
index 3ac30af7d3e7600216d36b6c3911a2a17af2b352..8d2f0648a05f241376e2d762807cc9c2b2f8bc4a 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -161,8 +161,8 @@ def commits(ref = nil, opts = {})
CommitCollection.new(container, commits, ref)
end
- def commits_between(from, to)
- commits = Gitlab::Git::Commit.between(raw_repository, from, to)
+ def commits_between(from, to, limit: nil)
+ commits = Gitlab::Git::Commit.between(raw_repository, from, to, limit: limit)
commits = Commit.decorate(commits, container) if commits.present?
commits
end
diff --git a/app/services/git/base_hooks_service.rb b/app/services/git/base_hooks_service.rb
index aee2f685e97d9720bd90a66ac22042a6be5a663d..63f3f73905afe2bf81e6ffa90e93d2c4c8220abd 100644
--- a/app/services/git/base_hooks_service.rb
+++ b/app/services/git/base_hooks_service.rb
@@ -25,17 +25,13 @@ def hook_name
raise NotImplementedError, "Please implement #{self.class}##{__method__}"
end
- # The changeset, ordered with the newest commit last
- def commits
- raise NotImplementedError, "Please implement #{self.class}##{__method__}"
- end
-
+ # This should return PROCESS_COMMIT_LIMIT commits, ordered with newest last
def limited_commits
- @limited_commits ||= commits.last(PROCESS_COMMIT_LIMIT)
+ raise NotImplementedError, "Please implement #{self.class}##{__method__}"
end
def commits_count
- commits.count
+ raise NotImplementedError, "Please implement #{self.class}##{__method__}"
end
def event_message
diff --git a/app/services/git/branch_hooks_service.rb b/app/services/git/branch_hooks_service.rb
index 7a22d7ffcdfd45715ad23388795d04ffdf3b8ef8..9b113be5465dec245a19c516476b3b551d417f09 100644
--- a/app/services/git/branch_hooks_service.rb
+++ b/app/services/git/branch_hooks_service.rb
@@ -18,20 +18,25 @@ def hook_name
:push_hooks
end
- def commits
- strong_memoize(:commits) do
+ def limited_commits
+ strong_memoize(:limited_commits) { threshold_commits.last(PROCESS_COMMIT_LIMIT) }
+ end
+
+ # Taking limit+1 commits allows us to detect when the limit is in effect
+ def threshold_commits
+ strong_memoize(:threshold_commits) do
if creating_default_branch?
# The most recent PROCESS_COMMIT_LIMIT commits in the default branch.
# They are returned newest-to-oldest, but we need to present them oldest-to-newest
- project.repository.commits(newrev, limit: PROCESS_COMMIT_LIMIT).reverse
+ project.repository.commits(newrev, limit: PROCESS_COMMIT_LIMIT + 1).reverse!
elsif creating_branch?
# Use the pushed commits that aren't reachable by the default branch
# as a heuristic. This may include more commits than are actually
# pushed, but that shouldn't matter because we check for existing
# cross-references later.
- project.repository.commits_between(project.default_branch, newrev)
+ project.repository.commits_between(project.default_branch, newrev, limit: PROCESS_COMMIT_LIMIT + 1)
elsif updating_branch?
- project.repository.commits_between(oldrev, newrev)
+ project.repository.commits_between(oldrev, newrev, limit: PROCESS_COMMIT_LIMIT + 1)
else # removing branch
[]
end
@@ -39,9 +44,21 @@ def commits
end
def commits_count
- return count_commits_in_branch if creating_default_branch?
+ strong_memoize(:commits_count) do
+ next threshold_commits.count if
+ strong_memoized?(:threshold_commits) &&
+ threshold_commits.count <= PROCESS_COMMIT_LIMIT
- super
+ if creating_default_branch?
+ project.repository.commit_count_for_ref(ref)
+ elsif creating_branch?
+ project.repository.count_commits_between(project.default_branch, newrev)
+ elsif updating_branch?
+ project.repository.count_commits_between(oldrev, newrev)
+ else # removing branch
+ 0
+ end
+ end
end
override :invalidated_file_types
@@ -179,12 +196,6 @@ def creating_default_branch?
creating_branch? && default_branch?
end
- def count_commits_in_branch
- strong_memoize(:count_commits_in_branch) do
- project.repository.commit_count_for_ref(ref)
- end
- end
-
def default_branch?
strong_memoize(:default_branch) do
[nil, branch_name].include?(project.default_branch)
diff --git a/app/services/git/tag_hooks_service.rb b/app/services/git/tag_hooks_service.rb
index d83924fec28d91093457f19533ca822b5997032c..01174d8a94250d8ca728a6e7998a473c9df8f053 100644
--- a/app/services/git/tag_hooks_service.rb
+++ b/app/services/git/tag_hooks_service.rb
@@ -8,10 +8,14 @@ def hook_name
:tag_push_hooks
end
- def commits
+ def limited_commits
[tag_commit].compact
end
+ def commits_count
+ limited_commits.count
+ end
+
def event_message
tag&.message
end
diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb
index 49fe365e09eb05e03750f801aaf2b57a72d95598..6605e896ef1d4306dead127d2f0cfc513a5e204a 100644
--- a/lib/gitlab/git/commit.rb
+++ b/lib/gitlab/git/commit.rb
@@ -108,17 +108,26 @@ def last_for_path(repo, ref, path = nil, literal_pathspec: false)
# See also #repository.commits_between
#
# Ex.
- # Commit.between(repo, '29eda46b', 'master')
+ # Commit.between(repo, '29eda46b', 'master') # all commits, ordered oldest to newest
+ # Commit.between(repo, '29eda46b', 'master', limit: 100) # 100 newest commits, ordered oldest to newest
#
- def between(repo, base, head)
+ def between(repo, base, head, limit: nil)
# In either of these cases, we are guaranteed to return no commits, so
# shortcut the RPC call
return [] if Gitlab::Git.blank_ref?(base) || Gitlab::Git.blank_ref?(head)
wrapped_gitaly_errors do
revisions = [head, "^#{base}"] # base..head
-
- repo.gitaly_commit_client.list_commits(revisions, reverse: true)
+ client = repo.gitaly_commit_client
+
+ # We must return the commits in chronological order but using both
+ # limit and reverse in the Gitaly RPC would return the oldest N,
+ # rather than newest N, commits, so reorder in Ruby with limit
+ if limit
+ client.list_commits(revisions, pagination_params: { limit: limit }).reverse!
+ else
+ client.list_commits(revisions, reverse: true)
+ end
end
end
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index fa616a252e4b246e1bca6507a6ee2cd0b7a567f2..ddf04bd8e79d79743e717dd16f06f2e9d5084d5a 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -255,11 +255,12 @@ def find_all_commits(opts = {})
consume_commits_response(response)
end
- def list_commits(revisions, reverse: false)
+ def list_commits(revisions, reverse: false, pagination_params: nil)
request = Gitaly::ListCommitsRequest.new(
repository: @gitaly_repo,
revisions: Array.wrap(revisions),
- reverse: reverse
+ reverse: reverse,
+ pagination_params: pagination_params
)
response = GitalyClient.call(@repository.storage, :commit_service, :list_commits, request, timeout: GitalyClient.medium_timeout)
diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb
index 174ed43bd401dd910e2ac80e6bafec9453c3e53e..f4dba5e8d586978352f16237f3df7fec4f2c6124 100644
--- a/spec/lib/gitlab/git/commit_spec.rb
+++ b/spec/lib/gitlab/git/commit_spec.rb
@@ -364,12 +364,40 @@
end
describe '.between' do
- subject do
- commits = described_class.between(repository, SeedRepo::Commit::PARENT_ID, SeedRepo::Commit::ID)
- commits.map { |c| c.id }
+ let(:limit) { nil }
+ let(:commit_ids) { commits.map(&:id) }
+
+ subject(:commits) { described_class.between(repository, from, to, limit: limit) }
+
+ context 'requesting a single commit' do
+ let(:from) { SeedRepo::Commit::PARENT_ID }
+ let(:to) { SeedRepo::Commit::ID }
+
+ it { expect(commit_ids).to contain_exactly(to) }
end
- it { is_expected.to contain_exactly(SeedRepo::Commit::ID) }
+ context 'requesting a commit range' do
+ let(:from) { 'v1.0.0' }
+ let(:to) { 'v1.2.0' }
+
+ let(:commits_in_range) do
+ %w[
+ 570e7b2abdd848b95f2f578043fc23bd6f6fd24d
+ 5937ac0a7beb003549fc5fd26fc247adbce4a52e
+ eb49186cfa5c4338011f5f590fac11bd66c5c631
+ ]
+ end
+
+ context 'no limit' do
+ it { expect(commit_ids).to eq(commits_in_range) }
+ end
+
+ context 'limited' do
+ let(:limit) { 2 }
+
+ it { expect(commit_ids).to eq(commits_in_range.last(2)) }
+ end
+ end
end
describe '.shas_with_signatures' do
diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
index a0e2d43cf4501320b0ffdd453664966003710c82..554a91f2bc589a7e0c5f9f1e9b24dd16efd400a7 100644
--- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
@@ -311,6 +311,10 @@
end
describe '#list_commits' do
+ let(:revisions) { 'master' }
+ let(:reverse) { false }
+ let(:pagination_params) { nil }
+
shared_examples 'a ListCommits request' do
before do
::Gitlab::GitalyClient.clear_stubs!
@@ -318,26 +322,35 @@
it 'sends a list_commits message' do
expect_next_instance_of(Gitaly::CommitService::Stub) do |service|
- expect(service)
- .to receive(:list_commits)
- .with(gitaly_request_with_params(expected_params), kind_of(Hash))
- .and_return([])
+ expected_request = gitaly_request_with_params(
+ Array.wrap(revisions),
+ reverse: reverse,
+ pagination_params: pagination_params
+ )
+
+ expect(service).to receive(:list_commits).with(expected_request, kind_of(Hash)).and_return([])
end
- client.list_commits(revisions)
+ client.list_commits(revisions, reverse: reverse, pagination_params: pagination_params)
end
end
- context 'with a single revision' do
- let(:revisions) { 'master' }
- let(:expected_params) { %w[master] }
+ it_behaves_like 'a ListCommits request'
+
+ context 'with multiple revisions' do
+ let(:revisions) { %w[master --not --all] }
+
+ it_behaves_like 'a ListCommits request'
+ end
+
+ context 'with reverse: true' do
+ let(:reverse) { true }
it_behaves_like 'a ListCommits request'
end
- context 'with multiple revisions' do
- let(:revisions) { %w[master --not --all] }
- let(:expected_params) { %w[master --not --all] }
+ context 'with pagination params' do
+ let(:pagination_params) { { limit: 1, page_token: 'foo' } }
it_behaves_like 'a ListCommits request'
end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 064d793588f8104218729f89331475a81ced150b..949892beed5395e130ebd49aae7ff1ab3426deff 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -470,6 +470,29 @@ def create_commit_with_invalid_utf8_path
end
end
+ describe '#commits_between' do
+ let(:commit) { project.commit }
+
+ it 'delegates to Gitlab::Git::Commit#between, returning decorated commits' do
+ expect(Gitlab::Git::Commit)
+ .to receive(:between)
+ .with(repository.raw_repository, commit.parent_id, commit.id, limit: 5)
+ .and_call_original
+
+ result = repository.commits_between(commit.parent_id, commit.id, limit: 5)
+
+ expect(result).to contain_exactly(instance_of(Commit), instance_of(Commit))
+ end
+
+ it 'defaults to no limit' do
+ expect(Gitlab::Git::Commit)
+ .to receive(:between)
+ .with(repository.raw_repository, commit.parent_id, commit.id, limit: nil)
+
+ repository.commits_between(commit.parent_id, commit.id)
+ end
+ end
+
describe '#find_commits_by_message' do
it 'returns commits with messages containing a given string' do
commit_ids = repository.find_commits_by_message('submodule').map(&:id)
diff --git a/spec/services/git/base_hooks_service_spec.rb b/spec/services/git/base_hooks_service_spec.rb
index 539c294a2e796cb481c988e329a3b9d30327d85f..a8d753ff1244c11f835243acb0528021132bc6f8 100644
--- a/spec/services/git/base_hooks_service_spec.rb
+++ b/spec/services/git/base_hooks_service_spec.rb
@@ -19,9 +19,13 @@ def hook_name
:push_hooks
end
- def commits
+ def limited_commits
[]
end
+
+ def commits_count
+ 0
+ end
end
end
diff --git a/spec/services/git/branch_hooks_service_spec.rb b/spec/services/git/branch_hooks_service_spec.rb
index a93f594b360bfbd8d98341b8f130bce963c9eb1f..79c2cb1fca3a58d144700789477732be62268d66 100644
--- a/spec/services/git/branch_hooks_service_spec.rb
+++ b/spec/services/git/branch_hooks_service_spec.rb
@@ -362,6 +362,9 @@ def clears_extended_cache
end
end
+ let(:commits_count) { service.send(:commits_count) }
+ let(:threshold_limit) { described_class::PROCESS_COMMIT_LIMIT + 1 }
+
let(:oldrev) { project.commit(commit_ids.first).parent_id }
let(:newrev) { commit_ids.last }
@@ -373,17 +376,31 @@ def clears_extended_cache
let(:oldrev) { Gitlab::Git::BLANK_SHA }
it 'processes a limited number of commit messages' do
+ expect(project.repository)
+ .to receive(:commits)
+ .with(newrev, limit: threshold_limit)
+ .and_call_original
+
expect(ProcessCommitWorker).to receive(:perform_async).twice
service.execute
+
+ expect(commits_count).to eq(project.repository.commit_count_for_ref(newrev))
end
end
context 'updating the default branch' do
it 'processes a limited number of commit messages' do
+ expect(project.repository)
+ .to receive(:commits_between)
+ .with(oldrev, newrev, limit: threshold_limit)
+ .and_call_original
+
expect(ProcessCommitWorker).to receive(:perform_async).twice
service.execute
+
+ expect(commits_count).to eq(project.repository.count_commits_between(oldrev, newrev))
end
end
@@ -391,9 +408,13 @@ def clears_extended_cache
let(:newrev) { Gitlab::Git::BLANK_SHA }
it 'does not process commit messages' do
+ expect(project.repository).not_to receive(:commits)
+ expect(project.repository).not_to receive(:commits_between)
expect(ProcessCommitWorker).not_to receive(:perform_async)
service.execute
+
+ expect(commits_count).to eq(0)
end
end
@@ -402,9 +423,16 @@ def clears_extended_cache
let(:oldrev) { Gitlab::Git::BLANK_SHA }
it 'processes a limited number of commit messages' do
+ expect(project.repository)
+ .to receive(:commits_between)
+ .with(project.default_branch, newrev, limit: threshold_limit)
+ .and_call_original
+
expect(ProcessCommitWorker).to receive(:perform_async).twice
service.execute
+
+ expect(commits_count).to eq(project.repository.count_commits_between(project.default_branch, branch))
end
end
@@ -412,9 +440,15 @@ def clears_extended_cache
let(:branch) { 'fix' }
it 'processes a limited number of commit messages' do
+ expect(project.repository)
+ .to receive(:commits_between)
+ .with(oldrev, newrev, limit: threshold_limit)
+ .and_call_original
+
expect(ProcessCommitWorker).to receive(:perform_async).twice
service.execute
+ expect(commits_count).to eq(project.repository.count_commits_between(oldrev, newrev))
end
end
@@ -423,9 +457,13 @@ def clears_extended_cache
let(:newrev) { Gitlab::Git::BLANK_SHA }
it 'does not process commit messages' do
+ expect(project.repository).not_to receive(:commits)
+ expect(project.repository).not_to receive(:commits_between)
expect(ProcessCommitWorker).not_to receive(:perform_async)
service.execute
+
+ expect(commits_count).to eq(0)
end
end