Commit 5dd013f1 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'gh-pull-requests'

parents 78a67fc4 bd1a6208
...@@ -52,6 +52,7 @@ v 8.8.0 (unreleased) ...@@ -52,6 +52,7 @@ v 8.8.0 (unreleased)
- Hide left sidebar on phone screens to give more space for content - Hide left sidebar on phone screens to give more space for content
- Redesign navigation for profile and group pages - Redesign navigation for profile and group pages
- Add counter metrics for rails cache - Add counter metrics for rails cache
- Import pull requests from GitHub where the source or target branches were removed
v 8.7.5 v 8.7.5
- Fix relative links in wiki pages. !4050 - Fix relative links in wiki pages. !4050
......
...@@ -26,6 +26,10 @@ class MergeRequest < ActiveRecord::Base ...@@ -26,6 +26,10 @@ class MergeRequest < ActiveRecord::Base
# when creating new merge request # when creating new merge request
attr_accessor :can_be_created, :compare_commits, :compare attr_accessor :can_be_created, :compare_commits, :compare
# Temporary fields to store target_sha, and base_sha to
# compare when importing pull requests from GitHub
attr_accessor :base_target_sha, :head_source_sha
state_machine :state, initial: :opened do state_machine :state, initial: :opened do
event :close do event :close do
transition [:reopened, :opened] => :closed transition [:reopened, :opened] => :closed
...@@ -490,10 +494,14 @@ class MergeRequest < ActiveRecord::Base ...@@ -490,10 +494,14 @@ class MergeRequest < ActiveRecord::Base
end end
def target_sha def target_sha
@target_sha ||= target_project.repository.commit(target_branch).try(:sha) return @base_target_sha if defined?(@base_target_sha)
target_project.repository.commit(target_branch).try(:sha)
end end
def source_sha def source_sha
return @head_source_sha if defined?(@head_source_sha)
last_commit.try(:sha) || source_tip.try(:sha) last_commit.try(:sha) || source_tip.try(:sha)
end end
......
...@@ -6,7 +6,7 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -6,7 +6,7 @@ class MergeRequestDiff < ActiveRecord::Base
belongs_to :merge_request belongs_to :merge_request
delegate :target_branch, :source_branch, to: :merge_request, prefix: nil delegate :head_source_sha, :target_branch, :source_branch, to: :merge_request, prefix: nil
state_machine :state, initial: :empty do state_machine :state, initial: :empty do
state :collected state :collected
...@@ -38,8 +38,8 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -38,8 +38,8 @@ class MergeRequestDiff < ActiveRecord::Base
@diffs_no_whitespace ||= begin @diffs_no_whitespace ||= begin
compare = Gitlab::Git::Compare.new( compare = Gitlab::Git::Compare.new(
self.repository.raw_repository, self.repository.raw_repository,
self.target_branch, self.base,
self.source_sha, self.head,
) )
compare.diffs(options) compare.diffs(options)
end end
...@@ -144,7 +144,7 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -144,7 +144,7 @@ class MergeRequestDiff < ActiveRecord::Base
self.st_diffs = new_diffs self.st_diffs = new_diffs
self.base_commit_sha = self.repository.merge_base(self.source_sha, self.target_branch) self.base_commit_sha = self.repository.merge_base(self.head, self.base)
self.save self.save
end end
...@@ -160,10 +160,24 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -160,10 +160,24 @@ class MergeRequestDiff < ActiveRecord::Base
end end
def source_sha def source_sha
return head_source_sha if head_source_sha.present?
source_commit = merge_request.source_project.commit(source_branch) source_commit = merge_request.source_project.commit(source_branch)
source_commit.try(:sha) source_commit.try(:sha)
end end
def target_sha
merge_request.target_sha
end
def base
self.target_sha || self.target_branch
end
def head
self.source_sha
end
def compare def compare
@compare ||= @compare ||=
begin begin
...@@ -172,8 +186,8 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -172,8 +186,8 @@ class MergeRequestDiff < ActiveRecord::Base
Gitlab::Git::Compare.new( Gitlab::Git::Compare.new(
self.repository.raw_repository, self.repository.raw_repository,
self.target_branch, self.base,
self.source_sha self.head
) )
end end
end end
......
...@@ -195,6 +195,10 @@ class Repository ...@@ -195,6 +195,10 @@ class Repository
cache.fetch(:branch_names) { branches.map(&:name) } cache.fetch(:branch_names) { branches.map(&:name) }
end end
def branch_exists?(branch_name)
branch_names.include?(branch_name)
end
def tag_names def tag_names
cache.fetch(:tag_names) { raw_repository.tag_names } cache.fetch(:tag_names) { raw_repository.tag_names }
end end
......
module Gitlab
module GithubImport
class BranchFormatter < BaseFormatter
delegate :repo, :sha, :ref, to: :raw_data
def exists?
project.repository.branch_exists?(ref)
end
def name
@name ||= exists? ? ref : "#{ref}-#{short_id}"
end
def valid?
repo.present?
end
def valid?
repo.present?
end
private
def short_id
sha.to_s[0..7]
end
end
end
end
...@@ -3,12 +3,15 @@ module Gitlab ...@@ -3,12 +3,15 @@ module Gitlab
class Importer class Importer
include Gitlab::ShellAdapter include Gitlab::ShellAdapter
attr_reader :project, :client attr_reader :client, :project, :repo, :repo_url
def initialize(project) def initialize(project)
@project = project @project = project
if import_data_credentials @repo = project.import_source
@client = Client.new(import_data_credentials[:user]) @repo_url = project.import_url
if credentials
@client = Client.new(credentials[:user])
@formatter = Gitlab::ImportFormatter.new @formatter = Gitlab::ImportFormatter.new
else else
raise Projects::ImportService::Error, "Unable to find project import data credentials for project ID: #{@project.id}" raise Projects::ImportService::Error, "Unable to find project import data credentials for project ID: #{@project.id}"
...@@ -22,12 +25,12 @@ module Gitlab ...@@ -22,12 +25,12 @@ module Gitlab
private private
def import_data_credentials def credentials
@import_data_credentials ||= project.import_data.credentials if project.import_data @credentials ||= project.import_data.credentials if project.import_data
end end
def import_labels def import_labels
client.labels(project.import_source).each do |raw_data| client.labels(repo).each do |raw_data|
Label.create!(LabelFormatter.new(project, raw_data).attributes) Label.create!(LabelFormatter.new(project, raw_data).attributes)
end end
...@@ -37,7 +40,7 @@ module Gitlab ...@@ -37,7 +40,7 @@ module Gitlab
end end
def import_milestones def import_milestones
client.list_milestones(project.import_source, state: :all).each do |raw_data| client.list_milestones(repo, state: :all).each do |raw_data|
Milestone.create!(MilestoneFormatter.new(project, raw_data).attributes) Milestone.create!(MilestoneFormatter.new(project, raw_data).attributes)
end end
...@@ -47,9 +50,7 @@ module Gitlab ...@@ -47,9 +50,7 @@ module Gitlab
end end
def import_issues def import_issues
client.list_issues(project.import_source, state: :all, client.list_issues(repo, state: :all, sort: :created, direction: :asc).each do |raw_data|
sort: :created,
direction: :asc).each do |raw_data|
gh_issue = IssueFormatter.new(project, raw_data) gh_issue = IssueFormatter.new(project, raw_data)
if gh_issue.valid? if gh_issue.valid?
...@@ -68,12 +69,17 @@ module Gitlab ...@@ -68,12 +69,17 @@ module Gitlab
end end
def import_pull_requests def import_pull_requests
client.pull_requests(project.import_source, state: :all, pull_requests = client.pull_requests(repo, state: :all, sort: :created, direction: :asc)
sort: :created, .map { |raw| PullRequestFormatter.new(project, raw) }
direction: :asc).each do |raw_data| .select(&:valid?)
pull_request = PullRequestFormatter.new(project, raw_data)
source_branches_removed = pull_requests.reject(&:source_branch_exists?).map { |pr| [pr.source_branch_name, pr.source_branch_sha] }
target_branches_removed = pull_requests.reject(&:target_branch_exists?).map { |pr| [pr.target_branch_name, pr.target_branch_sha] }
branches_removed = source_branches_removed | target_branches_removed
create_refs(branches_removed)
if pull_request.valid? pull_requests.each do |pull_request|
merge_request = MergeRequest.new(pull_request.attributes) merge_request = MergeRequest.new(pull_request.attributes)
if merge_request.save if merge_request.save
...@@ -82,15 +88,31 @@ module Gitlab ...@@ -82,15 +88,31 @@ module Gitlab
import_comments_on_diff(pull_request.number, merge_request) import_comments_on_diff(pull_request.number, merge_request)
end end
end end
end
delete_refs(branches_removed)
true true
rescue ActiveRecord::RecordInvalid => e rescue ActiveRecord::RecordInvalid => e
raise Projects::ImportService::Error, e.message raise Projects::ImportService::Error, e.message
end end
def create_refs(branches)
branches.each do |name, sha|
client.create_ref(repo, "refs/heads/#{name}", sha)
end
project.repository.fetch_ref(repo_url, '+refs/heads/*', 'refs/heads/*')
end
def delete_refs(branches)
branches.each do |name, _|
client.delete_ref(repo, "heads/#{name}")
project.repository.rm_branch(project.creator, name)
end
end
def apply_labels(number, issuable) def apply_labels(number, issuable)
issue = client.issue(project.import_source, number) issue = client.issue(repo, number)
if issue.labels.count > 0 if issue.labels.count > 0
label_ids = issue.labels.map do |raw| label_ids = issue.labels.map do |raw|
...@@ -102,12 +124,12 @@ module Gitlab ...@@ -102,12 +124,12 @@ module Gitlab
end end
def import_comments(issue_number, noteable) def import_comments(issue_number, noteable)
comments = client.issue_comments(project.import_source, issue_number) comments = client.issue_comments(repo, issue_number)
create_comments(comments, noteable) create_comments(comments, noteable)
end end
def import_comments_on_diff(pull_request_number, merge_request) def import_comments_on_diff(pull_request_number, merge_request)
comments = client.pull_request_comments(project.import_source, pull_request_number) comments = client.pull_request_comments(repo, pull_request_number)
create_comments(comments, merge_request) create_comments(comments, merge_request)
end end
......
module Gitlab module Gitlab
module GithubImport module GithubImport
class PullRequestFormatter < BaseFormatter class PullRequestFormatter < BaseFormatter
delegate :exists?, :name, :project, :repo, :sha, to: :source_branch, prefix: true
delegate :exists?, :name, :project, :repo, :sha, to: :target_branch, prefix: true
def attributes def attributes
{ {
iid: number, iid: number,
title: raw_data.title, title: raw_data.title,
description: description, description: description,
source_project: source_project, source_project: source_branch_project,
source_branch: source_branch.name, source_branch: source_branch_name,
target_project: target_project, head_source_sha: source_branch_sha,
target_branch: target_branch.name, target_project: target_branch_project,
target_branch: target_branch_name,
base_target_sha: target_branch_sha,
state: state, state: state,
milestone: milestone, milestone: milestone,
author_id: author_id, author_id: author_id,
...@@ -24,7 +29,15 @@ module Gitlab ...@@ -24,7 +29,15 @@ module Gitlab
end end
def valid? def valid?
!cross_project? && source_branch.present? && target_branch.present? source_branch.valid? && target_branch.valid? && !cross_project?
end
def source_branch
@source_branch ||= BranchFormatter.new(project, raw_data.head)
end
def target_branch
@target_branch ||= BranchFormatter.new(project, raw_data.base)
end end
private private
...@@ -52,7 +65,7 @@ module Gitlab ...@@ -52,7 +65,7 @@ module Gitlab
end end
def cross_project? def cross_project?
source_repo.present? && target_repo.present? && source_repo.id != target_repo.id source_branch_repo.id != target_branch_repo.id
end end
def description def description
...@@ -65,30 +78,6 @@ module Gitlab ...@@ -65,30 +78,6 @@ module Gitlab
end end
end end
def source_project
project
end
def source_repo
raw_data.head.repo
end
def source_branch
source_project.repository.find_branch(raw_data.head.ref)
end
def target_project
project
end
def target_repo
raw_data.base.repo
end
def target_branch
target_project.repository.find_branch(raw_data.base.ref)
end
def state def state
@state ||= case true @state ||= case true
when raw_data.state == 'closed' && raw_data.merged_at.present? when raw_data.state == 'closed' && raw_data.merged_at.present?
......
require 'spec_helper'
describe Gitlab::GithubImport::BranchFormatter, lib: true do
let(:project) { create(:project) }
let(:repo) { double }
let(:raw) do
{
ref: 'feature',
repo: repo,
sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b'
}
end
describe '#exists?' do
it 'returns true when branch exists' do
branch = described_class.new(project, double(raw))
expect(branch.exists?).to eq true
end
it 'returns false when branch does not exist' do
branch = described_class.new(project, double(raw.merge(ref: 'removed-branch')))
expect(branch.exists?).to eq false
end
end
describe '#name' do
it 'returns raw ref when branch exists' do
branch = described_class.new(project, double(raw))
expect(branch.name).to eq 'feature'
end
it 'returns formatted ref when branch does not exist' do
branch = described_class.new(project, double(raw.merge(ref: 'removed-branch')))
expect(branch.name).to eq 'removed-branch-2e5d3239'
end
end
describe '#repo' do
it 'returns raw repo' do
branch = described_class.new(project, double(raw))
expect(branch.repo).to eq repo
end
end
describe '#sha' do
it 'returns raw sha' do
branch = described_class.new(project, double(raw))
expect(branch.sha).to eq '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b'
end
end
describe '#valid?' do
it 'returns true when repository exists' do
branch = described_class.new(project, double(raw))
expect(branch.valid?).to eq true
end
it 'returns false when repository does not exist' do
branch = described_class.new(project, double(raw.merge(repo: nil)))
expect(branch.valid?).to eq false
end
end
end
...@@ -4,9 +4,9 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do ...@@ -4,9 +4,9 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:repository) { double(id: 1, fork: false) } let(:repository) { double(id: 1, fork: false) }
let(:source_repo) { repository } let(:source_repo) { repository }
let(:source_branch) { double(ref: 'feature', repo: source_repo) } let(:source_branch) { double(ref: 'feature', repo: source_repo, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b') }
let(:target_repo) { repository } let(:target_repo) { repository }
let(:target_branch) { double(ref: 'master', repo: target_repo) } let(:target_branch) { double(ref: 'master', repo: target_repo, sha: '8ffb3c15a5475e59ae909384297fede4badcb4c7') }
let(:octocat) { double(id: 123456, login: 'octocat') } let(:octocat) { double(id: 123456, login: 'octocat') }
let(:created_at) { DateTime.strptime('2011-01-26T19:01:12Z') } let(:created_at) { DateTime.strptime('2011-01-26T19:01:12Z') }
let(:updated_at) { DateTime.strptime('2011-01-27T19:01:12Z') } let(:updated_at) { DateTime.strptime('2011-01-27T19:01:12Z') }
...@@ -41,8 +41,10 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do ...@@ -41,8 +41,10 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
description: "*Created by: octocat*\n\nPlease pull these awesome changes", description: "*Created by: octocat*\n\nPlease pull these awesome changes",
source_project: project, source_project: project,
source_branch: 'feature', source_branch: 'feature',
head_source_sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b',
target_project: project, target_project: project,
target_branch: 'master', target_branch: 'master',
base_target_sha: '8ffb3c15a5475e59ae909384297fede4badcb4c7',
state: 'opened', state: 'opened',
milestone: nil, milestone: nil,
author_id: project.creator_id, author_id: project.creator_id,
...@@ -66,8 +68,10 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do ...@@ -66,8 +68,10 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
description: "*Created by: octocat*\n\nPlease pull these awesome changes", description: "*Created by: octocat*\n\nPlease pull these awesome changes",
source_project: project, source_project: project,
source_branch: 'feature', source_branch: 'feature',
head_source_sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b',
target_project: project, target_project: project,
target_branch: 'master', target_branch: 'master',
base_target_sha: '8ffb3c15a5475e59ae909384297fede4badcb4c7',
state: 'closed', state: 'closed',
milestone: nil, milestone: nil,
author_id: project.creator_id, author_id: project.creator_id,
...@@ -91,8 +95,10 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do ...@@ -91,8 +95,10 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
description: "*Created by: octocat*\n\nPlease pull these awesome changes", description: "*Created by: octocat*\n\nPlease pull these awesome changes",
source_project: project, source_project: project,
source_branch: 'feature', source_branch: 'feature',
head_source_sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b',
target_project: project, target_project: project,
target_branch: 'master', target_branch: 'master',
base_target_sha: '8ffb3c15a5475e59ae909384297fede4badcb4c7',
state: 'merged', state: 'merged',
milestone: nil, milestone: nil,
author_id: project.creator_id, author_id: project.creator_id,
...@@ -137,11 +143,11 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do ...@@ -137,11 +143,11 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
let(:milestone) { double(number: 45) } let(:milestone) { double(number: 45) }
let(:raw_data) { double(base_data.merge(milestone: milestone)) } let(:raw_data) { double(base_data.merge(milestone: milestone)) }
it 'returns nil when milestone does not exists' do it 'returns nil when milestone does not exist' do
expect(pull_request.attributes.fetch(:milestone)).to be_nil expect(pull_request.attributes.fetch(:milestone)).to be_nil
end end
it 'returns milestone when is exists' do it 'returns milestone when it exists' do
milestone = create(:milestone, project: project, iid: 45) milestone = create(:milestone, project: project, iid: 45)
expect(pull_request.attributes.fetch(:milestone)).to eq milestone expect(pull_request.attributes.fetch(:milestone)).to eq milestone
...@@ -158,36 +164,16 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do ...@@ -158,36 +164,16 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
end end
describe '#valid?' do describe '#valid?' do
let(:invalid_branch) { double(ref: 'invalid-branch').as_null_object } context 'when source, and target repos are not a fork' do
let(:raw_data) { double(base_data) }
context 'when source, and target repositories are the same' do
context 'and source and target branches exists' do
let(:raw_data) { double(base_data.merge(head: source_branch, base: target_branch)) }
it 'returns true' do it 'returns true' do
expect(pull_request.valid?).to eq true expect(pull_request.valid?).to eq true
end end
end end
context 'and source branch doesn not exists' do
let(:raw_data) { double(base_data.merge(head: invalid_branch, base: target_branch)) }
it 'returns false' do
expect(pull_request.valid?).to eq false
end
end
context 'and target branch doesn not exists' do
let(:raw_data) { double(base_data.merge(head: source_branch, base: invalid_branch)) }
it 'returns false' do
expect(pull_request.valid?).to eq false
end
end
end
context 'when source repo is a fork' do context 'when source repo is a fork' do
let(:source_repo) { double(id: 2, fork: true) } let(:source_repo) { double(id: 2) }
let(:raw_data) { double(base_data) } let(:raw_data) { double(base_data) }
it 'returns false' do it 'returns false' do
...@@ -196,7 +182,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do ...@@ -196,7 +182,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
end end
context 'when target repo is a fork' do context 'when target repo is a fork' do
let(:target_repo) { double(id: 2, fork: true) } let(:target_repo) { double(id: 2) }
let(:raw_data) { double(base_data) } let(:raw_data) { double(base_data) }
it 'returns false' do it 'returns false' do
......
...@@ -64,7 +64,13 @@ describe MergeRequest, models: true do ...@@ -64,7 +64,13 @@ describe MergeRequest, models: true do
describe '#target_sha' do describe '#target_sha' do
context 'when the target branch does not exist anymore' do context 'when the target branch does not exist anymore' do
subject { create(:merge_request).tap { |mr| mr.update_attribute(:target_branch, 'deleted') } } let(:project) { create(:project) }
subject { create(:merge_request, source_project: project, target_project: project) }
before do
project.repository.raw_repository.delete_branch(subject.target_branch)
end
it 'returns nil' do it 'returns nil' do
expect(subject.target_sha).to be_nil expect(subject.target_sha).to be_nil
...@@ -289,7 +295,12 @@ describe MergeRequest, models: true do ...@@ -289,7 +295,12 @@ describe MergeRequest, models: true do
let(:fork_project) { create(:project, forked_from_project: project) } let(:fork_project) { create(:project, forked_from_project: project) }
context 'when the target branch does not exist anymore' do context 'when the target branch does not exist anymore' do
subject { create(:merge_request).tap { |mr| mr.update_attribute(:target_branch, 'deleted') } } subject { create(:merge_request, source_project: project, target_project: project) }
before do
project.repository.raw_repository.delete_branch(subject.target_branch)
subject.reload
end
it 'does not crash' do it 'does not crash' do
expect{ subject.diverged_commits_count }.not_to raise_error expect{ subject.diverged_commits_count }.not_to raise_error
......
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