diff options
author | Robert Speicher <robert@gitlab.com> | 2017-04-03 14:18:00 +0000 |
---|---|---|
committer | DJ Mountney <david@twkie.net> | 2017-04-03 14:23:29 -0700 |
commit | 75e6580af4e4569a273a2d79f02c78e8246555d6 (patch) | |
tree | 75900f8d997ea63f5e7a5fbd3061af119e093149 | |
parent | c21e9dd45cc949437c5df99148a6d3ddcba36d3b (diff) | |
download | gitlab-ce-75e6580af4e4569a273a2d79f02c78e8246555d6.tar.gz |
Merge branch '29541-fix-github-importer-deleted-fork' into 'master'
Fix GitHub importer for PRs of deleted forked repositories
Closes #29541
See merge request !9992
5 files changed, 73 insertions, 23 deletions
diff --git a/changelogs/unreleased/29541-fix-github-importer-deleted-fork.yml b/changelogs/unreleased/29541-fix-github-importer-deleted-fork.yml new file mode 100644 index 00000000000..fcb5798a619 --- /dev/null +++ b/changelogs/unreleased/29541-fix-github-importer-deleted-fork.yml @@ -0,0 +1,4 @@ +--- +title: Fix GitHub Importer for PRs of deleted forked repositories +merge_request: 9992 +author: diff --git a/lib/gitlab/github_import/branch_formatter.rb b/lib/gitlab/github_import/branch_formatter.rb index 0a8d05b5fe1..e7c6d2d5346 100644 --- a/lib/gitlab/github_import/branch_formatter.rb +++ b/lib/gitlab/github_import/branch_formatter.rb @@ -11,6 +11,14 @@ module Gitlab sha.present? && ref.present? end + def user + raw_data.user&.login || 'unknown' + end + + def short_sha + Commit.truncate_sha(sha) + end + private def branch_exists? diff --git a/lib/gitlab/github_import/pull_request_formatter.rb b/lib/gitlab/github_import/pull_request_formatter.rb index 28812fd0cb9..6d57ac335eb 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 < IssuableFormatter - delegate :exists?, :project, :ref, :repo, :sha, to: :source_branch, prefix: true - delegate :exists?, :project, :ref, :repo, :sha, to: :target_branch, prefix: true + delegate :user, :project, :ref, :repo, :sha, to: :source_branch, prefix: true + delegate :user, :exists?, :project, :ref, :repo, :sha, :short_sha, to: :target_branch, prefix: true def attributes { @@ -37,13 +37,20 @@ module Gitlab end def source_branch_name - @source_branch_name ||= begin - if cross_project? - "pull/#{number}/#{source_branch_repo.full_name}/#{source_branch_ref}" + @source_branch_name ||= + if cross_project? || !source_branch_exists? + source_branch_name_prefixed else - source_branch_exists? ? source_branch_ref : "pull/#{number}/#{source_branch_ref}" + source_branch_ref end - end + end + + def source_branch_name_prefixed + "gh-#{target_branch_short_sha}/#{number}/#{source_branch_user}/#{source_branch_ref}" + end + + def source_branch_exists? + !cross_project? && source_branch.exists? end def target_branch @@ -51,13 +58,17 @@ module Gitlab end def target_branch_name - @target_branch_name ||= begin - target_branch_exists? ? target_branch_ref : "pull/#{number}/#{target_branch_ref}" - end + @target_branch_name ||= target_branch_exists? ? target_branch_ref : target_branch_name_prefixed + end + + def target_branch_name_prefixed + "gl-#{target_branch_short_sha}/#{number}/#{target_branch_user}/#{target_branch_ref}" end def cross_project? - source_branch.repo.id != target_branch.repo.id + return true if source_branch_repo.nil? + + source_branch_repo.id != target_branch_repo.id end private diff --git a/spec/lib/gitlab/github_import/importer_spec.rb b/spec/lib/gitlab/github_import/importer_spec.rb index 33d83d6d2f1..85ee2b77b6b 100644 --- a/spec/lib/gitlab/github_import/importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer_spec.rb @@ -130,9 +130,9 @@ describe Gitlab::GithubImport::Importer, lib: true do let!(:user) { create(:user, email: octocat.email) } let(:repository) { double(id: 1, fork: false) } let(:source_sha) { create(:commit, project: project).id } - let(:source_branch) { double(ref: 'feature', repo: repository, sha: source_sha) } + let(:source_branch) { double(ref: 'feature', repo: repository, sha: source_sha, user: octoca) } let(:target_sha) { create(:commit, project: project, git_commit: RepoHelpers.another_sample_commit).id } - let(:target_branch) { double(ref: 'master', repo: repository, sha: target_sha) } + let(:target_branch) { double(ref: 'master', repo: repository, sha: target_sha, user: octoca) } let(:pull_request) do double( number: 1347, 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 b085e4720c6..736e3bcad63 100644 --- a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb +++ b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb @@ -4,15 +4,18 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do let(:client) { double } let(:project) { create(:project, :repository) } let(:source_sha) { create(:commit, project: project).id } - let(:target_sha) { create(:commit, project: project, git_commit: RepoHelpers.another_sample_commit).id } + let(:target_commit) { create(:commit, project: project, git_commit: RepoHelpers.another_sample_commit) } + let(:target_sha) { target_commit.id } + let(:target_short_sha) { target_commit.id.to_s[0..7] } let(:repository) { double(id: 1, fork: false) } let(:source_repo) { repository } let(:source_branch) { double(ref: 'feature', repo: source_repo, sha: source_sha) } let(:forked_source_repo) { double(id: 2, fork: true, name: 'otherproject', full_name: 'company/otherproject') } 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(:forked_branch) { double(ref: 'master', repo: forked_source_repo, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b') } + let(:target_branch) { double(ref: 'master', repo: target_repo, sha: target_sha, user: octocat) } + let(:removed_branch) { double(ref: 'removed-branch', repo: source_repo, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b', user: octocat) } + let(:forked_branch) { double(ref: 'master', repo: forked_source_repo, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b', user: octocat) } + let(:branch_deleted_repo) { double(ref: 'master', repo: nil, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b', user: octocat) } let(:octocat) { double(id: 123456, login: 'octocat', email: 'octocat@example.com') } let(:created_at) { DateTime.strptime('2011-01-26T19:01:12Z') } let(:updated_at) { DateTime.strptime('2011-01-27T19:01:12Z') } @@ -203,16 +206,24 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do 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' + it 'prefixes branch name with gh-:short_sha/:number/:user pattern to avoid collision' do + expect(pull_request.source_branch_name).to eq "gh-#{target_short_sha}/1347/octocat/removed-branch" end end context 'when source branch is from a fork' do let(:raw_data) { double(base_data.merge(head: forked_branch)) } - it 'prefixes branch name with pull request number and project with namespace to avoid collision' do - expect(pull_request.source_branch_name).to eq 'pull/1347/company/otherproject/master' + it 'prefixes branch name with gh-:short_sha/:number/:user pattern to avoid collision' do + expect(pull_request.source_branch_name).to eq "gh-#{target_short_sha}/1347/octocat/master" + end + end + + context 'when source branch is from a deleted fork' do + let(:raw_data) { double(base_data.merge(head: branch_deleted_repo)) } + + it 'prefixes branch name with gh-:short_sha/:number/:user pattern to avoid collision' do + expect(pull_request.source_branch_name).to eq "gh-#{target_short_sha}/1347/octocat/master" end end end @@ -229,8 +240,8 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do 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' + it 'prefixes branch name with gh-:short_sha/:number/:user pattern to avoid collision' do + expect(pull_request.target_branch_name).to eq 'gl-2e5d3239/1347/octocat/removed-branch' end end end @@ -290,6 +301,14 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do end end + context 'when source repository does not exist anymore' do + let(:raw_data) { double(base_data.merge(head: branch_deleted_repo)) } + + it 'returns true' do + expect(pull_request.cross_project?).to eq true + end + end + context 'when source and target repositories are the same' do let(:raw_data) { double(base_data.merge(head: source_branch)) } @@ -299,6 +318,14 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do end end + describe '#source_branch_exists?' do + let(:raw_data) { double(base_data.merge(head: forked_branch)) } + + it 'returns false when is a cross_project' do + expect(pull_request.source_branch_exists?).to eq false + end + end + describe '#url' do let(:raw_data) { double(base_data) } |