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

Fix importer for GitHub Pull Requests when a branch was reused across Pull Requests

Closes #17766

## What does this MR do?

GitHub importer fails when a repository has two Pull Requests, with the same branch name and different SHA, one of which was closed without merging, and one which is open or was merged in. This happens because we only checks if the branch exists in the [Gitlab::GithubImport::BranchFormatter#exists?](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/gitlab/github_import/branch_formatter.rb#L6-8), before creating the missing references on Github. With this MR we check if both branch, and SHA exists in the current project. If no, we create the missing reference on GitHub before import the PR.

## Are there points in the code the reviewer needs to double check?

Nope.

## Why was this MR needed?

Github importer will fail if branch was reused across Pull Requests.

## What are the relevant issue numbers?

https://gitlab.com/gitlab-org/gitlab-ce/issues/19439

/cc @royaldark @thomasjachmann

See merge request !5103

CHANGELOG

@@ -44,6 +44,7 @@ v 8.10.0 (unreleased)
   - More descriptive message for git hooks and file locks
   - Handle custom Git hook result in GitLab UI
   - Allow '?', or '&' for label names
+  - Fix importer for GitHub Pull Requests when a branch was reused across Pull Requests
 
 v 8.9.5 (unreleased)
   - Improve the request / withdraw access button. !4860

lib/gitlab/github_import/branch_formatter.rb

@@ -4,7 +4,7 @@ module Gitlab
       delegate :repo, :sha, :ref, to: :raw_data
 
       def exists?
-        project.repository.branch_exists?(ref)
+        branch_exists? && commit_exists?
       end
 
       def name
@@ -15,11 +15,15 @@ module Gitlab
         repo.present?
       end
 
-      def valid?
+      private
-        repo.present?
+
+      def branch_exists?
+        project.repository.branch_exists?(ref)
       end
 
-      private
+      def commit_exists?
+        project.repository.commit(sha).present?
+      end
 
       def short_id
         sha.to_s[0..7]

lib/gitlab/github_import/importer.rb

@@ -131,8 +131,10 @@ module Gitlab
       def clean_up_restored_branches(branches)
         branches.each do |name, _|
           client.delete_ref(repo, "heads/#{name}")
-          project.repository.rm_branch(project.creator, name)
+          project.repository.delete_branch(name) rescue Rugged::ReferenceError
         end
+
+        project.repository.after_remove_branch
       end
 
       def apply_labels(issuable)

spec/lib/gitlab/github_import/branch_formatter_spec.rb

@@ -2,17 +2,18 @@ require 'spec_helper'
 
 describe Gitlab::GithubImport::BranchFormatter, lib: true do
   let(:project) { create(:project) }
+  let(:commit) { create(:commit, project: project) }
   let(:repo) { double }
   let(:raw) do
     {
       ref: 'feature',
       repo: repo,
-      sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b'
+      sha: commit.id
     }
   end
 
   describe '#exists?' do
-    it 'returns true when branch exists' do
+    it 'returns true when both branch, and commit exists' do
       branch = described_class.new(project, double(raw))
 
       expect(branch.exists?).to eq true
@@ -23,6 +24,12 @@ describe Gitlab::GithubImport::BranchFormatter, lib: true do
 
       expect(branch.exists?).to eq false
     end
+
+    it 'returns false when commit does not exist' do
+      branch = described_class.new(project, double(raw.merge(sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b')))
+
+      expect(branch.exists?).to eq false
+    end
   end
 
   describe '#name' do
@@ -33,7 +40,7 @@ describe Gitlab::GithubImport::BranchFormatter, lib: true do
     end
 
     it 'returns formatted ref when branch does not exist' do
-      branch = described_class.new(project, double(raw.merge(ref: 'removed-branch')))
+      branch = described_class.new(project, double(raw.merge(ref: 'removed-branch', sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b')))
 
       expect(branch.name).to eq 'removed-branch-2e5d3239'
     end
@@ -51,18 +58,18 @@ describe Gitlab::GithubImport::BranchFormatter, lib: true do
     it 'returns raw sha' do
       branch = described_class.new(project, double(raw))
 
-      expect(branch.sha).to eq '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b'
+      expect(branch.sha).to eq commit.id
     end
   end
 
   describe '#valid?' do
-    it 'returns true when repository exists' do
+    it 'returns true when raw repo is present' do
       branch = described_class.new(project, double(raw))
 
       expect(branch.valid?).to eq true
     end
 
-    it 'returns false when repository does not exist' do
+    it 'returns false when raw repo is blank' do
       branch = described_class.new(project, double(raw.merge(repo: nil)))
 
       expect(branch.valid?).to eq false

spec/lib/gitlab/github_import/pull_request_formatter_spec.rb

@@ -2,11 +2,13 @@ require 'spec_helper'
 
 describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
   let(:project) { create(:project) }
+  let(:source_sha) { create(:commit, project: project).id }
+  let(:target_sha) { create(:commit, project: project, git_commit: RepoHelpers.another_sample_commit).id }
   let(:repository) { double(id: 1, fork: false) }
   let(:source_repo) { repository }
-  let(:source_branch) { double(ref: 'feature', repo: source_repo, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b') }
+  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: '8ffb3c15a5475e59ae909384297fede4badcb4c7') }
+  let(:target_branch) { double(ref: 'master', repo: target_repo, sha: target_sha) }
   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') }
@@ -41,10 +43,10 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
           description: "*Created by: octocat*\n\nPlease pull these awesome changes",
           source_project: project,
           source_branch: 'feature',
-          head_source_sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b',
+          head_source_sha: source_sha,
           target_project: project,
           target_branch: 'master',
-          base_target_sha: '8ffb3c15a5475e59ae909384297fede4badcb4c7',
+          base_target_sha: target_sha,
           state: 'opened',
           milestone: nil,
           author_id: project.creator_id,
@@ -68,10 +70,10 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
           description: "*Created by: octocat*\n\nPlease pull these awesome changes",
           source_project: project,
           source_branch: 'feature',
-          head_source_sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b',
+          head_source_sha: source_sha,
           target_project: project,
           target_branch: 'master',
-          base_target_sha: '8ffb3c15a5475e59ae909384297fede4badcb4c7',
+          base_target_sha: target_sha,
           state: 'closed',
           milestone: nil,
           author_id: project.creator_id,
@@ -95,10 +97,10 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
           description: "*Created by: octocat*\n\nPlease pull these awesome changes",
           source_project: project,
           source_branch: 'feature',
-          head_source_sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b',
+          head_source_sha: source_sha,
           target_project: project,
           target_branch: 'master',
-          base_target_sha: '8ffb3c15a5475e59ae909384297fede4badcb4c7',
+          base_target_sha: target_sha,
           state: 'merged',
           milestone: nil,
           author_id: project.creator_id,