Merge branch 'gh-pull-requests' into 'master'

Check out GH pull requests locally where the source/target branch had been deleted

## What does this MR do?

Check out GitHub pull requests where source/target branches that had been deleted locally rather than temporarily restoring them on GitHub using their References API. This helps us to not get rate limited, and allow us to import cross-repository pull requests (those from forks).

## What are the relevant issue numbers?

Fixes #15528

Fixes #17766

Fixes #19310

Fixes #19439

Fixes #19998

Fixes #20153

Fixes #20552

Fixes https://gitlab.com/gitlab-com/support-forum/issues/801

#20385

## Screenshots (if relevant)

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [x] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [X] ~~API support added~~
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !5630

10 files changed, 81 insertions, 177 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 845e68c62af..0a0e894ae64 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -49,6 +49,7 @@ v 8.11.0 (unreleased)
   - Document that webhook secret token is sent in X-Gitlab-Token HTTP header !5664 (lycoperdon)
   - Gitlab::Highlight is now instrumented
   - All created issues, API or WebUI, can be submitted to Akismet for spam check !5333
+  - Allow users to import cross-repository pull requests from GitHub
   - The overhead of instrumented method calls has been reduced
   - Remove `search_id` of labels dropdown filter to fix 'Missleading URI for labels in Merge Requests and Issues view'. !5368 (Scott Le)
   - Load project invited groups and members eagerly in `ProjectTeam#fetch_members`
diff --git a/app/views/import/github/status.html.haml b/app/views/import/github/status.html.haml
index deaaf9af875..54ff1d27c67 100644
--- a/app/views/import/github/status.html.haml
+++ b/app/views/import/github/status.html.haml
@@ -4,10 +4,6 @@
   %i.fa.fa-github
   Import projects from GitHub
 
-%p
-  %i.fa.fa-warning
-  To import GitHub pull requests, any pull request source branches that had been deleted are temporarily restored on GitHub. To prevent any connected CI services from being overloaded with dozens of irrelevant branches being created and deleted again, GitHub webhooks are temporarily disabled during the import process, but only if you have admin access to the GitHub repository.
-
 %p.light
   Select projects you want to import.
 %hr
diff --git a/doc/workflow/importing/import_projects_from_github.md b/doc/workflow/importing/import_projects_from_github.md
index a2b2a4b88f9..306caabf6e6 100644
--- a/doc/workflow/importing/import_projects_from_github.md
+++ b/doc/workflow/importing/import_projects_from_github.md
@@ -18,9 +18,6 @@ At its current state, GitHub importer can import:
 

 With GitLab 8.7+, references to pull requests and issues are preserved.

 

-It is not yet possible to import your cross-repository pull requests (those from

-forks). We are working on improving this in the near future.

-

 The importer page is visible when you [create a new project][new-project].

 Click on the **GitHub** link and, if you are logged in via the GitHub

 integration, you will be redirected to GitHub for permission to access your

diff --git a/lib/gitlab/github_import/branch_formatter.rb b/lib/gitlab/github_import/branch_formatter.rb
index 7d2d545b84e..4750675ae9d 100644
--- a/lib/gitlab/github_import/branch_formatter.rb
+++ b/lib/gitlab/github_import/branch_formatter.rb
@@ -7,10 +7,6 @@ module Gitlab
         branch_exists? && commit_exists?
       end
 
-      def name
-        @name ||= exists? ? ref : "#{ref}-#{short_id}"
-      end
-
       def valid?
         repo.present?
       end
diff --git a/lib/gitlab/github_import/hook_formatter.rb b/lib/gitlab/github_import/hook_formatter.rb
deleted file mode 100644
index db1fabaa18a..00000000000
--- a/lib/gitlab/github_import/hook_formatter.rb
+++ /dev/null
@@ -1,23 +0,0 @@
-module Gitlab
-  module GithubImport
-    class HookFormatter
-      EVENTS = %w[* create delete pull_request push].freeze
-
-      attr_reader :raw
-
-      delegate :id, :name, :active, to: :raw
-
-      def initialize(raw)
-        @raw = raw
-      end
-
-      def config
-        raw.config.attrs
-      end
-
-      def valid?
-        (EVENTS & raw.events).any? && active
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/github_import/importer.rb b/lib/gitlab/github_import/importer.rb
index 3932fcb1eda..9ddc8905bd6 100644
--- a/lib/gitlab/github_import/importer.rb
+++ b/lib/gitlab/github_import/importer.rb
@@ -12,7 +12,6 @@ module Gitlab
 
         if credentials
           @client = Client.new(credentials[:user])
-          @formatter = Gitlab::ImportFormatter.new
         else
           raise Projects::ImportService::Error, "Unable to find project import data credentials for project ID: #{@project.id}"
         end
@@ -66,73 +65,45 @@ module Gitlab
       end
 
       def import_pull_requests
-        disable_webhooks
-
         pull_requests = client.pull_requests(repo, state: :all, sort: :created, direction: :asc, per_page: 100)
         pull_requests = pull_requests.map { |raw| PullRequestFormatter.new(project, raw) }.select(&:valid?)
 
