Commit 74aa23c3 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'fix-33259' into 'master'

Fix GitHub importer performance on branch existence check

Closes #33259

See merge request !12324
parents 043f8c26 be108ae6
---
title: Fix GitHub importer performance on branch existence check
merge_request:
author:
...@@ -172,7 +172,7 @@ module Github ...@@ -172,7 +172,7 @@ module Github
next unless merge_request.new_record? && pull_request.valid? next unless merge_request.new_record? && pull_request.valid?
begin begin
restore_branches(pull_request) pull_request.restore_branches!
author_id = user_id(pull_request.author, project.creator_id) author_id = user_id(pull_request.author, project.creator_id)
description = format_description(pull_request.description, pull_request.author) description = format_description(pull_request.description, pull_request.author)
...@@ -208,7 +208,7 @@ module Github ...@@ -208,7 +208,7 @@ module Github
rescue => e rescue => e
error(:pull_request, pull_request.url, e.message) error(:pull_request, pull_request.url, e.message)
ensure ensure
clean_up_restored_branches(pull_request) pull_request.remove_restored_branches!
end end
end end
...@@ -325,32 +325,6 @@ module Github ...@@ -325,32 +325,6 @@ module Github
end end
end end
def restore_branches(pull_request)
restore_source_branch(pull_request) unless pull_request.source_branch_exists?
restore_target_branch(pull_request) unless pull_request.target_branch_exists?
end
def restore_source_branch(pull_request)
repository.create_branch(pull_request.source_branch_name, pull_request.source_branch_sha)
end
def restore_target_branch(pull_request)
repository.create_branch(pull_request.target_branch_name, pull_request.target_branch_sha)
end
def remove_branch(name)
repository.delete_branch(name)
rescue Rugged::ReferenceError
errors << { type: :branch, url: nil, error: "Could not clean up restored branch: #{name}" }
end
def clean_up_restored_branches(pull_request)
return if pull_request.opened?
remove_branch(pull_request.source_branch_name) unless pull_request.source_branch_exists?
remove_branch(pull_request.target_branch_name) unless pull_request.target_branch_exists?
end
def label_ids(labels) def label_ids(labels)
labels.map { |attrs| cached[:label_ids][attrs.fetch('name')] }.compact labels.map { |attrs| cached[:label_ids][attrs.fetch('name')] }.compact
end end
......
...@@ -26,13 +26,25 @@ module Github ...@@ -26,13 +26,25 @@ module Github
end end
def exists? def exists?
branch_exists? && commit_exists? @exists ||= branch_exists? && commit_exists?
end end
def valid? def valid?
sha.present? && ref.present? sha.present? && ref.present?
end end
def restore!(name)
repository.create_branch(name, sha)
rescue Gitlab::Git::Repository::InvalidRef => e
Rails.logger.error("#{self.class.name}: Could not restore branch #{name}: #{e}")
end
def remove!(name)
repository.delete_branch(name)
rescue Rugged::ReferenceError => e
Rails.logger.error("#{self.class.name}: Could not remove branch #{name}: #{e}")
end
private private
def branch_exists? def branch_exists?
......
module Github module Github
module Representation module Representation
class PullRequest < Representation::Issuable class PullRequest < Representation::Issuable
attr_reader :project
delegate :user, :repo, :ref, :sha, to: :source_branch, prefix: true delegate :user, :repo, :ref, :sha, to: :source_branch, prefix: true
delegate :user, :exists?, :repo, :ref, :sha, :short_sha, to: :target_branch, prefix: true delegate :user, :exists?, :repo, :ref, :sha, :short_sha, to: :target_branch, prefix: true
...@@ -10,10 +8,6 @@ module Github ...@@ -10,10 +8,6 @@ module Github
project project
end end
def source_branch_exists?
!cross_project? && source_branch.exists?
end
def source_branch_name def source_branch_name
@source_branch_name ||= @source_branch_name ||=
if cross_project? || !source_branch_exists? if cross_project? || !source_branch_exists?
...@@ -23,6 +17,12 @@ module Github ...@@ -23,6 +17,12 @@ module Github
end end
end end
def source_branch_exists?
return @source_branch_exists if defined?(@source_branch_exists)
@source_branch_exists = !cross_project? && source_branch.exists?
end
def target_project def target_project
project project
end end
...@@ -31,6 +31,10 @@ module Github ...@@ -31,6 +31,10 @@ module Github
@target_branch_name ||= target_branch_exists? ? target_branch_ref : target_branch_name_prefixed @target_branch_name ||= target_branch_exists? ? target_branch_ref : target_branch_name_prefixed
end end
def target_branch_exists?
@target_branch_exists ||= target_branch.exists?
end
def state def state
return 'merged' if raw['state'] == 'closed' && raw['merged_at'].present? return 'merged' if raw['state'] == 'closed' && raw['merged_at'].present?
return 'closed' if raw['state'] == 'closed' return 'closed' if raw['state'] == 'closed'
...@@ -46,6 +50,18 @@ module Github ...@@ -46,6 +50,18 @@ module Github
source_branch.valid? && target_branch.valid? source_branch.valid? && target_branch.valid?
end end
def restore_branches!
restore_source_branch!
restore_target_branch!
end
def remove_restored_branches!
return if opened?
remove_source_branch!
remove_target_branch!
end
private private
def project def project
...@@ -73,6 +89,32 @@ module Github ...@@ -73,6 +89,32 @@ module Github
source_branch_repo.id != target_branch_repo.id source_branch_repo.id != target_branch_repo.id
end end
def restore_source_branch!
return if source_branch_exists?
source_branch.restore!(source_branch_name)
end
def restore_target_branch!
return if target_branch_exists?
target_branch.restore!(target_branch_name)
end
def remove_source_branch!
# We should remove the source/target branches only if they were
# restored. Otherwise, we'll remove branches like 'master' that
# target_branch_exists? returns true. In other words, we need
# to clean up only the restored branches that (source|target)_branch_exists?
# returns false for the first time it has been called, because of
# this that is important to memoize these values.
source_branch.remove!(source_branch_name) unless source_branch_exists?
end
def remove_target_branch!
target_branch.remove!(target_branch_name) unless target_branch_exists?
end
end end
end end
end end
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