Commit f74cc6c5 authored by Jan Provaznik's avatar Jan Provaznik

Merge branch 'pb-refactor-post-receive' into 'master'

Refactor PostReceive to be more readable

See merge request gitlab-org/gitlab!17869
parents 1fbc48ef dff1e638
# frozen_string_literal: true
module Git
module ChangeParams
private
%i[oldrev newrev ref].each do |method|
define_method method do
change[method]
end
end
def change
@change ||= params.fetch(:change, {})
end
end
end
......@@ -3,6 +3,7 @@
module Git
class BaseHooksService < ::BaseService
include Gitlab::Utils::StrongMemoize
include ChangeParams
# The N most recent commits to process in a single push payload.
PROCESS_COMMIT_LIMIT = 100
......@@ -77,20 +78,20 @@ module Git
def pipeline_params
{
before: params[:oldrev],
after: params[:newrev],
ref: params[:ref],
before: oldrev,
after: newrev,
ref: ref,
push_options: params[:push_options] || {},
checkout_sha: Gitlab::DataBuilder::Push.checkout_sha(
project.repository, params[:newrev], params[:ref])
project.repository, newrev, ref)
}
end
def push_data_params(commits:, with_changed_files: true)
{
oldrev: params[:oldrev],
newrev: params[:newrev],
ref: params[:ref],
oldrev: oldrev,
newrev: newrev,
ref: ref,
project: project,
user: current_user,
commits: commits,
......
......@@ -20,15 +20,15 @@ module Git
strong_memoize(:commits) do
if creating_default_branch?
# The most recent PROCESS_COMMIT_LIMIT commits in the default branch
project.repository.commits(params[:newrev], limit: PROCESS_COMMIT_LIMIT)
project.repository.commits(newrev, limit: PROCESS_COMMIT_LIMIT)
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
# pushed, but that shouldn't matter because we check for existing
# cross-references later.
project.repository.commits_between(project.default_branch, params[:newrev])
project.repository.commits_between(project.default_branch, newrev)
elsif updating_branch?
project.repository.commits_between(params[:oldrev], params[:newrev])
project.repository.commits_between(oldrev, newrev)
else # removing branch
[]
end
......@@ -70,7 +70,7 @@ module Git
def branch_update_hooks
# Update the bare repositories info/attributes file using the contents of
# the default branch's .gitattributes file
project.repository.copy_gitattributes(params[:ref]) if default_branch?
project.repository.copy_gitattributes(ref) if default_branch?
end
def branch_change_hooks
......@@ -118,7 +118,7 @@ module Git
# https://gitlab.com/gitlab-org/gitlab-foss/issues/59257
def creating_branch?
strong_memoize(:creating_branch) do
Gitlab::Git.blank_ref?(params[:oldrev]) ||
Gitlab::Git.blank_ref?(oldrev) ||
!project.repository.branch_exists?(branch_name)
end
end
......@@ -128,7 +128,7 @@ module Git
end
def removing_branch?
Gitlab::Git.blank_ref?(params[:newrev])
Gitlab::Git.blank_ref?(newrev)
end
def creating_default_branch?
......@@ -137,7 +137,7 @@ module Git
def count_commits_in_branch
strong_memoize(:count_commits_in_branch) do
project.repository.commit_count_for_ref(params[:ref])
project.repository.commit_count_for_ref(ref)
end
end
......@@ -148,7 +148,7 @@ module Git
end
def branch_name
strong_memoize(:branch_name) { Gitlab::Git.ref_name(params[:ref]) }
strong_memoize(:branch_name) { Gitlab::Git.ref_name(ref) }
end
def upstream_commit_ids(commits)
......
......@@ -4,6 +4,7 @@ module Git
class BranchPushService < ::BaseService
include Gitlab::Access
include Gitlab::Utils::StrongMemoize
include ChangeParams
# This method will be called after each git update
# and only if the provided user and project are present in GitLab.
......@@ -19,7 +20,7 @@ module Git
# 6. Checks if the project's main language has changed
#
def execute
return unless Gitlab::Git.branch_ref?(params[:ref])
return unless Gitlab::Git.branch_ref?(ref)
enqueue_update_mrs
enqueue_detect_repository_languages
......@@ -38,9 +39,9 @@ module Git
UpdateMergeRequestsWorker.perform_async(
project.id,
current_user.id,
params[:oldrev],
params[:newrev],
params[:ref]
oldrev,
newrev,
ref
)
end
......@@ -69,11 +70,11 @@ module Git
end
def removing_branch?
Gitlab::Git.blank_ref?(params[:newrev])
Gitlab::Git.blank_ref?(newrev)
end
def branch_name
strong_memoize(:branch_name) { Gitlab::Git.ref_name(params[:ref]) }
strong_memoize(:branch_name) { Gitlab::Git.ref_name(ref) }
end
def default_branch?
......
......@@ -18,12 +18,12 @@ module Git
def tag
strong_memoize(:tag) do
next if Gitlab::Git.blank_ref?(params[:newrev])
next if Gitlab::Git.blank_ref?(newrev)
tag_name = Gitlab::Git.ref_name(params[:ref])
tag_name = Gitlab::Git.ref_name(ref)
tag = project.repository.find_tag(tag_name)
tag if tag && tag.target == params[:newrev]
tag if tag && tag.target == newrev
end
end
......
......@@ -2,8 +2,10 @@
module Git
class TagPushService < ::BaseService
include ChangeParams
def execute
return unless Gitlab::Git.tag_ref?(params[:ref])
return unless Gitlab::Git.tag_ref?(ref)
project.repository.before_push_tag
TagHooksService.new(project, current_user, params).execute
......
......@@ -37,41 +37,22 @@ class PostReceive
end
def process_project_changes(post_received)
changes = []
refs = Set.new
user = identify_user(post_received)
return false unless user
project = post_received.project
push_options = post_received.push_options
changes = post_received.changes
# We only need to expire certain caches once per push
expire_caches(post_received, post_received.project.repository)
enqueue_repository_cache_update(post_received)
post_received.enum_for(:changes_refs).with_index do |(oldrev, newrev, ref), index|
service_klass =
if Gitlab::Git.tag_ref?(ref)
Git::TagPushService
elsif Gitlab::Git.branch_ref?(ref)
Git::BranchPushService
end
if service_klass
service_klass.new(
post_received.project,
user,
oldrev: oldrev,
newrev: newrev,
ref: ref,
push_options: post_received.push_options,
create_pipelines: index < PIPELINE_PROCESS_LIMIT || Feature.enabled?(:git_push_create_all_pipelines, post_received.project)
).execute
end
changes << Gitlab::DataBuilder::Repository.single_change(oldrev, newrev, ref)
refs << ref
end
process_changes(Git::BranchPushService, project, user, push_options, changes.branch_changes)
process_changes(Git::TagPushService, project, user, push_options, changes.tag_changes)
update_remote_mirrors(post_received)
after_project_changes_hooks(post_received, user, refs.to_a, changes)
after_project_changes_hooks(project, user, changes.refs, changes.repository_data)
end
# Expire the repository status, branch, and tag cache once per push.
......@@ -94,6 +75,20 @@ class PostReceive
)
end
def process_changes(service_class, project, user, push_options, changes)
return if changes.empty?
changes.each do |change|
service_class.new(
project,
user,
change: change,
push_options: push_options,
create_pipelines: change[:index] < PIPELINE_PROCESS_LIMIT || Feature.enabled?(:git_push_create_all_pipelines, project)
).execute
end
end
def update_remote_mirrors(post_received)
return unless post_received.includes_branches? || post_received.includes_tags?
......@@ -104,9 +99,9 @@ class PostReceive
project.update_remote_mirrors
end
def after_project_changes_hooks(post_received, user, refs, changes)
hook_data = Gitlab::DataBuilder::Repository.update(post_received.project, user, changes, refs)
SystemHooksService.new.execute_hooks(hook_data, :repository_update_hooks)
def after_project_changes_hooks(project, user, refs, changes)
repository_update_hook_data = Gitlab::DataBuilder::Repository.update(project, user, changes, refs)
SystemHooksService.new.execute_hooks(repository_update_hook_data, :repository_update_hooks)
Gitlab::UsageDataCounters::SourceCodeCounter.count(:pushes)
end
......@@ -121,7 +116,7 @@ class PostReceive
# We only need to expire certain caches once per push
expire_caches(post_received, post_received.project.wiki.repository)
::Git::WikiPushService.new(post_received.project, user, changes: post_received.enum_for(:changes_refs)).execute
::Git::WikiPushService.new(post_received.project, user, changes: post_received.changes).execute
end
def log(message)
......
......@@ -150,9 +150,11 @@ class Gitlab::Seeder::CycleAnalytics
::Git::BranchPushService.new(
issue.project,
@user,
oldrev: issue.project.repository.commit("master").sha,
newrev: commit_sha,
ref: 'refs/heads/master'
change: {
oldrev: issue.project.repository.commit("master").sha,
newrev: commit_sha,
ref: 'refs/heads/master'
}
).execute
branch_name
......
......@@ -18,7 +18,7 @@ module EE
def enqueue_elasticsearch_indexing
return unless should_index_commits?
project.repository.index_commits_and_blobs(from_rev: params[:oldrev], to_rev: params[:newrev])
project.repository.index_commits_and_blobs(from_rev: oldrev, to_rev: newrev)
end
def enqueue_update_external_pull_requests
......@@ -28,7 +28,7 @@ module EE
UpdateExternalPullRequestsWorker.perform_async(
project.id,
current_user.id,
params[:ref]
ref
)
end
......
......@@ -9,11 +9,11 @@ module EE
return unless project.use_elasticsearch?
# Check if one of the changes we got was for the default branch. If it was, trigger an ES update
params[:changes].each do |_oldrev, newrev, ref|
branch_name = ::Gitlab::Git.ref_name(ref)
params[:changes].each do |change|
branch_name = ::Gitlab::Git.ref_name(change[:ref])
next unless project.wiki.default_branch == branch_name
project.wiki.index_wiki_blobs(newrev)
project.wiki.index_wiki_blobs(change[:newrev])
end
end
end
......
......@@ -83,12 +83,12 @@ module Projects
Git::TagPushService.new(
project,
current_user,
{
change: {
oldrev: old_tag_target,
newrev: tag_target,
ref: "#{Gitlab::Git::TAG_REF_PREFIX}#{tag.name}",
mirror_update: true
}
ref: "#{Gitlab::Git::TAG_REF_PREFIX}#{tag.name}"
},
mirror_update: true
).execute
end
......
......@@ -10,11 +10,9 @@ module EE
private
def after_project_changes_hooks(post_received, user, refs, changes)
def after_project_changes_hooks(project, user, refs, changes)
super
project = post_received.project
if audit_push?(project)
::RepositoryPushAuditEventWorker.perform_async(changes, project.id, user.id)
end
......
......@@ -87,9 +87,11 @@ class Gitlab::Seeder::ProductivityAnalytics
::Git::BranchPushService.new(
issue.project,
@user,
oldrev: issue.project.repository.commit("master").sha,
newrev: commit_sha,
ref: 'refs/heads/master'
change: {
oldrev: issue.project.repository.commit("master").sha,
newrev: commit_sha,
ref: 'refs/heads/master'
}
).execute
end
......
......@@ -12,7 +12,7 @@ describe Git::BranchPushService do
let(:ref) { 'refs/heads/master' }
let(:params) do
{ oldrev: oldrev, newrev: newrev, ref: ref }
{ change: { oldrev: oldrev, newrev: newrev, ref: ref } }
end
subject do
......@@ -60,16 +60,20 @@ describe Git::BranchPushService do
subject.execute
end
it "does not trigger indexer when push to non-default branch" do
expect_any_instance_of(Gitlab::Elastic::Indexer).not_to receive(:run)
it "triggers indexer when push to default branch" do
expect_any_instance_of(Gitlab::Elastic::Indexer).to receive(:run)
execute_service(project, user, oldrev, newrev, 'refs/heads/other')
subject.execute
end
it "triggers indexer when push to default branch" do
expect_any_instance_of(Gitlab::Elastic::Indexer).to receive(:run)
context 'when push to non-default branch' do
let(:ref) { 'refs/heads/other' }
execute_service(project, user, oldrev, newrev, ref)
it 'does not trigger indexer when push to non-default branch' do
expect_any_instance_of(Gitlab::Elastic::Indexer).not_to receive(:run)
subject.execute
end
end
context 'when limited indexing is on' do
......@@ -231,10 +235,4 @@ describe Git::BranchPushService do
it_behaves_like 'does not enqueue Jira sync worker'
end
end
def execute_service(project, user, oldrev, newrev, ref)
service = described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
service.execute
service
end
end
......@@ -26,7 +26,7 @@ describe Git::WikiPushService do
it 'triggers a wiki update' do
expect(project.wiki).to receive(:index_wiki_blobs).with("797823")
described_class.new(project, project.owner, changes: post_received.enum_for(:changes_refs)).execute
described_class.new(project, project.owner, changes: post_received.changes).execute
end
end
......@@ -36,7 +36,7 @@ describe Git::WikiPushService do
it 'does not trigger a wiki update' do
expect(project.wiki).not_to receive(:index_wiki_blobs)
described_class.new(project, project.owner, changes: post_received.enum_for(:changes_refs)).execute
described_class.new(project, project.owner, changes: post_received.changes).execute
end
end
end
......@@ -52,7 +52,7 @@ describe Git::WikiPushService do
it 'does nothing even if changes include master ref' do
expect(project.wiki).not_to receive(:index_wiki_blobs)
described_class.new(project, project.owner, changes: post_received.enum_for(:changes_refs)).execute
described_class.new(project, project.owner, changes: post_received.changes).execute
end
end
end
......
......@@ -65,7 +65,7 @@ describe Projects::UpdateMirrorService do
stub_fetch_mirror(project)
expect(Git::TagPushService).to receive(:new)
.with(project, project.owner, hash_including(ref: 'refs/tags/new-tag'))
.with(project, project.owner, change: hash_including(ref: 'refs/tags/new-tag'), mirror_update: true)
.and_return(double(execute: true))
service.execute
......
# frozen_string_literal: true
module Gitlab
module Git
class Changes
include Enumerable
attr_reader :repository_data
def initialize
@refs = Set.new
@items = []
@branches_index = []
@tags_index = []
@repository_data = []
end
def includes_branches?
branches_index.any?
end
def includes_tags?
tags_index.any?
end
def add_branch_change(change)
@branches_index << add_change(change)
self
end
def add_tag_change(change)
@tags_index << add_change(change)
self
end
def each
items.each do |item|
yield item
end
end
def refs
@refs.to_a
end
def branch_changes
items.values_at(*branches_index)
end
def tag_changes
items.values_at(*tags_index)
end
private
attr_reader :items, :branches_index, :tags_index
def add_change(change)
# refs and repository_data are being cached when a change is added to
# the collection to remove the need to iterate through changes multiple
# times.
@refs << change[:ref]
@repository_data << build_change_repository_data(change)
@items << change
@items.size - 1
end
def build_change_repository_data(change)
DataBuilder::Repository.single_change(change[:oldrev], change[:newrev], change[:ref])
end
end
end
end
......@@ -8,7 +8,7 @@ module Gitlab
def initialize(project, identifier, changes, push_options = {})
@project = project
@identifier = identifier
@changes = deserialize_changes(changes)
@changes = parse_changes(changes)
@push_options = push_options
end
......@@ -16,27 +16,12 @@ module Gitlab
super(identifier)
end
def changes_refs
return changes unless block_given?
changes.each do |change|
change.strip!
oldrev, newrev, ref = change.split(' ')
yield oldrev, newrev, ref
end
end
def includes_branches?
enum_for(:changes_refs).any? do |_oldrev, _newrev, ref|
Gitlab::Git.branch_ref?(ref)
end
changes.includes_branches?
end
def includes_tags?
enum_for(:changes_refs).any? do |_oldrev, _newrev, ref|
Gitlab::Git.tag_ref?(ref)
end
changes.includes_tags?
end
def includes_default_branch?
......@@ -44,16 +29,28 @@ module Gitlab
# first branch pushed will be the default.
return true unless project.default_branch.present?
enum_for(:changes_refs).any? do |_oldrev, _newrev, ref|
Gitlab::Git.branch_ref?(ref) &&
Gitlab::Git.branch_name(ref) == project.default_branch
changes.branch_changes.any? do |change|
Gitlab::Git.branch_name(change[:ref]) == project.default_branch
end
end
private
def deserialize_changes(changes)
utf8_encode_changes(changes).each_line
def parse_changes(changes)
deserialized_changes = utf8_encode_changes(changes).each_line
Git::Changes.new.tap do |collection|
deserialized_changes.each_with_index do |raw_change, index|
oldrev, newrev, ref = raw_change.strip.split(' ')
change = { index: index, oldrev: oldrev, newrev: newrev, ref: ref }
if Git.branch_ref?(ref)
collection.add_branch_change(change)
elsif Git.tag_ref?(ref)
collection.add_tag_change(change)
end
end
end
end
def utf8_encode_changes(changes)
......
......@@ -304,9 +304,11 @@ describe 'Environment' do
#
def remove_branch_with_hooks(project, user, branch)
params = {
oldrev: project.commit(branch).id,
newrev: Gitlab::Git::BLANK_SHA,
ref: "refs/heads/#{branch}"
change: {
oldrev: project.commit(branch).id,
newrev: Gitlab::Git::BLANK_SHA,
ref: "refs/heads/#{branch}"
}
}
yield
......
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::Git::Changes do
let(:changes) { described_class.new }
describe '#includes_branches?' do
subject { changes.includes_branches? }
context 'has changes for branches' do
before do
changes.add_branch_change(oldrev: 'abc123', newrev: 'def456', ref: 'branch')
end
it { is_expected.to be_truthy }
end
context 'has no changes for branches' do
before do
changes.add_tag_change(oldrev: 'abc123', newrev: 'def456', ref: 'tag')
end
it { is_expected.to be_falsey }
end
end
describe '#includes_tags?' do
subject { changes.includes_tags? }
context 'has changes for tags' do
before do
changes.add_tag_change(oldrev: 'abc123', newrev: 'def456', ref: 'tag')
end
it { is_expected.to be_truthy }
end
context 'has no changes for tags' do
before do
changes.add_branch_change(oldrev: 'abc123', newrev: 'def456', ref: 'branch')
end
it { is_expected.to be_falsey }
end
end
describe '#add_branch_change' do
let(:change) { { oldrev: 'abc123', newrev: 'def456', ref: 'branch' } }
subject { changes.add_branch_change(change) }
it 'adds the branch change to the collection' do
expect(subject).to include(change)
expect(subject.refs).to include(change[:ref])
expect(subject.repository_data).to include(before: change[:oldrev], after: change[:newrev], ref: change[:ref])
expect(subject.branch_changes).to include(change)
end
it 'does not add the change as a tag change' do
expect(subject.tag_changes).not_to include(change)
end
end
describe '#add_tag_change' do
let(:change) { { oldrev: 'abc123', newrev: 'def456', ref: 'tag' } }
subject { changes.add_tag_change(change) }
it 'adds the tag change to the collection' do
expect(subject).to include(change)
expect(subject.refs).to include(change[:ref])
expect(subject.repository_data).to include(before: change[:oldrev], after: change[:newrev], ref: change[:ref])
expect(subject.tag_changes).to include(change)
end
it 'does not add the change as a branch change' do
expect(subject.branch_changes).not_to include(change)
end
end
end
......@@ -8,7 +8,6 @@ describe Git::BaseHooksService do
let(:user) { create(:user) }
let(:project) { create(:project, :repository) }
let(:service) { described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref) }
let(:oldrev) { Gitlab::Git::BLANK_SHA }
let(:newrev) { "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b" } # gitlab-test: git rev-parse refs/tags/v1.1.0
......@@ -27,7 +26,7 @@ describe Git::BaseHooksService do
let(:project) { create(:project, :repository) }
subject { TestService.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref) }
subject { TestService.new(project, user, change: { oldrev: oldrev, newrev: newrev, ref: ref }) }
context '#execute_hooks' do
before do
......
......@@ -16,7 +16,7 @@ describe Git::BranchHooksService do
let(:newrev) { commit.id }
let(:service) do
described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
described_class.new(project, user, change: { oldrev: oldrev, newrev: newrev, ref: ref })
end
describe "Git Push Data" do
......@@ -350,7 +350,7 @@ describe Git::BranchHooksService do
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)
described_class.new(forked_project, user, change: { oldrev: oldrev, newrev: newrev, ref: ref })
end
context 'when commits already exists in the upstream project' do
......
......@@ -15,7 +15,7 @@ describe Git::TagHooksService, :service do
let(:commit) { tag.dereferenced_target }
let(:service) do
described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
described_class.new(project, user, change: { oldrev: oldrev, newrev: newrev, ref: ref })
end
describe 'System hooks' do
......
......@@ -8,7 +8,7 @@ describe Git::TagPushService do
let(:user) { create(:user) }
let(:project) { create(:project, :repository) }
let(:service) { described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref) }
let(:service) { described_class.new(project, user, change: { oldrev: oldrev, newrev: newrev, ref: ref }) }
let(:oldrev) { Gitlab::Git::BLANK_SHA }
let(:newrev) { "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b" } # gitlab-test: git rev-parse refs/tags/v1.1.0
......
......@@ -25,11 +25,15 @@ module CycleAnalyticsHelpers
return if skip_push_handler
Git::BranchPushService.new(project,
user,
oldrev: oldrev,
newrev: commit_shas.last,
ref: 'refs/heads/master').execute
Git::BranchPushService.new(
project,
user,
change: {
oldrev: oldrev,
newrev: commit_shas.last,
ref: 'refs/heads/master'
}
).execute
end
def create_cycle(user, project, issue, mr, milestone, pipeline)
......
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