-        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
-
-        restore_branches(branches_removed)
-
         pull_requests.each do |pull_request|
-          merge_request = pull_request.create!
-          apply_labels(merge_request)
-          import_comments(merge_request)
-          import_comments_on_diff(merge_request)
+          begin
+            restore_source_branch(pull_request) unless pull_request.source_branch_exists?
+            restore_target_branch(pull_request) unless pull_request.target_branch_exists?
+
+            merge_request = pull_request.create!
+            apply_labels(merge_request)
+            import_comments(merge_request)
+            import_comments_on_diff(merge_request)
+          rescue ActiveRecord::RecordInvalid => e
+            raise Projects::ImportService::Error, e.message
+          ensure
+            clean_up_restored_branches(pull_request)
+          end
         end
 
         true
-      rescue ActiveRecord::RecordInvalid => e
-        raise Projects::ImportService::Error, e.message
-      ensure
-        clean_up_restored_branches(branches_removed)
-        clean_up_disabled_webhooks
       end
 
-      def disable_webhooks
-        update_webhooks(hooks, active: false)
+      def restore_source_branch(pull_request)
+        project.repository.fetch_ref(repo_url, "pull/#{pull_request.number}/head", pull_request.source_branch_name)
       end
 
-      def clean_up_disabled_webhooks
-        update_webhooks(hooks, active: true)
+      def restore_target_branch(pull_request)
+        project.repository.create_branch(pull_request.target_branch_name, pull_request.target_branch_sha)
       end
 
-      def update_webhooks(hooks, options)
-        hooks.each do |hook|
-          client.edit_hook(repo, hook.id, hook.name, hook.config, options)
-        end
+      def remove_branch(name)
+        project.repository.delete_branch(name)
+      rescue Rugged::ReferenceError
+        nil
       end
 
-      def hooks
-        @hooks ||=
-          begin
-            client.hooks(repo).map { |raw| HookFormatter.new(raw) }.select(&:valid?)
-
-          # The GitHub Repository Webhooks API returns 404 for users
-          # without admin access to the repository when listing hooks.
-          # In this case we just want to return gracefully instead of
-          # spitting out an error and stop the import process.
-          rescue Octokit::NotFound
-            []
-          end
-      end
-
-      def restore_branches(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 clean_up_restored_branches(branches)
-        branches.each do |name, _|
-          client.delete_ref(repo, "heads/#{name}")
-          project.repository.delete_branch(name) rescue Rugged::ReferenceError
-        end
+      def clean_up_restored_branches(pull_request)
+        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?
 
         project.repository.after_remove_branch
       end
diff --git a/lib/gitlab/github_import/pull_request_formatter.rb b/lib/gitlab/github_import/pull_request_formatter.rb
index a4ea2210abd..b84538a090a 100644
--- a/lib/gitlab/github_import/pull_request_formatter.rb
+++ b/lib/gitlab/github_import/pull_request_formatter.rb
@@ -1,8 +1,8 @@
 module Gitlab
   module GithubImport
     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
+      delegate :exists?, :project, :ref, :repo, :sha, to: :source_branch, prefix: true
+      delegate :exists?, :project, :ref, :repo, :sha, to: :target_branch, prefix: true
 
       def attributes
         {
@@ -33,17 +33,29 @@ module Gitlab
       end
 
       def valid?
-        source_branch.valid? && target_branch.valid? && !cross_project?
+        source_branch.valid? && target_branch.valid?
       end
 
       def source_branch
         @source_branch ||= BranchFormatter.new(project, raw_data.head)
       end
 
+      def source_branch_name
+        @source_branch_name ||= begin
+          source_branch_exists? ? source_branch_ref : "pull/#{number}/#{source_branch_ref}"
+        end
+      end
+
       def target_branch
         @target_branch ||= BranchFormatter.new(project, raw_data.base)
       end
 
+      def target_branch_name
+        @target_branch_name ||= begin
+          target_branch_exists? ? target_branch_ref : "pull/#{number}/#{target_branch_ref}"
+        end
+      end
+
       private
 
       def assigned?
@@ -68,10 +80,6 @@ module Gitlab
         raw_data.body || ""
       end
 
-      def cross_project?
-        source_branch_repo.id != target_branch_repo.id
-      end
-
       def description
         formatter.author_line(author) + body
       end
diff --git a/spec/lib/gitlab/github_import/branch_formatter_spec.rb b/spec/lib/gitlab/github_import/branch_formatter_spec.rb
index fc9d5204148..e5300dbba1e 100644
--- a/spec/lib/gitlab/github_import/branch_formatter_spec.rb
+++ b/spec/lib/gitlab/github_import/branch_formatter_spec.rb
@@ -32,20 +32,6 @@ describe Gitlab::GithubImport::BranchFormatter, lib: true do
     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', sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b')))
