diff --git a/app/controllers/projects/starrers_controller.rb b/app/controllers/projects/starrers_controller.rb
index c8facea1d70730dc21f965134761698541062c91..e4093bed0efbd19fb6b43e5febcc8065c42a985f 100644
--- a/app/controllers/projects/starrers_controller.rb
+++ b/app/controllers/projects/starrers_controller.rb
@@ -5,23 +5,9 @@ class Projects::StarrersController < Projects::ApplicationController
def index
@starrers = UsersStarProjectsFinder.new(@project, params, current_user: @current_user).execute
-
- # Normally the number of public starrers is equal to the number of visible
- # starrers. We need to fix the counts in two cases: when the current user
- # is an admin (and can see everything) and when the current user has a
- # private profile and has starred the project (and can see itself).
- @public_count =
- if @current_user&.admin?
- @starrers.with_public_profile.count
- elsif @current_user&.private_profile && has_starred_project?(@starrers)
- @starrers.size - 1
- else
- @starrers.size
- end
-
- @total_count = @project.starrers.size
+ @public_count = @project.starrers.with_public_profile.size
+ @total_count = @project.starrers.size
@private_count = @total_count - @public_count
-
@sort = params[:sort].presence || sort_value_name
@starrers = @starrers.sort_by_attribute(@sort).page(params[:page])
end
diff --git a/app/services/git/branch_hooks_service.rb b/app/services/git/branch_hooks_service.rb
index 313da7370768b53c30fd905a8290b5262a6120e2..9f9d12d6cf811626a2f24dbe1d60cfd14a736800 100644
--- a/app/services/git/branch_hooks_service.rb
+++ b/app/services/git/branch_hooks_service.rb
@@ -83,8 +83,16 @@ def branch_remove_hooks
# Schedules processing of commit messages
def enqueue_process_commit_messages
- limited_commits.each do |commit|
- next unless commit.matches_cross_reference_regex?
+ referencing_commits = limited_commits.select(&:matches_cross_reference_regex?)
+
+ upstream_commit_ids = upstream_commit_ids(referencing_commits)
+
+ referencing_commits.each do |commit|
+ # Avoid reprocessing commits that already exist upstream if the project
+ # is a fork. This will prevent duplicated/superfluous system notes on
+ # mentionables referenced by a commit that is pushed to the upstream,
+ # that is then also pushed to forks when these get synced by users.
+ next if upstream_commit_ids.include?(commit.id)
ProcessCommitWorker.perform_async(
project.id,
@@ -142,6 +150,19 @@ def default_branch?
def branch_name
strong_memoize(:branch_name) { Gitlab::Git.ref_name(params[:ref]) }
end
+
+ def upstream_commit_ids(commits)
+ set = Set.new
+
+ upstream_project = project.fork_source
+ if upstream_project
+ upstream_project
+ .commits_by(oids: commits.map(&:id))
+ .each { |commit| set << commit.id }
+ end
+
+ set
+ end
end
end
diff --git a/app/workers/process_commit_worker.rb b/app/workers/process_commit_worker.rb
index 3efb5343a9606c132f575ce6b8d51878936c0f06..f6ebe4ab006597af0057b5dca160d18041fe084f 100644
--- a/app/workers/process_commit_worker.rb
+++ b/app/workers/process_commit_worker.rb
@@ -2,7 +2,8 @@
# Worker for processing individual commit messages pushed to a repository.
#
-# Jobs for this worker are scheduled for every commit that is being pushed. As a
+# Jobs for this worker are scheduled for every commit that contains mentionable
+# references in its message and does not exist in the upstream project. As a
# result of this the workload of this worker should be kept to a bare minimum.
# Consider using an extra worker if you need to add any extra (and potentially
# slow) processing of commits.
@@ -19,7 +20,6 @@ def perform(project_id, user_id, commit_hash, default = false)
project = Project.find_by(id: project_id)
return unless project
- return if commit_exists_in_upstream?(project, commit_hash)
user = User.find_by(id: user_id)
@@ -77,17 +77,4 @@ def build_commit(project, hash)
Commit.from_hash(hash, project)
end
-
- private
-
- # Avoid reprocessing commits that already exist in the upstream
- # when project is forked. This will also prevent duplicated system notes.
- def commit_exists_in_upstream?(project, commit_hash)
- upstream_project = project.fork_source
-
- return false unless upstream_project
-
- commit_id = commit_hash.with_indifferent_access[:id]
- upstream_project.commit(commit_id).present?
- end
end
diff --git a/changelogs/unreleased/66023-starrers-count-do-not-match-after-searching.yml b/changelogs/unreleased/66023-starrers-count-do-not-match-after-searching.yml
new file mode 100644
index 0000000000000000000000000000000000000000..1caa5fa84ce9dbfd493208aee866ce979ee9de25
--- /dev/null
+++ b/changelogs/unreleased/66023-starrers-count-do-not-match-after-searching.yml
@@ -0,0 +1,5 @@
+---
+title: Fix starrers counts after searching
+merge_request: 31823
+author:
+type: fixed
diff --git a/changelogs/unreleased/dm-process-commit-worker-n-1.yml b/changelogs/unreleased/dm-process-commit-worker-n-1.yml
new file mode 100644
index 0000000000000000000000000000000000000000..0bd7de6730a8e1431193cae935d724bef4a3626a
--- /dev/null
+++ b/changelogs/unreleased/dm-process-commit-worker-n-1.yml
@@ -0,0 +1,5 @@
+---
+title: Look up upstream commits once before queuing ProcessCommitWorkers
+merge_request:
+author:
+type: performance
diff --git a/spec/controllers/projects/starrers_controller_spec.rb b/spec/controllers/projects/starrers_controller_spec.rb
index 59d258e99ceb75dcbbfb645386e5cbd9f63d4008..7085cba08d5124953d8b624b3519df01391a905c 100644
--- a/spec/controllers/projects/starrers_controller_spec.rb
+++ b/spec/controllers/projects/starrers_controller_spec.rb
@@ -3,23 +3,33 @@
require 'spec_helper'
describe Projects::StarrersController do
- let(:user) { create(:user) }
- let(:private_user) { create(:user, private_profile: true) }
+ let(:user_1) { create(:user, name: 'John') }
+ let(:user_2) { create(:user, name: 'Michael') }
+ let(:private_user) { create(:user, name: 'Michael Douglas', private_profile: true) }
let(:admin) { create(:user, admin: true) }
- let(:project) { create(:project, :public, :repository) }
+ let(:project) { create(:project, :public) }
before do
- user.toggle_star(project)
+ user_1.toggle_star(project)
+ user_2.toggle_star(project)
private_user.toggle_star(project)
end
describe 'GET index' do
- def get_starrers
- get :index,
- params: {
- namespace_id: project.namespace,
- project_id: project
- }
+ def get_starrers(search: nil)
+ get :index, params: { namespace_id: project.namespace, project_id: project, search: search }
+ end
+
+ def user_ids
+ assigns[:starrers].map { |s| s['user_id'] }
+ end
+
+ shared_examples 'starrers counts' do
+ it 'starrers counts are correct' do
+ expect(assigns[:total_count]).to eq(3)
+ expect(assigns[:public_count]).to eq(2)
+ expect(assigns[:private_count]).to eq(1)
+ end
end
context 'when project is public' do
@@ -28,55 +38,118 @@ def get_starrers
end
context 'when no user is logged in' do
+ context 'with no searching' do
+ before do
+ get_starrers
+ end
+
+ it 'only users with public profiles are visible' do
+ expect(user_ids).to contain_exactly(user_1.id, user_2.id)
+ end
+
+ include_examples 'starrers counts'
+ end
+
+ context 'when searching by user' do
+ before do
+ get_starrers(search: 'Michael')
+ end
+
+ it 'only users with public profiles are visible' do
+ expect(user_ids).to contain_exactly(user_2.id)
+ end
+
+ include_examples 'starrers counts'
+ end
+ end
+
+ context 'when public user is logged in' do
before do
- get_starrers
+ sign_in(user_1)
end
- it 'only public starrers are visible' do
- user_ids = assigns[:starrers].map { |s| s['user_id'] }
- expect(user_ids).to include(user.id)
- expect(user_ids).not_to include(private_user.id)
+ context 'with no searching' do
+ before do
+ get_starrers
+ end
+
+ it 'their star is also visible' do
+ expect(user_ids).to contain_exactly(user_1.id, user_2.id)
+ end
+
+ include_examples 'starrers counts'
end
- it 'public/private starrers counts are correct' do
- expect(assigns[:public_count]).to eq(1)
- expect(assigns[:private_count]).to eq(1)
+ context 'when searching by user' do
+ before do
+ get_starrers(search: 'Michael')
+ end
+
+ it 'only users with public profiles are visible' do
+ expect(user_ids).to contain_exactly(user_2.id)
+ end
+
+ include_examples 'starrers counts'
end
end
context 'when private user is logged in' do
before do
sign_in(private_user)
-
- get_starrers
end
- it 'their star is also visible' do
- user_ids = assigns[:starrers].map { |s| s['user_id'] }
- expect(user_ids).to include(user.id, private_user.id)
+ context 'with no searching' do
+ before do
+ get_starrers
+ end
+
+ it 'their star is also visible' do
+ expect(user_ids).to contain_exactly(user_1.id, user_2.id, private_user.id)
+ end
+
+ include_examples 'starrers counts'
end
- it 'public/private starrers counts are correct' do
- expect(assigns[:public_count]).to eq(1)
- expect(assigns[:private_count]).to eq(1)
+ context 'when searching by user' do
+ before do
+ get_starrers(search: 'Michael')
+ end
+
+ it 'only users with public profiles are visible' do
+ expect(user_ids).to contain_exactly(user_2.id, private_user.id)
+ end
+
+ include_examples 'starrers counts'
end
end
context 'when admin is logged in' do
before do
sign_in(admin)
-
- get_starrers
end
- it 'all stars are visible' do
- user_ids = assigns[:starrers].map { |s| s['user_id'] }
- expect(user_ids).to include(user.id, private_user.id)
+ context 'with no searching' do
+ before do
+ get_starrers
+ end
+
+ it 'all users are visible' do
+ expect(user_ids).to include(user_1.id, user_2.id, private_user.id)
+ end
+
+ include_examples 'starrers counts'
end
- it 'public/private starrers counts are correct' do
- expect(assigns[:public_count]).to eq(1)
- expect(assigns[:private_count]).to eq(1)
+ context 'when searching by user' do
+ before do
+ get_starrers(search: 'Michael')
+ end
+
+ it 'public and private starrers are visible' do
+ expect(user_ids).to contain_exactly(user_2.id, private_user.id)
+ end
+
+ include_examples 'starrers counts'
end
end
end
@@ -95,15 +168,14 @@ def get_starrers
context 'when user is logged in' do
before do
sign_in(project.creator)
- end
-
- it 'only public starrers are visible' do
get_starrers
+ end
- user_ids = assigns[:starrers].map { |s| s['user_id'] }
- expect(user_ids).to include(user.id)
- expect(user_ids).not_to include(private_user.id)
+ it 'only users with public profiles are visible' do
+ expect(user_ids).to contain_exactly(user_1.id, user_2.id)
end
+
+ include_examples 'starrers counts'
end
end
end
diff --git a/spec/services/git/branch_hooks_service_spec.rb b/spec/services/git/branch_hooks_service_spec.rb
index 3929f51a0e2ffa3034529e74f33c4ad77379e337..2bf7dc3243693a74096d68067567f0f08502a48d 100644
--- a/spec/services/git/branch_hooks_service_spec.rb
+++ b/spec/services/git/branch_hooks_service_spec.rb
@@ -4,6 +4,7 @@
describe Git::BranchHooksService do
include RepoHelpers
+ include ProjectForksHelper
let(:project) { create(:project, :repository) }
let(:user) { project.creator }
@@ -272,10 +273,10 @@ def clears_extended_cache
end
describe 'Processing commit messages' do
- # Create 4 commits, 2 of which have references. Limiting to 2 commits, we
- # expect to see one commit message processor enqueued.
- let(:commit_ids) do
- Array.new(4) do |i|
+ # Create 6 commits, 3 of which have references. Limiting to 4 commits, we
+ # expect to see two commit message processors enqueued.
+ let!(:commit_ids) do
+ Array.new(6) do |i|
message = "Issue #{'#' if i.even?}#{i}"
project.repository.update_file(
user, 'README.md', '', message: message, branch_name: branch
@@ -283,18 +284,18 @@ def clears_extended_cache
end
end
- let(:oldrev) { commit_ids.first }
+ let(:oldrev) { project.commit(commit_ids.first).parent_id }
let(:newrev) { commit_ids.last }
before do
- stub_const("::Git::BaseHooksService::PROCESS_COMMIT_LIMIT", 2)
+ stub_const("::Git::BaseHooksService::PROCESS_COMMIT_LIMIT", 4)
end
context 'creating the default branch' do
let(:oldrev) { Gitlab::Git::BLANK_SHA }
it 'processes a limited number of commit messages' do
- expect(ProcessCommitWorker).to receive(:perform_async).once
+ expect(ProcessCommitWorker).to receive(:perform_async).twice
service.execute
end
@@ -302,7 +303,7 @@ def clears_extended_cache
context 'updating the default branch' do
it 'processes a limited number of commit messages' do
- expect(ProcessCommitWorker).to receive(:perform_async).once
+ expect(ProcessCommitWorker).to receive(:perform_async).twice
service.execute
end
@@ -323,7 +324,7 @@ def clears_extended_cache
let(:oldrev) { Gitlab::Git::BLANK_SHA }
it 'processes a limited number of commit messages' do
- expect(ProcessCommitWorker).to receive(:perform_async).once
+ expect(ProcessCommitWorker).to receive(:perform_async).twice
service.execute
end
@@ -333,7 +334,7 @@ def clears_extended_cache
let(:branch) { 'fix' }
it 'processes a limited number of commit messages' do
- expect(ProcessCommitWorker).to receive(:perform_async).once
+ expect(ProcessCommitWorker).to receive(:perform_async).twice
service.execute
end
@@ -349,6 +350,55 @@ def clears_extended_cache
service.execute
end
end
+
+ context 'when the project is forked' do
+ let(:upstream_project) { project }
+ let(:forked_project) { fork_project(upstream_project, user, repository: true) }
+
+ let!(:forked_service) do
+ described_class.new(forked_project, user, oldrev: oldrev, newrev: newrev, ref: ref)
+ end
+
+ context 'when commits already exists in the upstream project' do
+ it 'does not process commit messages' do
+ expect(ProcessCommitWorker).not_to receive(:perform_async)
+
+ forked_service.execute
+ end
+ end
+
+ context 'when a commit does not exist in the upstream repo' do
+ # On top of the existing 6 commits, 3 of which have references,
+ # create 2 more, 1 of which has a reference. Limiting to 4 commits, we
+ # expect to see one commit message processor enqueued.
+ let!(:forked_commit_ids) do
+ Array.new(2) do |i|
+ message = "Issue #{'#' if i.even?}#{i}"
+ forked_project.repository.update_file(
+ user, 'README.md', '', message: message, branch_name: branch
+ )
+ end
+ end
+
+ let(:newrev) { forked_commit_ids.last }
+
+ it 'processes the commit message' do
+ expect(ProcessCommitWorker).to receive(:perform_async).once
+
+ forked_service.execute
+ end
+ end
+
+ context 'when the upstream project no longer exists' do
+ it 'processes the commit messages' do
+ upstream_project.destroy!
+
+ expect(ProcessCommitWorker).to receive(:perform_async).twice
+
+ forked_service.execute
+ end
+ end
+ end
end
describe 'New branch detection' do
diff --git a/spec/workers/process_commit_worker_spec.rb b/spec/workers/process_commit_worker_spec.rb
index 47bac63511eac31e2d2312963d161a47485d99c4..eb1d3c364ac7d6abafbd8decbee76b03ec9c6325 100644
--- a/spec/workers/process_commit_worker_spec.rb
+++ b/spec/workers/process_commit_worker_spec.rb
@@ -3,8 +3,6 @@
require 'spec_helper'
describe ProcessCommitWorker do
- include ProjectForksHelper
-
let(:worker) { described_class.new }
let(:user) { create(:user) }
let(:project) { create(:project, :public, :repository) }
@@ -35,44 +33,6 @@
worker.perform(project.id, user.id, commit.to_hash)
end
-
- context 'when the project is forked' do
- context 'when commit already exists in the upstream project' do
- it 'does not process the commit message' do
- forked = fork_project(project, user, repository: true)
-
- expect(worker).not_to receive(:process_commit_message)
-
- worker.perform(forked.id, user.id, forked.commit.to_hash)
- end
- end
-
- context 'when the commit does not exist in the upstream project' do
- it 'processes the commit message' do
- empty_project = create(:project, :public)
- forked = fork_project(empty_project, user, repository: true)
-
- TestEnv.copy_repo(forked,
- bare_repo: TestEnv.factory_repo_path_bare,
- refs: TestEnv::BRANCH_SHA)
-
- expect(worker).to receive(:process_commit_message)
-
- worker.perform(forked.id, user.id, forked.commit.to_hash)
- end
- end
-
- context 'when the upstream project no longer exists' do
- it 'processes the commit message' do
- forked = fork_project(project, user, repository: true)
- project.destroy!
-
- expect(worker).to receive(:process_commit_message)
-
- worker.perform(forked.id, user.id, forked.commit.to_hash)
- end
- end
- end
end
describe '#process_commit_message' do