diff --git a/app/validators/sha_validator.rb b/app/validators/sha_validator.rb index 085fca4d65d..77e7cfa4f6b 100644 --- a/app/validators/sha_validator.rb +++ b/app/validators/sha_validator.rb @@ -2,7 +2,7 @@ class ShaValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) - return if value.blank? || value.match(/\A\h{40}\z/) + return if value.blank? || Commit.valid_hash?(value) record.errors.add(attribute, 'is not a valid SHA') end diff --git a/changelogs/unreleased/fj-58804-fix-bitbucket-import.yml b/changelogs/unreleased/fj-58804-fix-bitbucket-import.yml new file mode 100644 index 00000000000..dc44c64a055 --- /dev/null +++ b/changelogs/unreleased/fj-58804-fix-bitbucket-import.yml @@ -0,0 +1,5 @@ +--- +title: Fix bug in BitBucket imports with SHA shorter than 40 chars +merge_request: 26050 +author: +type: fixed diff --git a/spec/lib/gitlab/bitbucket_import/importer_spec.rb b/spec/lib/gitlab/bitbucket_import/importer_spec.rb index c432cc223b9..e1a2bae5fe8 100644 --- a/spec/lib/gitlab/bitbucket_import/importer_spec.rb +++ b/spec/lib/gitlab/bitbucket_import/importer_spec.rb @@ -95,6 +95,9 @@ describe Gitlab::BitbucketImport::Importer do subject { described_class.new(project) } describe '#import_pull_requests' do + let(:source_branch_sha) { sample.commits.last } + let(:target_branch_sha) { sample.commits.first } + before do allow(subject).to receive(:import_wiki) allow(subject).to receive(:import_issues) @@ -102,9 +105,9 @@ describe Gitlab::BitbucketImport::Importer do pull_request = instance_double( Bitbucket::Representation::PullRequest, iid: 10, - source_branch_sha: sample.commits.last, + source_branch_sha: source_branch_sha, source_branch_name: Gitlab::Git::BRANCH_REF_PREFIX + sample.source_branch, - target_branch_sha: sample.commits.first, + target_branch_sha: target_branch_sha, target_branch_name: Gitlab::Git::BRANCH_REF_PREFIX + sample.target_branch, title: 'This is a title', description: 'This is a test pull request', @@ -162,6 +165,19 @@ describe Gitlab::BitbucketImport::Importer do expect(reply_note).to be_a(DiffNote) expect(reply_note.note).to eq(@reply.note) end + + context "when branches' sha is not found in the repository" do + let(:source_branch_sha) { 'a' * Commit::MIN_SHA_LENGTH } + let(:target_branch_sha) { 'b' * Commit::MIN_SHA_LENGTH } + + it 'uses the pull request sha references' do + expect { subject.execute }.to change { MergeRequest.count }.by(1) + + merge_request_diff = MergeRequest.first.merge_request_diff + expect(merge_request_diff.head_commit_sha).to eq source_branch_sha + expect(merge_request_diff.start_commit_sha).to eq target_branch_sha + end + end end context 'issues statuses' do diff --git a/spec/validators/sha_validator_spec.rb b/spec/validators/sha_validator_spec.rb index dc1539cf318..0a76570f65e 100644 --- a/spec/validators/sha_validator_spec.rb +++ b/spec/validators/sha_validator_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' describe ShaValidator do let(:validator) { described_class.new(attributes: [:base_commit_sha]) } - let(:merge_diff) { build(:merge_request_diff) } + let!(:merge_diff) { build(:merge_request_diff) } subject { validator.validate_each(merge_diff, :base_commit_sha, value) } @@ -12,6 +12,8 @@ describe ShaValidator do let(:value) { nil } it 'does not add any error if value is empty' do + expect(Commit).not_to receive(:valid_hash?) + subject expect(merge_diff.errors).to be_empty @@ -21,7 +23,9 @@ describe ShaValidator do context 'with valid sha' do let(:value) { Digest::SHA1.hexdigest(SecureRandom.hex) } - it 'does not add any error if value is empty' do + it 'does not add any error' do + expect(Commit).to receive(:valid_hash?).and_call_original + subject expect(merge_diff.errors).to be_empty @@ -32,6 +36,7 @@ describe ShaValidator do let(:value) { 'foo' } it 'adds error to the record' do + expect(Commit).to receive(:valid_hash?).and_call_original expect(merge_diff.errors).to be_empty subject