-
-      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))
diff --git a/spec/lib/gitlab/github_import/hook_formatter_spec.rb b/spec/lib/gitlab/github_import/hook_formatter_spec.rb
deleted file mode 100644
index 110ba428258..00000000000
--- a/spec/lib/gitlab/github_import/hook_formatter_spec.rb
+++ /dev/null
@@ -1,65 +0,0 @@
-require 'spec_helper'
-
-describe Gitlab::GithubImport::HookFormatter, lib: true do
-  describe '#id' do
-    it 'returns raw id' do
-      raw = double(id: 100000)
-      formatter = described_class.new(raw)
-      expect(formatter.id).to eq 100000
-    end
-  end
-
-  describe '#name' do
-    it 'returns raw id' do
-      raw = double(name: 'web')
-      formatter = described_class.new(raw)
-      expect(formatter.name).to eq 'web'
-    end
-  end
-
-  describe '#config' do
-    it 'returns raw config.attrs' do
-      raw = double(config: double(attrs: { url: 'http://something.com/webhook' }))
-      formatter = described_class.new(raw)
-      expect(formatter.config).to eq({ url: 'http://something.com/webhook' })
-    end
-  end
-
-  describe '#valid?' do
-    it 'returns true when events contains the wildcard event' do
-      raw = double(events: ['*', 'commit_comment'], active: true)
-      formatter = described_class.new(raw)
-      expect(formatter.valid?).to eq true
-    end
-
-    it 'returns true when events contains the create event' do
-      raw = double(events: ['create', 'commit_comment'], active: true)
-      formatter = described_class.new(raw)
-      expect(formatter.valid?).to eq true
-    end
-
-    it 'returns true when events contains delete event' do
-      raw = double(events: ['delete', 'commit_comment'], active: true)
-      formatter = described_class.new(raw)
-      expect(formatter.valid?).to eq true
-    end
-
-    it 'returns true when events contains pull_request event' do
-      raw = double(events: ['pull_request', 'commit_comment'], active: true)
-      formatter = described_class.new(raw)
-      expect(formatter.valid?).to eq true
-    end
-
-    it 'returns false when events does not contains branch related events' do
-      raw = double(events: ['member', 'commit_comment'], active: true)
-      formatter = described_class.new(raw)
-      expect(formatter.valid?).to eq false
-    end
-
-    it 'returns false when hook is not active' do
-      raw = double(events: ['pull_request', 'commit_comment'], active: false)
-      formatter = described_class.new(raw)
-      expect(formatter.valid?).to eq false
-    end
-  end
-end
diff --git a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb
index 79931ecd134..aa28e360993 100644
--- a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb
+++ b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb
@@ -9,6 +9,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
   let(:source_branch) { double(ref: 'feature', repo: source_repo, sha: source_sha) }
   let(:target_repo) { repository }
   let(:target_branch) { double(ref: 'master', repo: target_repo, sha: target_sha) }
+  let(:removed_branch) { double(ref: 'removed-branch', repo: source_repo, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b') }
   let(:octocat) { double(id: 123456, login: 'octocat') }
   let(:created_at) { DateTime.strptime('2011-01-26T19:01:12Z') }
   let(:updated_at) { DateTime.strptime('2011-01-27T19:01:12Z') }
@@ -165,6 +166,42 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
     end
   end
 
+  describe '#source_branch_name' do
+    context 'when source branch exists' do
+      let(:raw_data) { double(base_data) }
+
+      it 'returns branch ref' do
+        expect(pull_request.source_branch_name).to eq 'feature'
+      end
+    end
+
+    context 'when source branch does not exist' do
+      let(:raw_data) { double(base_data.merge(head: removed_branch)) }
+
+      it 'prefixes branch name with pull request number' do
+        expect(pull_request.source_branch_name).to eq 'pull/1347/removed-branch'
+      end
+    end
+  end
+
+  describe '#target_branch_name' do
+    context 'when source branch exists' do
+      let(:raw_data) { double(base_data) }
+
+      it 'returns branch ref' do
+        expect(pull_request.target_branch_name).to eq 'master'
+      end
+    end
+
+    context 'when target branch does not exist' do
+      let(:raw_data) { double(base_data.merge(base: removed_branch)) }
+
+      it 'prefixes branch name with pull request number' do
+        expect(pull_request.target_branch_name).to eq 'pull/1347/removed-branch'
+      end
+    end
+  end
+
   describe '#valid?' do
     context 'when source, and target repos are not a fork' do
       let(:raw_data) { double(base_data) }
@@ -178,8 +215,8 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
       let(:source_repo) { double(id: 2) }
       let(:raw_data) { double(base_data) }
 
-      it 'returns false' do
-        expect(pull_request.valid?).to eq false
+      it 'returns true' do
+        expect(pull_request.valid?).to eq true
       end
     end
 
@@ -187,8 +224,8 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
       let(:target_repo) { double(id: 2) }
       let(:raw_data) { double(base_data) }
 
-      it 'returns false' do
-        expect(pull_request.valid?).to eq false
+      it 'returns true' do
+        expect(pull_request.valid?).to eq true
       end
     end
   end