Automatic merge of gitlab-org/gitlab-ce master

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
-
+    @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

app/services/git/branch_hooks_service.rb

@@ -83,8 +83,16 @@ module Git
 
     # 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 @@ module Git
     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
 

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 @@ class ProcessCommitWorker
     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 @@ class ProcessCommitWorker
 
     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

changelogs/unreleased/66023-starrers-count-do-not-match-after-searching.yml

+---
+title: Fix starrers counts after searching
+merge_request: 31823
+author:
+type: fixed

changelogs/unreleased/dm-process-commit-worker-n-1.yml

+---
+title: Look up upstream commits once before queuing ProcessCommitWorkers
+merge_request:
+author:
+type: performance

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 @@ describe Projects::StarrersController do
       end
 
       context 'when no user is logged in' do
+        context 'with no searching' do
           before do
             get_starrers
           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)
+          it 'only users with public profiles are visible' do
+            expect(user_ids).to contain_exactly(user_1.id, user_2.id)
           end
 
-        it 'public/private starrers counts are correct' do
-          expect(assigns[:public_count]).to eq(1)
-          expect(assigns[:private_count]).to eq(1)
+          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
+          sign_in(user_1)
+        end
+
+        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
+
+        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)
+        end
 
+        context 'with no searching' do
+          before do
             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)
+            expect(user_ids).to contain_exactly(user_1.id, user_2.id, private_user.id)
           end
 
-        it 'public/private starrers counts are correct' do
-          expect(assigns[:public_count]).to eq(1)
-          expect(assigns[:private_count]).to eq(1)
+          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, private_user.id)
+          end
+
+          include_examples 'starrers counts'
         end
       end
 
       context 'when admin is logged in' do
         before do
           sign_in(admin)
+        end
 
+        context 'with no searching' do
+          before do
             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)
+          it 'all users are visible' do
+            expect(user_ids).to include(user_1.id, user_2.id, private_user.id)
           end
 
-        it 'public/private starrers counts are correct' do
-          expect(assigns[:public_count]).to eq(1)
-          expect(assigns[:private_count]).to eq(1)
+          include_examples 'starrers counts'
+        end
+
+        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 @@ describe Projects::StarrersController do
       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

spec/services/git/branch_hooks_service_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 describe Git::BranchHooksService do
   include RepoHelpers
+  include ProjectForksHelper
 
   let(:project) { create(:project, :repository) }
   let(:user) { project.creator }
@@ -272,10 +273,10 @@ describe Git::BranchHooksService do
   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 @@ describe Git::BranchHooksService do
       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 @@ describe Git::BranchHooksService do
 
     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 @@ describe Git::BranchHooksService 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
@@ -333,7 +334,7 @@ describe Git::BranchHooksService do
       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 @@ describe Git::BranchHooksService do
         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

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 @@ describe ProcessCommitWorker do
 
       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