Merge branch '195667-use-limited-commits-for-event-hook-data' into 'master'

Always use the newest commit in a branch for push events

See merge request gitlab-org/gitlab!68168

app/services/git/base_hooks_service.rb

@@ -25,6 +25,7 @@ module Git
       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
@@ -132,10 +133,10 @@ module Git
     end
 
     def event_push_data
-      # We only need the last commit for the event push, and we don't
+      # We only need the newest commit for the event push, and we don't
       # need the full deltas either.
       @event_push_data ||= Gitlab::DataBuilder::Push.build(
-        **push_data_params(commits: commits.last, with_changed_files: false)
+        **push_data_params(commits: limited_commits.last, with_changed_files: false)
       )
     end
 

app/services/git/branch_hooks_service.rb

@@ -21,8 +21,9 @@ module Git
     def commits
       strong_memoize(:commits) do
         if creating_default_branch?
-          # The most recent PROCESS_COMMIT_LIMIT commits in the default branch
-          project.repository.commits(newrev, limit: PROCESS_COMMIT_LIMIT)
+          # 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
         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

doc/user/project/integrations/webhooks.md

@@ -94,7 +94,7 @@ Triggered when you push to the repository except when pushing tags.
 
 NOTE:
 When more than 20 commits are pushed at once, the `commits` webhook
-attribute only contains the first 20 for performance reasons. Loading
+attribute only contains the newest 20 for performance reasons. Loading
 detailed commit data is expensive. Note that despite only 20 commits being
 present in the `commits` attribute, the `total_commits_count` attribute contains the actual total.
 

spec/services/git/branch_hooks_service_spec.rb

@@ -92,7 +92,7 @@ RSpec.describe Git::BranchHooksService, :clean_gitlab_redis_shared_state do
   end
 
   describe 'Push Event' do
-    let(:event) { Event.pushed_action.first }
+    let(:event) { Event.pushed_action.take }
 
     subject(:execute_service) { service.execute }
 
@@ -171,7 +171,7 @@ RSpec.describe Git::BranchHooksService, :clean_gitlab_redis_shared_state do
       end
     end
 
-    context "with a new branch" do
+    context "with a new default branch" do
       let(:oldrev) { Gitlab::Git::BLANK_SHA }
 
       it 'generates a push event with more than one commit' do
@@ -183,12 +183,32 @@ RSpec.describe Git::BranchHooksService, :clean_gitlab_redis_shared_state do
         expect(event.push_event_payload).to be_an_instance_of(PushEventPayload)
         expect(event.push_event_payload.commit_from).to be_nil
         expect(event.push_event_payload.commit_to).to eq(newrev)
-        expect(event.push_event_payload.commit_title).to eq('Initial commit')
+        expect(event.push_event_payload.commit_title).to eq('Change some files')
         expect(event.push_event_payload.ref).to eq('master')
         expect(event.push_event_payload.commit_count).to be > 1
       end
     end
 
+    context "with a new non-default branch" do
+      let(:oldrev) { Gitlab::Git::BLANK_SHA }
+      let(:branch) { 'fix' }
+      let(:commit_id) { project.commit(branch).id }
+
+      it 'generates a push event with more than one commit' do
+        execute_service
+
+        expect(event).to be_an_instance_of(PushEvent)
+        expect(event.project).to eq(project)
+        expect(event).to be_pushed_action
+        expect(event.push_event_payload).to be_an_instance_of(PushEventPayload)
+        expect(event.push_event_payload.commit_from).to be_nil
+        expect(event.push_event_payload.commit_to).to eq(newrev)
+        expect(event.push_event_payload.commit_title).to eq('Test file for directories with a leading dot')
+        expect(event.push_event_payload.ref).to eq('fix')
+        expect(event.push_event_payload.commit_count).to be > 1
+      end
+    end
+
     context 'removing a branch' do
       let(:newrev) { Gitlab::Git::BLANK_SHA }