Merge branch 'dm-bitbucket-import-truncated-shas' into 'master'

app/models/commit.rb

@@ -25,8 +25,8 @@ class Commit
   DIFF_HARD_LIMIT_FILES = 1000
   DIFF_HARD_LIMIT_LINES = 50000
 
-  # The SHA can be between 7 and 40 hex characters.
-  COMMIT_SHA_PATTERN = '\h{7,40}'.freeze
+  MIN_SHA_LENGTH = 7
+  COMMIT_SHA_PATTERN = /\h{#{MIN_SHA_LENGTH},40}/.freeze
 
   def banzai_render_context(field)
     context = { pipeline: :single_line, project: self.project }
@@ -53,7 +53,7 @@ class Commit
 
     # Truncate sha to 8 characters
     def truncate_sha(sha)
-      sha[0..7]
+      sha[0..MIN_SHA_LENGTH]
     end
 
     def max_diff_options
@@ -100,7 +100,7 @@ class Commit
   def self.reference_pattern
     @reference_pattern ||= %r{
       (?:#{Project.reference_pattern}#{reference_prefix})?
-      (?<commit>\h{7,40})
+      (?<commit>#{COMMIT_SHA_PATTERN})
     }x
   end
 
@@ -216,9 +216,8 @@ class Commit
     @raw.respond_to?(method, include_private) || super
   end
 
-  # Truncate sha to 8 characters
   def short_id
-    @raw.short_id(7)
+    @raw.short_id(MIN_SHA_LENGTH)
   end
 
   def diff_refs

changelogs/unreleased/dm-bitbucket-import-truncated-shas.yml

+---
+title: Fix bug that caused merge requests with diff notes imported from Bitbucket
+  to raise errors
+merge_request:
+author:
+type: fixed

lib/gitlab/bitbucket_import/importer.rb

@@ -149,16 +149,21 @@ module Gitlab
             description += @formatter.author_line(pull_request.author) unless find_user_id(pull_request.author)
             description += pull_request.description
 
+            source_branch_sha = pull_request.source_branch_sha
+            target_branch_sha = pull_request.target_branch_sha
+            source_branch_sha = project.repository.commit(source_branch_sha)&.sha || source_branch_sha
+            target_branch_sha = project.repository.commit(target_branch_sha)&.sha || target_branch_sha
+
             merge_request = project.merge_requests.create!(
               iid: pull_request.iid,
               title: pull_request.title,
               description: description,
               source_project: project,
               source_branch: pull_request.source_branch_name,
-              source_branch_sha: pull_request.source_branch_sha,
+              source_branch_sha: source_branch_sha,
               target_project: project,
               target_branch: pull_request.target_branch_name,
-              target_branch_sha: pull_request.target_branch_sha,
+              target_branch_sha: target_branch_sha,
               state: pull_request.state,
               author_id: gitlab_user_id(project, pull_request.author),
               assignee_id: nil,

lib/gitlab/diff/diff_refs.rb

@@ -13,9 +13,9 @@ module Gitlab
 
       def ==(other)
         other.is_a?(self.class) &&
-          base_sha == other.base_sha &&
-          start_sha == other.start_sha &&
-          head_sha == other.head_sha
+          shas_equal?(base_sha, other.base_sha) &&
+          shas_equal?(start_sha, other.start_sha) &&
+          shas_equal?(head_sha, other.head_sha)
       end
 
       alias_method :eql?, :==
@@ -47,6 +47,22 @@ module Gitlab
           CompareService.new(project, head_sha).execute(project, start_sha, straight: straight)
         end
       end
+
+      private
+
+      def shas_equal?(sha1, sha2)
+        return true if sha1 == sha2
+        return false if sha1.nil? || sha2.nil?
+        return false unless sha1.class == sha2.class
+
+        length = [sha1.length, sha2.length].min
+
+        # If either of the shas is below the minimum length, we cannot be sure
+        # that they actually refer to the same commit because of hash collision.
+        return false if length < Commit::MIN_SHA_LENGTH
+
+        sha1[0, length] == sha2[0, length]
+      end
     end
   end
 end

lib/gitlab/diff/position.rb

@@ -49,12 +49,13 @@ module Gitlab
         coder['attributes'] = self.to_h
       end
 
-      def key
-        @key ||= [base_sha, start_sha, head_sha, Digest::SHA1.hexdigest(old_path || ""), Digest::SHA1.hexdigest(new_path || ""), old_line, new_line]
-      end
-
       def ==(other)
-        other.is_a?(self.class) && key == other.key
+        other.is_a?(self.class) &&
+          other.diff_refs == diff_refs &&
+          other.old_path == old_path &&
+          other.new_path == new_path &&
+          other.old_line == old_line &&
+          other.new_line == new_line
       end
 
       def to_h

spec/lib/gitlab/diff/diff_refs_spec.rb

@@ -3,6 +3,61 @@ require 'spec_helper'
 describe Gitlab::Diff::DiffRefs do
   let(:project) { create(:project, :repository) }
 
+  describe '#==' do
+    let(:commit) { project.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863') }
+    subject { commit.diff_refs }
+
+    context 'when shas are missing' do
+      let(:other) { described_class.new(base_sha: subject.base_sha, start_sha: subject.start_sha, head_sha: nil) }
+
+      it 'returns false' do
+        expect(subject).not_to eq(other)
+      end
+    end
+
+    context 'when shas are equal' do
+      let(:other) { described_class.new(base_sha: subject.base_sha, start_sha: subject.start_sha, head_sha: subject.head_sha) }
+
+      it 'returns true' do
+        expect(subject).to eq(other)
+      end
+    end
+
+    context 'when shas are unequal' do
+      let(:other) { described_class.new(base_sha: subject.base_sha, start_sha: subject.start_sha, head_sha: subject.head_sha.reverse) }
+
+      it 'returns false' do
+        expect(subject).not_to eq(other)
+      end
+    end
+
+    context 'when shas are truncated' do
+      context 'when sha prefixes are too short' do
+        let(:other) { described_class.new(base_sha: subject.base_sha[0, 4], start_sha: subject.start_sha[0, 4], head_sha: subject.head_sha[0, 4]) }
+
+        it 'returns false' do
+          expect(subject).not_to eq(other)
+        end
+      end
+
+      context 'when sha prefixes are equal' do
+        let(:other) { described_class.new(base_sha: subject.base_sha[0, 10], start_sha: subject.start_sha[0, 10], head_sha: subject.head_sha[0, 10]) }
+
+        it 'returns true' do
+          expect(subject).to eq(other)
+        end
+      end
+
+      context 'when sha prefixes are unequal' do
+        let(:other) { described_class.new(base_sha: subject.base_sha[0, 10], start_sha: subject.start_sha[0, 10], head_sha: subject.head_sha[0, 10].reverse) }
+
+        it 'returns false' do
+          expect(subject).not_to eq(other)
+        end
+      end
+    end
+  end
+
   describe '#compare_in' do
     context 'with diff refs for the initial commit' do
       let(:commit) { project.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863') }

spec/lib/gitlab/diff/position_spec.rb

@@ -429,6 +429,44 @@ describe Gitlab::Diff::Position do
     end
   end
 
+  describe '#==' do
+    let(:commit) { project.commit("570e7b2abdd848b95f2f578043fc23bd6f6fd24d") }
+
+    subject do
+      described_class.new(
+        old_path: "files/ruby/popen.rb",
+        new_path: "files/ruby/popen.rb",
+        old_line: nil,
+        new_line: 14,
+        diff_refs: commit.diff_refs
+      )
+    end
+
+    context 'when positions are equal' do
+      let(:other) { described_class.new(subject.to_h) }
+
+      it 'returns true' do
+        expect(subject).to eq(other)
+      end
+    end
+
+    context 'when positions are equal, except for truncated shas' do
+      let(:other) { described_class.new(subject.to_h.merge(start_sha: subject.start_sha[0, 10])) }
+
+      it 'returns true' do
+        expect(subject).to eq(other)
+      end
+    end
+
+    context 'when positions are unequal' do
+      let(:other) { described_class.new(subject.to_h.merge(start_sha: subject.start_sha.reverse)) }
+
+      it 'returns false' do
+        expect(subject).not_to eq(other)
+      end
+    end
+  end
+
   describe "#to_json" do
     let(:hash) do
       {