Commit bd91f644 authored by Stan Hu's avatar Stan Hu

Fix wrong commit count in push event payload

This fixes a regression in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/20916.
We now only use the full commit count for the initial push to the default branch.
Otherwise, we rely on the number of commits that we calculated occurred in the push.

The original behavior in 11.1.4:

1. When a new branch is pushed, `@push_commits_count` was set to the total
number of refs available for the branch.
2. For other branches, `@push_commits_count` would remain `nil`.
3. `GitPushService#build_push_data` would build the push data with
`@push_commits_count`.
4. If this were `nil`, it would be set to the right value here:
https://gitlab.com/gitlab-org/gitlab-ce/blob/v11.1.4/lib/gitlab/data_builder/push.rb#L60

Broken behavior:

1. `GitPushService#push_commits_count` is always called.
2. The total number of commits is therefore always equal to the total number of refs available.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/49971
parent 0e9dc23d
...@@ -158,7 +158,7 @@ class GitPushService < BaseService ...@@ -158,7 +158,7 @@ class GitPushService < BaseService
end end
def process_default_branch def process_default_branch
offset = [push_commits_count - PROCESS_COMMIT_LIMIT, 0].max offset = [push_commits_count_for_ref - PROCESS_COMMIT_LIMIT, 0].max
@push_commits = project.repository.commits(params[:newrev], offset: offset, limit: PROCESS_COMMIT_LIMIT) @push_commits = project.repository.commits(params[:newrev], offset: offset, limit: PROCESS_COMMIT_LIMIT)
project.after_create_default_branch project.after_create_default_branch
...@@ -172,7 +172,7 @@ class GitPushService < BaseService ...@@ -172,7 +172,7 @@ class GitPushService < BaseService
params[:newrev], params[:newrev],
params[:ref], params[:ref],
@push_commits, @push_commits,
commits_count: push_commits_count) commits_count: commits_count)
end end
def push_to_existing_branch? def push_to_existing_branch?
...@@ -213,8 +213,14 @@ class GitPushService < BaseService ...@@ -213,8 +213,14 @@ class GitPushService < BaseService
end end
end end
def push_commits_count def commits_count
strong_memoize(:push_commits_count) do return push_commits_count_for_ref if default_branch? && push_to_new_branch?
Array(@push_commits).size
end
def push_commits_count_for_ref
strong_memoize(:push_commits_count_for_ref) do
project.repository.commit_count_for_ref(params[:ref]) project.repository.commit_count_for_ref(params[:ref])
end end
end end
......
---
title: Fix wrong commit count in push event payload
merge_request: 21338
author:
type: fixed
...@@ -204,16 +204,37 @@ describe GitPushService, services: true do ...@@ -204,16 +204,37 @@ describe GitPushService, services: true do
end end
describe "Push Event" do describe "Push Event" do
let!(:push_data) { push_data_from_service(project, user, oldrev, newrev, ref) } context "with an existing branch" do
let(:event) { Event.find_by_action(Event::PUSHED) } let!(:push_data) { push_data_from_service(project, user, oldrev, newrev, ref) }
let(:event) { Event.find_by_action(Event::PUSHED) }
it { expect(event).to be_an_instance_of(PushEvent) } it 'generates a push event with one commit' do
it { expect(event.project).to eq(project) } expect(event).to be_an_instance_of(PushEvent)
it { expect(event.action).to eq(Event::PUSHED) } expect(event.project).to eq(project)
it { expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) } expect(event.action).to eq(Event::PUSHED)
it { expect(event.push_event_payload.commit_from).to eq(oldrev) } expect(event.push_event_payload).to be_an_instance_of(PushEventPayload)
it { expect(event.push_event_payload.commit_to).to eq(newrev) } expect(event.push_event_payload.commit_from).to eq(oldrev)
it { expect(event.push_event_payload.ref).to eq('master') } expect(event.push_event_payload.commit_to).to eq(newrev)
expect(event.push_event_payload.ref).to eq('master')
expect(event.push_event_payload.commit_count).to eq(1)
end
end
context "with a new branch" do
let!(:new_branch_data) { push_data_from_service(project, user, Gitlab::Git::BLANK_SHA, newrev, ref) }
let(:event) { Event.find_by_action(Event::PUSHED) }
it 'generates a push event with more than one commit' do
expect(event).to be_an_instance_of(PushEvent)
expect(event.project).to eq(project)
expect(event.action).to eq(Event::PUSHED)
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.ref).to eq('master')
expect(event.push_event_payload.commit_count).to be > 1
end
end
context "Updates merge requests" do context "Updates merge requests" do
it "when pushing a new branch for the first time" do it "when pushing a new branch for the first time" do
...@@ -224,6 +245,7 @@ describe GitPushService, services: true do ...@@ -224,6 +245,7 @@ describe GitPushService, services: true do
end end
describe 'system hooks' do describe 'system hooks' do
let!(:push_data) { push_data_from_service(project, user, oldrev, newrev, ref) }
let(:system_hooks_service) { SystemHooksService.new } let(:system_hooks_service) { SystemHooksService.new }
it "sends a system hook after pushing a branch" do it "sends a system hook after pushing a branch" do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment