Always use the newest commit in a branch for push events

Prior to this commit, when creating a new default branch, the push
event would reference the *oldest* commit in the branch, rather than
the newest. This is the opposite of what we do when creating any other
branch, or when updating any branch, whether it's the default branch or
not.

Even worse, when the new default branch contained more than 100 commits
we would return the 100th commit, rather than the oldest. This is
unambiguously a bug - that behaviour is not useful at all.

Fix the ordering of commits when pulling data from the default branch
so we always use the newest one when creating the default branch.

Changelog: fixed

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 }